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

On Mon, Apr 14, 2014 at 11:36 PM, Michael Haggerty [off-list ref] wrote:

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

quoted

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.

Yes.
This patch series is mainly focused on making ref_transaction_commit()
more atomic
so that it does more checks for failures before it starts doing
irreversible changes to the underlying files.

These patches change ref_transaction_commit() to do even more checks
before it starts applying the
actual changes to disk.


There are also changes to other users of write_ref_sha1() too but
those are mainly just to
reflect that this function no longer actually does the changes to the
underlying files any more.

quoted

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 think you are right.

Lets put this patch series on the backburner for now and start by
making all callers use transactions
and remove write_ref_sha1() from the public API thar refs.c exports.

Once everything is switched over to transactions I can rework this
patchseries for ref_transaction_commit()
and resubmit to the mailing list.


Lets start preparing patches to change all external callers to use
transactions instead.
I am happy to help preparing patches for this. How do we ensure that
we do not create duplicate work
and work on the same functions?


regards
ronnie sahlberg

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/