Re: [PATCH v4] compat: Fix read() of 2GB and more on Mac OS X
Linus Torvalds writes: > So it would probably be a great idea to make the filtering code able > to do things in smaller chunks, but I suspect that the patch to chunk > up xread/xwrite is the right thing to do anyway. Yes and yes, but the first yes is a bit tricky for writing things out, as the recipient of the filter knows the size of the input but not of the output, and both loose and packed objects needs to record the length of the object at the very beginning. Even though our streaming API allows to write new objects directly to a packfile, for user-specified filters, CRLF, and ident can make the size of the output unknown before processing all the data, so the best we could do for these would be to stream to a temporary file and then copy it again with the length header (undeltified packed object deflates only the payload, so this "copy" can literally be a byte-for-byte copy, after writing the in-pack header out). As reading from the object store and writing it out to the filesystem (i.e. entry.c::write_entry() codepath) does not need to know the output size, convert.c::get_stream_filter() might want to be told in which direction a filter is asked for and return a streaming filter back even when those filters that are problematic for the opposite, writing-to-object-store direction. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] compat: Fix read() of 2GB and more on Mac OS X
On Mon, Aug 19, 2013 at 2:56 PM, Kyle J. McKay wrote: > > The fact that the entire file is read into memory when applying the filter > does not seem like a good thing (see #7-#10 above). Yeah, that's horrible. Its likely bad for performance too, because even if you have enough memory, it blows everything out of the L2/L3 caches, and if you don't have enough memory it obviously causes other problems. So it would probably be a great idea to make the filtering code able to do things in smaller chunks, but I suspect that the patch to chunk up xread/xwrite is the right thing to do anyway. Linus -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] compat: Fix read() of 2GB and more on Mac OS X
On Aug 19, 2013, at 10:16, Junio C Hamano wrote: Linus Torvalds writes: 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). Ahh. OK, not noticing EINVAL unconditionally, but always feed IOs in chunks that are big enough for sane systems but small enough for broken ones. That makes sense. Could somebody on MacOS X test this? I tested this on both i386 (OS X 32-bit intel) and x86_64 (OS X 64-bit intel). What I tested: 1. I started with branch pu: (965adb10 Merge branch 'sg/bash-prompt-lf-in-cwd-test' into pu) 2. I added Steffen's additional test (modified to always run) to t0021: diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh index e50f0f7..b92e6cb 100755 --- a/t/t0021-conversion.sh +++ b/t/t0021-conversion.sh @@ -190,4 +190,16 @@ test_expect_success 'required filter clean failure' ' test_must_fail git add test.fc ' +test_expect_success 'filter large file' ' + git config filter.largefile.smudge cat && + git config filter.largefile.clean cat && + for i in $(test_seq 1 2048); do printf "%1048576d" 1; done >2GB && + echo "2GB filter=largefile" >.gitattributes && + git add 2GB 2>err && + ! test -s err && + rm -f 2GB && + git checkout -- 2GB 2>err && + ! test -s err +' + test_done 3. I verified that the test fails with an unpatched build on both 32- bit and 64-bit. 4. I applied Linus's unmodified patch to wrapper.c. 5. I tested again. The t0021 test now passes on 64-bit. It still fails on 32-bit for another reason unrelated to Linus's patch. It fails when attempting the "git add 2GB" line from the new 'filter large file' part of the test. The failure with backtrace: git(16806,0xa095c720) malloc: *** mmap(size=2147487744) failed (error code=12) *** error: can't allocate region *** set a breakpoint in malloc_error_break to debug # NOTE: error code 12 is ENOMEM on OS X Breakpoint 1, 0x97f634b1 in malloc_error_break () (gdb) bt #0 0x97f634b1 in malloc_error_break () #1 0x97f5e49f in szone_error () #2 0x97e8b876 in allocate_pages () #3 0x97e8c062 in large_and_huge_malloc () #4 0x97e831c8 in szone_malloc () #5 0x97e82fb8 in malloc_zone_malloc () #6 0x97e8c7b2 in realloc () #7 0x00128abe in xrealloc (ptr=0x0, size=2147483649) at wrapper.c:100 #8 0x00111a8c in strbuf_grow (sb=0xbfffe634, extra=2147483648) at strbuf.c:74 #9 0x00112bb9 in strbuf_read (sb=0xbfffe634, fd=6, hint=2548572518) at strbuf.c:349 #10 0x0009b899 in apply_filter (path=due to optimizations>, src=0x100 ' ' ..., len=2147483648, dst=0xbfffe774, cmd=0x402980 "cat") at convert.c:407 #11 0x0009c6f6 in convert_to_git (path=0x4028b4 "2GB", src=0x100 ' ' ..., len=2147483648, dst=0xbfffe774, checksafe=SAFE_CRLF_WARN) at convert.c:764 #12 0x0010bb38 in index_mem (sha1=0x402330 "", buf=0x100, size=2147483648, type=OBJ_BLOB, path=0x4028b4 "2GB", flags=1) at sha1_file.c:3044 #13 0x0010bf57 in index_core [inlined] () at /private/var/tmp/src/git/ sha1_file.c:3101 #14 0x0010bf57 in index_fd (sha1=0x402330 "", fd=5, st=0xbfffe900, type=OBJ_BLOB, path=0x4028b4 "2GB", flags=1) at sha1_file.c:3139 #15 0x0010c05e in index_path (sha1=0x402330 "", path=0x4028b4 "2GB", st=0xbfffe900, flags=1) at sha1_file.c:3157 #16 0x000e82f4 in add_to_index (istate=0x1a8820, path=0x4028b4 "2GB", st=0xbfffe900, flags=0) at read-cache.c:665 #17 0x000e87c8 in add_file_to_index (istate=0x1a8820, path=0x4028b4 "2GB", flags=0) at read-cache.c:694 #18 0x440a in cmd_add (argc=optimizations>, argv=0xb584, prefix=0x0) at builtin/add.c:299 #19 0x2e1f in run_builtin [inlined] () at /private/var/tmp/src/git/ git.c:303 #20 0x2e1f in handle_internal_command (argc=2, argv=0xb584) at git.c:466 #21 0x32d4 in run_argv [inlined] () at /private/var/tmp/src/git/ git.c:512 #22 0x32d4 in main (argc=2, av=0xbfffe28c) at git.c:595 The size 2147487744 is 2GB + 4096 bytes. Apparently git does not support a filter for a file unless the file can fit entirely into git's memory space. Normally a single 2GB + 4096 byte allocation works in an OS X 32-bit process, but something else is apparently eating up a large portion of the memory space in this case (perhaps an mmap'd copy?). In any case, if the file being filtered was closer to 4GB in size it would always fail on 32-bit regardless. The fact that the entire file is read into memory when applying the filter does not seem like a good thing (see #7-#10 above). --Kyle -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] compat: Fix read() of 2GB and more on Mac OS X
On Mon, Aug 19, 2013 at 10:16 AM, Junio C Hamano wrote: > Linus Torvalds writes: > > The same argument applies to xwrite(), but currently we explicitly > catch EINTR and EAGAIN knowing that on sane systems these are the > signs that we got interrupted. > > Do we catch EINVAL unconditionally in the same codepath? No, and we shouldn't. If EINVAL happens, it will keep happening. But with the size limiter, it doesn't matter, since we won't hit the OS X braindamage. > Could > EINVAL on saner systems mean completely different thing (like our > caller is passing bogus parameters to underlying read/write, which > is a program bug we would want to catch)? Yes. Even on OS X, it means that - it's just that OS X notion of what is "bogus" is pure crap. But the thing is, looping on EINVAL would be wrong even on OS X, since unless you change the size, it will keep happening forever. But with the "limit IO to 8MB" (or whatever) patch, the issue is moot. If you get an EINVAL, it will be due to something else being horribly horribly wrong. Linus -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] compat: Fix read() of 2GB and more on Mac OS X
Steffen Prohaska writes: > I'm happy to rework it again towards your suggestion. I would also remove > the compat wrapper for write(). But I got a bit tired. I'd appreciate if > I received more indication whether a version without compat wrappers would > be accepted. I think it is a reasonable way forward to remove the writer side wrapper and doing large IO in reasonably big (but small enough not to trigger MacOS X limitations) chunks in both read/write direction. Linus, thanks for a dose of sanity. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] compat: Fix read() of 2GB and more on Mac OS X
Linus Torvalds writes: > 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. The same argument applies to xwrite(), but currently we explicitly catch EINTR and EAGAIN knowing that on sane systems these are the signs that we got interrupted. Do we catch EINVAL unconditionally in the same codepath? Could EINVAL on saner systems mean completely different thing (like our caller is passing bogus parameters to underlying read/write, which is a program bug we would want to catch)? > 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). Ahh. OK, not noticing EINVAL unconditionally, but always feed IOs in chunks that are big enough for sane systems but small enough for broken ones. That makes sense. Could somebody on MacOS X test this? Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] compat: Fix read() of 2GB and more on Mac OS X
On Aug 19, 2013, at 6:04 PM, Linus Torvalds wrote: > 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? The first version was more straightforward [1]. But reviewers suggested that the compat wrappers would be the right way to do it and showed me that it has been done like this before [2]. I haven't submitted anything in a while, so I tried to be a kind person and followed the suggestions. I started to hate the patch a bit (maybe less than you), but I wasn't brave enough to reject the suggestions. This is why the patch became what it is. I'm happy to rework it again towards your suggestion. I would also remove the compat wrapper for write(). But I got a bit tired. I'd appreciate if I received more indication whether a version without compat wrappers would be accepted. Steffen [1] http://article.gmane.org/gmane.comp.version-control.git/232455 [2] 6c642a8 compate/clipped-write.c: large write(2) fails on Mac OS X/XNU-- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] compat: Fix read() of 2GB and more on Mac OS X
On Mon, Aug 19, 2013 at 8:41 AM, Steffen Prohaska 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 patch.diff Description: Binary data