Re: BUG: Race condition due to reflog expiry in "gc"
- From
Jeff King <peff@peff.net>
- Date
- Mar 13, 2019, 16:02 UTC
- Message-ID
- <20190313160204.GD24101@sigill.intra.peff.net>
- In-Reply-To
- <87sgvrax0o.fsf@evledraar.gmail.com>
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.
Show 8 quoted lines
> 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).
Show 19 quoted lines
> 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.
Show 12 quoted lines
> 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