threads / patch / 47773

patchfiles-backend: unlock packed store only if locked

Subject: [PATCH] files-backend: unlock packed store only if locked

## tl;dr

4 messages between Feb 6, 2018 and Feb 7, 2018. Diffs are folded; open one to read it.

replies: 3people: 3as markdown or json

Jonathan Tan· Feb 6, 2018, 20:36 UTC · lore

In commit 42c7f7ff9685 ("commit_packed_refs(): remove call to `packed_refs_unlock()`", 2017-06-23), a call to packed_refs_unlock() was added to files_initial_transaction_commit() in order to compensate for removing that call from commit_packed_refs(). However, that call was added in the cleanup section, which is run even if the packed_ref_store was never locked (which happens if an error occurs earlier in the function).

Create a new cleanup goto target which runs packed_refs_unlock(), and ensure that only goto statements after a successful invocation of packed_refs_lock() jump there.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
I noticed this when one of our servers sent duplicate refs in its ref
advertisement (noticed through GIT_TRACE_PACKET). With this change (and
before the aforementioned commit 42c7f7ff9685), the error message is
"fatal: multiple updates for ref '<ref>' not allowed", which gives a
bigger clue to the problem. Currently, it is "fatal: BUG:
packed_refs_unlock() called when not locked".

(I couldn't replicate this problem in C Git.)
---
 refs/files-backend.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Show changes to refs/files-backend.c +3 −2

diff --git a/refs/files-backend.c b/refs/files-backend.c
index f75d960e1..89bc5584a 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2931,13 +2931,14 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,
 
 	if (initial_ref_transaction_commit(packed_transaction, err)) {
 		ret = TRANSACTION_GENERIC_ERROR;
-		goto cleanup;
+		goto locked_cleanup;
 	}
 
+locked_cleanup:
+	packed_refs_unlock(refs->packed_ref_store);
 cleanup:
 	if (packed_transaction)
 		ref_transaction_free(packed_transaction);
-	packed_refs_unlock(refs->packed_ref_store);
 	transaction->state = REF_TRANSACTION_CLOSED;
 	string_list_clear(&affected_refnames, 0);
 	return ret;

-- 
2.16.0.rc1.238.g530d649a79-goog

Jeff King· Feb 7, 2018, 14:42 UTC · re: Jonathan Tan · lore

Re: [PATCH] files-backend: unlock packed store only if locked

On Tue, Feb 06, 2018 at 12:36:15PM -0800, Jonathan Tan wrote:

Show 11 quoted lines

> In commit 42c7f7ff9685 ("commit_packed_refs(): remove call to
> `packed_refs_unlock()`", 2017-06-23), a call to packed_refs_unlock() was
> added to files_initial_transaction_commit() in order to compensate for
> removing that call from commit_packed_refs(). However, that call was
> added in the cleanup section, which is run even if the packed_ref_store
> was never locked (which happens if an error occurs earlier in the
> function).
> 
> Create a new cleanup goto target which runs packed_refs_unlock(), and
> ensure that only goto statements after a successful invocation of
> packed_refs_lock() jump there.

The explanation and patch look good to me.

But this all seemed strangely familiar... I think this is the same bug as:

  https://public-inbox.org/git/20180118143841.1a4c674d@novascotia/

which is queued as mr/packed-ref-store-fix. It's listed as "will merge to next" in the "what's cooking" from Jan 31st.

Show 18 quoted lines

> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index f75d960e1..89bc5584a 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2931,13 +2931,14 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,
>  
>  	if (initial_ref_transaction_commit(packed_transaction, err)) {
>  		ret = TRANSACTION_GENERIC_ERROR;
> -		goto cleanup;
> +		goto locked_cleanup;
>  	}
>  
> +locked_cleanup:
> +	packed_refs_unlock(refs->packed_ref_store);
>  cleanup:
>  	if (packed_transaction)
>  		ref_transaction_free(packed_transaction);
> -	packed_refs_unlock(refs->packed_ref_store);

I actually like this double-label a bit more than what is queued on mr/packed-ref-store-fix, though I am OK with either solution.

-Peff

Jonathan Tan· Feb 7, 2018, 18:32 UTC · re: Jeff King · lore

Re: [PATCH] files-backend: unlock packed store only if locked

On Wed, 7 Feb 2018 09:42:51 -0500 Jeff King <peff@peff.net> wrote:

Show 7 quoted lines

> But this all seemed strangely familiar... I think this is the same bug
> as:
> 
>   https://public-inbox.org/git/20180118143841.1a4c674d@novascotia/
> 
> which is queued as mr/packed-ref-store-fix. It's listed as "will merge
> to next" in the "what's cooking" from Jan 31st.

Ah...thanks, I didn't notice that.

> I actually like this double-label a bit more than what is queued on
> mr/packed-ref-store-fix, though I am OK with either solution.

Same here.

Junio C Hamano· Feb 7, 2018, 20:53 UTC · re: Jonathan Tan · lore

Re: [PATCH] files-backend: unlock packed store only if locked

Jonathan Tan <jonathantanmy@google.com> writes:

Show 17 quoted lines

> On Wed, 7 Feb 2018 09:42:51 -0500
> Jeff King <peff@peff.net> wrote:
>
>> But this all seemed strangely familiar... I think this is the same bug
>> as:
>> 
>>   https://public-inbox.org/git/20180118143841.1a4c674d@novascotia/
>> 
>> which is queued as mr/packed-ref-store-fix. It's listed as "will merge
>> to next" in the "what's cooking" from Jan 31st.
>
> Ah...thanks, I didn't notice that.
>
>> I actually like this double-label a bit more than what is queued on
>> mr/packed-ref-store-fix, though I am OK with either solution.
>
> Same here.

I do agree that the double-label approach is more future-proof way, especially if we anticipate that there will be more code after the "attempt initial ref transaction commit" block before the packed-ref-store is unlocked. On the other hand, introduction of the locked_cleanup label can be done as part of such a change that adds new code that needs to be skipped, so I am OK with what is queued there.

Thanks, all.