Re: [PATCH 2/2] lock_packed_refs(): allow retries when acquiring the packed-refs lock

From: Jeff King <peff@peff.net>
Date: May 1, 2015, 18:22 UTC
Message-ID: <20150501182257.GA27728@peff.net>
In-Reply-To: <CAGZ79kZnhv+aW_GW8mBDyhfv_k54ScAFsHQz=8zfHBUJ8WrVUw@mail.gmail.com>

On Fri, May 01, 2015 at 10:51:47AM -0700, Stefan Beller wrote:

Show 20 quoted lines

> > diff --git a/refs.c b/refs.c
> > index 47e4e53..3f8ac63 100644
> > --- a/refs.c
> > +++ b/refs.c
> > @@ -2413,9 +2413,19 @@ static int write_packed_entry_fn(struct ref_entry *entry, void *cb_data)
> >  /* This should return a meaningful errno on failure */
> >  int lock_packed_refs(int flags)
> >  {
> > +       static int timeout_configured = 0;
> > +       static int timeout_value = 1000;
> 
> I'd personally be more happier with a default value of 100 ms or less
> The reason is found in the human nature, as humans tend to perceive
> anything faster than 100ms as "instant"[1], while a 100ms is a long time
> for computers.
> 
> Now a small default time may lead to to little retries, so maybe it's worth
> checking at the very end of the time again (ignoring the computed backoff
> times). As pushes to $server usually take a while (connecting, packing packs,
> writing objects etc), this may be overcautios bikeshedding from my side.

Keep in mind that this 1s is the maximum time to wait. The lock_file_timeout() code from patch 1 starts off at 1ms, grows quadratically, and quits as soon as it succeeds. So in most cases, the user will wait a much smaller amount of time.

The factors that go into this timeout length are really:

  1. If there's a stale lockfile, the user will have to wait the whole
     period. How long do we keep retrying before giving up?

  2. How long do we typically hold the lock for? Aside from absurd
     cases, writing out the packed-refs file isn't that expensive. But
     while holding the packed-refs lock, we may actually be iterating
     the loose refs, which can be rather slow on a cold-cache.

If we want to improve _responsiveness_ in the normal case, I think it's not the max timeout we want to tweak but the resolution of retries. That's set in patch 1 by the maximum backoff multiplier, which can put us up to 1s between retries. It might make sense to drop that to 500ms or even less.

-Peff