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 git_pread).
> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html