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