[PATCH v4] rbd: add ioctl for rbd

[PATCH v4] rbd: add ioctl for rbd

[Guangliang Zhao]

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.

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

Signed-off-by: Guangliang Zhao <guangliang@unitedstack.com>
Reviewed-by: Alex Elder <elder@linaro.org>
Reviewed-by: Josh Durgin <josh.durgin@inktank.com>
---
 drivers/block/rbd.c |   62 +++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 60 insertions(+), 2 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 2f00778..34bcdb7 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);
+	}
+
+	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 */
+	if (rbd_dev->open_count > 1) {
+		ret = -EBUSY;
+		goto out;
+	}
+
+	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
 };
 
 /*
@@ -3027,7 +3086,6 @@ static void rbd_request_fn(struct request_queue *q)
 		__releases(q->queue_lock) __acquires(q->queue_lock)
 {
 	struct rbd_device *rbd_dev = q->queuedata;
-	bool read_only = rbd_dev->mapping.read_only;
 	struct request *rq;
 	int result;
 
@@ -3063,7 +3121,7 @@ static void rbd_request_fn(struct request_queue *q)
 
 		if (write_request) {
 			result = -EROFS;
-			if (read_only)
+			if (rbd_dev->mapping.read_only)
 				goto end_request;
 			rbd_assert(rbd_dev->spec->snap_id == CEPH_NOSNAP);
 		}
-- 
1.7.9.5

[PATCH 0/2] clean up read only mode

[Josh Durgin]

Running with lockdep showed that there were problems holding
rbd_dev->lock while doing in the BLKROSET ioctl patch. Fix this, and
clean up the initial read-only setting to occur during device
initialization instead of every time the device is opened.

These are also available in the wip-rbd-ro branch of ceph-client.git.

Josh Durgin (2):
  rbd: move calls that may sleep out of spin lock range
  rbd: only set disk to read-only once

 drivers/block/rbd.c |   38 ++++++++++++++++++++++++--------------
 1 files changed, 24 insertions(+), 14 deletions(-)

-- 
1.7.2.5

Re: [PATCH 0/2] clean up read only mode

[Guangliang Zhao]

On Mon, Sep 30, 2013 at 10:15:57PM -0700, Josh Durgin wrote:
> Running with lockdep showed that there were problems holding
> rbd_dev->lock while doing in the BLKROSET ioctl patch. Fix this, and
> clean up the initial read-only setting to occur during device
> initialization instead of every time the device is opened.
> 
> These are also available in the wip-rbd-ro branch of ceph-client.git.
> 
> Josh Durgin (2):
>   rbd: move calls that may sleep out of spin lock range
>   rbd: only set disk to read-only once

Both look good.

> 
>  drivers/block/rbd.c |   38 ++++++++++++++++++++++++--------------
>  1 files changed, 24 insertions(+), 14 deletions(-)
> 
> -- 
> 1.7.2.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Best regards,
Guangliang

[PATCH 1/2] rbd: move calls that may sleep out of spin lock range

[Josh Durgin]

get_user() and set_disk_ro() may allocate memory, leading to a
potential deadlock if theye are called while a spin lock is held.

Move the acquisition and release of rbd_dev->lock from rbd_ioctl()
into rbd_ioctl_set_ro(), so it can occur between get_user() and
set_disk_ro().

Signed-off-by: Josh Durgin <josh.durgin@inktank.com>
---
 drivers/block/rbd.c |   36 +++++++++++++++++++++++-------------
 1 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 34bcdb7..b3b1b57 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -510,23 +510,42 @@ static void rbd_release(struct gendisk *disk, fmode_t mode)
 
 static int rbd_ioctl_set_ro(struct rbd_device *rbd_dev, unsigned long arg)
 {
+	int ret = 0;
 	int val;
 	bool ro;
+	bool ro_changed = false;
 
+	/* get_user() may sleep, so call it before taking rbd_dev->lock */
 	if (get_user(val, (int __user *)(arg)))
 		return -EFAULT;
 
+	spin_lock_irq(&rbd_dev->lock);
+	/* prevent others open this device */
+	if (rbd_dev->open_count > 1) {
+		ret = -EBUSY;
+		goto out;
+	}
+
 	ro = val ? true : false;
 	/* Snapshot doesn't allow to write*/
-	if (rbd_dev->spec->snap_id != CEPH_NOSNAP && !ro)
-		return -EROFS;
+	if (rbd_dev->spec->snap_id != CEPH_NOSNAP && !ro) {
+		ret = -EROFS;
+		goto out;
+	}
 
 	if (rbd_dev->mapping.read_only != ro) {
 		rbd_dev->mapping.read_only = ro;
-		set_disk_ro(rbd_dev->disk, ro ? 1 : 0);
+		ro_changed = true;
 	}
 
-	return 0;
+out:
+	spin_unlock_irq(&rbd_dev->lock);
+	/* set_disk_ro() may sleep, so call it after releasing rbd_dev->lock */
+	if (ret == 0 && ro_changed)
+		set_disk_ro(rbd_dev->disk, ro ? 1 : 0);
+
+
+	return ret;
 }
 
 static int rbd_ioctl(struct block_device *bdev, fmode_t mode,
@@ -535,13 +554,6 @@ static int rbd_ioctl(struct block_device *bdev, fmode_t mode,
 	struct rbd_device *rbd_dev = bdev->bd_disk->private_data;
 	int ret = 0;
 
-	spin_lock_irq(&rbd_dev->lock);
-	/* prevent others open this device */
-	if (rbd_dev->open_count > 1) {
-		ret = -EBUSY;
-		goto out;
-	}
-
 	switch (cmd) {
 	case BLKROSET:
 		ret = rbd_ioctl_set_ro(rbd_dev, arg);
@@ -550,8 +562,6 @@ static int rbd_ioctl(struct block_device *bdev, fmode_t mode,
 		ret = -ENOTTY;
 	}
 
-out:
-	spin_unlock_irq(&rbd_dev->lock);
 	return ret;
 }
 
-- 
1.7.2.5

Re: [PATCH 1/2] rbd: move calls that may sleep out of spin lock range

[Alex Elder]

On 10/01/2013 12:15 AM, Josh Durgin wrote:
> get_user() and set_disk_ro() may allocate memory, leading to a
> potential deadlock if theye are called while a spin lock is held.
> 
> Move the acquisition and release of rbd_dev->lock from rbd_ioctl()
> into rbd_ioctl_set_ro(), so it can occur between get_user() and
> set_disk_ro().

This fix looks good.  I have a couple small comments to consider.

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

> Signed-off-by: Josh Durgin <josh.durgin@inktank.com>
> ---
>  drivers/block/rbd.c |   36 +++++++++++++++++++++++-------------
>  1 files changed, 23 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 34bcdb7..b3b1b57 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -510,23 +510,42 @@ static void rbd_release(struct gendisk *disk, fmode_t mode)
>  
>  static int rbd_ioctl_set_ro(struct rbd_device *rbd_dev, unsigned long arg)
>  {
> +	int ret = 0;
>  	int val;
>  	bool ro;
> +	bool ro_changed = false;
>  
> +	/* get_user() may sleep, so call it before taking rbd_dev->lock */
>  	if (get_user(val, (int __user *)(arg)))
>  		return -EFAULT;
>  
> +	spin_lock_irq(&rbd_dev->lock);
> +	/* prevent others open this device */
> +	if (rbd_dev->open_count > 1) {
> +		ret = -EBUSY;
> +		goto out;
> +	}

I like to do as little as possible inside spinlock protection.

> +
>  	ro = val ? true : false;

This assignment can be made outside the lock.

>  	/* Snapshot doesn't allow to write*/
> -	if (rbd_dev->spec->snap_id != CEPH_NOSNAP && !ro)
> -		return -EROFS;
> +	if (rbd_dev->spec->snap_id != CEPH_NOSNAP && !ro) {
> +		ret = -EROFS;
> +		goto out;
> +	}

This check can be too.

>  
>  	if (rbd_dev->mapping.read_only != ro) {
>  		rbd_dev->mapping.read_only = ro;
> -		set_disk_ro(rbd_dev->disk, ro ? 1 : 0);
> +		ro_changed = true;
>  	}
>  
> -	return 0;
> +out:
> +	spin_unlock_irq(&rbd_dev->lock);
> +	/* set_disk_ro() may sleep, so call it after releasing rbd_dev->lock */
> +	if (ret == 0 && ro_changed)
> +		set_disk_ro(rbd_dev->disk, ro ? 1 : 0);
> +

I like white space a lot, but this is one too many.

> +
> +	return ret;
>  }
>  
>  static int rbd_ioctl(struct block_device *bdev, fmode_t mode,
> @@ -535,13 +554,6 @@ static int rbd_ioctl(struct block_device *bdev, fmode_t mode,
>  	struct rbd_device *rbd_dev = bdev->bd_disk->private_data;
>  	int ret = 0;
>  
> -	spin_lock_irq(&rbd_dev->lock);
> -	/* prevent others open this device */
> -	if (rbd_dev->open_count > 1) {
> -		ret = -EBUSY;
> -		goto out;
> -	}
> -
>  	switch (cmd) {
>  	case BLKROSET:
>  		ret = rbd_ioctl_set_ro(rbd_dev, arg);
> @@ -550,8 +562,6 @@ static int rbd_ioctl(struct block_device *bdev, fmode_t mode,
>  		ret = -ENOTTY;
>  	}
>  
> -out:
> -	spin_unlock_irq(&rbd_dev->lock);
>  	return ret;
>  }
>  
> 

[PATCH 2/2] rbd: only set disk to read-only once

[Josh Durgin]

rbd_open(), called every time the device is opened, calls
set_device_ro().  There's no reason to set the device read-only or
read-write every time it is opened. Just do this once during device
setup, using set_disk_ro() instead because the struct block_device
isn't available to us there.

Signed-off-by: Josh Durgin <josh.durgin@inktank.com>
---
 drivers/block/rbd.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index b3b1b57..fc3ebd9 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -490,7 +490,6 @@ static int rbd_open(struct block_device *bdev, fmode_t mode)
 		return -ENOENT;
 
 	(void) get_device(&rbd_dev->dev);
-	set_device_ro(bdev, rbd_dev->mapping.read_only);
 
 	return 0;
 }
@@ -4949,6 +4948,7 @@ static int rbd_dev_device_setup(struct rbd_device *rbd_dev)
 	if (ret)
 		goto err_out_disk;
 	set_capacity(rbd_dev->disk, rbd_dev->mapping.size / SECTOR_SIZE);
+	set_disk_ro(rbd_dev->disk, rbd_dev->mapping.read_only);
 
 	ret = rbd_bus_add_dev(rbd_dev);
 	if (ret)
-- 
1.7.2.5

Re: [PATCH 2/2] rbd: only set disk to read-only once

[Alex Elder]

On 10/01/2013 12:15 AM, Josh Durgin wrote:
> rbd_open(), called every time the device is opened, calls
> set_device_ro().  There's no reason to set the device read-only or
> read-write every time it is opened. Just do this once during device
> setup, using set_disk_ro() instead because the struct block_device
> isn't available to us there.

Looks good; set_disk_ro() sort of makes more sense anyway without
any partitions.

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

> Signed-off-by: Josh Durgin <josh.durgin@inktank.com>
> ---
>  drivers/block/rbd.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index b3b1b57..fc3ebd9 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -490,7 +490,6 @@ static int rbd_open(struct block_device *bdev, fmode_t mode)
>  		return -ENOENT;
>  
>  	(void) get_device(&rbd_dev->dev);
> -	set_device_ro(bdev, rbd_dev->mapping.read_only);
>  
>  	return 0;
>  }
> @@ -4949,6 +4948,7 @@ static int rbd_dev_device_setup(struct rbd_device *rbd_dev)
>  	if (ret)
>  		goto err_out_disk;
>  	set_capacity(rbd_dev->disk, rbd_dev->mapping.size / SECTOR_SIZE);
> +	set_disk_ro(rbd_dev->disk, rbd_dev->mapping.read_only);
>  
>  	ret = rbd_bus_add_dev(rbd_dev);
>  	if (ret)
>