* [PATCH] rbd: fix error paths in rbd_img_request_fill()
@ 2014-03-03 15:38 Ilya Dryomov
2014-03-03 21:59 ` Alex Elder
0 siblings, 1 reply; 3+ messages in thread
From: Ilya Dryomov @ 2014-03-03 15:38 UTC (permalink / raw)
To: ceph-devel
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(-)
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;
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] rbd: fix error paths in rbd_img_request_fill()
2014-03-03 15:38 [PATCH] rbd: fix error paths in rbd_img_request_fill() Ilya Dryomov
@ 2014-03-03 21:59 ` Alex Elder
2014-03-04 11:03 ` Ilya Dryomov
0 siblings, 1 reply; 3+ messages in thread
From: Alex Elder @ 2014-03-03 21:59 UTC (permalink / raw)
To: Ilya Dryomov, ceph-devel
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;
> }
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] rbd: fix error paths in rbd_img_request_fill()
2014-03-03 21:59 ` Alex Elder
@ 2014-03-04 11:03 ` Ilya Dryomov
0 siblings, 0 replies; 3+ messages in thread
From: Ilya Dryomov @ 2014-03-04 11:03 UTC (permalink / raw)
To: Alex Elder; +Cc: Ceph Development
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-03-04 11:03 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-03-03 15:38 [PATCH] rbd: fix error paths in rbd_img_request_fill() Ilya Dryomov
2014-03-03 21:59 ` Alex Elder
2014-03-04 11:03 ` Ilya Dryomov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.