Re: [RFC/PATCH] gc: run more pre-detach operations under lock

On Thu, Jun 20 2019, Duy Nguyen wrote:

I don't mean we can't ever change the algorithm, but that we've documented:

    When common porcelain operations that create objects are run, they
    will check whether the repository has grown substantially since the
    last maintenance[...]

The "fetch" command is a common porcelain operation, when it fetches from N remotes it just runs an invocation of itself, so thus far it's both worked & been intuitive that if we needed (potentially multiple) gc's while doing that we'd just go ahead and run it then, even if something concurrent was happening.

No that's not optimal in many cases, but at least doesn't create caveats we don't have now where we have runaway object growth.

At least for me it doesn't require custom tuning before this patch of yours.

I.e. now "gc --auto" is dumb enough that you can run it on everything from stuff that just does "commit" from cron, user's laptops, massive rebases that take forever, and e.g. "stats" like jobs where for <reasons> I'll add thousands of repos and "fetch --all" them (so e.g. I can run "log --author=<x> --all").

Yeah of course I'm an advanced user and I can just grumble and manually invoke fetch, actually I'll probably submit a follow-up patch to add a gc.* config to disable this thing.

But I think even if the use-case is rather obscure it's important that if at all possible we keep "gc" elastic enough to work for pretty much all combinations of object-adding porcelain commands, and I think in this case we're better off doing things differently...

Even a user with two remotes will run into issues with your patch where "gc" will print things twice, or outright error due to concurrent access by another process, which as has been discussed here on-list is *very* common e.g. with editor integration.

So the extent of my complains about this is:

 1) The edge case with runaway object growth (obscure)

 2) It doesn't really fix the bug except for the narrow case of users
    who invoke things one-terminal-at-a-time and don't e.g. have an
    editor with "git" integration & a terminal (not obscure).

Maybe I should have led with #2 :)

Anyway, I'm not *just* complaining. I have patches too, but so far I'm up to 8 patches on what's probably just the first one-third of it. *Sigh*.

But the 2/3 of that if you want to dig through my crappy WIP code on GitHub is instrumenting the test suite to demonstrate that with an approach like what your patch does we still get these GC race conditions, because our locking still sucks, which brings me to...

I've poked at it a bit now, and it's really not hard, I think it's just that nobody looked at it hard enough before.

The issue is that currently we do:

    1. parent: do_we_need_gc();
    2. parent: say_way_will_gc();
    3. parent: lock();
    4. parent: do_a_bit_of_work();
    5. parent: unlock();
    6. parent: fork();
    7. child: lock();
    8. child: do_a_lot_of_work();
    9. child: unlock();

My RFC patch currently changes that to:

    1. parent: do_we_need_gc();
    2. parent: lock_or_silently_exit();
    3. parent: say_way_will_gc();
    4. parent: do_a_bit_of_work();
    5. parent: unlock();
    6. parent: fork();
    7. child: lock();
    8. child: do_a_lot_of_work();
    9. child: unlock();

I.e. we won't duplicate the message, but *do* introduce the caveat that it's in principle possible nobody gc's, but in practice when we fail to get the lock in lock_or_silently_exit() it's because we lost the race to a sister process that's going to do the actual GC, so all is well.

But we are left with the brief race when we fork. My RFC patch proposed some elaborate PID hand-over dance to deal with this. But having looked at it again I think we can easily get rid of that race too with just:

    1. parent: do_we_need_gc();
    2. parent: lock_or_silently_exit();
    3. parent: say_way_will_gc();
    4. parent: do_a_bit_of_work();
    5. parent: fork();
    6. parent: <writes child pid to gc.lock>
    7. parent: <exits without unlocking gc.lock>
    8. child: do_a_lot_of_work();
    9. child: unlock();

Which can fail in cases where the child segfaults, or manages to exit earlier than the parent etc, hence my earlier proposed elaborate pid hand-over dance.

But looking at it again we only usurp an existing gc.lock if the mtime is >12hrs, so it's OK if we have very rare cases where the PID info got corrupted, we can still back ourselves out of it, which is what I was paranoid about.

Furthermore the gc.lock contains the <hostname><pid> of the working process, but we can in a backwards-compatible way add new entries to that file, i.e. list both the child & parent pid. Older clients will just read whichever one comes first, but if we make new versions check both we can be more paranoid going forward.