Re: [PATCH v4 27/28] reftable: fixup for new base topic 2/3

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Aug 30, 2021, 13:01 UTC
Message-ID: <87r1ebxe2y.fsf@evledraar.gmail.com>
In-Reply-To: <CAFQ2z_MnJr58Cu970Be7SXE1jFkgOGThQ0Geq2+kNihJE3_BvQ@mail.gmail.com>

On Mon, Aug 30 2021, Han-Wen Nienhuys wrote:

Show 25 quoted lines

> On Mon, Aug 23, 2021 at 2:13 PM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>>
>> Since my "refs API: remove OID argument to reflog_expire()" we don't
>> have the "oid" as part of the reflog_expire() signature. Instead the
>> reflog_expire() should pass the OID of the tip of the "locked" ref to
>> the prepare_fn().
>>
>> In files_reflog_expire() we do that by getting the OID from
>> lock_ref_oid_basic(). I'm assuming (but am not familiar enough with
>> reftable...) that by the time we get here we've got a locked ref
>> already in some way, so let's just use
>> refs_resolve_ref_unsafe_with_errno() to lookup the current OID of that
>> presumably-locked ref.
>
> I quickly looked at the files code, but I don't understand why the OID
> needs to be passed-in (before your refactoring): in builtin/reflog.c
> (before), the current OID is read, with any protection. This means
> that its value can't be trusted.
>
> After your refactoring, you lock the ref. I guess in the files backend
> this protects against non-atomic update of (ref, reflog) racing with a
> concurrent reflog expiry? In reftable, the (ref,reflog) update is
> atomic, so there is no need for locking to properly sequence
> operations.

Before my [1] we'd do:

 1. Read the OID for the branch in builtin/reflog.c
 2. Pass it to refs/files-backend.c, it would lock at that OID (or fail if it changed)
 3. Pass the OID with the now-locked OID to the builtin/reflog.c code

After that [1] we do:

 1. Lock the branch in refs/files-backend.c
 2. Pass the OID with the now-locked OID to the builtin/reflog.c code
    (ditto #3 above)

Whatever reftable itself does with updates doesn't change that we need to do that 2nd step of passing the OID to builtin/reflog.c, as it makes use of it.

That code is a bit confusing, if you want to understand it I recommend reading it at the tip of my yet-unsubmitted avar/cleanup-refs-api-and-reflog-expire-post-no-eisdir, it makes the control flow a lot cleaner.

So as far as what we do here is concerned, we're stuck with the refs files backend inherently wanting to pass "I locked this for you, here's the OID".

I guess it could also pass "OK, now go ahead and expire" and pass no OID. We'd then in builtin/reflog.c lookup the current OID for the logic there, but just having the reftable backend appease the common API by looking up the OID and passing it seemed like the most straightforward thing to do.

I haven't tested this or thought it through, but I don't understand how reftable isn't going to race in reflog expiry then. Sure, the ref/reflog update itself is atomic, so it won't suffer from the needing-a-lock problem of two concurrent file backend writers doing the equivalent of:

    echo $NEW_SHA1 >.git/refs/heads/some-branch

But we will need at least the optimistic locking of code like builtin/reflog.c wanting to do an expiry, and deciding whether to do that expiry based on a given state of the ref/reflog. I.e. we don't want:

    1. Start reflog expiry
    2. Code in builtin/reflog.c looks up the OID
    3. Code in builtin/reflog.c decides whether expire the reflog
    4. Concurrent with #4, another writer updates the ref/reflog pair
    5. Code in builtin/reflog.c says "OK, expire it!"
    6. Reftable queues a delete/prune of the reflog per #5.

This would be a sequente of updates to the ref/reflog, none of whom were racy as far as the reftable semantics itself are concerneb, but where we'd do the wrong thing because the writer thought we had A when we really had B. So we need the equivalent of an "git update-ref" with the "<oldvalue>".

Is there a better way to do that in this case that I'm missing?

1. https://lore.kernel.org/git/patch-v5-09.13-aba12606cea-20210823T113115Z-avarab@gmail.com/
2. https://github.com/avar/git/compare/avar/files-backend-remove-dead-errno-eisdir-6...avar:avar/cleanup-refs-api-and-reflog-expire-post-no-eisdir