| Message ID | 20171205205442.25414-1-ddiss@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2017-12-05 at 21:54 +0100, David Disseldorp wrote: > A read-only block device must currently be flagged as such when > configuring the iblock backstore, via a readonly=1 configfs parameter. > If a read-only block device is used without explicitly providing the > parameter, then backstore enablement fails with EACCES, e.g. > losetup --read-only /dev/loop0 /lofile > mkdir -p /sys/kernel/config/target/core/iblock_0/loopy > echo "udev_path=/dev/loop0" \ > > /sys/kernel/config/target/core/iblock_0/loopy/control > echo "1" > /sys/kernel/config/target/core/iblock_0/loopy/enable > sh: echo: write error: Permission denied > > This change allows an iblock backstore to be demoted to read-only if the > underlying device is detected as such, via a -EACCES return value from > blkdev_get_by_path(udev_path=$readonly_device, mode & FMODE_WRITE, ...). Hello David, Although the patch itself looks fine to me: do we really need this kind of functionality in the kernel? Is it possible to implement the same functionality in user space? Thanks, Bart.
Hi Bart and David, Bart Van Assche <Bart.VanAssche@wdc.com> writes: > Although the patch itself looks fine to me: do we really need this kind of > functionality in the kernel? Is it possible to implement the same functionality > in user space? We're having exactly the same discussion in NVMe currently, please see the following thread: http://lists.infradead.org/pipermail/linux-nvme/2017-December/014341.html I tend to agree with David (and Sagi for the NVMe case) in that we should handle this in the kernel. Byte, Johannes
On Wed, 6 Dec 2017 04:33:16 +0000, Bart Van Assche wrote: > Although the patch itself looks fine to me: do we really need this kind of > functionality in the kernel? Is it possible to implement the same functionality > in user space? Thanks for the feedback, Bart and Johannes. This functionality could be handled in user-space, but having the error reported when the backstore is enabled, rather than when the backstore->control node is setup makes it less than intuitive for targetcli, etc. Having targetcli call ioctl(BLKROGET) prior to issuing the backstore->control I/O would probably be a cleaner user-space option. Cheers, David -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 6 Dec 2017 13:29:21 +0100, David Disseldorp wrote: > On Wed, 6 Dec 2017 04:33:16 +0000, Bart Van Assche wrote: > > > Although the patch itself looks fine to me: do we really need this kind of > > functionality in the kernel? Is it possible to implement the same functionality > > in user space? > > Thanks for the feedback, Bart and Johannes. This functionality could > be handled in user-space, but having the error reported when the > backstore is enabled, rather than when the backstore->control node is > setup makes it less than intuitive for targetcli, etc. > Having targetcli call ioctl(BLKROGET) prior to issuing the > backstore->control I/O would probably be a cleaner user-space option. FWIW, I've proposed targetcli changes to handle this via: https://github.com/open-iscsi/targetcli-fb/pull/97 This kernel change could be revisited in future if we decide to have LIO support obtaining iblock read-only status from underlying block devices. Cheers, David -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/target/target_core_iblock.c b/drivers/target/target_core_iblock.c index 07c814c42648..cafba262a440 100644 --- a/drivers/target/target_core_iblock.c +++ b/drivers/target/target_core_iblock.c @@ -103,6 +103,7 @@ static int iblock_configure_device(struct se_device *dev) pr_debug( "IBLOCK: Claiming struct block_device: %s\n", ib_dev->ibd_udev_path); +ro_fallback: mode = FMODE_READ|FMODE_EXCL; if (!ib_dev->ibd_readonly) mode |= FMODE_WRITE; @@ -112,6 +113,17 @@ static int iblock_configure_device(struct se_device *dev) bd = blkdev_get_by_path(ib_dev->ibd_udev_path, mode, ib_dev); if (IS_ERR(bd)) { ret = PTR_ERR(bd); + if (!ib_dev->ibd_readonly && (ret == -EACCES)) { + /* + * EACCES is returned if FMODE_WRITE is set while + * getting a read-only block device. Try once again + * without FMODE_WRITE to allow for configuration of a + * read-only block device without an explicit readonly=1 + * configfs parameter in iblock_set_configfs_dev_params(). + */ + ib_dev->ibd_readonly = 1; + goto ro_fallback; + } goto out_free_bioset; } ib_dev->ibd_bd = bd;