Re: [PATCH v4] compat: Fix read() of 2GB and more on Mac OS X

[Linus Torvalds <torvalds@linux-foundation.org>]

[-- Attachment #1: Type: text/plain, Size: 1959 bytes --]

On Mon, Aug 19, 2013 at 8:41 AM, Steffen Prohaska <prohaska@zib.de> wrote:
>
> The reason was that read() immediately returns with EINVAL if nbyte >=
> 2GB.  According to POSIX [1], if the value of nbyte passed to read() is
> greater than SSIZE_MAX, the result is implementation-defined.

Yeah, the OS X filesystem layer is an incredible piece of shit. Not
only doesn't it follow POSIX, it fails *badly*. Because OS X kernel
engineers apparently have the mental capacity of a retarded rodent on
crack.

Linux also refuses to actually read more than a maximum value in one
go (because quite frankly, doing more than 2GB at a time is just not
reasonable, especially in unkillable disk wait), but at least Linux
gives you the partial read, so that the usual "read until you're
happy" works (which you have to do anyway with sockets, pipes, NFS
intr mounts, etc etc). Returning EINVAL is a sign of a diseased mind.

I hate your patch for other reasons, though:

> The problem for read() is addressed in a similar way by introducing
> a wrapper function in compat that always reads less than 2GB.

Why do you do that? We already _have_ wrapper functions for read(),
namely xread().  Exactly because you basically have to, in order to
handle signals on interruptible filesystems (which aren't POSIX
either, but at least sanely so) or from other random sources. And to
handle the "you can't do reads that big" issue.

So why isn't the patch much more straightforward? Like the attached
totally untested one that just limits the read/write size to 8MB
(which is totally arbitrary, but small enough to not have any latency
issues even on slow disks, and big enough that any reasonable IO
subsystem will still get good throughput).

And by "totally untested" I mean that it actually passes the git test
suite, but since I didn't apply your patch nor do I have OS X
anywhere, I can't actually test that it fixes *your* problem. But it
should.


                   Linus

[-- Attachment #2: patch.diff --]
[-- Type: application/octet-stream, Size: 1269 bytes --]

 wrapper.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/wrapper.c b/wrapper.c
index 6a015de5f056..e996f3dae467 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -131,6 +131,13 @@ void *xcalloc(size_t nmemb, size_t size)
 }
 
 /*
+ * Doing IO in huge chunks only results in pain. OS X is buggy,
+ * and even in the absense of bugs it can result in bad latencies
+ * when you decide to kill the process.
+ */
+#define MAX_IO_SIZE (8*1024*1024)
+
+/*
  * xread() is the same a read(), but it automatically restarts read()
  * operations with a recoverable error (EAGAIN and EINTR). xread()
  * DOES NOT GUARANTEE that "len" bytes is read even if the data is available.
@@ -139,7 +146,8 @@ ssize_t xread(int fd, void *buf, size_t len)
 {
 	ssize_t nr;
 	while (1) {
-		nr = read(fd, buf, len);
+		nr = len < MAX_IO_SIZE ? len : MAX_IO_SIZE;
+		nr = read(fd, buf, nr);
 		if ((nr < 0) && (errno == EAGAIN || errno == EINTR))
 			continue;
 		return nr;
@@ -155,7 +163,8 @@ ssize_t xwrite(int fd, const void *buf, size_t len)
 {
 	ssize_t nr;
 	while (1) {
-		nr = write(fd, buf, len);
+		nr = len < MAX_IO_SIZE ? len : MAX_IO_SIZE;
+		nr = write(fd, buf, nr);
 		if ((nr < 0) && (errno == EAGAIN || errno == EINTR))
 			continue;
 		return nr;