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

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

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/