| Message ID | 1403716607-13535-10-git-send-email-ilya.dryomov@inktank.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. - __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.) I'm going to keep looking at this; I have about 20 minutes before I have to leave for a while. -Alex > Signed-off-by: Ilya Dryomov <ilya.dryomov@inktank.com> > --- > include/linux/ceph/osd_client.h | 1 + > net/ceph/osd_client.c | 31 +++++++++++++++++++++++++------ > 2 files changed, 26 insertions(+), 6 deletions(-) > > diff --git a/include/linux/ceph/osd_client.h b/include/linux/ceph/osd_client.h > index a8d5652f589d..de09cad7b7c7 100644 > --- a/include/linux/ceph/osd_client.h > +++ b/include/linux/ceph/osd_client.h > @@ -334,6 +334,7 @@ extern void ceph_osdc_put_request(struct ceph_osd_request *req); > extern int ceph_osdc_start_request(struct ceph_osd_client *osdc, > struct ceph_osd_request *req, > bool nofail); > +extern void ceph_osdc_cancel_request(struct ceph_osd_request *req); > extern int ceph_osdc_wait_request(struct ceph_osd_client *osdc, > struct ceph_osd_request *req); > extern void ceph_osdc_sync(struct ceph_osd_client *osdc); > diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c > index 8104db24bc09..ef44cc0bbc6a 100644 > --- a/net/ceph/osd_client.c > +++ b/net/ceph/osd_client.c > @@ -2470,6 +2470,25 @@ int ceph_osdc_start_request(struct ceph_osd_client *osdc, > EXPORT_SYMBOL(ceph_osdc_start_request); > > /* > + * Unregister a registered request. The request is not completed (i.e. > + * no callbacks or wakeups) - higher layers are supposed to know what > + * they are canceling. > + */ > +void ceph_osdc_cancel_request(struct ceph_osd_request *req) > +{ > + struct ceph_osd_client *osdc = req->r_osdc; > + > + mutex_lock(&osdc->request_mutex); > + if (req->r_linger) > + __unregister_linger_request(osdc, req); > + __unregister_request(osdc, req); > + mutex_unlock(&osdc->request_mutex); > + > + dout("%s %p tid %llu canceled\n", __func__, req, req->r_tid); > +} > +EXPORT_SYMBOL(ceph_osdc_cancel_request); > + > +/* > * wait for a request to complete > */ > int ceph_osdc_wait_request(struct ceph_osd_client *osdc, > @@ -2477,18 +2496,18 @@ int ceph_osdc_wait_request(struct ceph_osd_client *osdc, > { > int rc; > > + dout("%s %p tid %llu\n", __func__, req, req->r_tid); > + > rc = wait_for_completion_interruptible(&req->r_completion); > if (rc < 0) { > - mutex_lock(&osdc->request_mutex); > - __cancel_request(req); > - __unregister_request(osdc, req); > - mutex_unlock(&osdc->request_mutex); > + dout("%s %p tid %llu interrupted\n", __func__, req, req->r_tid); > + ceph_osdc_cancel_request(req); > complete_request(req); > - dout("wait_request tid %llu canceled/timed out\n", req->r_tid); > return rc; > } > > - dout("wait_request tid %llu result %d\n", req->r_tid, req->r_result); > + dout("%s %p tid %llu result %d\n", __func__, req, req->r_tid, > + req->r_result); > return req->r_result; > } > EXPORT_SYMBOL(ceph_osdc_wait_request); > -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jun 30, 2014 at 5:39 PM, Alex Elder <elder@ieee.org> 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 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. > - __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. Thanks, Ilya -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/30/2014 09:34 AM, Ilya Dryomov wrote: > On Mon, Jun 30, 2014 at 5:39 PM, Alex Elder <elder@ieee.org> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 7, 2014 at 5:47 PM, Alex Elder <elder@ieee.org> wrote: > On 06/30/2014 09:34 AM, Ilya Dryomov wrote: >> On Mon, Jun 30, 2014 at 5:39 PM, Alex Elder <elder@ieee.org> 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. I have added the explanation to the commit message. > >> 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. Yeah, ceph_osdc_unregister_linger_request() is removed in favor of ceph_osdc_cancel_request() later in the series. r_linger is now treated is a sort of immutable field - it's never zeroed after it's been set. It's still safe to call __unregister_linger_request() at any point in time though, because unless the request is *actually* lingering it won't do a thing. Are you OK with your Reviewed-by for this patch? Thanks, Ilya -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/08/2014 06:15 AM, Ilya Dryomov wrote: > Are you OK with your Reviewed-by for this patch? Reviewed-by: Alex Elder <elder@linaro.org> -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/ceph/osd_client.h b/include/linux/ceph/osd_client.h index a8d5652f589d..de09cad7b7c7 100644 --- a/include/linux/ceph/osd_client.h +++ b/include/linux/ceph/osd_client.h @@ -334,6 +334,7 @@ extern void ceph_osdc_put_request(struct ceph_osd_request *req); extern int ceph_osdc_start_request(struct ceph_osd_client *osdc, struct ceph_osd_request *req, bool nofail); +extern void ceph_osdc_cancel_request(struct ceph_osd_request *req); extern int ceph_osdc_wait_request(struct ceph_osd_client *osdc, struct ceph_osd_request *req); extern void ceph_osdc_sync(struct ceph_osd_client *osdc); diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c index 8104db24bc09..ef44cc0bbc6a 100644 --- a/net/ceph/osd_client.c +++ b/net/ceph/osd_client.c @@ -2470,6 +2470,25 @@ int ceph_osdc_start_request(struct ceph_osd_client *osdc, EXPORT_SYMBOL(ceph_osdc_start_request); /* + * Unregister a registered request. The request is not completed (i.e. + * no callbacks or wakeups) - higher layers are supposed to know what + * they are canceling. + */ +void ceph_osdc_cancel_request(struct ceph_osd_request *req) +{ + struct ceph_osd_client *osdc = req->r_osdc; + + mutex_lock(&osdc->request_mutex); + if (req->r_linger) + __unregister_linger_request(osdc, req); + __unregister_request(osdc, req); + mutex_unlock(&osdc->request_mutex); + + dout("%s %p tid %llu canceled\n", __func__, req, req->r_tid); +} +EXPORT_SYMBOL(ceph_osdc_cancel_request); + +/* * wait for a request to complete */ int ceph_osdc_wait_request(struct ceph_osd_client *osdc, @@ -2477,18 +2496,18 @@ int ceph_osdc_wait_request(struct ceph_osd_client *osdc, { int rc; + dout("%s %p tid %llu\n", __func__, req, req->r_tid); + rc = wait_for_completion_interruptible(&req->r_completion); if (rc < 0) { - mutex_lock(&osdc->request_mutex); - __cancel_request(req); - __unregister_request(osdc, req); - mutex_unlock(&osdc->request_mutex); + dout("%s %p tid %llu interrupted\n", __func__, req, req->r_tid); + ceph_osdc_cancel_request(req); complete_request(req); - dout("wait_request tid %llu canceled/timed out\n", req->r_tid); return rc; } - dout("wait_request tid %llu result %d\n", req->r_tid, req->r_result); + dout("%s %p tid %llu result %d\n", __func__, req, req->r_tid, + req->r_result); return req->r_result; } EXPORT_SYMBOL(ceph_osdc_wait_request);
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. Signed-off-by: Ilya Dryomov <ilya.dryomov@inktank.com> --- include/linux/ceph/osd_client.h | 1 + net/ceph/osd_client.c | 31 +++++++++++++++++++++++++------ 2 files changed, 26 insertions(+), 6 deletions(-)