Re: [PATCH 2/2] rbd: prevent open for image being removed

[Josh Durgin <josh.durgin@inktank.com>]

Enums should be capitalized according to Documentation/CodingStyle.

Other than that, looks good.
Reviewed-by: Josh Durgin <josh.durgin@inktank.com>

On 01/28/2013 02:09 PM, Alex Elder wrote:
> An open request for a mapped rbd image can arrive while removal of
> that mapping is underway.  We need to prevent such an open request
> from succeeding.  (It appears that Maciej Galkiewicz ran into this
> problem.)
>
> Define and use a "removing" flag to indicate a mapping is getting
> removed.  Set it in the remove path after verifying nothing holds
> the device open.  And check it in the open path before allowing the
> open to proceed.  Acquire the rbd device's lock around each of these
> spots to avoid any races accessing the flags and open_count fields.
>
> This addresses:
>      http://tracker.newdream.net/issues/3427
>
> Reported-by:  Maciej Galkiewicz <maciejgalkiewicz@ragnarson.com>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   42 +++++++++++++++++++++++++++++++++---------
>   1 file changed, 33 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 107df40..03b15b8 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -259,10 +259,10 @@ struct rbd_device {
>
>   	char			name[DEV_NAME_LEN]; /* blkdev name, e.g. rbd3 */
>
> -	spinlock_t		lock;		/* queue lock */
> +	spinlock_t		lock;		/* queue, flags, open_count */
>
>   	struct rbd_image_header	header;
> -	unsigned long		flags;
> +	unsigned long		flags;		/* possibly lock protected */
>   	struct rbd_spec		*spec;
>
>   	char			*header_name;
> @@ -288,13 +288,20 @@ struct rbd_device {
>
>   	/* sysfs related */
>   	struct device		dev;
> -	unsigned long		open_count;
> +	unsigned long		open_count;	/* protected by lock */
>   };
>
> -/* Flag bits for rbd_dev->flags */
> +/*
> + * Flag bits for rbd_dev->flags.  If atomicity is required,
> + * rbd_dev->lock is used to protect access.
> + *
> + * Currently, only the "removing" flag (which is coupled with the
> + * "open_count" field) requires atomic access.
> + */
>
>   enum rbd_dev_flags {
>   	rbd_dev_flag_exists,	/* mapped snapshot has not been deleted */
> +	rbd_dev_flag_removing,	/* this mapping is being removed */
>   };
>
>   static DEFINE_MUTEX(ctl_mutex);	  /* Serialize open/close/setup/teardown */
> @@ -383,14 +390,23 @@ static int rbd_dev_v2_refresh(struct rbd_device
> *rbd_dev, u64 *hver);
>   static int rbd_open(struct block_device *bdev, fmode_t mode)
>   {
>   	struct rbd_device *rbd_dev = bdev->bd_disk->private_data;
> +	bool removing = false;
>
>   	if ((mode & FMODE_WRITE) && rbd_dev->mapping.read_only)
>   		return -EROFS;
>
> +	spin_lock(&rbd_dev->lock);
> +	if (test_bit(rbd_dev_flag_removing, &rbd_dev->flags))
> +		removing = true;
> +	else
> +		rbd_dev->open_count++;
> +	spin_unlock(&rbd_dev->lock);
> +	if (removing)
> +		return -ENOENT;
> +
>   	mutex_lock_nested(&ctl_mutex, SINGLE_DEPTH_NESTING);
>   	(void) get_device(&rbd_dev->dev);
>   	set_device_ro(bdev, rbd_dev->mapping.read_only);
> -	rbd_dev->open_count++;
>   	mutex_unlock(&ctl_mutex);
>
>   	return 0;
> @@ -399,10 +415,14 @@ static int rbd_open(struct block_device *bdev,
> fmode_t mode)
>   static int rbd_release(struct gendisk *disk, fmode_t mode)
>   {
>   	struct rbd_device *rbd_dev = disk->private_data;
> +	unsigned long open_count_before;
> +
> +	spin_lock(&rbd_dev->lock);
> +	open_count_before = rbd_dev->open_count--;
> +	spin_unlock(&rbd_dev->lock);
> +	rbd_assert(open_count_before > 0);
>
>   	mutex_lock_nested(&ctl_mutex, SINGLE_DEPTH_NESTING);
> -	rbd_assert(rbd_dev->open_count > 0);
> -	rbd_dev->open_count--;
>   	put_device(&rbd_dev->dev);
>   	mutex_unlock(&ctl_mutex);
>
> @@ -4135,10 +4155,14 @@ static ssize_t rbd_remove(struct bus_type *bus,
>   		goto done;
>   	}
>
> -	if (rbd_dev->open_count) {
> +	spin_lock(&rbd_dev->lock);
> +	if (rbd_dev->open_count)
>   		ret = -EBUSY;
> +	else
> +		set_bit(rbd_dev_flag_removing, &rbd_dev->flags);
> +	spin_unlock(&rbd_dev->lock);
> +	if (ret < 0)
>   		goto done;
> -	}
>
>   	while (rbd_dev->parent_spec) {
>   		struct rbd_device *first = rbd_dev;
>