Re: [PATCH v3] rbd: add ioctl for rbd

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

On 09/18/2013 01:35 AM, Guangliang Zhao wrote:
> When running the following commands:
>     [root@ceph0 mnt]# blockdev --setro /dev/rbd1
>     [root@ceph0 mnt]# blockdev --getro /dev/rbd1
>     0
> 
> The block setro didn't take effect, it is because
> the rbd doesn't support ioctl of block driver.

Nicely done.

I have a couple small comments below, and one simple
change that needs to be made.

I also point out another issue about the use of
the spinlock to protect against read-only state
changes; I'd like Josh to weigh in on how he thinks
it might best be handled.

Provided you make the simple change, and Josh has no
problem with the read-only state thing:

Reviewed-by: Alex Elder <elder@linaro.org>

The required change is something Josh mentioned.  In
rbd_request_fn(), the variable read_only is defined
and assigned at the top of the function.  That line
needs to be inside the while loop, to ensure that
the most up-to-date value is used (in case it gets
changed between requests).

> The results with this patch:
> root@ceph-client01:~# rbd map img
> root@ceph-client01:~# blockdev --getro /dev/rbd1
> 0
> root@ceph-client01:~# cat /sys/block/rbd1/ro
> 0
> root@ceph-client01:~# blockdev --setro /dev/rbd1
> root@ceph-client01:~# blockdev --getro /dev/rbd1
> 1
> root@ceph-client01:~# cat /sys/block/rbd1/ro
> 1
> root@ceph-client01:~# dd if=/dev/zero of=/dev/rbd1 count=1
> dd: opening `/dev/rbd1': Read-only file system`
> root@ceph-client01:~# blockdev --setrw /dev/rbd1
> root@ceph-client01:~# blockdev --getro /dev/rbd1
> 0
> root@ceph-client01:~# cat /sys/block/rbd1/ro
> 0
> root@ceph-client01:~# dd if=/dev/zero of=/dev/rbd1 count=1
> 1+0 records in
> 1+0 records out
> 512 bytes (512 B) copied, 0.14989 s, 3.4 kB/s

I don't necessarily want to see all this testing output
in the commit description (a summary would suffice).  As
Josh suggested, some sort of test script to validate all
of this would be much appreciated.  It can be very simple:
- map rbd device
- run "blockdev --getro" on it, and verify it reports 0
- run "blockdev --setro" and verify it is now read-only
... and so on.

Tests we suggested, which I expect will pass but I
don't see evidence of it in your description above:
- verify changing the state (setro or setrw) fails on
  a device that's already open (e.g. mounted) (EBUSY)
- verify setro on a snapshot that's mounted (EBUSY)
- verify setrw on a non-snapshot image that is mapped
  read-only (EROFS)
- verify setrw on a snapshot fails (EROFS)

> This resolves:
> 	http://tracker.ceph.com/issues/6265
> 
> Signed-off-by: Guangliang Zhao <guangliang@unitedstack.com>
> ---
>  drivers/block/rbd.c |   59 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 59 insertions(+)
> 
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 2f00778..fea826d 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -508,10 +508,69 @@ static void rbd_release(struct gendisk *disk, fmode_t mode)
>  	put_device(&rbd_dev->dev);
>  }
>  
> +static int rbd_ioctl_set_ro(struct rbd_device *rbd_dev, unsigned long arg)
> +{
> +	int val;
> +	bool ro;
> +
> +	if (get_user(val, (int __user *)(arg)))
> +		return -EFAULT;
> +
> +	ro = val ? true : false;
> +	/* Snapshot doesn't allow to write*/
> +	if (rbd_dev->spec->snap_id != CEPH_NOSNAP && !ro)
> +		return -EROFS;
> +
> +	if (rbd_dev->mapping.read_only != ro) {
> +		rbd_dev->mapping.read_only = ro;
> +		set_disk_ro(rbd_dev->disk, ro ? 1 : 0);

This is interesting.  Josh mentioned that the set_device_ro()
call you had here previously was not needed, because the generic
ioctl code handled it.

But I believe calling set_disk_ro() (which triggers a udev event)
is the right thing to do, even though rbd doesn't support partitions.
In fact, it might be more appropriate for the generic code to call
that using bdev->bd_disk.  Hmmm.  (I don't have time to look into
this any further right now, maybe if someone thinks it's worth
pursuing they can do so.)

In the mean time this does more than set_device_ro(), and the
redundancy is harmless.  HOWEVER I believe that calling
this while holding the spinlock will cause locking problems
(or may just be a bad idea).  I don't know for sure; let
lockdep be your guide.

> +	}
> +
> +	return 0;
> +}
> +
> +static int rbd_ioctl(struct block_device *bdev, fmode_t mode,
> +			unsigned int cmd, unsigned long arg)
> +{
> +	struct rbd_device *rbd_dev = bdev->bd_disk->private_data;
> +	int ret = 0;
> +
> +	spin_lock_irq(&rbd_dev->lock);
> +	/* prevent others open this device */

I think indicating something more like "make sure we hold the only
reference to this device" would make it clear why you're checking
against 1 rather than 0.

> +	if (rbd_dev->open_count > 1) {
> +		ret = -EBUSY;
> +		goto out;
> +	}

You are adding a new ability to change a fundamental state
for an image.  You're using the spinlock now to protect
the read-only state, where previously it was only used to
protect the open count and REMOVING state/flag.  This
may mean you should check the read_only flag in rbd_open()
inside the spinlock there.  And you may need to call the
set_device_ro() there under protection of that lock.

As I mentioned above, holding the lock could also lead
to a problem when calling outside code (set_disk_ro()).
If that's the case you may need to devise a different
(possibly additional) way to protect this.

Josh, please offer your insights.

> +
> +	switch (cmd) {
> +	case BLKROSET:
> +		ret = rbd_ioctl_set_ro(rbd_dev, arg);
> +		break;
> +	default:
> +		ret = -ENOTTY;
> +	}
> +
> +out:
> +	spin_unlock_irq(&rbd_dev->lock);
> +	return ret;
> +}
> +
> +#ifdef CONFIG_COMPAT
> +static int rbd_compat_ioctl(struct block_device *bdev, fmode_t mode,
> +				unsigned int cmd, unsigned long arg)
> +{
> +	return rbd_ioctl(bdev, mode, cmd, arg);
> +}
> +#endif /* CONFIG_COMPAT */
> +
>  static const struct block_device_operations rbd_bd_ops = {
>  	.owner			= THIS_MODULE,
>  	.open			= rbd_open,
>  	.release		= rbd_release,
> +	.ioctl			= rbd_ioctl,
> +#ifdef CONFIG_COMPAT
> +	.compat_ioctl		= rbd_compat_ioctl,
> +#endif
>  };
>  
>  /*
>