Re: [PATCH 08/10] files_ref_store: use a transaction to update packed refs

[Jeff King <peff@peff.net>]

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