On 06/30/2014 09:34 AM, Ilya Dryomov wrote: > On Mon, Jun 30, 2014 at 5:39 PM, Alex Elder <elder@xxxxxxxx> 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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
