* [PATCH 01/10] mtd_blkdevs: don't override BLKFLSBUF
2020-11-03 10:00 block ioctl cleanups v2 Christoph Hellwig
@ 2020-11-03 10:00 ` Christoph Hellwig
2020-11-03 10:00 ` [PATCH 02/10] block: don't call into the driver for BLKFLSBUF Christoph Hellwig
` (9 subsequent siblings)
10 siblings, 0 replies; 19+ messages in thread
From: Christoph Hellwig @ 2020-11-03 10:00 UTC (permalink / raw)
To: Jens Axboe
Cc: Ilya Dryomov, Song Liu, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, Stefan Haberland, Jan Hoeppner, linux-block,
ceph-devel, linux-bcache, linux-raid, linux-mtd, linux-s390
BLKFLSBUF is not supposed to actually send a flush command to the device,
but to tear down buffer cache structures. Remove the mtd_blkdevs
implementation and just use the default semantics instead.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Acked-by: Richard Weinberger <richard@nod.at>
---
drivers/mtd/mtd_blkdevs.c | 28 ----------------------------
1 file changed, 28 deletions(-)
diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c
index 0c05f77f9b216e..fb8e12d590a13a 100644
--- a/drivers/mtd/mtd_blkdevs.c
+++ b/drivers/mtd/mtd_blkdevs.c
@@ -298,38 +298,10 @@ static int blktrans_getgeo(struct block_device *bdev, struct hd_geometry *geo)
return ret;
}
-static int blktrans_ioctl(struct block_device *bdev, fmode_t mode,
- unsigned int cmd, unsigned long arg)
-{
- struct mtd_blktrans_dev *dev = blktrans_dev_get(bdev->bd_disk);
- int ret = -ENXIO;
-
- if (!dev)
- return ret;
-
- mutex_lock(&dev->lock);
-
- if (!dev->mtd)
- goto unlock;
-
- switch (cmd) {
- case BLKFLSBUF:
- ret = dev->tr->flush ? dev->tr->flush(dev) : 0;
- break;
- default:
- ret = -ENOTTY;
- }
-unlock:
- mutex_unlock(&dev->lock);
- blktrans_dev_put(dev);
- return ret;
-}
-
static const struct block_device_operations mtd_block_ops = {
.owner = THIS_MODULE,
.open = blktrans_open,
.release = blktrans_release,
- .ioctl = blktrans_ioctl,
.getgeo = blktrans_getgeo,
};
--
2.28.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 02/10] block: don't call into the driver for BLKFLSBUF
2020-11-03 10:00 block ioctl cleanups v2 Christoph Hellwig
2020-11-03 10:00 ` [PATCH 01/10] mtd_blkdevs: don't override BLKFLSBUF Christoph Hellwig
@ 2020-11-03 10:00 ` Christoph Hellwig
2020-11-03 10:00 ` [PATCH 03/10] block: add a new set_read_only method Christoph Hellwig
` (8 subsequent siblings)
10 siblings, 0 replies; 19+ messages in thread
From: Christoph Hellwig @ 2020-11-03 10:00 UTC (permalink / raw)
To: Jens Axboe
Cc: Ilya Dryomov, Song Liu, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, Stefan Haberland, Jan Hoeppner, linux-block,
ceph-devel, linux-bcache, linux-raid, linux-mtd, linux-s390
BLKFLSBUF is entirely contained in the block core, and there is no
good reason to give the driver a hook into processing it.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
block/ioctl.c | 7 -------
1 file changed, 7 deletions(-)
diff --git a/block/ioctl.c b/block/ioctl.c
index 3fbc382eb926d4..c6d8863f040945 100644
--- a/block/ioctl.c
+++ b/block/ioctl.c
@@ -369,15 +369,8 @@ static inline int is_unrecognized_ioctl(int ret)
static int blkdev_flushbuf(struct block_device *bdev, fmode_t mode,
unsigned cmd, unsigned long arg)
{
- int ret;
-
if (!capable(CAP_SYS_ADMIN))
return -EACCES;
-
- ret = __blkdev_driver_ioctl(bdev, mode, cmd, arg);
- if (!is_unrecognized_ioctl(ret))
- return ret;
-
fsync_bdev(bdev);
invalidate_bdev(bdev);
return 0;
--
2.28.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 03/10] block: add a new set_read_only method
2020-11-03 10:00 block ioctl cleanups v2 Christoph Hellwig
2020-11-03 10:00 ` [PATCH 01/10] mtd_blkdevs: don't override BLKFLSBUF Christoph Hellwig
2020-11-03 10:00 ` [PATCH 02/10] block: don't call into the driver for BLKFLSBUF Christoph Hellwig
@ 2020-11-03 10:00 ` Christoph Hellwig
2020-11-03 10:00 ` [PATCH 04/10] rbd: implement ->set_read_only to hook into BLKROSET processing Christoph Hellwig
` (7 subsequent siblings)
10 siblings, 0 replies; 19+ messages in thread
From: Christoph Hellwig @ 2020-11-03 10:00 UTC (permalink / raw)
To: Jens Axboe
Cc: Ilya Dryomov, Song Liu, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, Stefan Haberland, Jan Hoeppner, linux-block,
ceph-devel, linux-bcache, linux-raid, linux-mtd, linux-s390
Add a new method to allow for driver-specific processing when setting or
clearing the block device read-only state. This allows to replace the
cumbersome and error-prone override of the whole ioctl implementation.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
block/ioctl.c | 5 +++++
include/linux/blkdev.h | 1 +
2 files changed, 6 insertions(+)
diff --git a/block/ioctl.c b/block/ioctl.c
index c6d8863f040945..a6fa16b9770593 100644
--- a/block/ioctl.c
+++ b/block/ioctl.c
@@ -389,6 +389,11 @@ static int blkdev_roset(struct block_device *bdev, fmode_t mode,
return ret;
if (get_user(n, (int __user *)arg))
return -EFAULT;
+ if (bdev->bd_disk->fops->set_read_only) {
+ ret = bdev->bd_disk->fops->set_read_only(bdev, n);
+ if (ret)
+ return ret;
+ }
set_device_ro(bdev, n);
return 0;
}
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 639cae2c158b59..5c1ba8a8d2bc7e 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1850,6 +1850,7 @@ struct block_device_operations {
void (*unlock_native_capacity) (struct gendisk *);
int (*revalidate_disk) (struct gendisk *);
int (*getgeo)(struct block_device *, struct hd_geometry *);
+ int (*set_read_only)(struct block_device *bdev, bool ro);
/* this callback is with swap_lock and sometimes page table lock held */
void (*swap_slot_free_notify) (struct block_device *, unsigned long);
int (*report_zones)(struct gendisk *, sector_t sector,
--
2.28.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 04/10] rbd: implement ->set_read_only to hook into BLKROSET processing
2020-11-03 10:00 block ioctl cleanups v2 Christoph Hellwig
` (2 preceding siblings ...)
2020-11-03 10:00 ` [PATCH 03/10] block: add a new set_read_only method Christoph Hellwig
@ 2020-11-03 10:00 ` Christoph Hellwig
2020-11-03 10:00 ` [PATCH 05/10] md: " Christoph Hellwig
` (6 subsequent siblings)
10 siblings, 0 replies; 19+ messages in thread
From: Christoph Hellwig @ 2020-11-03 10:00 UTC (permalink / raw)
To: Jens Axboe
Cc: Ilya Dryomov, Song Liu, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, Stefan Haberland, Jan Hoeppner, linux-block,
ceph-devel, linux-bcache, linux-raid, linux-mtd, linux-s390
Implement the ->set_read_only method instead of parsing the actual
ioctl command.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Acked-by: Ilya Dryomov <idryomov@gmail.com>
---
drivers/block/rbd.c | 40 ++++------------------------------------
1 file changed, 4 insertions(+), 36 deletions(-)
diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index f84128abade319..671733e459cf47 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -692,12 +692,9 @@ 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)
+static int rbd_set_read_only(struct block_device *bdev, bool ro)
{
- int ro;
-
- if (get_user(ro, (int __user *)arg))
- return -EFAULT;
+ struct rbd_device *rbd_dev = bdev->bd_disk->private_data;
/*
* Both images mapped read-only and snapshots can't be marked
@@ -710,43 +707,14 @@ static int rbd_ioctl_set_ro(struct rbd_device *rbd_dev, unsigned long arg)
rbd_assert(!rbd_is_snap(rbd_dev));
}
- /* Let blkdev_roset() handle it */
- return -ENOTTY;
-}
-
-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;
-
- switch (cmd) {
- case BLKROSET:
- ret = rbd_ioctl_set_ro(rbd_dev, arg);
- break;
- default:
- ret = -ENOTTY;
- }
-
- 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);
+ return 0;
}
-#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
+ .set_read_only = rbd_set_read_only,
};
/*
--
2.28.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 05/10] md: implement ->set_read_only to hook into BLKROSET processing
2020-11-03 10:00 block ioctl cleanups v2 Christoph Hellwig
` (3 preceding siblings ...)
2020-11-03 10:00 ` [PATCH 04/10] rbd: implement ->set_read_only to hook into BLKROSET processing Christoph Hellwig
@ 2020-11-03 10:00 ` Christoph Hellwig
2020-11-06 0:59 ` Song Liu
2020-11-03 10:00 ` [PATCH 06/10] dasd: " Christoph Hellwig
` (5 subsequent siblings)
10 siblings, 1 reply; 19+ messages in thread
From: Christoph Hellwig @ 2020-11-03 10:00 UTC (permalink / raw)
To: Jens Axboe
Cc: Ilya Dryomov, Song Liu, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, Stefan Haberland, Jan Hoeppner, linux-block,
ceph-devel, linux-bcache, linux-raid, linux-mtd, linux-s390
Implement the ->set_read_only method instead of parsing the actual
ioctl command.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/md/md.c | 62 ++++++++++++++++++++++++-------------------------
1 file changed, 31 insertions(+), 31 deletions(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 98bac4f304ae26..96d31336ad43fe 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -7480,7 +7480,6 @@ static inline bool md_ioctl_valid(unsigned int cmd)
{
switch (cmd) {
case ADD_NEW_DISK:
- case BLKROSET:
case GET_ARRAY_INFO:
case GET_BITMAP_FILE:
case GET_DISK_INFO:
@@ -7507,7 +7506,6 @@ static int md_ioctl(struct block_device *bdev, fmode_t mode,
int err = 0;
void __user *argp = (void __user *)arg;
struct mddev *mddev = NULL;
- int ro;
bool did_set_md_closing = false;
if (!md_ioctl_valid(cmd))
@@ -7687,35 +7685,6 @@ static int md_ioctl(struct block_device *bdev, fmode_t mode,
goto unlock;
}
break;
-
- case BLKROSET:
- if (get_user(ro, (int __user *)(arg))) {
- err = -EFAULT;
- goto unlock;
- }
- err = -EINVAL;
-
- /* if the bdev is going readonly the value of mddev->ro
- * does not matter, no writes are coming
- */
- if (ro)
- goto unlock;
-
- /* are we are already prepared for writes? */
- if (mddev->ro != 1)
- goto unlock;
-
- /* transitioning to readauto need only happen for
- * arrays that call md_write_start
- */
- if (mddev->pers) {
- err = restart_array(mddev);
- if (err == 0) {
- mddev->ro = 2;
- set_disk_ro(mddev->gendisk, 0);
- }
- }
- goto unlock;
}
/*
@@ -7809,6 +7778,36 @@ static int md_compat_ioctl(struct block_device *bdev, fmode_t mode,
}
#endif /* CONFIG_COMPAT */
+static int md_set_read_only(struct block_device *bdev, bool ro)
+{
+ struct mddev *mddev = bdev->bd_disk->private_data;
+ int err;
+
+ err = mddev_lock(mddev);
+ if (err)
+ return err;
+
+ if (!mddev->raid_disks && !mddev->external) {
+ err = -ENODEV;
+ goto out_unlock;
+ }
+
+ /*
+ * Transitioning to read-auto need only happen for arrays that call
+ * md_write_start and which are not ready for writes yet.
+ */
+ if (!ro && mddev->ro == 1 && mddev->pers) {
+ err = restart_array(mddev);
+ if (err)
+ goto out_unlock;
+ mddev->ro = 2;
+ }
+
+out_unlock:
+ mddev_unlock(mddev);
+ return err;
+}
+
static int md_open(struct block_device *bdev, fmode_t mode)
{
/*
@@ -7886,6 +7885,7 @@ const struct block_device_operations md_fops =
#endif
.getgeo = md_getgeo,
.check_events = md_check_events,
+ .set_read_only = md_set_read_only,
};
static int md_thread(void *arg)
--
2.28.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 05/10] md: implement ->set_read_only to hook into BLKROSET processing
2020-11-03 10:00 ` [PATCH 05/10] md: " Christoph Hellwig
@ 2020-11-06 0:59 ` Song Liu
0 siblings, 0 replies; 19+ messages in thread
From: Song Liu @ 2020-11-06 0:59 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Jens Axboe, Ilya Dryomov, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, Stefan Haberland, Jan Hoeppner, linux-block,
ceph-devel, linux-bcache, linux-raid, linux-mtd, linux-s390
On Tue, Nov 3, 2020 at 2:13 AM Christoph Hellwig <hch@lst.de> wrote:
>
> Implement the ->set_read_only method instead of parsing the actual
> ioctl command.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
LGTM!
Acked-by: Song Liu <song@kernel.org>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 06/10] dasd: implement ->set_read_only to hook into BLKROSET processing
2020-11-03 10:00 block ioctl cleanups v2 Christoph Hellwig
` (4 preceding siblings ...)
2020-11-03 10:00 ` [PATCH 05/10] md: " Christoph Hellwig
@ 2020-11-03 10:00 ` Christoph Hellwig
2020-11-05 20:56 ` Stefan Haberland
2020-11-03 10:00 ` [PATCH 07/10] block: don't call into the driver for BLKROSET Christoph Hellwig
` (4 subsequent siblings)
10 siblings, 1 reply; 19+ messages in thread
From: Christoph Hellwig @ 2020-11-03 10:00 UTC (permalink / raw)
To: Jens Axboe
Cc: Ilya Dryomov, Song Liu, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, Stefan Haberland, Jan Hoeppner, linux-block,
ceph-devel, linux-bcache, linux-raid, linux-mtd, linux-s390
Implement the ->set_read_only method instead of parsing the actual
ioctl command.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/s390/block/dasd.c | 1 +
drivers/s390/block/dasd_int.h | 3 ++-
drivers/s390/block/dasd_ioctl.c | 27 +++++++++------------------
3 files changed, 12 insertions(+), 19 deletions(-)
diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
index eb17fea8075c6f..db24e04ee9781e 100644
--- a/drivers/s390/block/dasd.c
+++ b/drivers/s390/block/dasd.c
@@ -3394,6 +3394,7 @@ dasd_device_operations = {
.ioctl = dasd_ioctl,
.compat_ioctl = dasd_ioctl,
.getgeo = dasd_getgeo,
+ .set_read_only = dasd_set_read_only,
};
/*******************************************************************************
diff --git a/drivers/s390/block/dasd_int.h b/drivers/s390/block/dasd_int.h
index fa552f9f166671..c59a0d63b506e6 100644
--- a/drivers/s390/block/dasd_int.h
+++ b/drivers/s390/block/dasd_int.h
@@ -844,7 +844,8 @@ int dasd_scan_partitions(struct dasd_block *);
void dasd_destroy_partitions(struct dasd_block *);
/* externals in dasd_ioctl.c */
-int dasd_ioctl(struct block_device *, fmode_t, unsigned int, unsigned long);
+int dasd_ioctl(struct block_device *, fmode_t, unsigned int, unsigned long);
+int dasd_set_read_only(struct block_device *bdev, bool ro);
/* externals in dasd_proc.c */
int dasd_proc_init(void);
diff --git a/drivers/s390/block/dasd_ioctl.c b/drivers/s390/block/dasd_ioctl.c
index cb6427fb9f3d16..3359559517bfcf 100644
--- a/drivers/s390/block/dasd_ioctl.c
+++ b/drivers/s390/block/dasd_ioctl.c
@@ -532,28 +532,22 @@ static int dasd_ioctl_information(struct dasd_block *block, void __user *argp,
/*
* Set read only
*/
-static int
-dasd_ioctl_set_ro(struct block_device *bdev, void __user *argp)
+int dasd_set_read_only(struct block_device *bdev, bool ro)
{
struct dasd_device *base;
- int intval, rc;
+ int rc;
- if (!capable(CAP_SYS_ADMIN))
- return -EACCES;
+ /* do not manipulate hardware state for partitions */
if (bdev_is_partition(bdev))
- // ro setting is not allowed for partitions
- return -EINVAL;
- if (get_user(intval, (int __user *)argp))
- return -EFAULT;
+ return 0;
+
base = dasd_device_from_gendisk(bdev->bd_disk);
if (!base)
return -ENODEV;
- if (!intval && test_bit(DASD_FLAG_DEVICE_RO, &base->flags)) {
- dasd_put_device(base);
- return -EROFS;
- }
- set_disk_ro(bdev->bd_disk, intval);
- rc = dasd_set_feature(base->cdev, DASD_FEATURE_READONLY, intval);
+ if (!ro && test_bit(DASD_FLAG_DEVICE_RO, &base->flags))
+ rc = -EROFS;
+ else
+ rc = dasd_set_feature(base->cdev, DASD_FEATURE_READONLY, ro);
dasd_put_device(base);
return rc;
}
@@ -633,9 +627,6 @@ int dasd_ioctl(struct block_device *bdev, fmode_t mode,
case BIODASDPRRST:
rc = dasd_ioctl_reset_profile(block);
break;
- case BLKROSET:
- rc = dasd_ioctl_set_ro(bdev, argp);
- break;
case DASDAPIVER:
rc = dasd_ioctl_api_version(argp);
break;
--
2.28.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 06/10] dasd: implement ->set_read_only to hook into BLKROSET processing
2020-11-03 10:00 ` [PATCH 06/10] dasd: " Christoph Hellwig
@ 2020-11-05 20:56 ` Stefan Haberland
2020-11-06 14:02 ` Christoph Hellwig
0 siblings, 1 reply; 19+ messages in thread
From: Stefan Haberland @ 2020-11-05 20:56 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Jens Axboe, Ilya Dryomov, Song Liu, Miquel Raynal,
Richard Weinberger, Vignesh Raghavendra, Jan Hoeppner,
linux-block, ceph-devel, linux-bcache, linux-raid, linux-mtd,
linux-s390
Christoph Hellwig <hch@lst.de> schrieb am Tue, 03. Nov 11:00:
> Implement the ->set_read_only method instead of parsing the actual
> ioctl command.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> drivers/s390/block/dasd.c | 1 +
> drivers/s390/block/dasd_int.h | 3 ++-
> drivers/s390/block/dasd_ioctl.c | 27 +++++++++------------------
> 3 files changed, 12 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
> index eb17fea8075c6f..db24e04ee9781e 100644
> --- a/drivers/s390/block/dasd.c
> +++ b/drivers/s390/block/dasd.c
> @@ -3394,6 +3394,7 @@ dasd_device_operations = {
> .ioctl = dasd_ioctl,
> .compat_ioctl = dasd_ioctl,
> .getgeo = dasd_getgeo,
> + .set_read_only = dasd_set_read_only,
> };
>
> /*******************************************************************************
> diff --git a/drivers/s390/block/dasd_int.h b/drivers/s390/block/dasd_int.h
> index fa552f9f166671..c59a0d63b506e6 100644
> --- a/drivers/s390/block/dasd_int.h
> +++ b/drivers/s390/block/dasd_int.h
> @@ -844,7 +844,8 @@ int dasd_scan_partitions(struct dasd_block *);
> void dasd_destroy_partitions(struct dasd_block *);
>
> /* externals in dasd_ioctl.c */
> -int dasd_ioctl(struct block_device *, fmode_t, unsigned int, unsigned long);
> +int dasd_ioctl(struct block_device *, fmode_t, unsigned int, unsigned long);
> +int dasd_set_read_only(struct block_device *bdev, bool ro);
>
> /* externals in dasd_proc.c */
> int dasd_proc_init(void);
> diff --git a/drivers/s390/block/dasd_ioctl.c b/drivers/s390/block/dasd_ioctl.c
> index cb6427fb9f3d16..3359559517bfcf 100644
> --- a/drivers/s390/block/dasd_ioctl.c
> +++ b/drivers/s390/block/dasd_ioctl.c
> @@ -532,28 +532,22 @@ static int dasd_ioctl_information(struct dasd_block *block, void __user *argp,
> /*
> * Set read only
> */
> -static int
> -dasd_ioctl_set_ro(struct block_device *bdev, void __user *argp)
> +int dasd_set_read_only(struct block_device *bdev, bool ro)
> {
> struct dasd_device *base;
> - int intval, rc;
> + int rc;
>
> - if (!capable(CAP_SYS_ADMIN))
> - return -EACCES;
> + /* do not manipulate hardware state for partitions */
> if (bdev_is_partition(bdev))
> - // ro setting is not allowed for partitions
> - return -EINVAL;
> - if (get_user(intval, (int __user *)argp))
> - return -EFAULT;
> + return 0;
> +
> base = dasd_device_from_gendisk(bdev->bd_disk);
> if (!base)
> return -ENODEV;
> - if (!intval && test_bit(DASD_FLAG_DEVICE_RO, &base->flags)) {
> - dasd_put_device(base);
> - return -EROFS;
> - }
> - set_disk_ro(bdev->bd_disk, intval);
While testing this patch I just noticed that when I set a device readonly this is
not going to be passed on to the partitions on this device any longer.
This is caused by the removed call to set_disk_ro().
Is this intentional or was this removed by accident?
> - rc = dasd_set_feature(base->cdev, DASD_FEATURE_READONLY, intval);
> + if (!ro && test_bit(DASD_FLAG_DEVICE_RO, &base->flags))
> + rc = -EROFS;
> + else
> + rc = dasd_set_feature(base->cdev, DASD_FEATURE_READONLY, ro);
> dasd_put_device(base);
> return rc;
> }
> @@ -633,9 +627,6 @@ int dasd_ioctl(struct block_device *bdev, fmode_t mode,
> case BIODASDPRRST:
> rc = dasd_ioctl_reset_profile(block);
> break;
> - case BLKROSET:
> - rc = dasd_ioctl_set_ro(bdev, argp);
> - break;
> case DASDAPIVER:
> rc = dasd_ioctl_api_version(argp);
> break;
> --
> 2.28.0
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 06/10] dasd: implement ->set_read_only to hook into BLKROSET processing
2020-11-05 20:56 ` Stefan Haberland
@ 2020-11-06 14:02 ` Christoph Hellwig
2020-11-06 16:08 ` Stefan Haberland
0 siblings, 1 reply; 19+ messages in thread
From: Christoph Hellwig @ 2020-11-06 14:02 UTC (permalink / raw)
To: Stefan Haberland
Cc: Christoph Hellwig, Jens Axboe, Ilya Dryomov, Song Liu,
Miquel Raynal, Richard Weinberger, Vignesh Raghavendra,
Jan Hoeppner, linux-block, ceph-devel, linux-bcache, linux-raid,
linux-mtd, linux-s390
On Thu, Nov 05, 2020 at 09:56:47PM +0100, Stefan Haberland wrote:
> > + /* do not manipulate hardware state for partitions */
> > if (bdev_is_partition(bdev))
> > - // ro setting is not allowed for partitions
> > - return -EINVAL;
> > - if (get_user(intval, (int __user *)argp))
> > - return -EFAULT;
> > + return 0;
> > +
> > base = dasd_device_from_gendisk(bdev->bd_disk);
> > if (!base)
> > return -ENODEV;
> > - if (!intval && test_bit(DASD_FLAG_DEVICE_RO, &base->flags)) {
> > - dasd_put_device(base);
> > - return -EROFS;
> > - }
> > - set_disk_ro(bdev->bd_disk, intval);
>
>
> While testing this patch I just noticed that when I set a device readonly this is
> not going to be passed on to the partitions on this device any longer.
>
> This is caused by the removed call to set_disk_ro().
>
> Is this intentional or was this removed by accident?
It was unintentionally intentional :)
The generic code used already by almost all drivers in mainline only
calls set_device_ro from blkdev_roset, that is it only sets the main
device read-only. dasd was the outlier here, and I didn't notice it
actually called set_disk_ro instead of set_device_ro. That being
said I think setting all the partitions read-only as well when the
full device is set read-only makes perfect sense. I'm just a little
worried it could cause regressions. Let me prepare a follow on patch
on top of the series that switches to that behavior.
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 06/10] dasd: implement ->set_read_only to hook into BLKROSET processing
2020-11-06 14:02 ` Christoph Hellwig
@ 2020-11-06 16:08 ` Stefan Haberland
0 siblings, 0 replies; 19+ messages in thread
From: Stefan Haberland @ 2020-11-06 16:08 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Jens Axboe, Ilya Dryomov, Song Liu, Miquel Raynal,
Richard Weinberger, Vignesh Raghavendra, Jan Hoeppner,
linux-block, ceph-devel, linux-bcache, linux-raid, linux-mtd,
linux-s390
Christoph Hellwig <hch@lst.de> schrieb am Fri, 06. Nov 15:02:
> On Thu, Nov 05, 2020 at 09:56:47PM +0100, Stefan Haberland wrote:
> > > + /* do not manipulate hardware state for partitions */
> > > if (bdev_is_partition(bdev))
> > > - // ro setting is not allowed for partitions
> > > - return -EINVAL;
> > > - if (get_user(intval, (int __user *)argp))
> > > - return -EFAULT;
> > > + return 0;
> > > +
> > > base = dasd_device_from_gendisk(bdev->bd_disk);
> > > if (!base)
> > > return -ENODEV;
> > > - if (!intval && test_bit(DASD_FLAG_DEVICE_RO, &base->flags)) {
> > > - dasd_put_device(base);
> > > - return -EROFS;
> > > - }
> > > - set_disk_ro(bdev->bd_disk, intval);
> >
> >
> > While testing this patch I just noticed that when I set a device readonly this is
> > not going to be passed on to the partitions on this device any longer.
> >
> > This is caused by the removed call to set_disk_ro().
> >
> > Is this intentional or was this removed by accident?
>
> It was unintentionally intentional :)
>
> The generic code used already by almost all drivers in mainline only
> calls set_device_ro from blkdev_roset, that is it only sets the main
> device read-only. dasd was the outlier here, and I didn't notice it
> actually called set_disk_ro instead of set_device_ro. That being
> said I think setting all the partitions read-only as well when the
> full device is set read-only makes perfect sense. I'm just a little
> worried it could cause regressions. Let me prepare a follow on patch
> on top of the series that switches to that behavior.
Makes sense.
I am fine with that.
With this in mind:
Reviewed-by: Stefan Haberland <sth@linux.ibm.com>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 07/10] block: don't call into the driver for BLKROSET
2020-11-03 10:00 block ioctl cleanups v2 Christoph Hellwig
` (5 preceding siblings ...)
2020-11-03 10:00 ` [PATCH 06/10] dasd: " Christoph Hellwig
@ 2020-11-03 10:00 ` Christoph Hellwig
2020-11-03 10:00 ` [PATCH 08/10] loop: use set_disk_ro Christoph Hellwig
` (3 subsequent siblings)
10 siblings, 0 replies; 19+ messages in thread
From: Christoph Hellwig @ 2020-11-03 10:00 UTC (permalink / raw)
To: Jens Axboe
Cc: Ilya Dryomov, Song Liu, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, Stefan Haberland, Jan Hoeppner, linux-block,
ceph-devel, linux-bcache, linux-raid, linux-mtd, linux-s390
Now that all drivers that want to hook into setting or clearing the
read-only flag use the set_read_only method, this code can be removed.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
block/ioctl.c | 23 -----------------------
1 file changed, 23 deletions(-)
diff --git a/block/ioctl.c b/block/ioctl.c
index a6fa16b9770593..96cb4544736468 100644
--- a/block/ioctl.c
+++ b/block/ioctl.c
@@ -346,26 +346,6 @@ static int blkdev_pr_clear(struct block_device *bdev,
return ops->pr_clear(bdev, c.key);
}
-/*
- * Is it an unrecognized ioctl? The correct returns are either
- * ENOTTY (final) or ENOIOCTLCMD ("I don't know this one, try a
- * fallback"). ENOIOCTLCMD gets turned into ENOTTY by the ioctl
- * code before returning.
- *
- * Confused drivers sometimes return EINVAL, which is wrong. It
- * means "I understood the ioctl command, but the parameters to
- * it were wrong".
- *
- * We should aim to just fix the broken drivers, the EINVAL case
- * should go away.
- */
-static inline int is_unrecognized_ioctl(int ret)
-{
- return ret == -EINVAL ||
- ret == -ENOTTY ||
- ret == -ENOIOCTLCMD;
-}
-
static int blkdev_flushbuf(struct block_device *bdev, fmode_t mode,
unsigned cmd, unsigned long arg)
{
@@ -384,9 +364,6 @@ static int blkdev_roset(struct block_device *bdev, fmode_t mode,
if (!capable(CAP_SYS_ADMIN))
return -EACCES;
- ret = __blkdev_driver_ioctl(bdev, mode, cmd, arg);
- if (!is_unrecognized_ioctl(ret))
- return ret;
if (get_user(n, (int __user *)arg))
return -EFAULT;
if (bdev->bd_disk->fops->set_read_only) {
--
2.28.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 08/10] loop: use set_disk_ro
2020-11-03 10:00 block ioctl cleanups v2 Christoph Hellwig
` (6 preceding siblings ...)
2020-11-03 10:00 ` [PATCH 07/10] block: don't call into the driver for BLKROSET Christoph Hellwig
@ 2020-11-03 10:00 ` Christoph Hellwig
2020-11-03 10:00 ` [PATCH 09/10] block: remove set_device_ro Christoph Hellwig
` (2 subsequent siblings)
10 siblings, 0 replies; 19+ messages in thread
From: Christoph Hellwig @ 2020-11-03 10:00 UTC (permalink / raw)
To: Jens Axboe
Cc: Ilya Dryomov, Song Liu, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, Stefan Haberland, Jan Hoeppner, linux-block,
ceph-devel, linux-bcache, linux-raid, linux-mtd, linux-s390
Use set_disk_ro instead of set_device_ro to match all other block
drivers and to ensure all partitions mirror the read-only flag.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/block/loop.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index cb1191d6e945f2..3e1ea45bb315d8 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1137,7 +1137,7 @@ static int loop_configure(struct loop_device *lo, fmode_t mode,
if (error)
goto out_unlock;
- set_device_ro(bdev, (lo->lo_flags & LO_FLAGS_READ_ONLY) != 0);
+ set_disk_ro(lo->lo_disk, (lo->lo_flags & LO_FLAGS_READ_ONLY) != 0);
lo->use_dio = lo->lo_flags & LO_FLAGS_DIRECT_IO;
lo->lo_device = bdev;
--
2.28.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 09/10] block: remove set_device_ro
2020-11-03 10:00 block ioctl cleanups v2 Christoph Hellwig
` (7 preceding siblings ...)
2020-11-03 10:00 ` [PATCH 08/10] loop: use set_disk_ro Christoph Hellwig
@ 2020-11-03 10:00 ` Christoph Hellwig
2020-11-03 10:00 ` [PATCH 10/10] block: remove __blkdev_driver_ioctl Christoph Hellwig
2020-11-11 7:58 ` block ioctl cleanups v2 Christoph Hellwig
10 siblings, 0 replies; 19+ messages in thread
From: Christoph Hellwig @ 2020-11-03 10:00 UTC (permalink / raw)
To: Jens Axboe
Cc: Ilya Dryomov, Song Liu, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, Stefan Haberland, Jan Hoeppner, linux-block,
ceph-devel, linux-bcache, linux-raid, linux-mtd, linux-s390
Fold set_device_ro into its only remaining caller.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
block/genhd.c | 7 -------
block/ioctl.c | 2 +-
include/linux/genhd.h | 1 -
3 files changed, 1 insertion(+), 9 deletions(-)
diff --git a/block/genhd.c b/block/genhd.c
index 0a273211fec283..a487590b365e89 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -1843,13 +1843,6 @@ static void set_disk_ro_uevent(struct gendisk *gd, int ro)
kobject_uevent_env(&disk_to_dev(gd)->kobj, KOBJ_CHANGE, envp);
}
-void set_device_ro(struct block_device *bdev, int flag)
-{
- bdev->bd_part->policy = flag;
-}
-
-EXPORT_SYMBOL(set_device_ro);
-
void set_disk_ro(struct gendisk *disk, int flag)
{
struct disk_part_iter piter;
diff --git a/block/ioctl.c b/block/ioctl.c
index 96cb4544736468..04255dc5f3bff3 100644
--- a/block/ioctl.c
+++ b/block/ioctl.c
@@ -371,7 +371,7 @@ static int blkdev_roset(struct block_device *bdev, fmode_t mode,
if (ret)
return ret;
}
- set_device_ro(bdev, n);
+ bdev->bd_part->policy = n;
return 0;
}
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index 38f23d75701379..f232f26d7c777b 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -304,7 +304,6 @@ extern void del_gendisk(struct gendisk *gp);
extern struct gendisk *get_gendisk(dev_t dev, int *partno);
extern struct block_device *bdget_disk(struct gendisk *disk, int partno);
-extern void set_device_ro(struct block_device *bdev, int flag);
extern void set_disk_ro(struct gendisk *disk, int flag);
static inline int get_disk_ro(struct gendisk *disk)
--
2.28.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 10/10] block: remove __blkdev_driver_ioctl
2020-11-03 10:00 block ioctl cleanups v2 Christoph Hellwig
` (8 preceding siblings ...)
2020-11-03 10:00 ` [PATCH 09/10] block: remove set_device_ro Christoph Hellwig
@ 2020-11-03 10:00 ` Christoph Hellwig
2020-11-11 7:58 ` block ioctl cleanups v2 Christoph Hellwig
10 siblings, 0 replies; 19+ messages in thread
From: Christoph Hellwig @ 2020-11-03 10:00 UTC (permalink / raw)
To: Jens Axboe
Cc: Ilya Dryomov, Song Liu, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, Stefan Haberland, Jan Hoeppner, linux-block,
ceph-devel, linux-bcache, linux-raid, linux-mtd, linux-s390
Just open code it in the few callers.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
block/ioctl.c | 25 +++++--------------------
drivers/block/pktcdvd.c | 6 ++++--
drivers/md/bcache/request.c | 5 +++--
drivers/md/dm.c | 5 ++++-
include/linux/blkdev.h | 2 --
5 files changed, 16 insertions(+), 27 deletions(-)
diff --git a/block/ioctl.c b/block/ioctl.c
index 04255dc5f3bff3..6b785181344fe1 100644
--- a/block/ioctl.c
+++ b/block/ioctl.c
@@ -219,23 +219,6 @@ static int compat_put_ulong(compat_ulong_t __user *argp, compat_ulong_t val)
}
#endif
-int __blkdev_driver_ioctl(struct block_device *bdev, fmode_t mode,
- unsigned cmd, unsigned long arg)
-{
- struct gendisk *disk = bdev->bd_disk;
-
- if (disk->fops->ioctl)
- return disk->fops->ioctl(bdev, mode, cmd, arg);
-
- return -ENOTTY;
-}
-/*
- * For the record: _GPL here is only because somebody decided to slap it
- * on the previous export. Sheer idiocy, since it wasn't copyrightable
- * at all and could be open-coded without any exports by anybody who cares.
- */
-EXPORT_SYMBOL_GPL(__blkdev_driver_ioctl);
-
#ifdef CONFIG_COMPAT
/*
* This is the equivalent of compat_ptr_ioctl(), to be used by block
@@ -594,10 +577,12 @@ int blkdev_ioctl(struct block_device *bdev, fmode_t mode, unsigned cmd,
}
ret = blkdev_common_ioctl(bdev, mode, cmd, arg, argp);
- if (ret == -ENOIOCTLCMD)
- return __blkdev_driver_ioctl(bdev, mode, cmd, arg);
+ if (ret != -ENOIOCTLCMD)
+ return ret;
- return ret;
+ if (!bdev->bd_disk->fops->ioctl)
+ return -ENOTTY;
+ return bdev->bd_disk->fops->ioctl(bdev, mode, cmd, arg);
}
EXPORT_SYMBOL_GPL(blkdev_ioctl); /* for /dev/raw */
diff --git a/drivers/block/pktcdvd.c b/drivers/block/pktcdvd.c
index 467dbd06b7cdb1..ef1c1f094ea4fc 100644
--- a/drivers/block/pktcdvd.c
+++ b/drivers/block/pktcdvd.c
@@ -2584,9 +2584,11 @@ static int pkt_ioctl(struct block_device *bdev, fmode_t mode, unsigned int cmd,
case CDROM_LAST_WRITTEN:
case CDROM_SEND_PACKET:
case SCSI_IOCTL_SEND_COMMAND:
- ret = __blkdev_driver_ioctl(pd->bdev, mode, cmd, arg);
+ if (!bdev->bd_disk->fops->ioctl)
+ ret = -ENOTTY;
+ else
+ ret = bdev->bd_disk->fops->ioctl(bdev, mode, cmd, arg);
break;
-
default:
pkt_dbg(2, pd, "Unknown ioctl (%x)\n", cmd);
ret = -ENOTTY;
diff --git a/drivers/md/bcache/request.c b/drivers/md/bcache/request.c
index 21432638314562..afac8d07c1bd00 100644
--- a/drivers/md/bcache/request.c
+++ b/drivers/md/bcache/request.c
@@ -1230,8 +1230,9 @@ static int cached_dev_ioctl(struct bcache_device *d, fmode_t mode,
if (dc->io_disable)
return -EIO;
-
- return __blkdev_driver_ioctl(dc->bdev, mode, cmd, arg);
+ if (!dc->bdev->bd_disk->fops->ioctl)
+ return -ENOTTY;
+ return dc->bdev->bd_disk->fops->ioctl(dc->bdev, mode, cmd, arg);
}
void bch_cached_dev_request_init(struct cached_dev *dc)
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index c18fc25485186d..6db395c3d28be8 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -570,7 +570,10 @@ static int dm_blk_ioctl(struct block_device *bdev, fmode_t mode,
}
}
- r = __blkdev_driver_ioctl(bdev, mode, cmd, arg);
+ if (!bdev->bd_disk->fops->ioctl)
+ r = -ENOTTY;
+ else
+ r = bdev->bd_disk->fops->ioctl(bdev, mode, cmd, arg);
out:
dm_unprepare_ioctl(md, srcu_idx);
return r;
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 5c1ba8a8d2bc7e..05b346a68c2eee 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1867,8 +1867,6 @@ extern int blkdev_compat_ptr_ioctl(struct block_device *, fmode_t,
#define blkdev_compat_ptr_ioctl NULL
#endif
-extern int __blkdev_driver_ioctl(struct block_device *, fmode_t, unsigned int,
- unsigned long);
extern int bdev_read_page(struct block_device *, sector_t, struct page *);
extern int bdev_write_page(struct block_device *, sector_t, struct page *,
struct writeback_control *);
--
2.28.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: block ioctl cleanups v2
2020-11-03 10:00 block ioctl cleanups v2 Christoph Hellwig
` (9 preceding siblings ...)
2020-11-03 10:00 ` [PATCH 10/10] block: remove __blkdev_driver_ioctl Christoph Hellwig
@ 2020-11-11 7:58 ` Christoph Hellwig
2020-11-11 16:13 ` Jens Axboe
10 siblings, 1 reply; 19+ messages in thread
From: Christoph Hellwig @ 2020-11-11 7:58 UTC (permalink / raw)
To: Jens Axboe
Cc: Ilya Dryomov, Song Liu, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, Stefan Haberland, Jan Hoeppner, linux-block,
ceph-devel, linux-bcache, linux-raid, linux-mtd, linux-s390
Jens, can you take a look and possibly pick this series up?
On Tue, Nov 03, 2020 at 11:00:08AM +0100, Christoph Hellwig wrote:
> Hi Jens,
>
> this series has a bunch of cleanups for the block layer ioctl code.
>
> Changes since v1:
> - drop the patch to return the correct error for unknown ioctls in mtip
> - add back an empty line in rbd
> - various spelling fixes
---end quoted text---
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: block ioctl cleanups v2
2020-11-11 7:58 ` block ioctl cleanups v2 Christoph Hellwig
@ 2020-11-11 16:13 ` Jens Axboe
2020-11-11 16:20 ` Christoph Hellwig
0 siblings, 1 reply; 19+ messages in thread
From: Jens Axboe @ 2020-11-11 16:13 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Ilya Dryomov, Song Liu, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, Stefan Haberland, Jan Hoeppner, linux-block,
ceph-devel, linux-bcache, linux-raid, linux-mtd, linux-s390
On 11/11/20 12:58 AM, Christoph Hellwig wrote:
> Jens, can you take a look and possibly pick this series up?
Looks good to me - but what is the final resolution on the BLKROSET
propagation?
--
Jens Axboe
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: block ioctl cleanups v2
2020-11-11 16:13 ` Jens Axboe
@ 2020-11-11 16:20 ` Christoph Hellwig
2020-11-11 16:23 ` Jens Axboe
0 siblings, 1 reply; 19+ messages in thread
From: Christoph Hellwig @ 2020-11-11 16:20 UTC (permalink / raw)
To: Jens Axboe
Cc: Christoph Hellwig, Ilya Dryomov, Song Liu, Miquel Raynal,
Richard Weinberger, Vignesh Raghavendra, Stefan Haberland,
Jan Hoeppner, linux-block, ceph-devel, linux-bcache, linux-raid,
linux-mtd, linux-s390
On Wed, Nov 11, 2020 at 09:13:05AM -0700, Jens Axboe wrote:
> On 11/11/20 12:58 AM, Christoph Hellwig wrote:
> > Jens, can you take a look and possibly pick this series up?
>
> Looks good to me - but what is the final resolution on the BLKROSET
> propagation?
Do it properly including a hard read only flag. Martin has an old
patch that I'm going to forward port. For now I think we need to
adjust dasd to match the behavior of all drivers, so that we can fix
the common code to behave more like dasd in the next step.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: block ioctl cleanups v2
2020-11-11 16:20 ` Christoph Hellwig
@ 2020-11-11 16:23 ` Jens Axboe
0 siblings, 0 replies; 19+ messages in thread
From: Jens Axboe @ 2020-11-11 16:23 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Ilya Dryomov, Song Liu, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, Stefan Haberland, Jan Hoeppner, linux-block,
ceph-devel, linux-bcache, linux-raid, linux-mtd, linux-s390
On 11/11/20 9:20 AM, Christoph Hellwig wrote:
> On Wed, Nov 11, 2020 at 09:13:05AM -0700, Jens Axboe wrote:
>> On 11/11/20 12:58 AM, Christoph Hellwig wrote:
>>> Jens, can you take a look and possibly pick this series up?
>>
>> Looks good to me - but what is the final resolution on the BLKROSET
>> propagation?
>
> Do it properly including a hard read only flag. Martin has an old
> patch that I'm going to forward port. For now I think we need to
> adjust dasd to match the behavior of all drivers, so that we can fix
> the common code to behave more like dasd in the next step.
I applied the series, so just send something for this on top.
--
Jens Axboe
^ permalink raw reply [flat|nested] 19+ messages in thread