Issue 89900044: code review 89900044: os: cap reads and writes to 2GB on Darwin and FreeBSD (Closed)
Descriptionos: cap reads and writes to 2GB on Darwin and FreeBSD
Fixes Issue 7812
Patch Set 1 #Patch Set 2 : diff -r 2b0a7f247bb3 https://go.googlecode.com/hg/ #Patch Set 3 : diff -r 2b0a7f247bb3 https://go.googlecode.com/hg/ #Patch Set 4 : diff -r 2b0a7f247bb3 https://go.googlecode.com/hg/ #
Total comments: 3
Patch Set 5 : diff -r acf346c00e56 https://go.googlecode.com/hg/ #
Total comments: 5
Patch Set 6 : diff -r 490c2d4fda2b https://go.googlecode.com/hg/ #
Total comments: 1
Patch Set 7 : diff -r 490c2d4fda2b https://go.googlecode.com/hg/ #
Total comments: 1
Patch Set 8 : diff -r aefa5032b453 https://go.googlecode.com/hg/ #MessagesTotal messages: 22
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/
Sign in to reply to this message.
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) > } > > > >
Sign in to reply to this message.
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
Sign in to reply to this message.
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?
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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. >
Sign in to reply to this message.
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?
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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/ >
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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?
Sign in to reply to this message.
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 ||.
Sign in to reply to this message.
I agree that pread and pwrite can be simpler, and their comments can be updated to "up to len(b)".
Sign in to reply to this message.
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/ >
Sign in to reply to this message.
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/ > >
Sign in to reply to this message.
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.
Sign in to reply to this message.
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/,/
Sign in to reply to this message.
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?
Sign in to reply to this message.
*** 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
Sign in to reply to this message.
|
