On Fri, Sep 08, 2017 at 02:44:57PM +0200, Michael Haggerty wrote: > > That means we're holding the packed-refs lock for a slightly longer > > period. I think this could mean worse lock contention between otherwise > > unrelated transactions over the packed-refs file. I wonder if the > > lock-retry timeout might need to be increased to accommodate this. On > > the other hand, it looks like we take it after getting the individual > > locks, which I'd think would be the expensive part. > > That was my thinking, yes. While the packed-refs lock is held, the > references being created/updated have their reflogs written and are > renamed into place. I don't see how that can be shortened without > compromising on correctness (in particular, that we want to process > creates/updates before deletions to try to preserve reachability as much > as possible during the transaction). As an added optimization, the > packed-refs lock is not acquired at all if no references are being deleted. Makes sense. I guess in theory a process could do significant unrelated work between the "prepare" and "finish" steps, while holding the lock. But I don't know why it would, and arguably that itself is a bug. > The packed-refs file and loose references are never locked at the same > time during pack-refs, so no deadlock is possible. > > But you are right to assume that it *should* be so. The algorithm > written above is a tiny bit unsafe (and has been for years). It is > possible, though admittedly very unlikely, for the following to happen > in the gap between steps 5 and 6: Thanks for explaining (and I think that your "should" is why I thought it was so; we've discussed this race before). > This would leave "foo" at the obsolete value "B" (i.e., the value > written to the `packed-refs` file for it by the nested `pack-refs` process. > > I think that fixing this problem would require the `packed-refs` lock to > be held while `pack-refs` is pruning the loose references. But given how > unlikely that chain of events seems, and that fixing it would increase > contention on the `packed-refs` file and allow the deadlock that you > described, I lean towards leaving it as-is. Though admittedly, > contention over a loose reference lock could make the race more likely > to be hit. Agreed. It's a pretty unlikely sequence of events. And IMHO the real solution is a new storage format that's easier to reason about. -Peff
