[PATCH] rbd: avoid dropping extra reference in rbd_free_disk()

[PATCH] rbd: avoid dropping extra reference in rbd_free_disk()

[Alex Elder]

I found during some failure injection testing that the call to
rbd_free_disk() in the error path of rbd_dev_probe_finish() was
dropping an extra reference to the disk queue.  The problem
occurred when put_disk tried to drop a reference to the disk's
queue.  A call to blk_cleanup_queue() just prior to that will have
also dropped a reference to the queue.

The problem is that the reference dropped by put_disk() is assumed
to have been taken by add_disk().  Our code has error paths that can
occur after the disk and its queue are initialized, but before the
call to add_disk(), and in those paths we won't have that extra
reference.

The fix is easy though.  In rbd_free_disk() we're already checking
the disk's GENHD_FL_UP flag.  That flag is an indication that
add_disk() has been called, so just call blk_cleanup_queue()
conditional on that flag being set.

This resolves:
    http://tracker.ceph.com/issues/4800

Signed-off-by: Alex Elder <elder@inktank.com>
---
 drivers/block/rbd.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index dcd8e58..9e38967 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -2844,10 +2844,12 @@ static void rbd_free_disk(struct rbd_device
*rbd_dev)
 	if (!disk)
 		return;

-	if (disk->flags & GENHD_FL_UP)
+	rbd_dev->disk = NULL;
+	if (disk->flags & GENHD_FL_UP) {
 		del_gendisk(disk);
-	if (disk->queue)
-		blk_cleanup_queue(disk->queue);
+		if (disk->queue)
+			blk_cleanup_queue(disk->queue);
+	}
 	put_disk(disk);
 }

-- 
1.7.9.5

Re: [PATCH] rbd: avoid dropping extra reference in rbd_free_disk()

[Alex Elder]

This patch, and the series of 4 that I also just posted,
are available in the "review/wip-rbd-cleanup-1" branch of
the ceph-client git repository.  That branch is based on
the branch "review/wip-rbd-cleanup".

					-Alex

Re: [PATCH] rbd: avoid dropping extra reference in rbd_free_disk()

[Josh Durgin]

Reviewed-by: Josh Durgin <josh.durgin@inktank.com>

On 04/26/2013 07:50 AM, Alex Elder wrote:
> I found during some failure injection testing that the call to
> rbd_free_disk() in the error path of rbd_dev_probe_finish() was
> dropping an extra reference to the disk queue.  The problem
> occurred when put_disk tried to drop a reference to the disk's
> queue.  A call to blk_cleanup_queue() just prior to that will have
> also dropped a reference to the queue.
>
> The problem is that the reference dropped by put_disk() is assumed
> to have been taken by add_disk().  Our code has error paths that can
> occur after the disk and its queue are initialized, but before the
> call to add_disk(), and in those paths we won't have that extra
> reference.
>
> The fix is easy though.  In rbd_free_disk() we're already checking
> the disk's GENHD_FL_UP flag.  That flag is an indication that
> add_disk() has been called, so just call blk_cleanup_queue()
> conditional on that flag being set.
>
> This resolves:
>      http://tracker.ceph.com/issues/4800
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |    8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index dcd8e58..9e38967 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -2844,10 +2844,12 @@ static void rbd_free_disk(struct rbd_device
> *rbd_dev)
>   	if (!disk)
>   		return;
>
> -	if (disk->flags & GENHD_FL_UP)
> +	rbd_dev->disk = NULL;
> +	if (disk->flags & GENHD_FL_UP) {
>   		del_gendisk(disk);
> -	if (disk->queue)
> -		blk_cleanup_queue(disk->queue);
> +		if (disk->queue)
> +			blk_cleanup_queue(disk->queue);
> +	}
>   	put_disk(disk);
>   }
>