Am 14.02.2014 20:16, schrieb Zachary Turner:
> 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?

Yeah, I think that's it. The problem is that the single-threaded part 
(parse_pack_objects/parse_pack_header) _also_ calls pread (via sha1_object -> 
get_data_from_pack -> unpack_data). So a pread() that modifies the file 
position would naturally be bad in this single-threaded scenario. Incidentally, 
that's exactly what the lstat64 in the version below fixes (similar to 

> BTW, the version you posted isn't thread safe.

It is true that, in a multi-threaded scenario, my version modifies the file 
position in some indeterministic way. However, as you noted above, the file 
position is irrelevant to pread(), so that's perfectly thread-safe, as long as 
all threads use pread() exclusively.

Using [x]read() in one of the threads would _not_ be thread-safe, but we're not 
doing that here. Both fill()/xread() and parse_pack_objects()/lseek() are 
unreachable from threaded_second_pass(), and the main thread just waits for the 
background threads to complete...

>>> 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
More majordomo info at

Reply via email to