code review 89900044: os: cap reads and writes to 2GB on Darwin and FreeBSD

os: cap reads and writes to 2GB on Darwin and FreeBSD

Fixes Issue 7812

[bradfitz, 2014-04-21 22:54:05]

Hello rsc@golang.org, iant@golang.org (cc: golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[bradfitz, 2014-04-21 22:57:12]

Do other BSDs do the same? Should we just do this everywhere, at least for
Read? Linux at least seems fine.



On Mon, Apr 21, 2014 at 3:54 PM, <bradfitz@golang.org> wrote:

> Reviewers: rsc, iant,
>
> Message:
> Hello rsc@golang.org, iant@golang.org (cc:
> golang-codereviews@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> os: cap syscall.Read calls to under 2GB on Darwin
>
> Fixes Issue 7812
>
> Please review this at https://codereview.appspot.com/89900044/
>
> Affected files (+10, -0 lines):
>   M src/pkg/os/file_unix.go
>
>
> Index: src/pkg/os/file_unix.go
> ===================================================================
> --- a/src/pkg/os/file_unix.go
> +++ b/src/pkg/os/file_unix.go
> @@ -172,9 +172,16 @@
>         return fi, err
>  }
>
> +// Darwin can't read more 2GB+ at a time from syscall.Read,
> +// even on 64-bit systems. See golang.org/issue/7812
> +const darwinMaxRead = 2<<30 - 1
> +
>  // read reads up to len(b) bytes from the File.
>  // It returns the number of bytes read and an error, if any.
>  func (f *File) read(b []byte) (n int, err error) {
> +       if runtime.GOOS == "darwin" && len(b) > darwinMaxRead {
> +               b = b[:darwinMaxRead]
> +       }
>         return syscall.Read(f.fd, b)
>  }
>
> @@ -182,12 +189,14 @@
>  // It returns the number of bytes read and the error, if any.
>  // EOF is signaled by a zero count with err set to 0.
>  func (f *File) pread(b []byte, off int64) (n int, err error) {
> +       // TODO: check 2GB pread on darwin; golang.org/issue/7812
>         return syscall.Pread(f.fd, b, off)
>  }
>
>  // write writes len(b) bytes to the File.
>  // It returns the number of bytes written and an error, if any.
>  func (f *File) write(b []byte) (n int, err error) {
> +       // TODO: check 2GB writes on darwin; golang.org/issue/7812
>         for {
>                 m, err := syscall.Write(f.fd, b)
>                 n += m
> @@ -207,6 +216,7 @@
>  // pwrite writes len(b) bytes to the File starting at byte offset off.
>  // It returns the number of bytes written and an error, if any.
>  func (f *File) pwrite(b []byte, off int64) (n int, err error) {
> +       // TODO: check 2GB writes on darwin; golang.org/issue/7812
>         return syscall.Pwrite(f.fd, b, off)
>  }
>
>
>
>

[adg, 2014-04-22 03:09:28]

On 2014/04/21 22:57:12, bradfitz wrote:
> Do other BSDs do the same? Should we just do this everywhere, at least for
> Read? Linux at least seems fine.

On FreeBSD read also fails at buflen 2<<30 with the same error:

$ ~/go/bin/go run read.go
2014/04/21 23:08:34 For buf size 2147483648: Read = -1, invalid argument

[adg, 2014-04-22 03:15:19]

https://codereview.appspot.com/89900044/diff/30002/src/pkg/os/file_unix.go
File src/pkg/os/file_unix.go (right):

https://codereview.appspot.com/89900044/diff/30002/src/pkg/os/file_unix.go#ne...
src/pkg/os/file_unix.go:182: if runtime.GOOS == "darwin" && len(b) >
darwinMaxRead {
One or two extra if statements on every read; what's the performance impact?
negligible?

Shouldn't the check just be in darwin's syscall package?

[bradfitz, 2014-04-22 03:18:15]

https://codereview.appspot.com/89900044/diff/30002/src/pkg/os/file_unix.go
File src/pkg/os/file_unix.go (right):

https://codereview.appspot.com/89900044/diff/30002/src/pkg/os/file_unix.go#ne...
src/pkg/os/file_unix.go:182: if runtime.GOOS == "darwin" && len(b) >
darwinMaxRead {
On 2014/04/22 03:15:19, adg wrote:
> One or two extra if statements on every read; what's the performance impact?
> negligible?
> 
> Shouldn't the check just be in darwin's syscall package?

There's only one check here. The first one is a compile-time constant.

Compared to a system call & disk I/O, it is nothing.

I don't think we want to go down the road of teaching the syscall package about
the semantics of io.Reader and io.Writer and io.ReaderAt and so on.  They're
supposed to be small wrappers to the actual system calls, not portability
layers.  That's what the os pkg is.

[adg, 2014-04-22 03:24:07]

https://codereview.appspot.com/89900044/diff/30002/src/pkg/os/file_unix.go
File src/pkg/os/file_unix.go (right):

https://codereview.appspot.com/89900044/diff/30002/src/pkg/os/file_unix.go#ne...
src/pkg/os/file_unix.go:182: if runtime.GOOS == "darwin" && len(b) >
darwinMaxRead {
On 2014/04/22 03:18:15, bradfitz wrote:
> On 2014/04/22 03:15:19, adg wrote:
> > One or two extra if statements on every read; what's the performance impact?
> > negligible?
> > 
> > Shouldn't the check just be in darwin's syscall package?
> 
> There's only one check here. The first one is a compile-time constant.
> 
> Compared to a system call & disk I/O, it is nothing.
> 
> I don't think we want to go down the road of teaching the syscall package
about
> the semantics of io.Reader and io.Writer and io.ReaderAt and so on.  They're
> supposed to be small wrappers to the actual system calls, not portability
> layers.  That's what the os pkg is.

Ah, I only skimmed the code. I see what you mean.

[ruiu, 2014-04-22 03:32:13]

FYI, on NetBSD/amd64 6.1.4, read had no problem with >2GB buffer. I tested
with a 5GB buffer.


On Mon, Apr 21, 2014 at 8:09 PM, <adg@golang.org> wrote:

> On 2014/04/21 22:57:12, bradfitz wrote:
>
>> Do other BSDs do the same? Should we just do this everywhere, at least
>>
> for
>
>> Read? Linux at least seems fine.
>>
>
> On FreeBSD read also fails at buflen 2<<30 with the same error:
>
> $ ~/go/bin/go run read.go
> 2014/04/21 23:08:34 For buf size 2147483648: Read = -1, invalid argument
>
> https://codereview.appspot.com/89900044/
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>

[iant, 2014-04-22 04:24:50]

Looks basically fine to me.  Sounds like it should handle pread, and sounds like
it should handle FreeBSD.

Anybody have the patience to test a 2GB write on Darwin?

[minux1, 2014-04-22 05:52:28]

On Tue, Apr 22, 2014 at 12:24 AM, <iant@golang.org> wrote:

> Looks basically fine to me.  Sounds like it should handle pread, and
> sounds like it should handle FreeBSD.
>
> Anybody have the patience to test a 2GB write on Darwin?

Actually we don't need to test. I did a little research into the source
code.

For OS X, the latest available xnu source is xnu-2422.1.72:
write syscall is here:
http://www.opensource.apple.com/source/xnu/xnu-2422.1.72/bsd/kern/sys_generic.c
search for "Write system call".
In function dofilewrite, it compares nbyte with INT_MAX, and if nbyte >
INT_MAX,
it will return EINVAL.
This affects at least write(2), pwrite(2),

For FreeBSD:
sys_write is defined here:
http://svnweb.freebsd.org/base/head/sys/kern/sys_generic.c?revision=HEAD&view...
it compares uap->nbyte with IOSIZE_MAX, where IOSIZE_MAX is defined
inhttp://
svnweb.freebsd.org/base/head/sys/sys/systm.h?revision=HEAD&view=markup#l152as:
#define IOSIZE_MAX (iosize_max_clamp ? INT_MAX : SSIZE_MAX)
and iosize_max_clamp is a sysctl, "debug.iosize_max_clamp", which defaults
to 0 on FB 11.

Similarly, this affects at least read(2), pread(2), write(2), pwrite(2). I
think readv(2) and
writev(2) are also affected because uio subsystem also enforces the
restriction.

NetBSD allows transfer size up to SSIZE_MAX, so it's not affected.

OpenBSD, the relevant code is here:
http://www.openbsd.org/cgi-bin/cvsweb/src/sys/kern/sys_generic.c?rev=HEAD;con...
It's similar to NetBSD code, not affected.

DragonflyBSD, similar to NetBSD, compares ((ssize_t)uap->nbyte < 0). Not
affected.

[bradfitz, 2014-04-25 15:51:29]

Hello rsc@golang.org, iant@golang.org, adg@golang.org, ruiu@google.com,
minux.ma@gmail.com (cc: golang-codereviews@googlegroups.com),

Please take another look.

[bradfitz, 2014-04-25 15:52:07]

PTAL with a paranoid eye




On Fri, Apr 25, 2014 at 8:51 AM, <bradfitz@golang.org> wrote:

> Hello rsc@golang.org, iant@golang.org, adg@golang.org, ruiu@google.com,
> minux.ma@gmail.com (cc: golang-codereviews@googlegroups.com),
>
> Please take another look.
>
>
> https://codereview.appspot.com/89900044/
>

[josharian, 2014-04-25 18:03:33]

https://codereview.appspot.com/89900044/diff/50001/src/pkg/os/file_unix.go
File src/pkg/os/file_unix.go (right):

https://codereview.appspot.com/89900044/diff/50001/src/pkg/os/file_unix.go#ne...
src/pkg/os/file_unix.go:175: // Darwin and FreeBSD can't read or write 2GB+ at a
time from
s/from//

https://codereview.appspot.com/89900044/diff/50001/src/pkg/os/file_unix.go#ne...
src/pkg/os/file_unix.go:196: for len(b) > 0 && err == nil {
File.pread is only used by File.ReadAt, and File.ReadAt already has a loop like
this to fill the buffer, so it looks naively like you could just use the same
simple approach here as in File.read above. (And perhaps update the doc above
appropriately.)

Same comment for File.pwrite and File.WriteAt.

[ruiu, 2014-04-25 20:05:17]

https://codereview.appspot.com/89900044/diff/50001/src/pkg/os/file_unix.go
File src/pkg/os/file_unix.go (right):

https://codereview.appspot.com/89900044/diff/50001/src/pkg/os/file_unix.go#ne...
src/pkg/os/file_unix.go:234: if needsMaxRW && len(bcap) != len(b) && err == nil
{
Also check m == len(bcap)?
Previously if syscall.Write is a short write but err is nil, this function
returns immediately.

[bradfitz, 2014-04-25 22:23:12]

https://codereview.appspot.com/89900044/diff/50001/src/pkg/os/file_unix.go
File src/pkg/os/file_unix.go (right):

https://codereview.appspot.com/89900044/diff/50001/src/pkg/os/file_unix.go#ne...
src/pkg/os/file_unix.go:234: if needsMaxRW && len(bcap) != len(b) && err == nil
{
On 2014/04/25 20:05:18, ruiu wrote:
> Also check m == len(bcap)?
> Previously if syscall.Write is a short write but err is nil, this function
> returns immediately.

If m != len(bcap) (well, less), it's already continuing on line 229/231 already,
no?

[ruiu, 2014-04-25 22:31:07]

https://codereview.appspot.com/89900044/diff/50001/src/pkg/os/file_unix.go
File src/pkg/os/file_unix.go (right):

https://codereview.appspot.com/89900044/diff/50001/src/pkg/os/file_unix.go#ne...
src/pkg/os/file_unix.go:234: if needsMaxRW && len(bcap) != len(b) && err == nil
{
Ah, yes. I thought the condition on line 229 was && but is actually ||.

[iant, 2014-04-26 02:57:53]

I agree that pread and pwrite can be simpler, and their comments can be updated
to "up to len(b)".

[bradfitz, 2014-04-26 03:00:03]

Existing semantics were unclear. I was treading cautiously. How would you
simplify pread/pwrite?

Also, in Linux and non-buggy OSes there are no code changes due to the
compile time constants. Should that not be a goal?
 On Apr 25, 2014 8:57 PM, <iant@golang.org> wrote:

> I agree that pread and pwrite can be simpler, and their comments can be
> updated to "up to len(b)".
>
> https://codereview.appspot.com/89900044/
>

[iant, 2014-04-26 03:17:47]

On 2014/04/26 03:00:03, bradfitz wrote:
> Existing semantics were unclear. I was treading cautiously. How would you
> simplify pread/pwrite?
>
> Also, in Linux and non-buggy OSes there are no code changes due to the
> compile time constants. Should that not be a goal?

Sorry, I didn't mean to simplify the existing pread and pwrite code.  I only
meant to simplify the code you are adding.  You don't need the loop.  You can
just make the same changes to pread and pwrite as you made to read and write. 
The comments on pread and pwrite can change from "reads len(b) bytes" to "reads
up to len(b) bytes".  This is because the only callers of pread and pwrite, in
file.go, do not actually require them to read/write the full buffer.

Ian


>  On Apr 25, 2014 8:57 PM, <mailto:iant@golang.org> wrote:
> 
> > I agree that pread and pwrite can be simpler, and their comments can be
> > updated to "up to len(b)".
> >
> > https://codereview.appspot.com/89900044/
> >

[bradfitz, 2014-04-26 16:34:18]

Hello rsc@golang.org, iant@golang.org, adg@golang.org, ruiu@google.com,
minux.ma@gmail.com, josharian@gmail.com (cc:
golang-codereviews@googlegroups.com),

Please take another look.

[josharian, 2014-04-26 17:01:11]

LGTM but please wait for another paranoid reviewer as well

https://codereview.appspot.com/89900044/diff/70001/src/pkg/os/file_unix.go
File src/pkg/os/file_unix.go (right):

https://codereview.appspot.com/89900044/diff/70001/src/pkg/os/file_unix.go#ne...
src/pkg/os/file_unix.go:175: // Darwin and FreeBSD can't read or write 2GB+ at a
time from
s/from/,/

[iant, 2014-04-26 17:23:36]

LGTM

https://codereview.appspot.com/89900044/diff/90001/src/pkg/os/file_unix.go
File src/pkg/os/file_unix.go (right):

https://codereview.appspot.com/89900044/diff/90001/src/pkg/os/file_unix.go#ne...
src/pkg/os/file_unix.go:176: // even on 64-bit systems. See
golang.org/issue/7812
Period at end of sentence?

[bradfitz, 2014-04-26 17:34:36]

*** Submitted as https://code.google.com/p/go/source/detail?r=e141dda580af ***

os: cap reads and writes to 2GB on Darwin and FreeBSD

Fixes Issue 7812

LGTM=josharian, iant
R=rsc, iant, adg, ruiu, minux.ma, josharian
CC=golang-codereviews
https://codereview.appspot.com/89900044