| Message ID | 1474304608-17958-4-git-send-email-idryomov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 09/19/2016 12:03 PM, Ilya Dryomov wrote: > If stat request fails with something other than -ENOENT (which just > means that we need to copyup), the original object request is never > marked as done and therefore never completed. Fix this by moving the > mark done + complete snippet from rbd_img_obj_parent_read_full() into > rbd_img_obj_exists_callback(). The former remains covered, as the > latter is its only caller (through rbd_img_obj_request_submit()). > > Signed-off-by: Ilya Dryomov <idryomov@gmail.com> Did you ever see this happen? This little block of code (setting an error and marking a request done) is found in two other spots in the code; maybe it could be encapsulated. (That wouldn't have helped in this case, however.) It took me a while to refresh on all the moving parts here. Let me explain what I see. No matter what, when an object request gets submitted, it must eventually have its result code set, get marked done, and be completed by a call to rbd_obj_request_complete(). An image object gets submitted by rbd_img_obj_request_submit(). Initially, rbd_img_request_submit() calls this for every object request in the image request. If any errors occur here, the image request is dropped, and in the process of cleaning up the image request, its reference to its objects are dropped as well. (Here it's possible some requests are never submitted, and therefore never marked done... But I think that's OK.) There is one other place rbd_img_obj_request_submit() gets called. Before getting to that, let's look at what it does. Simple image object requests (reads, or non-layered writes, or layered writes that are known to not overlap the parent) require no special treatment. The object request is submitted, and it will eventually be marked done and be completed. For a layered write request, we need to know whether an image object exists, and if we don't know we submit a stat call for that object so we can find out. When that call completes, rbd_img_obj_exists_callback() records the result (whether the object exists), and then re-submits the original image object request. This is the second spot rbd_img_obj_request_submit() is called. If an error occurs at this stage, we need to mark the original request done and complete it. --> This is the location of the bug this patch fixes--the original request was not getting marked done in the event of an error. The last case is a call to rbd_img_obj_request_submit() for a non-simple request in which we know the target image object doesn't exist. So in that case we issue a read of the data in the parent object backing the full (target) image object, in rbd_img_obj_parent_read_full(). This is necessary so that data can be supplied to the target OSD in a copyup request. Previously, if an error occurred calling that function, the original image object request would be marked done and completed. Otherwise, that parent image would, when complete, cause rbd_img_obj_parent_read_full_callback() to be called. This patch changes that (discussed below). If an error occurs in rbd_img_obj_parent_read_full_callback(), that function marks the original image object request done and completes it. Otherwise the original request is converted into a copyup request, and that gets submitted to the original image object. Eventually rbd_osd_copyup_callback() is called, which marks the original request done, and as a result, rbd_osd_req_callback() completes that request. This covers all the cases. Now about the change... In in rbd_img_obj_parent_read_full() you have eliminated setting the object request result, marking it done, and completing it. Now that function will simply return an error if one occurs. The only caller of that function is rbd_img_obj_request_submit(), and as laid out above, we know an error occurring there leads to the original image object request being marked done and completed. Or rather, it's done properly provided the fix for the bug in rbd_img_obj_exists_callback() is in place. So I guess I'd say this looks good, and in summary: Reviewed-by: Alex Elder <elder@linaro.org> I have one minor suggestion below. > --- > drivers/block/rbd.c | 24 +++++++++++++----------- > 1 file changed, 13 insertions(+), 11 deletions(-) > > diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c > index 027e0817a118..b247200a0f28 100644 > --- a/drivers/block/rbd.c > +++ b/drivers/block/rbd.c > @@ -2805,10 +2805,6 @@ out_err: > ceph_release_page_vector(pages, page_count); > if (parent_request) > rbd_img_request_put(parent_request); > - obj_request->result = result; > - obj_request->xferred = 0; > - obj_request_done_set(obj_request); > - You should change the comment at the top of this function so it indicates that it is no longer this function that takes care of passing the error on to the original image object request. > return result; > } > > @@ -2860,19 +2856,25 @@ static void rbd_img_obj_exists_callback(struct rbd_obj_request *obj_request) > obj_request_existence_set(orig_request, true); > } else if (result == -ENOENT) { > obj_request_existence_set(orig_request, false); > - } else if (result) { > - orig_request->result = result; > - goto out; > + } else { > + goto fail_orig_request; > } > > /* > * Resubmit the original request now that we have recorded > * whether the target object exists. > */ > - orig_request->result = rbd_img_obj_request_submit(orig_request); > -out: > - if (orig_request->result) > - rbd_obj_request_complete(orig_request); > + result = rbd_img_obj_request_submit(orig_request); > + if (result) > + goto fail_orig_request; > + > + return; > + > +fail_orig_request: > + orig_request->result = result; > + orig_request->xferred = 0; > + obj_request_done_set(orig_request); > + rbd_obj_request_complete(orig_request); > } > > static int rbd_img_obj_exists_submit(struct rbd_obj_request *obj_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 Fri, Sep 23, 2016 at 11:39 PM, Alex Elder <elder@linaro.org> wrote: > On 09/19/2016 12:03 PM, Ilya Dryomov wrote: >> If stat request fails with something other than -ENOENT (which just >> means that we need to copyup), the original object request is never >> marked as done and therefore never completed. Fix this by moving the >> mark done + complete snippet from rbd_img_obj_parent_read_full() into >> rbd_img_obj_exists_callback(). The former remains covered, as the >> latter is its only caller (through rbd_img_obj_request_submit()). >> >> Signed-off-by: Ilya Dryomov <idryomov@gmail.com> > > Did you ever see this happen? No, this series was all just staring at the surroundings of rbd_img_obj_exists_submit(), prompted by David's patches. > > This little block of code (setting an error and marking a > request done) is found in two other spots in the code; maybe > it could be encapsulated. (That wouldn't have helped in > this case, however.) I'll go ahead and pull it into a new rbd_obj_request_error(). > > It took me a while to refresh on all the moving parts here. > Let me explain what I see. > > No matter what, when an object request gets submitted, it > must eventually have its result code set, get marked done, > and be completed by a call to rbd_obj_request_complete(). > > An image object gets submitted by rbd_img_obj_request_submit(). > Initially, rbd_img_request_submit() calls this for every object > request in the image request. If any errors occur here, the > image request is dropped, and in the process of cleaning up > the image request, its reference to its objects are dropped as > well. (Here it's possible some requests are never submitted, > and therefore never marked done... But I think that's OK.) > > There is one other place rbd_img_obj_request_submit() gets > called. Before getting to that, let's look at what it does. > > Simple image object requests (reads, or non-layered writes, > or layered writes that are known to not overlap the parent) > require no special treatment. The object request is submitted, > and it will eventually be marked done and be completed. > > For a layered write request, we need to know whether an image > object exists, and if we don't know we submit a stat call for > that object so we can find out. When that call completes, > rbd_img_obj_exists_callback() records the result (whether the > object exists), and then re-submits the original image object > request. This is the second spot rbd_img_obj_request_submit() > is called. If an error occurs at this stage, we need to mark > the original request done and complete it. > --> This is the location of the bug this patch fixes--the > original request was not getting marked done in the event > of an error. Correct. > > The last case is a call to rbd_img_obj_request_submit() for > a non-simple request in which we know the target image object > doesn't exist. So in that case we issue a read of the data > in the parent object backing the full (target) image object, > in rbd_img_obj_parent_read_full(). This is necessary so that > data can be supplied to the target OSD in a copyup request. > Previously, if an error occurred calling that function, the > original image object request would be marked done and completed. > Otherwise, that parent image would, when complete, cause > rbd_img_obj_parent_read_full_callback() to be called. This > patch changes that (discussed below). > > If an error occurs in rbd_img_obj_parent_read_full_callback(), > that function marks the original image object request done > and completes it. Otherwise the original request is converted > into a copyup request, and that gets submitted to the original > image object. Eventually rbd_osd_copyup_callback() is called, > which marks the original request done, and as a result, > rbd_osd_req_callback() completes that request. > > This covers all the cases. > > Now about the change... > > In in rbd_img_obj_parent_read_full() you have eliminated > setting the object request result, marking it done, and > completing it. Now that function will simply return an > error if one occurs. The only caller of that function is > rbd_img_obj_request_submit(), and as laid out above, we > know an error occurring there leads to the original image > object request being marked done and completed. > > Or rather, it's done properly provided the fix for the > bug in rbd_img_obj_exists_callback() is in place. > > So I guess I'd say this looks good, and in summary: > > Reviewed-by: Alex Elder <elder@linaro.org> > > I have one minor suggestion below. > >> --- >> drivers/block/rbd.c | 24 +++++++++++++----------- >> 1 file changed, 13 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c >> index 027e0817a118..b247200a0f28 100644 >> --- a/drivers/block/rbd.c >> +++ b/drivers/block/rbd.c >> @@ -2805,10 +2805,6 @@ out_err: >> ceph_release_page_vector(pages, page_count); >> if (parent_request) >> rbd_img_request_put(parent_request); >> - obj_request->result = result; >> - obj_request->xferred = 0; >> - obj_request_done_set(obj_request); >> - > > You should change the comment at the top of this function so > it indicates that it is no longer this function that takes > care of passing the error on to the original image object > request. Done. 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
diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c index 027e0817a118..b247200a0f28 100644 --- a/drivers/block/rbd.c +++ b/drivers/block/rbd.c @@ -2805,10 +2805,6 @@ out_err: ceph_release_page_vector(pages, page_count); if (parent_request) rbd_img_request_put(parent_request); - obj_request->result = result; - obj_request->xferred = 0; - obj_request_done_set(obj_request); - return result; } @@ -2860,19 +2856,25 @@ static void rbd_img_obj_exists_callback(struct rbd_obj_request *obj_request) obj_request_existence_set(orig_request, true); } else if (result == -ENOENT) { obj_request_existence_set(orig_request, false); - } else if (result) { - orig_request->result = result; - goto out; + } else { + goto fail_orig_request; } /* * Resubmit the original request now that we have recorded * whether the target object exists. */ - orig_request->result = rbd_img_obj_request_submit(orig_request); -out: - if (orig_request->result) - rbd_obj_request_complete(orig_request); + result = rbd_img_obj_request_submit(orig_request); + if (result) + goto fail_orig_request; + + return; + +fail_orig_request: + orig_request->result = result; + orig_request->xferred = 0; + obj_request_done_set(orig_request); + rbd_obj_request_complete(orig_request); } static int rbd_img_obj_exists_submit(struct rbd_obj_request *obj_request)
If stat request fails with something other than -ENOENT (which just means that we need to copyup), the original object request is never marked as done and therefore never completed. Fix this by moving the mark done + complete snippet from rbd_img_obj_parent_read_full() into rbd_img_obj_exists_callback(). The former remains covered, as the latter is its only caller (through rbd_img_obj_request_submit()). Signed-off-by: Ilya Dryomov <idryomov@gmail.com> --- drivers/block/rbd.c | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-)