[PATCH 00/29] Yet more preparation for reference backends

classic Classic list List threaded Threaded
70 messages Options
1234
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 19/29] refs: don't dereference on rename

Michael Haggerty-2
On 04/29/2016 02:12 PM, Jeff King wrote:

> On Fri, Apr 29, 2016 at 12:57:29PM +0200, Michael Haggerty wrote:
>
>> Remember, we're talking about rename_ref() only, not reference deletion
>> in general. rename_ref() is not very robust anyway--it doesn't happen in
>> a single transaction, and it is vulnerable to being defeated by
>> simultaneous reference updates by other processes. It *does* wrap the
>> deletion of newrefname in a transaction; the only question is whether an
>> old_sha1 is supplied to that transaction.
>>
>> So, suppose that newrefname starts at value A, and there are two updates
>> running simultaneously:
>>
>> 1. An update of reference newrefname from A -> B
>>
>> 2. A rename of reference oldrefname to newrefname, which includes
>>    a. read_ref_full("newrefname") and
>>    b. delete_ref("newrefname").
>
> I wondered at first if you meant "oldrefname" in 2b. That is, a rename
> would involve writing to "newrefname" and then deleting "oldrefname",
> and not doing the old_sha1 and normal locking on the deletion of
> oldrefname would be bad (in case somebody else updated it while we were
> operating).
>
> But reading the patch again, that's not what's going on. You're talking
> just about the case where we force-overwrite an existing newrefname, and
> delete it first to do so. But what I don't understand is:

It's beyond the ambition of this patch to fix this old rename_ref()
code, but...

>   1. Why do we read_ref_full() at all? Since it is not done under lock
>      anyway, it is useless for helping with race conditions, and I think
>      that is what you are arguing (that we should just be deleting
>      regardless). But then why not just call delete_ref(), and handle
>      the ENOENT case as a noop (which closes another race if somebody
>      deletes it between your 2a and 2b).

I thought about that too, and agree it would be an improvement. But it's
not quite so trivial. The ENOENT doesn't make it out of delete_ref()
(which does a full-fledged ref_transaction internally). The error is
only reported via "strbuf *err".

>   2. Why delete it at all? The point is to overwrite, so I guess it is
>      trying to make room. But we could just rename the loose ref file
>      and reflog overtop the old, couldn't we?
>
> Or am I totally misreading the purpose of this code?

I wouldn't want to just rename the files, because (1) I think it is
better to use the existing ref_transaction code, and (2) that wouldn't
work if there is a D/F conflict between the old and new reference names,
which the existing code handles (though honestly I'm skeptical that it
comes up a lot).

It would be more plausible to use ref_transaction_update() to write
oldrefname's *value* on top of newrefname without actually moving the
file. oldrefname could even be deleted in the same transaction, if you
were willing to stop supporting old/new refnames that D/F conflict with
each other. But we also want to move the reflog, and that should be done
while the newrefname lock and (contrary to the current code) also the
oldrefname lock are held. There's currently no way to slip an arbitrary
action like that into the middle of a transaction.

If I had lots of time and interest to work on this, I think the best
approach would be to add a ref_transaction_rename(), which takes an old
and a new reference name as arguments, and adds a whole rename operation
(including of the reflog) to a transaction. This could probably be
implemented by adding one ref_update() and one ref_delete(), and using
the parent_update pointer that is introduced later to link the two so
that ref_transaction_commit() knows to move the reflog too.

If you were willing to punt on D/F conflicts, you would be done. If not,
then it would be better to teach ref_transaction_commit() to deal with
D/F conflicts in the general case rather than using special-purpose code
in rename_ref().

Then rename_ref() could be omitted from the vtable entirely.

Michael

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 19/29] refs: don't dereference on rename

Jeff King
On Fri, Apr 29, 2016 at 03:55:00PM +0200, Michael Haggerty wrote:

> It's beyond the ambition of this patch to fix this old rename_ref()
> code, but...
> [...]

Thanks for the explanation. That all makes sense to me, and I can
definitely live with "historical warts that aren't worth touching in
this series" as the verdict.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 15/29] ref_transaction_create(): disallow recursive pruning

Michael Haggerty-2
In reply to this post by Junio C Hamano
On 04/29/2016 10:41 AM, Junio C Hamano wrote:
> [...Long, thoughtful comments omitted...]
> So perhaps your original might be the best version
> among those that have been discussed in this thread.

Very well. I'll also add a comment near the definition of REF_ISPRUNING
that it must only be used together with REF_NODEREF.

Michael

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 19/29] refs: don't dereference on rename

Junio C Hamano
In reply to this post by Michael Haggerty-2
Michael Haggerty <[hidden email]> writes:

> On 04/29/2016 02:12 PM, Jeff King wrote:
>> On Fri, Apr 29, 2016 at 12:57:29PM +0200, Michael Haggerty wrote:
>>
>>> Remember, we're talking about rename_ref() only, not reference deletion
>>> in general. rename_ref() is not very robust anyway--it doesn't happen in
>>> a single transaction, and it is vulnerable to being defeated by
>>> simultaneous reference updates by other processes. It *does* wrap the
>>> deletion of newrefname in a transaction; the only question is whether an
>>> old_sha1 is supplied to that transaction.
>>>
>>> So, suppose that newrefname starts at value A, and there are two updates
>>> running simultaneously:
>>>
>>> 1. An update of reference newrefname from A -> B
>>>
>>> 2. A rename of reference oldrefname to newrefname, which includes
>>>    a. read_ref_full("newrefname") and
>>>    b. delete_ref("newrefname").
>>
>> I wondered at first if you meant "oldrefname" in 2b. That is, a rename
>> would involve writing to "newrefname" and then deleting "oldrefname",
>> and not doing the old_sha1 and normal locking on the deletion of
>> oldrefname would be bad (in case somebody else updated it while we were
>> operating).
>>
>> But reading the patch again, that's not what's going on. You're talking
>> just about the case where we force-overwrite an existing newrefname, and
>> delete it first to do so. But what I don't understand is:
>
> It's beyond the ambition of this patch to fix this old rename_ref()
> code, but...

Thanks for a clear explanation.  The NULL might want to be explained
in in-code comment (e.g. "here we do not care about verifying old
value because..."), then.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 29/29] lock_ref_sha1_basic(): only handle REF_NODEREF mode

Junio C Hamano
In reply to this post by Michael Haggerty-2
Michael Haggerty <[hidden email]> writes:

> Now lock_ref_sha1_basic() is only called with flags==REF_NODEREF. So we
> don't have to handle other cases anymore.
>
> This enables several simplifications, the most interesting of which come
> from the fact that ref_lock::orig_ref_name is now always the same as
> ref_lock::ref_name:
>
> * Remove ref_lock::orig_ref_name
> * Remove local variable orig_refname from lock_ref_sha1_basic()
> * commit_ref_update() never has to write to the reflog for
>   lock->orig_ref_name
>
> Signed-off-by: Michael Haggerty <[hidden email]>
> ---
>  refs/files-backend.c | 54 ++++++++++++++++++++--------------------------------
>  1 file changed, 21 insertions(+), 33 deletions(-)

Finished reading the whole thing and it made quite a lot of sense.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 25/29] refs: resolve symbolic refs first

David Turner
In reply to this post by Michael Haggerty-2
On Fri, 2016-04-29 at 11:51 +0200, Michael Haggerty wrote:

> On 04/29/2016 01:40 AM, David Turner wrote:
> > On Wed, 2016-04-27 at 18:57 +0200, Michael Haggerty wrote:
> > +retry:
> > ...
> > > + if (--attempts_remaining > 0)
> > > + goto retry;
> >
> > could this be a loop instead of using gotos?
>
> It certainly could. The goto-vs-loop question was debated on the
> mailing
> list when I first added very similar code elsewhere (unfortunately
> I'm
> unable to find a link to that conversation). I was persuaded to
> change
> my loop into gotos, the argument being that the "retry" case is
> exceptional and shouldn't be such a dominant part of the function
> structure. Plus the goto code is briefer and feels less awkward to me
> in
> this case (that's subjective, of course).

It's part of the structure anyway; it's just implicit rather than
explicit. I won't block consensus tho.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 19/29] refs: don't dereference on rename

David Turner
In reply to this post by Michael Haggerty-2
On Fri, 2016-04-29 at 09:38 +0200, Michael Haggerty wrote:

> On 04/27/2016 08:55 PM, Junio C Hamano wrote:
> > Michael Haggerty <[hidden email]> writes:
> >
> > > @@ -2380,8 +2381,8 @@ int rename_ref(const char *oldrefname,
> > > const char *newrefname, const char *logms
> > >   goto rollback;
> > >   }
> > >  
> > > - if (!read_ref_full(newrefname, RESOLVE_REF_READING,
> > > sha1, NULL) &&
> > > -    delete_ref(newrefname, sha1, REF_NODEREF)) {
> > > + if (!read_ref_full(newrefname, resolve_flags, sha1,
> > > NULL) &&
> > > +    delete_ref(newrefname, NULL, REF_NODEREF)) {
> >
> > Could you explain s/sha1/NULL/ here in the proposed log message?
>
> Good question.
>
> Passing sha1 to delete_ref() doesn't add any safety, because the same
> sha1 was just read a moment before, and it is not used for anything
> else. So the check only protects us from a concurrent update to
> newrefname between the call to read_ref_full() and the call to
> delete_ref(). But such a race is indistinguishable from the case that
> a
> modification to newrefname happens just before the call to
> read_ref_full(), which would have the same outcome as the new code.
> So
> the "sha1" check only adds ways for the rename() to fail in
> situations
> where nothing harmful would have happened anyway.
>
> That being said, this is a very unlikely race, and I don't think it
> matters much either way. In any case, the change s/sha1/NULL/ here
> seems
> orthogonal to the rest of the patch.
>
> David, you wrote the original version of this patch. Am I overlooking
> something?

I think I might have been handling some weird case related to symbolic
refs, but I don't recall the details.  Your argument seems right to me.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 19/29] refs: don't dereference on rename

Michael Haggerty-2
On 04/30/2016 01:21 AM, David Turner wrote:

> On Fri, 2016-04-29 at 09:38 +0200, Michael Haggerty wrote:
>> On 04/27/2016 08:55 PM, Junio C Hamano wrote:
>>> Michael Haggerty <[hidden email]> writes:
>>>
>>>> @@ -2380,8 +2381,8 @@ int rename_ref(const char *oldrefname,
>>>> const char *newrefname, const char *logms
>>>>   goto rollback;
>>>>   }
>>>>  
>>>> - if (!read_ref_full(newrefname, RESOLVE_REF_READING,
>>>> sha1, NULL) &&
>>>> -    delete_ref(newrefname, sha1, REF_NODEREF)) {
>>>> + if (!read_ref_full(newrefname, resolve_flags, sha1,
>>>> NULL) &&
>>>> +    delete_ref(newrefname, NULL, REF_NODEREF)) {
>>>
>>> Could you explain s/sha1/NULL/ here in the proposed log message?
>>
>> Good question.
>>
>> Passing sha1 to delete_ref() doesn't add any safety, because the same
>> sha1 was just read a moment before, and it is not used for anything
>> else. So the check only protects us from a concurrent update to
>> newrefname between the call to read_ref_full() and the call to
>> delete_ref(). But such a race is indistinguishable from the case that
>> a
>> modification to newrefname happens just before the call to
>> read_ref_full(), which would have the same outcome as the new code.
>> So
>> the "sha1" check only adds ways for the rename() to fail in
>> situations
>> where nothing harmful would have happened anyway.
>>
>> That being said, this is a very unlikely race, and I don't think it
>> matters much either way. In any case, the change s/sha1/NULL/ here
>> seems
>> orthogonal to the rest of the patch.
>>
>> David, you wrote the original version of this patch. Am I overlooking
>> something?
>
> I think I might have been handling some weird case related to symbolic
> refs, but I don't recall the details.  Your argument seems right to me.

Doh, of course. I should have just changed it back to `sha1` and run the
test suite, then I would have seen the failure...

The point is that `read_ref_full()` is now called with
`RESOLVE_REF_NO_RECURSE` turned on. So if `newrefname` is a symbolic
reference, then `read_ref_full()` sets `sha1` to zeros. But the
pre-check for `delete_ref()` compares `old_sha1` to the recursively
resolved value of the reference, so that check would fail. (In fact,
`ref_transaction_delete()` refuses even to add the deletion to the
transaction if `old_sha1` is zeros, so it doesn't even get that far.)

So for shallow technical reasons we can't pass `sha1` to `delete_ref()`
anymore, and for the deeper reasons discussed in this thread that's not
a problem.

I'll document this in v2 of this patch.

Michael

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 19/29] refs: don't dereference on rename

Junio C Hamano
Michael Haggerty <[hidden email]> writes:

> The point is that `read_ref_full()` is now called with
> `RESOLVE_REF_NO_RECURSE` turned on. So if `newrefname` is a symbolic
> reference, then `read_ref_full()` sets `sha1` to zeros.

Yes, that was an obvious rationale in the patch that was not
explained in the proposed log message (and made me ask you to
explain it).  I was wondering why this was not loosened
conditionally (i.e. only pass null_sha1 when symbolic ref is
involved, in which case you _must_ pass null_sha1 because we no
longer have anything to compare with).

Your explanation on the "in all possible interleaving, deletion of
what might have been updated in the middle by other people does not
matter" was a sufficient explanation why it does not have to be
conditional.

> I'll document this in v2 of this patch.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 25/29] refs: resolve symbolic refs first

Junio C Hamano
In reply to this post by David Turner
David Turner <[hidden email]> writes:

> On Wed, 2016-04-27 at 18:57 +0200, Michael Haggerty wrote:
> +retry:
> ...
>> + if (--attempts_remaining > 0)
>> + goto retry;
>
> could this be a loop instead of using gotos?

It could be, but given that there are many such exit points, I do
not think an added level of indentation with while() loop is
particularly a good way to go.

Unless that big body of code that is looped around by implicit "goto
retry" is made into a helper function to perform one round of attempt,
which may result in one of success, temporary failure, final failure.
Then we could do:

        int attempts_remaining = N;
        int failed_hard = 0;
        while (!failed_hard && attempts_remaining--)
        failed_hard = attempt();
       
and the end result _may_ become easier to read (even though I have
to think for a second what the correct code should look like that
comes after the loop).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
1234