Re: [PATCH v4 00/32] Lockfile correctness and refactoring

[Jeff King <peff@peff.net>]

On Sat, Sep 06, 2014 at 09:50:14AM +0200, Michael Haggerty wrote:

> Sorry for the long delay since v3. This version mostly cleans up a
> couple more places where the lockfile object was left in an
> ill-defined state. Thanks to Johannes Sixt and Torsten Bögershausen
> for their review of v3.
> 
> I believe that this series addresses all of the comments from v1 [1],
> v2 [2], and v3 [3].

This looks pretty good to me overall.

I did coincidentally have an interesting experience with our lockfile
code earlier today, which I'd like to relate.

I was running pack-refs on a repository with a very large number of
loose refs (about 1.8 million). Needless to say, this ran very slowly
and thrashed the disk, as that's almost 7G using 4K inodes. But it did
eventually generate a packed-refs file, at which point it tried to prune
the loose refs.

To do so, we have to lock each ref before removing it (to protect
against a simultaneous update). Each call to lock_ref_sha1_basic
allocates a "struct lock_file", which then gets added to the global
lock_file list. Each one contains a fixed PATH_MAX buffer (4K on this
machine). After we're done updating the ref, we leak the lock_file
struct, since there's no way to remove it from the list.

As a result, git tried to allocate 7G of RAM and got OOM-killed (the
machine had only 8G). In addition to thrashing the disk even harder,
since there was no room left for disk cache while we touched millions of
loose refs. :)

Your change in this series to use a strbuf would make this a lot better.
But I still wonder how hard it would be to just remove lock_file structs
from the global list when they are committed or rolled back. That would
presumably also make the "avoid transitory valid states" patch from your
series a bit easier, too (you could prepare the lockfile in peace, and
then link it in fully formed, and do the opposite when removing it).

I think with your strbuf patch, this leak at least becomes reasonable.
So maybe it's not worth going further. But I'd be interested to hear
your thoughts since you've been touching the area recently.

-Peff