Re: [PATCH v2 0/8] Multiple simultaneously locked ref updates

[Michael Haggerty <mhagger@alum.mit.edu>]

On 08/30/2013 08:11 PM, Brad King wrote:
> Here is the second revision of a series to support locking multiple
> refs at the same time to update all of them consistently.  The first
> series can be found at $gmane/233260.  This revision is ready to
> consider for integration.

I'm very interested in this area and I regret that I have been so
invisible lately.  I definitely like the way your changes are going.

I have been doing some work in the same neighborhood and our work
overlaps somewhat.  Namely, it is incorrect that delete_ref() currently
deletes the loose ref before rewriting packed-refs, because it can cause
a simultaneous reader to see the packed value for a moment in time, and
the packed value might not even point to a valid object anymore.  In
fact, the current version is even worse: it deletes the loose reference
before even locking the packed-refs file, so if the packed refs file
cannot be rewritten (e.g., because it is locked by another process) then
the reference is permanently left in a corrupt state.

On the other hand, writing the packed-refs file first would also be
incorrect, because a pack-refs process could jump in before the loose
refs file was deleted, read the loose value, and write it to the new
packed-refs file.  The result would be that the first process would
think that it had deleted the reference but it would still exist (not
such a catastrophe, but incorrect nevertheless).

The solution that I have been working on is to first lock *both* the
loose and packed refs files, then rewrite the packed-refs file *but
retain a lock on it*, then rewrite the loose-ref file and release its
lock, then release the lock on the packed-refs file.  By retaining the
lock on the packed-refs file during the whole "transaction", we prevent
another process from trying to pack the refs before our reference is
completely deleted.  This requires the file-locking API to be enhanced
to allow a file to be replaced by its new version while still retaining
a lock on the file.

Your code has the same bug as the original (it's not your fault!) so I
think it will eventually have to be fixed to look something like

    acquire lock on packed-refs

    for reference in ref_updates:
        lock reference
        if old sha1 known:
            verify old sha1 is still current

    for reference in ref_updates:
        if reference should be created/modified:
            modify reference
            release lock on reference

    delete references from packed-refs file and activate new
            version of the file *but retain a lock on the
            packed-refs file*
    for reference in ref_updates:
        if reference should be deleted:
            delete loose version of reference
            release lock on reference

    release lock on packed-refs file

This is really all just for your information; there is certainly no
obligation for you to fix this pre-existing problem.  And I'm working on
it anyway; if you happen to be interested you can view my current
work-in-progress on GitHub (though it still doesn't work!):

    https://github.com/mhagger/git/tree/WIP-delete-ref-locking

Feedback would of course be welcome.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/