On Mon, Apr 14, 2014 at 11:36 PM, Michael Haggerty <mhagger@xxxxxxxxxxxx> 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.
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.
>
>> 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@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