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

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Aug 19, 2013, 16:04 UTC
Message-ID: <CA+55aFzQhJqE4QDwJDKtkTtJpMNbz3_Aw5_Q3yTk5DnhLJyjCQ@mail.gmail.com>
In-Reply-To: <1376926879-30846-1-git-send-email-prohaska@zib.de>

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

 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;