Re: What's cooking in git.git (Apr 2012, #06; Sun, 15)

[Johannes Sixt <j.sixt@viscovery.net>]

Am 4/19/2012 8:16, schrieb Johannes Sixt:
> Am 4/18/2012 21:53, schrieb Ramsay Jones:
>> Johannes Sixt wrote:
>>> Am 4/16/2012 8:44, schrieb Junio C Hamano:
>>>> * nd/threaded-index-pack (2012-04-11) 3 commits
>>>>  - index-pack: support multithreaded delta resolving
>>>>  - index-pack: split second pass obj handling into own function
>>>>  - compat/win32/pthread.h: Add an pthread_key_delete() implementation
>>>
>>> With this series, t9300.92 (fast-import, Q: verify pack) consistently
>>> fails for me on Windows.
>>>
>>> I'll have to see when I can dig deeper into this topic...
>>
>> Hmm, this works just fine for me.
> 
> It's a Heisenbug. I see different failure modes:
> 
> error: inflate: data stream error (incorrect header check)
> fatal: serious inflate inconsistency
> 
> fatal: premature end of pack file, 79 bytes missing
> 
> fatal: premature end of pack file, 72 bytes missing
> 
> (and I even saw successful runs). It's always the same pack,
> pack-54fa663f5ec35*.pack. But running index-pack --verify manually does
> not fail. The packs look good because the installed index-pack (which was
> built without this topic) does not report an error, either.

Here's one backtrace:

#0  error (err=0x55f6e6 "inflate: %s (%s)") at usage.c:122
#1  0x004ba96d in git_inflate (strm=0x12afdb8, flush=0) at zlib.c:144
#2  0x00434823 in get_data_from_pack (obj=0xd82608)
    at builtin/index-pack.c:486
#3  0x00434f6e in resolve_delta (delta_obj=0xd82608, base=0xd891e0,
    result=0x12b01f0) at builtin/index-pack.c:687
#4  0x0043528a in find_unresolved_deltas_1 (base=0xd891e0, prev_base=0x0)
    at builtin/index-pack.c:743
#5  0x004352ee in find_unresolved_deltas (base=0xd891e0)
    at builtin/index-pack.c:759
#6  0x004353cd in second_pass (obj=0xd81bc0) at builtin/index-pack.c:798
#7  0x004354c4 in threaded_second_pass (arg=0xd811e8)
    at builtin/index-pack.c:821
#8  0x00505200 in win32_start_routine (arg=0xd811e8)
    at compat/win32/pthread.c:20
#9  ...

I don't see any mutual exclusion happening in this chain. Perhaps it is
not needed, provided that the pread() call in get_data_from_pack is
atomic. But our git_pread() from compat/pread.c, which we use on Windows,
is not atomic.

:-(

I don't think that it is possible to make git_pread() atomic because it
would have to be protected against all file accesses that can modify the
file position.

Is get_data_from_pack() the only function that accesses the pack data?
Then we could add some mutual exclusion there.

-- Hannes