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

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Apr 15, 2014, 06:36 UTC
Message-ID: <534CD376.7080108@alum.mit.edu>
In-Reply-To: <1397500163-7617-1-git-send-email-sahlberg@google.com>

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

Show 9 quoted lines

> 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.

Show 18 quoted lines

> 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/