On Mon, Apr 14, 2014 at 11:36 PM, Michael Haggerty <mhag...@alum.mit.edu> wrote:
> 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.

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.

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

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
> mhag...@alum.mit.edu
> http://softwareswirl.blogspot.com/
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to