Re: BUG: Race condition due to reflog expiry in "gc"

[Jeff King <peff@peff.net>]

On Wed, Mar 13, 2019 at 11:28:39AM +0100, Ævar Arnfjörð Bjarmason wrote:

> I was under the impression that git-gc was supposed to support operating
> on a repository that's concurrently being modified, as long as you don't
> set the likes of gc.pruneExpire too aggressively.

To some degree. If it has to take locks to modify items, then inherently
there's going to be lock contention. But we may be able to work around
it some.

A big one we used to hit at GitHub is that running `git pack-refs` needs
to take the individual ref locks when pruning loose refs that have just
been packed. We dealt with that by adding retry-with-timeout logic to
ref lock acquisition, in 4ff0f01cb7 (refs: retry acquiring reference
locks for 100ms, 2017-08-21). Since then, I can't remember seeing a
single instance of this coming up in production use.

I don't think we use a timeout with the reflog lock. Maybe we ought to.
It might need to be longer than the 100ms default for refs, since I
think we'd do more significant work during the reflog expiration. On the
other hand, I think we hold the lock on the ref itself, too, during that
expiration.

We don't actually expire reflogs regularly at GitHub, so I can't say one
way or the other if there would be lock contention there.

> Running a "gc" in a loop without "git reflog expire --all" and when
> watching the repository being GC'd with:
> 
>     fswatch -l 0.1 -t -r . 2>&1 | grep lock
> 
> We only create .git/MERGE_RR.lock, .git/gc.pid.lock and
> git/packed-refs.lock. These are all things that would only cause another
> concurrent GC to fail, not a normal git command.

The packed-refs.lock can conflict with a normal operation; regular
writers need to update it when they delete a ref.

You'd also be locking regular refs as part of "pack-refs --prune", but
you probably don't see it running gc in a loop, because all of the loose
refs are pruned after the first run.

> I'm just including that as illustration that add_reflogs_to_pending() in
> revision.c during "gc" already iterates over the reflogs without locking
> anything, but of course it's just reading them.

Right. It's always safe to read without locking (refs, packed-refs, and
reflogs).

> So just this fixes that:
> 
>     diff --git a/refs/files-backend.c b/refs/files-backend.c
>     index ef053f716c..b6576f28b9 100644
>     --- a/refs/files-backend.c
>     +++ b/refs/files-backend.c
>     @@ -3037,7 +3037,7 @@ static int files_reflog_expire(struct ref_store *ref_store,
>      	 * reference itself, plus we might need to update the
>      	 * reference if --updateref was specified:
>      	 */
>     -	lock = lock_ref_oid_basic(refs, refname, oid,
>     +	lock = lock_ref_oid_basic(refs, refname, NULL,
>      				  NULL, NULL, REF_NO_DEREF,
>      				  &type, &err);
>      	if (!lock) {
> 
> Which seems sensible to me. We'll still get the lock, we just don't
> assert that the refname we use to get the lock must be at that
> SHA-1. We'll still use it for the purposes of expiry.

I _think_ this should be OK, because we don't open the reflog until
after we hold the lock. So we don't really care what the value was at.
Wherever we got the lock, we'll do the expiration process atomically at
that point.

I don't think this makes all your problems go away, though, because
you'd still have the immediate contention of actually taking the lock
(at any value).

> But maybe I've missed some caveat in reflog_expiry_prepare() and friends
> and we really do need the reflog at that OID, then this would suck less:

If I'm reading the code correctly, we don't call reflog_expiry_prepare()
until we're holding the lock (because we pass it in as a callback to
reflog_expire()). So I think that would be OK.

>      	if (!lock) {
>     +		if (errno == EBUSY) {
>     +			warning("cannot lock ref '%s': %s. Skipping!", refname, err.buf);
>     +			strbuf_release(&err);
>     +			return -2;
>     +		}
>      		error("cannot lock ref '%s': %s", refname, err.buf);
>      		strbuf_release(&err);
>      		return -1;
> 
> I.e. we just detect the EBUSY that verify_lock() sets if the OID doesn't
> match, and don't prune that reflog.

That saves git-gc from failing. But do we have the problem in the other
direction? I.e., that the gc would take a lock, and the actual user
request to update a ref would fail?

That's what the retry-with-timeout is supposed to address, so maybe it
works. But I wouldn't be surprised if it's insufficient in practice,
since the reflog code may walk big parts of the graph under lock,
checking reachability.

-Peff