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

2013-08-26 Thread Junio C Hamano
Linus Torvalds torva...@linux-foundation.org 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


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

2013-08-19 Thread Steffen Prohaska
Previously, filtering 2GB or more through an external filter (see test)
failed on Mac OS X 10.8.4 (12E55) for a 64-bit executable with:

error: read from external filter cat failed
error: cannot feed the input to external filter cat
error: cat died of signal 13
error: external filter cat failed 141
error: external filter cat failed

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.  The write
function has the same restriction [2].  Since OS X still supports
running 32-bit executables, the 32-bit limit (SSIZE_MAX = INT_MAX
= 2GB - 1) seems to be also imposed on 64-bit executables under certain
conditions.  For write, the problem has been addressed in a earlier
commit [6c642a].

The problem for read() is addressed in a similar way by introducing
a wrapper function in compat that always reads less than 2GB.  It is
very likely that the read() and write() wrappers are always used
together.  To avoid introducing another option, NEEDS_CLIPPED_WRITE is
changed to NEEDS_CLIPPED_IO and used to activate both wrappers.

To avoid expanding the read compat macro in constructs like
'vtbl-read(...)', 'read' is renamed to 'readfn' in two cases.  The
solution seems more robust than using '#undef read'.

Note that 'git add' exits with 0 even if it prints filtering errors to
stderr.  The test, therefore, checks stderr.  'git add' should probably
be changed (sometime in another commit) to exit with nonzero if
filtering fails.  The test could then be changed to use test_must_fail.

Thanks to the following people for their suggestions:

Johannes Sixt j...@kdbg.org
John Keeping j...@keeping.me.uk
Jonathan Nieder jrnie...@gmail.com
Kyle J. McKay mack...@gmail.com
Torsten Bögershausen tbo...@web.de
Eric Sunshine sunsh...@sunshineco.com

[1] http://pubs.opengroup.org/onlinepubs/009695399/functions/read.html
[2] http://pubs.opengroup.org/onlinepubs/009695399/functions/write.html

[6c642a] 6c642a878688adf46b226903858b53e2d31ac5c3
compate/clipped-write.c: large write(2) fails on Mac OS X/XNU

Signed-off-by: Steffen Prohaska proha...@zib.de
---
 Makefile | 10 +-
 builtin/var.c| 10 +-
 compat/{clipped-write.c = clipped-io.c} | 11 ++-
 config.mak.uname |  2 +-
 git-compat-util.h|  5 -
 streaming.c  |  4 ++--
 t/t0021-conversion.sh| 14 ++
 7 files changed, 41 insertions(+), 15 deletions(-)
 rename compat/{clipped-write.c = clipped-io.c} (53%)

diff --git a/Makefile b/Makefile
index 3588ca1..f54134d 100644
--- a/Makefile
+++ b/Makefile
@@ -69,8 +69,8 @@ all::
 # Define NO_MSGFMT_EXTENDED_OPTIONS if your implementation of msgfmt
 # doesn't support GNU extensions like --check and --statistics
 #
-# Define NEEDS_CLIPPED_WRITE if your write(2) cannot write more than
-# INT_MAX bytes at once (e.g. MacOS X).
+# Define NEEDS_CLIPPED_IO if your read(2) and/or write(2) cannot handle more
+# than INT_MAX bytes at once (e.g. Mac OS X).
 #
 # Define HAVE_PATHS_H if you have paths.h and want to use the default PATH
 # it specifies.
@@ -1493,9 +1493,9 @@ ifndef NO_MSGFMT_EXTENDED_OPTIONS
MSGFMT += --check --statistics
 endif
 
-ifdef NEEDS_CLIPPED_WRITE
-   BASIC_CFLAGS += -DNEEDS_CLIPPED_WRITE
-   COMPAT_OBJS += compat/clipped-write.o
+ifdef NEEDS_CLIPPED_IO
+   BASIC_CFLAGS += -DNEEDS_CLIPPED_IO
+   COMPAT_OBJS += compat/clipped-io.o
 endif
 
 ifneq (,$(XDL_FAST_HASH))
diff --git a/builtin/var.c b/builtin/var.c
index aedbb53..06f8459 100644
--- a/builtin/var.c
+++ b/builtin/var.c
@@ -28,7 +28,7 @@ static const char *pager(int flag)
 
 struct git_var {
const char *name;
-   const char *(*read)(int);
+   const char *(*readfn)(int);
 };
 static struct git_var git_vars[] = {
{ GIT_COMMITTER_IDENT, git_committer_info },
@@ -43,8 +43,8 @@ static void list_vars(void)
struct git_var *ptr;
const char *val;
 
-   for (ptr = git_vars; ptr-read; ptr++)
-   if ((val = ptr-read(0)))
+   for (ptr = git_vars; ptr-readfn; ptr++)
+   if ((val = ptr-readfn(0)))
printf(%s=%s\n, ptr-name, val);
 }
 
@@ -53,9 +53,9 @@ static const char *read_var(const char *var)
struct git_var *ptr;
const char *val;
val = NULL;
-   for (ptr = git_vars; ptr-read; ptr++) {
+   for (ptr = git_vars; ptr-readfn; ptr++) {
if (strcmp(var, ptr-name) == 0) {
-   val = ptr-read(IDENT_STRICT);
+   val = ptr-readfn(IDENT_STRICT);
break;
}
}
diff --git a/compat/clipped-write.c b/compat/clipped-io.c
similarity index 53%
rename from compat/clipped-write.c
rename 

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

2013-08-19 Thread Linus Torvalds
On Mon, Aug 19, 2013 at 8:41 AM, Steffen Prohaska proha...@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


patch.diff
Description: Binary data


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

2013-08-19 Thread Steffen Prohaska

On Aug 19, 2013, at 6:04 PM, Linus Torvalds torva...@linux-foundation.org 
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

2013-08-19 Thread Junio C Hamano
Linus Torvalds torva...@linux-foundation.org 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

2013-08-19 Thread Junio C Hamano
Steffen Prohaska proha...@zib.de 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

2013-08-19 Thread Linus Torvalds
On Mon, Aug 19, 2013 at 10:16 AM, Junio C Hamano gits...@pobox.com wrote:
 Linus Torvalds torva...@linux-foundation.org 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

2013-08-19 Thread Kyle J. McKay

On Aug 19, 2013, at 10:16, Junio C Hamano wrote:


Linus Torvalds torva...@linux-foundation.org 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 2err 
+   ! test -s err 
+   rm -f 2GB 
+   git checkout -- 2GB 2err 
+   ! 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=value temporarily unavailable,  
due to optimizations, src=0x100 ' ' repeats 200 times...,  
len=2147483648, dst=0xbfffe774, cmd=0x402980 cat) at convert.c:407
#11 0x0009c6f6 in convert_to_git (path=0x4028b4 2GB, src=0x100 '  
' repeats 200 times..., 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=value temporarily unavailable, due to  
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  

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

2013-08-19 Thread Linus Torvalds
On Mon, Aug 19, 2013 at 2:56 PM, Kyle J. McKay mack...@gmail.com 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