Re: Make the git codebase thread-safe

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]