Re: [PATCH] repack: respect gc.pid lock

From: Jeff King <peff@peff.net>
Date: Apr 14, 2017, 19:33 UTC
Message-ID: <20170414193341.itr3ybiiu2brt63b@sigill.intra.peff.net>
In-Reply-To: <20170413202712.22192-1-dturner@twosigma.com>

On Thu, Apr 13, 2017 at 04:27:12PM -0400, David Turner wrote:

Show 12 quoted lines

> Git gc locks the repository (using a gc.pid file) so that other gcs
> don't run concurrently. Make git repack respect this lock.
> 
> Now repack, by default, will refuse to run at the same time as a gc.
> This fixes a concurrency issue: a repack which deleted packs would
> make a concurrent gc sad when its packs were deleted out from under
> it.  The gc would fail with: "fatal: ./objects/pack/pack-$sha.pack
> cannot be accessed".  Then it would die, probably leaving a large temp
> pack hanging around.
> 
> Git repack learns --no-lock, so that when run under git gc, it doesn't
> attempt to manage the lock itself.

This also means that two repack invocations cannot run simultaneously, because they want to take the same lock. But depending on the options, the two don't necessarily conflict. For example, two simultaneous incremental "git repack -d" invocations should be able to complete.

Do we know where the error message is coming from? I couldn't find the error message you've given above; grepping for "cannot be accessed" shows only error messages that would have "packfile" after the "fatal:". Is it a copy-paste error?

If that's the case, then it's the one in use_pack(). Do we know what program/operation is causing the error? Having a simultaneous gc delete a packfile is _supposed_ to work, through a combination of:

  1. Most sha1-access operations can re-scan the pack directory if they
     find the packfile went away.

  2. The pack-objects run by a simultaneous repack is somewhat special
     in that once it finds and commits to a copy of an object in a pack,
     we need to use exactly that pack, because we record its offset,
     delta representation, etc. Usually this works because we open and
     mmap the packfile before making that commitment, and open packfiles
     are only closed if you run out of file descriptors (which should
     only happen when you have a huge number of packs).

So I'm worried that this repack lock is going to regress some other cases that run fine together. But I'm also worried that it's a band-aid over a more subtle problem. If pack-objects is not able to run alongside a gc, then you're also going to have problems serving fetches, and obviously you wouldn't want to take a lock there.

-Peff