| Message ID | 1393861102-5123-1-git-send-email-ilya.dryomov@inktank.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 03/03/2014 09:38 AM, Ilya Dryomov wrote: > Doing rbd_obj_request_put() in rbd_img_request_fill() error paths is > not only insufficient, but also triggers an rbd_assert() in > rbd_obj_request_destroy(): > > Assertion failure in rbd_obj_request_destroy() at line 1867: > rbd_assert(obj_request->img_request == NULL); Does this have a tracker entry separate from 7327? (I didn't look, just curious.) > rbd_img_obj_request_add() adds obj_requests to the img_request, the > opposite is rbd_img_obj_request_del(). Use it.h This is the main bug here, and this is the right fix. > While at it, commit 03507db631c94 ("rbd: fix buffer size for writes to > images with snapshots") moved the call to rbd_img_obj_request_add() up, > making the out_partial label bogus. Remove it. Yes, this is also correct, and is a bug fix. Since it's a distinct bug *maybe* you could commit it separately, but I don't really think it's that important. Very nice. Reviewed-by: Alex Elder <elder@linaro.org> > Fixes: http://tracker.ceph.com/issues/7327 > > Signed-off-by: Ilya Dryomov <ilya.dryomov@inktank.com> > --- > drivers/block/rbd.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c > index b365e0dfccb6..53d492e83586 100644 > --- a/drivers/block/rbd.c > +++ b/drivers/block/rbd.c > @@ -2191,6 +2191,7 @@ static int rbd_img_request_fill(struct rbd_img_request *img_request, > rbd_segment_name_free(object_name); > if (!obj_request) > goto out_unwind; > + > /* > * set obj_request->img_request before creating the > * osd_request so that it gets the right snapc > @@ -2208,7 +2209,7 @@ static int rbd_img_request_fill(struct rbd_img_request *img_request, > clone_size, > GFP_ATOMIC); > if (!obj_request->bio_list) > - goto out_partial; > + goto out_unwind; > } else { > unsigned int page_count; > > @@ -2223,7 +2224,7 @@ static int rbd_img_request_fill(struct rbd_img_request *img_request, > osd_req = rbd_osd_req_create(rbd_dev, write_request, > obj_request); > if (!osd_req) > - goto out_partial; > + goto out_unwind; > obj_request->osd_req = osd_req; > obj_request->callback = rbd_img_obj_callback; > > @@ -2250,11 +2251,9 @@ static int rbd_img_request_fill(struct rbd_img_request *img_request, > > return 0; > > -out_partial: > - rbd_obj_request_put(obj_request); > out_unwind: > for_each_obj_request_safe(img_request, obj_request, next_obj_request) > - rbd_obj_request_put(obj_request); > + rbd_img_obj_request_del(img_request, obj_request); > > return -ENOMEM; > } > -- 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, Mar 3, 2014 at 11:59 PM, Alex Elder <elder@ieee.org> wrote: > On 03/03/2014 09:38 AM, Ilya Dryomov wrote: >> >> Doing rbd_obj_request_put() in rbd_img_request_fill() error paths is >> not only insufficient, but also triggers an rbd_assert() in >> rbd_obj_request_destroy(): >> >> Assertion failure in rbd_obj_request_destroy() at line 1867: >> rbd_assert(obj_request->img_request == NULL); > > > Does this have a tracker entry separate from 7327? (I > didn't look, just curious.) No, I don't think so. There are though 3 entries for the opposite assert, on which I unfortunately don't have anything yet: http://tracker.ceph.com/issues/5454 http://tracker.ceph.com/issues/5662 http://tracker.ceph.com/issues/7327 > >> rbd_img_obj_request_add() adds obj_requests to the img_request, the >> opposite is rbd_img_obj_request_del(). Use it.h > > > This is the main bug here, and this is the right fix. > > >> While at it, commit 03507db631c94 ("rbd: fix buffer size for writes to >> images with snapshots") moved the call to rbd_img_obj_request_add() up, >> making the out_partial label bogus. Remove it. > > > Yes, this is also correct, and is a bug fix. Since it's a distinct > bug *maybe* you could commit it separately, but I don't really think > it's that important. Done. > > Very nice. > > Reviewed-by: Alex Elder <elder@linaro.org> 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 b365e0dfccb6..53d492e83586 100644 --- a/drivers/block/rbd.c +++ b/drivers/block/rbd.c @@ -2191,6 +2191,7 @@ static int rbd_img_request_fill(struct rbd_img_request *img_request, rbd_segment_name_free(object_name); if (!obj_request) goto out_unwind; + /* * set obj_request->img_request before creating the * osd_request so that it gets the right snapc @@ -2208,7 +2209,7 @@ static int rbd_img_request_fill(struct rbd_img_request *img_request, clone_size, GFP_ATOMIC); if (!obj_request->bio_list) - goto out_partial; + goto out_unwind; } else { unsigned int page_count; @@ -2223,7 +2224,7 @@ static int rbd_img_request_fill(struct rbd_img_request *img_request, osd_req = rbd_osd_req_create(rbd_dev, write_request, obj_request); if (!osd_req) - goto out_partial; + goto out_unwind; obj_request->osd_req = osd_req; obj_request->callback = rbd_img_obj_callback; @@ -2250,11 +2251,9 @@ static int rbd_img_request_fill(struct rbd_img_request *img_request, return 0; -out_partial: - rbd_obj_request_put(obj_request); out_unwind: for_each_obj_request_safe(img_request, obj_request, next_obj_request) - rbd_obj_request_put(obj_request); + rbd_img_obj_request_del(img_request, obj_request); return -ENOMEM; }
Doing rbd_obj_request_put() in rbd_img_request_fill() error paths is not only insufficient, but also triggers an rbd_assert() in rbd_obj_request_destroy(): Assertion failure in rbd_obj_request_destroy() at line 1867: rbd_assert(obj_request->img_request == NULL); rbd_img_obj_request_add() adds obj_requests to the img_request, the opposite is rbd_img_obj_request_del(). Use it. While at it, commit 03507db631c94 ("rbd: fix buffer size for writes to images with snapshots") moved the call to rbd_img_obj_request_add() up, making the out_partial label bogus. Remove it. Fixes: http://tracker.ceph.com/issues/7327 Signed-off-by: Ilya Dryomov <ilya.dryomov@inktank.com> --- drivers/block/rbd.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)