subject:"Re\: Make the git codebase thread\-safe"

Re: Make the git codebase thread-safe

2014-02-18 Thread Junio C Hamano

Duy Nguyen pclo...@gmail.com writes:

 On Sat, Feb 15, 2014 at 8:15 AM, Zachary Turner ztur...@chromium.org wrote:
 ...
 2) Use TLS as you suggest and have one fd per pack thread.  Probably
 the most complicated code change (at least for me, being a first-time
 contributor)

 It's not so complicated. I suggested a patch [1] before (surprise!).
 ...
 [1] http://article.gmane.org/gmane.comp.version-control.git/196042

That message is at the tail end of the discussion. I wonder why
nothing came out of it back then.

While I do not see anything glaringly wrong with the change from a
quick glance over it, it would be nice to hear how well it performs
on their platform from Windows folks.

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: Make the git codebase thread-safe

2014-02-18 Thread Zachary Turner

It shouldn't be hard for us to run some tests with this patch applied.
 Will report back in a day or two.

On Tue, Feb 18, 2014 at 9:55 AM, Junio C Hamano gits...@pobox.com wrote:
 Duy Nguyen pclo...@gmail.com writes:

 On Sat, Feb 15, 2014 at 8:15 AM, Zachary Turner ztur...@chromium.org wrote:
 ...
 2) Use TLS as you suggest and have one fd per pack thread.  Probably
 the most complicated code change (at least for me, being a first-time
 contributor)

 It's not so complicated. I suggested a patch [1] before (surprise!).
 ...
 [1] http://article.gmane.org/gmane.comp.version-control.git/196042

 That message is at the tail end of the discussion. I wonder why
 nothing came out of it back then.

 While I do not see anything glaringly wrong with the change from a
 quick glance over it, it would be nice to hear how well it performs
 on their platform from Windows folks.

 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: Make the git codebase thread-safe

2014-02-14 Thread Karsten Blees

Am 14.02.2014 00:09, schrieb Zachary Turner:
 To elaborate a little bit more, you can verify with a sample program
 that ReadFile with OVERLAPPED does in fact modify the HANDLE's file
 position.  The documentation doesn't actually state one way or
 another.   My original attempt at a patch didn't have the ReOpenFile,
 and we experienced regular read corruption.  We scratched our heads
 over it for a bit, and then hypothesized that someone must be mixing
 read styles, which led to this ReOpenFile workaround, which
 incidentally also solved the corruption problems.  We wrote a similar
 sample program to verify that when using ReOpenHandle, and changing
 the file pointer of the duplicated handle, that the file pointer of
 the original handle is not modified.
 
 We did not actually try to identify the source of the mixed read
 styles, but it seems like the only possible explanation.
 
 On Thu, Feb 13, 2014 at 2:53 PM, Stefan Zager sza...@google.com wrote:
 On Thu, Feb 13, 2014 at 2:51 PM, Karsten Blees karsten.bl...@gmail.com 
 wrote:
 Am 13.02.2014 19:38, schrieb Zachary Turner:

 The only reason ReOpenFile is necessary at
 all is because some code somewhere is mixing read-styles against the same
 fd.


 I don't understand...ReadFile with OVERLAPPED parameter doesn't modify the 
 HANDLE's file position, so you should be able to mix read()/pread() however 
 you like (as long as read() is only called from one thread).

 That is, apparently, a bald-faced lie in the ReadFile API doc.  First
 implementation didn't use ReOpenFile, and it crashed all over the
 place.  ReOpenFile fixed it.

 Stefan

Damn...you're right, multi-threaded git-index-pack works fine, but some tests 
fail badly. Mixed reads would have to be from git_mmap, which is the only other 
caller of pread().

A simple alternative to ReOpenHandle is to reset the file pointer to its 
original position, as in compat/pread.c::git_pread. Thus single-theaded code 
can mix read()/pread() at will, but multi-threaded code has to use pread() 
exclusively (which is usually the case anyway). A main thread using read() and 
background threads using pread() (which is technically allowed by POSIX) will 
fail with this solution.

This version passes the test suite on msysgit:

8
ssize_t mingw_pread(int fd, void *buf, size_t count, off64_t offset)
{
DWORD bytes_read;
OVERLAPPED overlapped;
off64_t current;
memset(overlapped, 0, sizeof(overlapped));
overlapped.Offset = (DWORD) offset;
overlapped.OffsetHigh = (DWORD) (offset  32);

current = lseek64(fd, 0, SEEK_CUR);

if (!ReadFile((HANDLE)_get_osfhandle(fd), buf, count, bytes_read, 
overlapped)) {
errno = err_win_to_posix(GetLastError());
return -1;
}

lseek64(fd, current, SEEK_SET);

return (ssize_t) bytes_read;
}

--
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: Make the git codebase thread-safe

2014-02-14 Thread Zachary Turner

(Gah, sorry if you're receiving multiple emails to your personal
addresses, I need to get used to manually setting Plain-text mode
every time I send a message).

For the mixed read, we wouldn't be looking for another caller of
pread() (since it doesn't care what the file pointer is), but instead
a caller of read() or lseek() (since those do depend on the current
file pointer).  In index-pack.c, I see two possible culprits:

1) A call to xread() from inside fill()
2) A call to lseek in parse_pack_objects()

Do you think these could be related?  If so, maybe that opens up some
other solutions?

BTW, the version you posted isn't thread safe.  Suppose thread A and
thread B execute this function at the same time.  A executes through
the ReadFile(), but does not yet reset the second lseek64.  B then
executes the first lseek64(), storing off the modified file pointer.
Then A finishes, then B finishes.  At the end, the file pointer is
still modified.

On Fri, Feb 14, 2014 at 11:15 AM, Zachary Turner ztur...@chromium.org wrote:
 For the mixed read, we wouldn't be looking for another caller of pread()
 (since it doesn't care what the file pointer is), but instead a caller of
 read() or lseek().  In index-pack.c, I see two possible culprits:

 1) A call to xread() from inside fill()
 2) A call to lseek in parse_pack_objects()

 Do you think these could be related?  If so, maybe that opens up some other
 solutions?

 BTW, the version you posted isn't thread safe.  Suppose thread A and thread
 B execute this function at the same time.  A executes through the
 ReadFile(), but does not yet reset the second lseek64.  B then executes the
 first lseek64(), storing off the modified file pointer.  Then A finishes,
 then B finishes.  At the end, the file pointer is still modified.



 On Fri, Feb 14, 2014 at 11:04 AM, Karsten Blees karsten.bl...@gmail.com
 wrote:

 Am 14.02.2014 00:09, schrieb Zachary Turner:
  To elaborate a little bit more, you can verify with a sample program
  that ReadFile with OVERLAPPED does in fact modify the HANDLE's file
  position.  The documentation doesn't actually state one way or
  another.   My original attempt at a patch didn't have the ReOpenFile,
  and we experienced regular read corruption.  We scratched our heads
  over it for a bit, and then hypothesized that someone must be mixing
  read styles, which led to this ReOpenFile workaround, which
  incidentally also solved the corruption problems.  We wrote a similar
  sample program to verify that when using ReOpenHandle, and changing
  the file pointer of the duplicated handle, that the file pointer of
  the original handle is not modified.
 
  We did not actually try to identify the source of the mixed read
  styles, but it seems like the only possible explanation.
 
  On Thu, Feb 13, 2014 at 2:53 PM, Stefan Zager sza...@google.com wrote:
  On Thu, Feb 13, 2014 at 2:51 PM, Karsten Blees
  karsten.bl...@gmail.com wrote:
  Am 13.02.2014 19:38, schrieb Zachary Turner:
 
  The only reason ReOpenFile is necessary at
  all is because some code somewhere is mixing read-styles against the
  same
  fd.
 
 
  I don't understand...ReadFile with OVERLAPPED parameter doesn't modify
  the HANDLE's file position, so you should be able to mix read()/pread()
  however you like (as long as read() is only called from one thread).
 
  That is, apparently, a bald-faced lie in the ReadFile API doc.  First
  implementation didn't use ReOpenFile, and it crashed all over the
  place.  ReOpenFile fixed it.
 
  Stefan

 Damn...you're right, multi-threaded git-index-pack works fine, but some
 tests fail badly. Mixed reads would have to be from git_mmap, which is the
 only other caller of pread().

 A simple alternative to ReOpenHandle is to reset the file pointer to its
 original position, as in compat/pread.c::git_pread. Thus single-theaded code
 can mix read()/pread() at will, but multi-threaded code has to use pread()
 exclusively (which is usually the case anyway). A main thread using read()
 and background threads using pread() (which is technically allowed by POSIX)
 will fail with this solution.

 This version passes the test suite on msysgit:

 8
 ssize_t mingw_pread(int fd, void *buf, size_t count, off64_t offset)
 {
 DWORD bytes_read;
 OVERLAPPED overlapped;
 off64_t current;
 memset(overlapped, 0, sizeof(overlapped));
 overlapped.Offset = (DWORD) offset;
 overlapped.OffsetHigh = (DWORD) (offset  32);

 current = lseek64(fd, 0, SEEK_CUR);

 if (!ReadFile((HANDLE)_get_osfhandle(fd), buf, count, bytes_read,
 overlapped)) {
 errno = err_win_to_posix(GetLastError());
 return -1;
 }

 lseek64(fd, current, SEEK_SET);

 return (ssize_t) bytes_read;
 }


--
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: Make the git codebase thread-safe

2014-02-14 Thread Stefan Zager

On Fri, Feb 14, 2014 at 11:15 AM, Zachary Turner ztur...@chromium.org wrote:
For the mixed read, we wouldn't be looking for another caller of pread()
(since it doesn't care what the file pointer is), but instead a caller of
read() or lseek(). In index-pack.c, I see two possible culprits:

1) A call to xread() from inside fill()
2) A call to lseek in parse_pack_objects()

Do you think these could be related? If so, maybe that opens up some other
solutions?

From my observations, it's not that simple. As you pointed out to me
before, fill() is only called before the threading part of the code,
and lseek is only called after the threading part; and the lseek() is
lseek(fd, 0, SEEK_CUR), so it's purely advisory.

Also, here is the error output we got before you added ReOpenFile:

remote: Total 2514467 (delta 1997300), reused 2513040 (delta 1997113)
Checking connectivity... error: packfile
d:/src/chromium2/_gclient_src_a6y1bf/.git/objects/pack/pack-3b8d06040ac37f14d0b43859926f1153fea61a7a.pack
does not match index
warning: packfile
d:/src/chromium2/_gclient_src_a6y1bf/.git/objects/pack/pack-3b8d06040ac37f14d0b43859926f1153fea61a7a.pack
cannot be accessed
error: packfile
d:/src/chromium2/_gclient_src_a6y1bf/.git/objects/pack/pack-3b8d06040ac37f14d0b43859926f1153fea61a7a.pack
does not match index
warning: packfile
d:/src/chromium2/_gclient_src_a6y1bf/.git/objects/pack/pack-3b8d06040ac37f14d0b43859926f1153fea61a7a.pack
cannot be accessed
error: packfile
d:/src/chromium2/_gclient_src_a6y1bf/.git/objects/pack/pack-3b8d06040ac37f14d0b43859926f1153fea61a7a.pack
does not match index
warning: packfile
d:/src/chromium2/_gclient_src_a6y1bf/.git/objects/pack/pack-3b8d06040ac37f14d0b43859926f1153fea61a7a.pack
cannot be accessed
fatal: bad object e0f9f23f765a45e6d80863a8f881ee735c9347fe

The 'Checking connectivity...' message comes from builtin/clone.c,
which runs in a separate process from builtin/index-pack.c. What this
suggests to me is that file descriptors for the loose object files are
not being flushed or closed properly before index-pack finishes.

BTW, the version you posted isn't thread safe. Suppose thread A and thread
B execute this function at the same time. A executes through the
ReadFile(), but does not yet reset the second lseek64. B then executes the
first lseek64(), storing off the modified file pointer. Then A finishes,
then B finishes. At the end, the file pointer is still modified.

Yes, that. I would also point out that in our experiments, ReOpenFile
is not nearly as expensive as its name might suggest. Since the
solution using ReOpenFile is pretty solidly thread-safe (at least as
far as we can tell), I'm in favor of using it unless or until we
properly root-case the failure.

Stefan
--
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

48 matches

Mail list logo