Re: [PATCH v2 3/3] rbd: make rbd_obj_notify_ack() synchronous

[Alex Elder <alex.elder@linaro.org>]

On 08/29/2013 07:57 PM, Josh Durgin wrote:
> The only user is rbd_watch_cb(). If this is asynchronous, there is no

The only user *of rbd_obj_notify_ack()* is rbd_watch_cb().  ...

> tracking of when it completes, so it may still be in progress when the

I think what you're saying is that, because it's currently
asynchronous, the object request may still be in progress.
(I had a little trouble parsing this, as you can see...)

> osd_client is shut down.  This results in a BUG() since the osd client
> assumes no requests are in flight when it stops. Since all notifies
> are flushed now, waiting for the notify ack to complete before
> returning from the watch callback ensures there are no notify acks in
> flight during shutdown.
> 
> Signed-off-by: Josh Durgin <josh.durgin@inktank.com>
> ---
>  drivers/block/rbd.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 2223617..0e83a10 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -2826,13 +2826,15 @@ static int rbd_obj_notify_ack(struct rbd_device *rbd_dev, u64 notify_id)

Since you are now making this function wait for the object request
to complete, you should rename the function to match the convention of
all the others that do this.  That is, please rename it:
    rbd_obj_notify_ack_sync()

There's another problem though.  This function used a clever trick of
assigning rbd_obj_request_put() as the object request's callback
function.  This allowed the request to just be sent and cleaned up
whenever it completed.

Since you're now waiting for completion here you need to do the
cleanup here as well.  (Otherwise it might have completed before
the wait call even begins, with unpleasant results.)

So here's what I think you need to do:
- Rename the function rbd_obj_notify_ack_sync()
- delete this line in that function:
        obj_request->callback = rbd_obj_request_put;
- Add your call to rbd_obj_request_wait() (though
  you could ignore the return value).
- Make the call to rbd_obj_request_put() at the end of
  that function unconditional.  I.e., drop the "if (ret)"
  and fix the indentation.

					-Alex

>  	obj_request->osd_req = rbd_osd_req_create(rbd_dev, false, obj_request);
>  	if (!obj_request->osd_req)
>  		goto out;
> -	obj_request->callback = rbd_obj_request_put;
>  
>  	osd_req_op_watch_init(obj_request->osd_req, 0, CEPH_OSD_OP_NOTIFY_ACK,
>  					notify_id, 0, 0);
>  	rbd_osd_req_format_read(obj_request);
>  
>  	ret = rbd_obj_request_submit(osdc, obj_request);
> +	if (ret)
> +		goto out;
> +	ret = rbd_obj_request_wait(obj_request);
>  out:
>  	if (ret)
>  		rbd_obj_request_put(obj_request);
>