patch, 5 partswrite_ref_sha1(): inline function at callers

In ref_transaction_commit(), close the reference lockfiles immediately to avoid keeping too many file descriptors open at a time. This is pretty easy, because in the first loop (where we create the locks) we already know what, if anything, has to be written into the lockfile. So write it and close the lockfile immediately. In the second loop, rename the lockfiles for reference updates into place, and in the cleanup loop, unlock any references that are still locked (i.e., those that were only being verified or deleted).

I think this is a cleaner solution than Stefan's approach [1] of closing and reopening fds based on an estimate of how many fds we can afford to waste--we only need a single file descriptor at a time, and we never have to close then reopen a lockfile. But it is a bit more intrusive, so it might still be preferable to use Stefan's approach for release 2.4.0, if indeed any fix for this problem is still being considered for that release.

This patch series applies on top of Stefan's

    c1f0ca9 refs.c: remove lock_fd from struct ref_lock (2015-04-16)

and it fixes two tests that Stefan introduced earlier in that series.

It is also available from my GitHub account:

    https://github.com/mhagger/git branch close-ref-locks-promptly

Michael

[1] http://article.gmane.org/gmane.comp.version-control.git/267548

Michael Haggerty (5):
  write_ref_to_lockfile(): new function, extracted from write_ref_sha1()
  commit_ref_update(): new function, extracted from write_ref_sha1()
  write_ref_sha1(): inline function at callers
  ref_transaction_commit(): remove the local flags variables
  ref_transaction_commit(): only keep one lockfile open at a time

 refs.c                | 107 ++++++++++++++++++++++++++++++++++----------------
 t/t1400-update-ref.sh |   4 +-
 2 files changed, 75 insertions(+), 36 deletions(-)

This is the first step towards separating the checking and writing of the new reference value to committing the change.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c | 38 +++++++++++++++-----------------------
 1 file changed, 15 insertions(+), 23 deletions(-)

Instead, work directly with update->flags. This has the advantage that the REF_DELETING bit, set in the first loop, can be read in the third loop instead of having to compute the same expression again. Plus, it was kindof confusing having both update->flags and flags, which sometimes had different values.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

On Fri, Apr 24, 2015 at 01:35:48PM +0200, Michael Haggerty wrote:

Hmm. I think this is losing the distinction of "flags the caller has passed in to us" versus "flags we are using locally only during the transaction_commit routine". If callers look at the flags in the REF_TRANSACTION_CLOSED state, do they care about seeing these new flags?

My guess is probably not in practice, and "leaking" these flags is an acceptable tradeoff for keeping the transaction_commit function simpler. But I haven't looked that closely.

-Peff

On 04/24/2015 07:30 PM, Jeff King wrote:

"struct ref_update" is opaque to callers outside of the refs module, and ref_update::flags is not read anywhere outside of ref_transaction_commit() (and its value is passed to lock_ref_sha1_basic()). So I don't think we have to be shy about storing our own internal information there.

In fact, REF_DELETING, REF_ISPRUNING, REF_HAVE_NEW, and REF_HAVE_OLD are also private to the refs module.

I suppose we could mask out all the "private" bits in the flags parameter passed by the caller, to make sure that the caller hasn't accidentally set other bits. I think that would be more defensive than our usual practice, but I don't mind doing it if people think it would be prudent.

Michael

On Fri, Apr 24, 2015 at 11:15:09PM +0200, Michael Haggerty wrote:

Thanks for checking. If nobody is affected (and is not likely to be), I agree it's not worth worrying about.

I don't think it's necessary.

-Peff

On Fri, Apr 24, 2015 at 7:35 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:

s/kindof/kind of/

On 04/24/2015 11:51 PM, Eric Sunshine wrote:

Hehe thanks for looking over my scribblings. In this case, neither "kindof" nor "kind of" is in fact correct English. I used the slang word "kindof" (sometimes spelled "kinda") to mean "somewhat", I guess because "somewhat" sounded too formal for my slapdash opinion.

But I suppose it's kindof thoughtless to use slang in commit messages :-), especially given that they are also meant for people for whom English is a second language (let alone sloppy American English).

I suggest s/kindof/potentially/, at least until I have time to submit a patch to the English language to make "kindof" a reputable word :-)

Michael

The old code locked all of the references in the first loop, leaving all of the lockfiles open until later loops. Since we might be updating a lot of references, this could cause file descriptor exhaustion.

But there is no reason to keep the lockfiles open after the first loop:

* For references being deleted or verified, we don't have to write
  anything into the lockfile, so we can close it immediately.

* For references being updated, we can write the new SHA-1 into the
  lockfile immediately and close the lockfile without renaming it into
  place. In the second loop, the already-written contents can be
  renamed into place using commit_ref_update().

To simplify the bookkeeping across loops, add a new REF_NEEDS_COMMIT bit to update->flags, which keeps track of whether the corresponding lockfile needs to be committed, as opposed to just unlocked.

This change fixes two tests in t1400.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c                | 68 +++++++++++++++++++++++++++++++++++++--------------
 t/t1400-update-ref.sh |  4 +--
 2 files changed, 51 insertions(+), 21 deletions(-)

On 04/24/2015 01:35 PM, Michael Haggerty wrote:

I realize that I should have highlighted another, perhaps more serious bug that is fixed by this change. The old code was roughly

    for update in updates:
        acquire locks and check old_sha
    for update in updates:
        if changing value:
            write_ref_to_lockfile()
            commit_ref_update()
    for update in updates:
        if deleting value:
            unlink()
    for update in updates:
        if reference still locked:
            unlock_ref()

The atomicity of the update depends on all preconditions being checked in the first loop, before any changes have start being committed. The problem is that write_ref_to_lockfile() (which used to be part of write_ref_sha1()), which happens in the second loop, checks some more preconditions:

* It verifies that new_sha1 is a valid object
* If the reference being updated is a branch, it verifies that new_sha1
  points at a commit object (as opposed to a tag, tree, or blob).

If either of these preconditions fails, the "transaction" might be aborted after some reference updates have already been permanently committed. In other words, the all-or-nothing promise of "git update-ref --stdin" and "git push --atomic" would be violated.

I'm sorry that I didn't fully realize this problem earlier.

After this patch, the code looks like

    for update in updates:
        acquire locks and check old_sha
        if changing value:
            write_ref_to_lockfile()
    for update in updates:
        if changing value:
            commit_ref_update()
    for update in updates:
        if deleting value:
            unlink()
    for update in updates:
        if reference still locked:
            unlock_ref()

This has the important effect that the pre-checks in write_ref_to_lockfile() are carried out before any changes have been committed, so if any of those checks fail, the transaction can be rolled back correctly.

Given that "git push --atomic" is one of the main new features of 2.4.0, it would be unfortunate for the release to contain this bug, plus the bug that atomic pushes can fail due to file descriptor exhaustion.

Is this problem important enough to justify delaying the release to get the fix in?

Michael

Michael Haggerty <mhagger@alum.mit.edu> writes:

I am not too worried about "push --atomic", as we can just add a few words to Release Notes and documentation saying "this is still an experimental broken code that is unusable; don't use the feature in production".

I however am more worried about the other one "update-ref --stdin"; the change will be pure regression for those who want to do many updates and do not care if the update is atomic, no?

Unfortuntely it is pretty late in the game, and even though in principle I know that the only sensible way forward is to revert the original breakage, I find it very tempting to patch it up with your series.

Junio C Hamano <gitster@pobox.com> writes:

I should have refrained from touching the keyboard so late at night X-<. This regression was done long time ago (even in v2.1.0 I see that ref_transaction_commit() tries to grab all locks at once).

So it is only "push --atomic".

The choice is between (1) shipping "push --atomic" that is known to be broken, (2) applying your five-patch series which may (a) fix both "push --atomic" and "update-ref --stdin", or (b) break other transaction users including "update-ref -stdin" in unexpected ways.

I dunno. I am still tempted to go route (2) hoping that it would result in (2-a) not (2-b).

On Sat, Apr 25, 2015 at 12:21:07PM -0700, Junio C Hamano wrote:

I'm not sure "--atomic" is foolproof even with these patches.

Certainly there is the obvious problem that the filesystem is not atomic (so a power loss halfway through committing the locks will result in a non-atomic push). But there are also subtle locking issues related to D/F conflicts.

For instance, if we do (and I take no credit for this discovery; Michael showed it to me last week):

  {
    echo "create refs/heads/branch $sha1"
    echo "create refs/heads/foo/bar $sha1" &&
    echo "create refs/heads/foo $sha1"
  } | git update-ref --stdin

we do not notice the conflict between "foo" and "foo/bar" until the commit stage. Fortunately our ref-ordering rules mean we will always try to write "foo" before "foo/bar", which will fail because "foo" must be a directory (because we have "foo/bar.lock"). But because "branch" comes alphabetically first, we will have written that ref already, committing part of the request.

You can work around this with code to check for D/F conflicts in a single transaction, but then you are subject to races with other processes. E.g., one process writes "branch" and "foo", the other is simultaneously writing "foo/bar". There is no lock contention, but the write of "foo" may fail at the commit stage.

So I think it is OK to restrict the definition of "atomic" here to the common case of making sure that we can lock and check the $old_sha1 for each of the refs. Under that definition, "push --atomic" is not broken as-is, but there is room for improving its robustness. And I have no problem with trying to make our storage format as robust as possible in these cases, but I think we need to admit that the filesystem ref storage is never going to be 100% transactional.

I know that sounds a bit like moving the goalposts to say "eh, this feature is not broken, your definition of broken is just wrong". But my point is that shipping "push --atomic" as-is is still a useful step forward, and we can continue to iterate and improve on the concept in future releases.

-Peff

On Fri, Apr 24, 2015 at 01:35:44PM +0200, Michael Haggerty wrote:

I like this approach much better. It seems like the best of all worlds (same performance, and we don't have to worry about whether and when to close lockfiles).

Stefan's patch is just in pu at this point, right? I do not think there is any rushing/release concern. It is too late for either to be in v2.4.0, so the only decision is whether to aim for "master" or "maint". To me, they both seem to be in the same ballpark as far as risking a regression.

-Peff

On Fri, Apr 24, 2015 at 10:26 AM, Jeff King <peff@peff.net> wrote:

I would have guessed this approach to take more work to do it right. Thanks Michael for tackling the problem in an elegant way!

Yeah it's in pu, so it's easy to remove.

Thanks, Stefan

Jeff King <peff@peff.net> writes:

The series builds on c1f0ca9 that is on 2.4.0-rc2, so we would need to wiggle things around to apply to older codebase if we want to fix the "too many open file descriptor" issue on older maintenance releases for "update-ref --stdin".

I personally feel that it is OK to ship v2.4.0 without the fix, leaving "push --atomic" broken, and fix it in v2.4.1, but I kinda prefer that the final fix to be applicable for older maintenance releases, at least to 2.3.x track, if not 2.2.x track.

Michael Haggerty <mhagger@alum.mit.edu> writes:

Nicely analysed and beautifully executed.  I like it.

Thanks.