Re: [PATCH] rbd: fix error paths in rbd_img_request_fill()

[Alex Elder <elder@ieee.org>]

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;
>   }
>