Re: [PATCH v4 0/3] Make update refs more atomic

On 04/14/2014 08:29 PM, Ronnie Sahlberg wrote:

refs.c:ref_transaction_commit() intermingles doing updates and checks with
actually applying changes to the refs in loops that abort on error.
This is done one ref at a time and means that if an error is detected that
will fail the operation partway through the list of refs to update we
will end up with some changes applied to disk and others not.

Without having transaction support from the filesystem, it is hard to
make an update that involves multiple refs to guarantee atomicity, but we
can do a somewhat better than we currently do.

It took me a moment to understand what you were talking about here,
because the code for ref_transaction_commit() already seems
superficially to do reference modifications in phases.  The problem is
that write_ref_sha1() internally contains additional checks that can
fail in "normal" circumstances.  So the most important part of this
patch series is allowing those checks to be done before committing anything.

These patches change the update and delete functions to use a three
call pattern of

1, lock
2, update, or flag for deletion
3, apply on disk  (rename() or unlink())

When a transaction is commited we first do all the locking, preparations
and most of the error checking before we actually start applying any changes
to the filesystem store.

This means that more of the error cases that will fail the commit
will trigger before we start doing any changes to the actual files.


This should make the changes of refs in refs_transaction_commit slightly
more atomic.
[...]

Yes, this is a good and important goal.

I wonder, however, whether your approach of changing callers from

    lock = lock_ref_sha1_basic() (or varient of)
    write_ref_sha1(lock)

to

    lock = lock_ref_sha1_basic() (or varient of)
    write_ref_sha1(lock)
    unlock_ref(lock) | commit_ref_lock(lock)

is not doing work that we will soon need to rework.  Would it be jumping
the gun to change the callers to

    transaction = ref_transaction_begin();
    ref_transaction_{update,delete,etc}(transaction, ...);
    ref_transaction_{commit,rollback}(transaction, ...);

instead?  Then we could bury the details of calling write_ref_sha1() and
commit_lock_ref() inside ref_transaction_commit() rather than having to
expose them in the public API.

I suspect that the answer is "no, ref transactions are not yet powerful
enough to do everything that the callers need".  But then I would
suggest that we *make* them powerful enough and *then* make the change
at the callers.

I'm not saying that we shouldn't accept your change as a first step [1]
and do the next step later, but wanted to get your reaction about making
the first step a bit more ambitious.

Michael

[1] Though I still need to review your patch series in detail.

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