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
[email protected]
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
