On 06/30/2014 09:34 AM, Ilya Dryomov wrote:
> On Mon, Jun 30, 2014 at 5:39 PM, Alex Elder <[email protected]> wrote:
>> On 06/25/2014 12:16 PM, Ilya Dryomov wrote:
>>> Introduce ceph_osdc_cancel_request() intended for canceling requests
>>> from the higher layers (rbd and cephfs). Because higher layers are in
>>> charge and are supposed to know what and when they are canceling, the
>>> request is not completed, only unref'ed and removed from the libceph
>>> data structures.
>>
>> This seems reasonable.
>>
>> But you make two changes here that I'd like to see a little
>> more explanation on. They seem significant enough to warrant
>> a little more attention in the commit description.
>>
>> - __cancel_request() is no longer called when
>> ceph_osdc_wait_request() fails due to an
>> an interrupt. This is my main concern, and I
>> plan to work through it but I'm in a small hurry
>> right now.
>
> Perhaps it should have been a separate commit. __unregister_request()
> revokes r_request, so I opted for not trying to do it twice. As for
OK, that makes sense--revoking the request is basically all
__cancel_request() does anyway. You ought to have mentioned
that in the description anyway, even if it wasn't a separate
commit.
> the r_sent condition and assignment, it doesn't seem to make much of
> a difference, given that the request is about to be unregistered
> anyway.
If the request is getting canceled (from a caller outside libceph)
it can't take into account whether it was sent or not. As you said,
it is getting revoked unconditionally by __unregister_request().
To be honest though, *that* revoke call should have been zeroing
the r_sent value also. It apparently won't matter, but I think it's
wrong. The revoke drops a reference, it doesn't necessarily free
the structure (though I expect it may always do so anyway).
>> - __unregister_linger_request() is now called when
>> a request is canceled, but it wasn't before. (Since
>> we're consistent about setting the r_linger flag
>> this should be fine, but it *is* a behavior change.)
>
> The general goal here is to make ceph_osdc_cancel_request() cancel
> *any* request correctly, so if r_linger is set, which means that the
> request in question *could* be lingering, __unregister_linger_request()
> is called.
The goal is good. Note that __unregister_linger_request() drops
a reference to the request though. There are three other callers
of this function. Two of them drop a reference that had just been
added by a call to __register_request(). The other one is in
ceph_osdc_unregister_linger_request(), initiated by a higher layer.
In that last case, r_linger will be zeroed, so calling it again
should be harmless.
I guess I'm going to move on... I expect all of this discussion
will be moot and you will have made everything work right and
better by the time I get to the end of the series.
-Alex
>
> Thanks,
>
> Ilya
>
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
