Re: [PATCHv2] refs.c: enable large transactions

From: Stefan Beller <sbeller@google.com>
Date: Apr 22, 2015, 19:09 UTC
Message-ID: <CAGZ79kbb=UUVq8np-ndiMS_id5GaBHpDafDe9ZB3TvsK3xYe_g@mail.gmail.com>
In-Reply-To: <5537AC00.5060208@alum.mit.edu>

On Wed, Apr 22, 2015 at 7:11 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:

>> +     if (lock->lk->fd == -1)
>> +             reopen_lock_file(lock->lk);
>
> You should check that reopen_lock_file() was successful.

ok

Show 11 quoted lines

>> @@ -3762,6 +3779,10 @@ int ref_transaction_commit(struct ref_transaction *transaction,
>>                                   update->refname);
>>                       goto cleanup;
>>               }
>> +             if (remaining_fds > 0)
>> +                     remaining_fds--;
>> +             else
>> +                     close_lock_file(update->lock->lk);
>
> I consider this code a stopgap, and simplicity is more important than
> optimization.

Can you explain a bit why you think this is a stopgap?

Looking at the patch this looks simple to me, as there are no huge pain points involved. (Compared to [1] as we'd change a lot in that series)

Also this is pretty good on performance. The small cases do not have an additional unneeded close and reopen, but only the larger cases do.

[1] http://git.661346.n2.nabble.com/PATCHv1-0-6-Fix-bug-in-large-transactions-tt7624363.html#a7624368

> But just for the sake of discussion, if we planned to keep
> this code around, it could be improved by not wasting open file
> descriptors for references that are only being verified or deleted, like so:

I'll pick that up for the resend.