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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Wed, Mar 13 2019, Jeff King wrote:

> 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).

Yeah, but as far as I can tell core.filesRefLockTimeout added in
4ff0f01cb7 (refs: retry acquiring reference locks for 100ms, 2017-08-21)
solves that.

I.e. the problem after that patch isn't the contention, but the call to
lock_ref_oid_basic() in files_reflog_expire() above which *after* we get
the lock effectively asserts that the value must be at the old OID.

It would then hard-die, so git-reflog would leave behind stay *.lock
files that e.g. a concurrent "commit" would die on (still there after
100ms, and beyond). Whereas if we're not picky about the OID the two
will have contention, but core.filesRefLockTimeout will work around it.

I've been running this whole thing in a loop for a while with that
s/oid/NULL/ fix, and it just works. To the extent that it doesn't (due
to load) I can just bump core.filesRefLockTimeout up a bit.

So with the benefit of some sleep & your comment I'm pretty sure that's
the right fix. Ideally it would have a test, do we have some simliar
contention-creating tests I can piggy-back on, or maybe we should just
change this and hope for the best...

>> 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?

No, because as with s/oid/NULL/ above core.filesRefLockTimeout saves the
day.

> 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.

I suppose this can happen if the reflog for a given branch is so big
that it takes us more than 100ms to parse it, but e.g. O(n) refs
shouldn't matter, since we only hold the lock on one ref at a time.