| Message ID | 1379486151-8939-1-git-send-email-guangliang@unitedstack.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 > }; > > /* > -- 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
On 09/21/2013 08:53 AM, Alex Elder wrote: > 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) To be clear, you have to use the sysfs interface with the 'ro' option to test this case, rather than using the 'rbd map' command, which has no way to pass the 'ro' or 'rw' options. The syntax is documented in Documentation/ABI/testing/sysfs-bus-rbd. > - 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. I'm not sure about this either, it'd be good to verify it's ok by running with lockdep. >> + } >> + >> + 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. I think with the current structure (rbd_open() calling set_device_ro()) protecting this with a spinlock won't cause a lock inversion, since rbd_open() is not called with the bdev lock held. With the change to rbd_request_fn() Alex mentioned, could you verify it has no problems with lockdep enabled? If so, it looks good to me. Reviewed-by: Josh Durgin <josh.durgin@inktank.com> >> + >> + 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 >> }; >> >> /* >> > -- 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
On Mon, Sep 23, 2013 at 01:06:35AM -0700, Josh Durgin wrote: > On 09/21/2013 08:53 AM, Alex Elder wrote: > >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). Sorry, forgot it. > > > >>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) All of above have been tested manually, and I also think it should be better if add these scripts into qa/rbd/rbd.sh > > To be clear, you have to use the sysfs interface with the 'ro' option > to test this case, rather than using the 'rbd map' command, which > has no way to pass the 'ro' or 'rw' options. > > The syntax is documented in Documentation/ABI/testing/sysfs-bus-rbd. Thanks. BTW, I have submitted a patch for it, and "rbd map" could map readonly device soon. > > >- 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. > > I'm not sure about this either, it'd be good to verify it's > ok by running with lockdep. I have turned on the lockdep, and I couldn't see anything related with rbd in /proc/lockdep and dmesg, so I think that should be OK. > > >>+ } > >>+ > >>+ 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. > > I think with the current structure (rbd_open() calling set_device_ro()) > protecting this with a spinlock won't cause a lock inversion, since > rbd_open() is not called with the bdev lock held. > > With the change to rbd_request_fn() Alex mentioned, could you verify > it has no problems with lockdep enabled? If so, it looks good to me. All of the changes and lockdep enabled. > > Reviewed-by: Josh Durgin <josh.durgin@inktank.com> > > >>+ > >>+ 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 > >> }; > >> > >> /* > >> > > >
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); + } + + 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 }; /*
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. 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 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(+)