ubd cleanups

ubd cleanups

[Christoph Hellwig]

Hi all,

this series cleans up the uml ubd drivers and gets rid of more blk_queue_*
calls.  It is a against Jens' for-6.9/block branch.

Diffstat:
 ubd_kern.c |  127 +++++++++++++++++++++----------------------------------------
 1 file changed, 44 insertions(+), 83 deletions(-)

[PATCH 1/7] ubd: remove the ubd_gendisk array

[Christoph Hellwig]

And add a disk pointer to the ubd structure instead to keep all
the per-device information together.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/um/drivers/ubd_kern.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/arch/um/drivers/ubd_kern.c b/arch/um/drivers/ubd_kern.c
index 48e11f073551b4..b203ebb1785125 100644
--- a/arch/um/drivers/ubd_kern.c
+++ b/arch/um/drivers/ubd_kern.c
@@ -125,9 +125,6 @@ static const struct block_device_operations ubd_blops = {
 	.getgeo		= ubd_getgeo,
 };
 
-/* Protected by ubd_lock */
-static struct gendisk *ubd_gendisk[MAX_DEV];
-
 #ifdef CONFIG_BLK_DEV_UBD_SYNC
 #define OPEN_FLAGS ((struct openflags) { .r = 1, .w = 1, .s = 1, .c = 0, \
 					 .cl = 1 })
@@ -165,6 +162,7 @@ struct ubd {
 	unsigned no_trim:1;
 	struct cow cow;
 	struct platform_device pdev;
+	struct gendisk *disk;
 	struct request_queue *queue;
 	struct blk_mq_tag_set tag_set;
 	spinlock_t lock;
@@ -922,7 +920,6 @@ static int ubd_add(int n, char **error_out)
 	if (err)
 		goto out_cleanup_disk;
 
-	ubd_gendisk[n] = disk;
 	return 0;
 
 out_cleanup_disk:
@@ -1014,7 +1011,6 @@ static int ubd_id(char **str, int *start_out, int *end_out)
 
 static int ubd_remove(int n, char **error_out)
 {
-	struct gendisk *disk = ubd_gendisk[n];
 	struct ubd *ubd_dev;
 	int err = -ENODEV;
 
@@ -1030,10 +1026,9 @@ static int ubd_remove(int n, char **error_out)
 	if(ubd_dev->count > 0)
 		goto out;
 
-	ubd_gendisk[n] = NULL;
-	if(disk != NULL){
-		del_gendisk(disk);
-		put_disk(disk);
+	if (ubd_dev->disk) {
+		del_gendisk(ubd_dev->disk);
+		put_disk(ubd_dev->disk);
 	}
 
 	err = 0;
-- 
2.39.2

[PATCH 2/7] ubd: remove ubd_disk_register

[Christoph Hellwig]

Fold it into the only caller to remove lots of references to the
global ubd_devs array.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/um/drivers/ubd_kern.c | 37 +++++++++++++++----------------------
 1 file changed, 15 insertions(+), 22 deletions(-)

diff --git a/arch/um/drivers/ubd_kern.c b/arch/um/drivers/ubd_kern.c
index b203ebb1785125..ef05157acd6b02 100644
--- a/arch/um/drivers/ubd_kern.c
+++ b/arch/um/drivers/ubd_kern.c
@@ -849,27 +849,6 @@ static const struct attribute_group *ubd_attr_groups[] = {
 	NULL,
 };
 
-static int ubd_disk_register(int major, u64 size, int unit,
-			     struct gendisk *disk)
-{
-	disk->major = major;
-	disk->first_minor = unit << UBD_SHIFT;
-	disk->minors = 1 << UBD_SHIFT;
-	disk->fops = &ubd_blops;
-	set_capacity(disk, size / 512);
-	sprintf(disk->disk_name, "ubd%c", 'a' + unit);
-
-	ubd_devs[unit].pdev.id   = unit;
-	ubd_devs[unit].pdev.name = DRIVER_NAME;
-	ubd_devs[unit].pdev.dev.release = ubd_device_release;
-	dev_set_drvdata(&ubd_devs[unit].pdev.dev, &ubd_devs[unit]);
-	platform_device_register(&ubd_devs[unit].pdev);
-
-	disk->private_data = &ubd_devs[unit];
-	disk->queue = ubd_devs[unit].queue;
-	return device_add_disk(&ubd_devs[unit].pdev.dev, disk, ubd_attr_groups);
-}
-
 #define ROUND_BLOCK(n) ((n + (SECTOR_SIZE - 1)) & (-SECTOR_SIZE))
 
 static const struct blk_mq_ops ubd_mq_ops = {
@@ -916,7 +895,21 @@ static int ubd_add(int n, char **error_out)
 	ubd_dev->queue = disk->queue;
 
 	blk_queue_write_cache(ubd_dev->queue, true, false);
-	err = ubd_disk_register(UBD_MAJOR, ubd_dev->size, n, disk);
+	disk->major = UBD_MAJOR;
+	disk->first_minor = n << UBD_SHIFT;
+	disk->minors = 1 << UBD_SHIFT;
+	disk->fops = &ubd_blops;
+	set_capacity(disk, ubd_dev->size / 512);
+	sprintf(disk->disk_name, "ubd%c", 'a' + n);
+	disk->private_data = ubd_dev;
+
+	ubd_dev->pdev.id = n;
+	ubd_dev->pdev.name = DRIVER_NAME;
+	ubd_dev->pdev.dev.release = ubd_device_release;
+	dev_set_drvdata(&ubd_dev->pdev.dev, ubd_dev);
+	platform_device_register(&ubd_dev->pdev);
+
+	err = device_add_disk(&ubd_dev->pdev.dev, disk, ubd_attr_groups);
 	if (err)
 		goto out_cleanup_disk;
 
-- 
2.39.2

[PATCH 3/7] ubd: move setting the nonrot flag to ubd_add

[Christoph Hellwig]

No reason to delay this until open time.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/um/drivers/ubd_kern.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/um/drivers/ubd_kern.c b/arch/um/drivers/ubd_kern.c
index ef05157acd6b02..9dcf41f7d49606 100644
--- a/arch/um/drivers/ubd_kern.c
+++ b/arch/um/drivers/ubd_kern.c
@@ -799,7 +799,6 @@ static int ubd_open_dev(struct ubd *ubd_dev)
 		blk_queue_max_discard_sectors(ubd_dev->queue, UBD_MAX_REQUEST);
 		blk_queue_max_write_zeroes_sectors(ubd_dev->queue, UBD_MAX_REQUEST);
 	}
-	blk_queue_flag_set(QUEUE_FLAG_NONROT, ubd_dev->queue);
 	return 0;
  error:
 	os_close_file(ubd_dev->fd);
@@ -894,6 +893,7 @@ static int ubd_add(int n, char **error_out)
 	}
 	ubd_dev->queue = disk->queue;
 
+	blk_queue_flag_set(QUEUE_FLAG_NONROT, disk->queue);
 	blk_queue_write_cache(ubd_dev->queue, true, false);
 	disk->major = UBD_MAJOR;
 	disk->first_minor = n << UBD_SHIFT;
-- 
2.39.2

[PATCH 4/7] ubd: move setting the variable queue limits to ubd_add

[Christoph Hellwig]

No reason to delay this until open time.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/um/drivers/ubd_kern.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/arch/um/drivers/ubd_kern.c b/arch/um/drivers/ubd_kern.c
index 9dcf41f7d49606..26bc8306356263 100644
--- a/arch/um/drivers/ubd_kern.c
+++ b/arch/um/drivers/ubd_kern.c
@@ -772,8 +772,6 @@ static int ubd_open_dev(struct ubd *ubd_dev)
 	ubd_dev->fd = fd;
 
 	if(ubd_dev->cow.file != NULL){
-		blk_queue_max_hw_sectors(ubd_dev->queue, 8 * sizeof(long));
-
 		err = -ENOMEM;
 		ubd_dev->cow.bitmap = vmalloc(ubd_dev->cow.bitmap_len);
 		if(ubd_dev->cow.bitmap == NULL){
@@ -795,10 +793,6 @@ static int ubd_open_dev(struct ubd *ubd_dev)
 		if(err < 0) goto error;
 		ubd_dev->cow.fd = err;
 	}
-	if (ubd_dev->no_trim == 0) {
-		blk_queue_max_discard_sectors(ubd_dev->queue, UBD_MAX_REQUEST);
-		blk_queue_max_write_zeroes_sectors(ubd_dev->queue, UBD_MAX_REQUEST);
-	}
 	return 0;
  error:
 	os_close_file(ubd_dev->fd);
@@ -867,6 +861,13 @@ static int ubd_add(int n, char **error_out)
 	if(ubd_dev->file == NULL)
 		goto out;
 
+	if (ubd_dev->cow.file)
+		lim.max_hw_sectors = 8 * sizeof(long);
+	if (!ubd_dev->no_trim) {
+		lim.max_hw_discard_sectors = UBD_MAX_REQUEST;
+		lim.max_write_zeroes_sectors = UBD_MAX_REQUEST;
+	}
+
 	err = ubd_file_size(ubd_dev, &ubd_dev->size);
 	if(err < 0){
 		*error_out = "Couldn't determine size of device's file";
-- 
2.39.2

[PATCH 5/7] ubd: move set_disk_ro to ubd_add

[Christoph Hellwig]

No need to delay this until open time.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/um/drivers/ubd_kern.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/um/drivers/ubd_kern.c b/arch/um/drivers/ubd_kern.c
index 26bc8306356263..c5d32e75426366 100644
--- a/arch/um/drivers/ubd_kern.c
+++ b/arch/um/drivers/ubd_kern.c
@@ -903,6 +903,7 @@ static int ubd_add(int n, char **error_out)
 	set_capacity(disk, ubd_dev->size / 512);
 	sprintf(disk->disk_name, "ubd%c", 'a' + n);
 	disk->private_data = ubd_dev;
+	set_disk_ro(disk, !ubd_dev->openflags.w);
 
 	ubd_dev->pdev.id = n;
 	ubd_dev->pdev.name = DRIVER_NAME;
@@ -1159,7 +1160,6 @@ static int ubd_open(struct gendisk *disk, blk_mode_t mode)
 		}
 	}
 	ubd_dev->count++;
-	set_disk_ro(disk, !ubd_dev->openflags.w);
 out:
 	mutex_unlock(&ubd_mutex);
 	return err;
-- 
2.39.2

[PATCH 6/7] ubd: remove the queue pointer in struct ubd

[Christoph Hellwig]

No need for it now, everything goes through the gendisk.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/um/drivers/ubd_kern.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/arch/um/drivers/ubd_kern.c b/arch/um/drivers/ubd_kern.c
index c5d32e75426366..9bf1d6a88bae59 100644
--- a/arch/um/drivers/ubd_kern.c
+++ b/arch/um/drivers/ubd_kern.c
@@ -163,7 +163,6 @@ struct ubd {
 	struct cow cow;
 	struct platform_device pdev;
 	struct gendisk *disk;
-	struct request_queue *queue;
 	struct blk_mq_tag_set tag_set;
 	spinlock_t lock;
 };
@@ -892,10 +891,9 @@ static int ubd_add(int n, char **error_out)
 		err = PTR_ERR(disk);
 		goto out_cleanup_tags;
 	}
-	ubd_dev->queue = disk->queue;
 
 	blk_queue_flag_set(QUEUE_FLAG_NONROT, disk->queue);
-	blk_queue_write_cache(ubd_dev->queue, true, false);
+	blk_queue_write_cache(disk->queue, true, false);
 	disk->major = UBD_MAJOR;
 	disk->first_minor = n << UBD_SHIFT;
 	disk->minors = 1 << UBD_SHIFT;
-- 
2.39.2

[PATCH 7/7] ubd: open the backing files in ubd_add

[Christoph Hellwig]

Opening the backing device only when the block device is opened is
a bit weird as no one configures block devices to not use them.
Opend them at add time, close them at remove time and remove the
now superflous opened counter as remove can simply check for
disk_openers.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/um/drivers/ubd_kern.c | 58 +++++++++++---------------------------
 1 file changed, 16 insertions(+), 42 deletions(-)

diff --git a/arch/um/drivers/ubd_kern.c b/arch/um/drivers/ubd_kern.c
index 9bf1d6a88bae59..63fc062add708c 100644
--- a/arch/um/drivers/ubd_kern.c
+++ b/arch/um/drivers/ubd_kern.c
@@ -108,8 +108,6 @@ static inline void ubd_set_bit(__u64 bit, unsigned char *data)
 static DEFINE_MUTEX(ubd_lock);
 static DEFINE_MUTEX(ubd_mutex); /* replaces BKL, might not be needed */
 
-static int ubd_open(struct gendisk *disk, blk_mode_t mode);
-static void ubd_release(struct gendisk *disk);
 static int ubd_ioctl(struct block_device *bdev, blk_mode_t mode,
 		     unsigned int cmd, unsigned long arg);
 static int ubd_getgeo(struct block_device *bdev, struct hd_geometry *geo);
@@ -118,8 +116,6 @@ static int ubd_getgeo(struct block_device *bdev, struct hd_geometry *geo);
 
 static const struct block_device_operations ubd_blops = {
         .owner		= THIS_MODULE,
-        .open		= ubd_open,
-        .release	= ubd_release,
         .ioctl		= ubd_ioctl,
         .compat_ioctl	= blkdev_compat_ptr_ioctl,
 	.getgeo		= ubd_getgeo,
@@ -152,7 +148,6 @@ struct ubd {
 	 * backing or the cow file. */
 	char *file;
 	char *serial;
-	int count;
 	int fd;
 	__u64 size;
 	struct openflags boot_openflags;
@@ -178,7 +173,6 @@ struct ubd {
 #define DEFAULT_UBD { \
 	.file = 		NULL, \
 	.serial =		NULL, \
-	.count =		0, \
 	.fd =			-1, \
 	.size =			-1, \
 	.boot_openflags =	OPEN_FLAGS, \
@@ -873,6 +867,13 @@ static int ubd_add(int n, char **error_out)
 		goto out;
 	}
 
+	err = ubd_open_dev(ubd_dev);
+	if (err) {
+		pr_err("ubd%c: Can't open \"%s\": errno = %d\n",
+			'a' + n, ubd_dev->file, -err);
+		goto out;
+	}
+
 	ubd_dev->size = ROUND_BLOCK(ubd_dev->size);
 
 	ubd_dev->tag_set.ops = &ubd_mq_ops;
@@ -884,7 +885,7 @@ static int ubd_add(int n, char **error_out)
 
 	err = blk_mq_alloc_tag_set(&ubd_dev->tag_set);
 	if (err)
-		goto out;
+		goto out_close;
 
 	disk = blk_mq_alloc_disk(&ubd_dev->tag_set, &lim, ubd_dev);
 	if (IS_ERR(disk)) {
@@ -919,6 +920,8 @@ static int ubd_add(int n, char **error_out)
 	put_disk(disk);
 out_cleanup_tags:
 	blk_mq_free_tag_set(&ubd_dev->tag_set);
+out_close:
+	ubd_close_dev(ubd_dev);
 out:
 	return err;
 }
@@ -1014,13 +1017,14 @@ static int ubd_remove(int n, char **error_out)
 	if(ubd_dev->file == NULL)
 		goto out;
 
-	/* you cannot remove a open disk */
-	err = -EBUSY;
-	if(ubd_dev->count > 0)
-		goto out;
-
 	if (ubd_dev->disk) {
+		/* you cannot remove a open disk */
+		err = -EBUSY;
+		if (disk_openers(ubd_dev->disk))
+			goto out;
+
 		del_gendisk(ubd_dev->disk);
+		ubd_close_dev(ubd_dev);
 		put_disk(ubd_dev->disk);
 	}
 
@@ -1143,36 +1147,6 @@ static int __init ubd_driver_init(void){
 
 device_initcall(ubd_driver_init);
 
-static int ubd_open(struct gendisk *disk, blk_mode_t mode)
-{
-	struct ubd *ubd_dev = disk->private_data;
-	int err = 0;
-
-	mutex_lock(&ubd_mutex);
-	if(ubd_dev->count == 0){
-		err = ubd_open_dev(ubd_dev);
-		if(err){
-			printk(KERN_ERR "%s: Can't open \"%s\": errno = %d\n",
-			       disk->disk_name, ubd_dev->file, -err);
-			goto out;
-		}
-	}
-	ubd_dev->count++;
-out:
-	mutex_unlock(&ubd_mutex);
-	return err;
-}
-
-static void ubd_release(struct gendisk *disk)
-{
-	struct ubd *ubd_dev = disk->private_data;
-
-	mutex_lock(&ubd_mutex);
-	if(--ubd_dev->count == 0)
-		ubd_close_dev(ubd_dev);
-	mutex_unlock(&ubd_mutex);
-}
-
 static void cowify_bitmap(__u64 io_offset, int length, unsigned long *cow_mask,
 			  __u64 *cow_offset, unsigned long *bitmap,
 			  __u64 bitmap_offset, unsigned long *bitmap_words,
-- 
2.39.2

Re: [PATCH 1/7] ubd: remove the ubd_gendisk array

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "hch" <hch@lst.de>
> An: "richard" <richard@nod.at>, "anton ivanov" <anton.ivanov@cambridgegreys.com>, "Johannes Berg"
> <johannes@sipsolutions.net>, "Jens Axboe" <axboe@kernel.dk>
> CC: "linux-um" <linux-um@lists.infradead.org>, "linux-block" <linux-block@vger.kernel.org>
> Gesendet: Donnerstag, 22. Februar 2024 08:24:11
> Betreff: [PATCH 1/7] ubd: remove the ubd_gendisk array

> And add a disk pointer to the ubd structure instead to keep all
> the per-device information together.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> arch/um/drivers/ubd_kern.c | 13 ++++---------
> 1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/um/drivers/ubd_kern.c b/arch/um/drivers/ubd_kern.c
> index 48e11f073551b4..b203ebb1785125 100644
> --- a/arch/um/drivers/ubd_kern.c
> +++ b/arch/um/drivers/ubd_kern.c
> @@ -125,9 +125,6 @@ static const struct block_device_operations ubd_blops = {
> 	.getgeo		= ubd_getgeo,
> };
> 
> -/* Protected by ubd_lock */
> -static struct gendisk *ubd_gendisk[MAX_DEV];
> -
> #ifdef CONFIG_BLK_DEV_UBD_SYNC
> #define OPEN_FLAGS ((struct openflags) { .r = 1, .w = 1, .s = 1, .c = 0, \
> 					 .cl = 1 })
> @@ -165,6 +162,7 @@ struct ubd {
> 	unsigned no_trim:1;
> 	struct cow cow;
> 	struct platform_device pdev;
> +	struct gendisk *disk;
> 	struct request_queue *queue;
> 	struct blk_mq_tag_set tag_set;
> 	spinlock_t lock;
> @@ -922,7 +920,6 @@ static int ubd_add(int n, char **error_out)
> 	if (err)
> 		goto out_cleanup_disk;
> 
> -	ubd_gendisk[n] = disk;
> 	return 0;
> 
> out_cleanup_disk:
> @@ -1014,7 +1011,6 @@ static int ubd_id(char **str, int *start_out, int
> *end_out)
> 
> static int ubd_remove(int n, char **error_out)
> {
> -	struct gendisk *disk = ubd_gendisk[n];
> 	struct ubd *ubd_dev;
> 	int err = -ENODEV;
> 
> @@ -1030,10 +1026,9 @@ static int ubd_remove(int n, char **error_out)
> 	if(ubd_dev->count > 0)
> 		goto out;
> 
> -	ubd_gendisk[n] = NULL;
> -	if(disk != NULL){
> -		del_gendisk(disk);
> -		put_disk(disk);
> +	if (ubd_dev->disk) {
> +		del_gendisk(ubd_dev->disk);
> +		put_disk(ubd_dev->disk);
> 	}

Reviewed-by: Richard Weinberger <richard@nod.at>

Thanks,
//richard

Re: [PATCH 2/7] ubd: remove ubd_disk_register

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "hch" <hch@lst.de>
> An: "richard" <richard@nod.at>, "anton ivanov" <anton.ivanov@cambridgegreys.com>, "Johannes Berg"
> <johannes@sipsolutions.net>, "Jens Axboe" <axboe@kernel.dk>
> CC: "linux-um" <linux-um@lists.infradead.org>, "linux-block" <linux-block@vger.kernel.org>
> Gesendet: Donnerstag, 22. Februar 2024 08:24:12
> Betreff: [PATCH 2/7] ubd: remove ubd_disk_register

> Fold it into the only caller to remove lots of references to the
> global ubd_devs array.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> arch/um/drivers/ubd_kern.c | 37 +++++++++++++++----------------------
> 1 file changed, 15 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/um/drivers/ubd_kern.c b/arch/um/drivers/ubd_kern.c
> index b203ebb1785125..ef05157acd6b02 100644
> --- a/arch/um/drivers/ubd_kern.c
> +++ b/arch/um/drivers/ubd_kern.c
> @@ -849,27 +849,6 @@ static const struct attribute_group *ubd_attr_groups[] = {
> 	NULL,
> };
> 
> -static int ubd_disk_register(int major, u64 size, int unit,
> -			     struct gendisk *disk)
> -{
> -	disk->major = major;
> -	disk->first_minor = unit << UBD_SHIFT;
> -	disk->minors = 1 << UBD_SHIFT;
> -	disk->fops = &ubd_blops;
> -	set_capacity(disk, size / 512);
> -	sprintf(disk->disk_name, "ubd%c", 'a' + unit);
> -
> -	ubd_devs[unit].pdev.id   = unit;
> -	ubd_devs[unit].pdev.name = DRIVER_NAME;
> -	ubd_devs[unit].pdev.dev.release = ubd_device_release;
> -	dev_set_drvdata(&ubd_devs[unit].pdev.dev, &ubd_devs[unit]);
> -	platform_device_register(&ubd_devs[unit].pdev);
> -
> -	disk->private_data = &ubd_devs[unit];
> -	disk->queue = ubd_devs[unit].queue;
> -	return device_add_disk(&ubd_devs[unit].pdev.dev, disk, ubd_attr_groups);
> -}
> -
> #define ROUND_BLOCK(n) ((n + (SECTOR_SIZE - 1)) & (-SECTOR_SIZE))
> 
> static const struct blk_mq_ops ubd_mq_ops = {
> @@ -916,7 +895,21 @@ static int ubd_add(int n, char **error_out)
> 	ubd_dev->queue = disk->queue;
> 
> 	blk_queue_write_cache(ubd_dev->queue, true, false);
> -	err = ubd_disk_register(UBD_MAJOR, ubd_dev->size, n, disk);
> +	disk->major = UBD_MAJOR;
> +	disk->first_minor = n << UBD_SHIFT;
> +	disk->minors = 1 << UBD_SHIFT;
> +	disk->fops = &ubd_blops;
> +	set_capacity(disk, ubd_dev->size / 512);
> +	sprintf(disk->disk_name, "ubd%c", 'a' + n);
> +	disk->private_data = ubd_dev;
> +
> +	ubd_dev->pdev.id = n;
> +	ubd_dev->pdev.name = DRIVER_NAME;
> +	ubd_dev->pdev.dev.release = ubd_device_release;
> +	dev_set_drvdata(&ubd_dev->pdev.dev, ubd_dev);
> +	platform_device_register(&ubd_dev->pdev);
> +
> +	err = device_add_disk(&ubd_dev->pdev.dev, disk, ubd_attr_groups);
> 	if (err)
> 		goto out_cleanup_disk;
> 
> --
> 2.39.2

Reviewed-by: Richard Weinberger <richard@nod.at>

Thanks,
//richard

Re: [PATCH 3/7] ubd: move setting the nonrot flag to ubd_add

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "hch" <hch@lst.de>
> An: "richard" <richard@nod.at>, "anton ivanov" <anton.ivanov@cambridgegreys.com>, "Johannes Berg"
> <johannes@sipsolutions.net>, "Jens Axboe" <axboe@kernel.dk>
> CC: "linux-um" <linux-um@lists.infradead.org>, "linux-block" <linux-block@vger.kernel.org>
> Gesendet: Donnerstag, 22. Februar 2024 08:24:13
> Betreff: [PATCH 3/7] ubd: move setting the nonrot flag to ubd_add

> No reason to delay this until open time.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> arch/um/drivers/ubd_kern.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/um/drivers/ubd_kern.c b/arch/um/drivers/ubd_kern.c
> index ef05157acd6b02..9dcf41f7d49606 100644
> --- a/arch/um/drivers/ubd_kern.c
> +++ b/arch/um/drivers/ubd_kern.c
> @@ -799,7 +799,6 @@ static int ubd_open_dev(struct ubd *ubd_dev)
> 		blk_queue_max_discard_sectors(ubd_dev->queue, UBD_MAX_REQUEST);
> 		blk_queue_max_write_zeroes_sectors(ubd_dev->queue, UBD_MAX_REQUEST);
> 	}
> -	blk_queue_flag_set(QUEUE_FLAG_NONROT, ubd_dev->queue);
> 	return 0;
>  error:
> 	os_close_file(ubd_dev->fd);
> @@ -894,6 +893,7 @@ static int ubd_add(int n, char **error_out)
> 	}
> 	ubd_dev->queue = disk->queue;
> 
> +	blk_queue_flag_set(QUEUE_FLAG_NONROT, disk->queue);
> 	blk_queue_write_cache(ubd_dev->queue, true, false);
> 	disk->major = UBD_MAJOR;
> 	disk->first_minor = n << UBD_SHIFT;
> --
> 2.39.2

Reviewed-by: Richard Weinberger <richard@nod.at>

Thanks,
//richard

Re: [PATCH 4/7] ubd: move setting the variable queue limits to ubd_add

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "hch" <hch@lst.de>
> An: "richard" <richard@nod.at>, "anton ivanov" <anton.ivanov@cambridgegreys.com>, "Johannes Berg"
> <johannes@sipsolutions.net>, "Jens Axboe" <axboe@kernel.dk>
> CC: "linux-um" <linux-um@lists.infradead.org>, "linux-block" <linux-block@vger.kernel.org>
> Gesendet: Donnerstag, 22. Februar 2024 08:24:14
> Betreff: [PATCH 4/7] ubd: move setting the variable queue limits to ubd_add

> No reason to delay this until open time.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> arch/um/drivers/ubd_kern.c | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/um/drivers/ubd_kern.c b/arch/um/drivers/ubd_kern.c
> index 9dcf41f7d49606..26bc8306356263 100644
> --- a/arch/um/drivers/ubd_kern.c
> +++ b/arch/um/drivers/ubd_kern.c
> @@ -772,8 +772,6 @@ static int ubd_open_dev(struct ubd *ubd_dev)
> 	ubd_dev->fd = fd;
> 
> 	if(ubd_dev->cow.file != NULL){
> -		blk_queue_max_hw_sectors(ubd_dev->queue, 8 * sizeof(long));
> -
> 		err = -ENOMEM;
> 		ubd_dev->cow.bitmap = vmalloc(ubd_dev->cow.bitmap_len);
> 		if(ubd_dev->cow.bitmap == NULL){
> @@ -795,10 +793,6 @@ static int ubd_open_dev(struct ubd *ubd_dev)
> 		if(err < 0) goto error;
> 		ubd_dev->cow.fd = err;
> 	}
> -	if (ubd_dev->no_trim == 0) {
> -		blk_queue_max_discard_sectors(ubd_dev->queue, UBD_MAX_REQUEST);
> -		blk_queue_max_write_zeroes_sectors(ubd_dev->queue, UBD_MAX_REQUEST);
> -	}
> 	return 0;
>  error:
> 	os_close_file(ubd_dev->fd);
> @@ -867,6 +861,13 @@ static int ubd_add(int n, char **error_out)
> 	if(ubd_dev->file == NULL)
> 		goto out;
> 
> +	if (ubd_dev->cow.file)
> +		lim.max_hw_sectors = 8 * sizeof(long);
> +	if (!ubd_dev->no_trim) {
> +		lim.max_hw_discard_sectors = UBD_MAX_REQUEST;
> +		lim.max_write_zeroes_sectors = UBD_MAX_REQUEST;
> +	}
> +
> 	err = ubd_file_size(ubd_dev, &ubd_dev->size);
> 	if(err < 0){
> 		*error_out = "Couldn't determine size of device's file";
> --
> 2.39.2

Reviewed-by: Richard Weinberger <richard@nod.at>

Thanks,
//richard

Re: [PATCH 5/7] ubd: move set_disk_ro to ubd_add

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "hch" <hch@lst.de>
> An: "richard" <richard@nod.at>, "anton ivanov" <anton.ivanov@cambridgegreys.com>, "Johannes Berg"
> <johannes@sipsolutions.net>, "Jens Axboe" <axboe@kernel.dk>
> CC: "linux-um" <linux-um@lists.infradead.org>, "linux-block" <linux-block@vger.kernel.org>
> Gesendet: Donnerstag, 22. Februar 2024 08:24:15
> Betreff: [PATCH 5/7] ubd: move set_disk_ro to ubd_add

> No need to delay this until open time.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> arch/um/drivers/ubd_kern.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/um/drivers/ubd_kern.c b/arch/um/drivers/ubd_kern.c
> index 26bc8306356263..c5d32e75426366 100644
> --- a/arch/um/drivers/ubd_kern.c
> +++ b/arch/um/drivers/ubd_kern.c
> @@ -903,6 +903,7 @@ static int ubd_add(int n, char **error_out)
> 	set_capacity(disk, ubd_dev->size / 512);
> 	sprintf(disk->disk_name, "ubd%c", 'a' + n);
> 	disk->private_data = ubd_dev;
> +	set_disk_ro(disk, !ubd_dev->openflags.w);
> 
> 	ubd_dev->pdev.id = n;
> 	ubd_dev->pdev.name = DRIVER_NAME;
> @@ -1159,7 +1160,6 @@ static int ubd_open(struct gendisk *disk, blk_mode_t mode)
> 		}
> 	}
> 	ubd_dev->count++;
> -	set_disk_ro(disk, !ubd_dev->openflags.w);
> out:
> 	mutex_unlock(&ubd_mutex);
> 	return err;
> --
> 2.39.2

Reviewed-by: Richard Weinberger <richard@nod.at>

Thanks,
//richard

Re: [PATCH 6/7] ubd: remove the queue pointer in struct ubd

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "hch" <hch@lst.de>
> An: "richard" <richard@nod.at>, "anton ivanov" <anton.ivanov@cambridgegreys.com>, "Johannes Berg"
> <johannes@sipsolutions.net>, "Jens Axboe" <axboe@kernel.dk>
> CC: "linux-um" <linux-um@lists.infradead.org>, "linux-block" <linux-block@vger.kernel.org>
> Gesendet: Donnerstag, 22. Februar 2024 08:24:16
> Betreff: [PATCH 6/7] ubd: remove the queue pointer in struct ubd

> No need for it now, everything goes through the gendisk.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> arch/um/drivers/ubd_kern.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/arch/um/drivers/ubd_kern.c b/arch/um/drivers/ubd_kern.c
> index c5d32e75426366..9bf1d6a88bae59 100644
> --- a/arch/um/drivers/ubd_kern.c
> +++ b/arch/um/drivers/ubd_kern.c
> @@ -163,7 +163,6 @@ struct ubd {
> 	struct cow cow;
> 	struct platform_device pdev;
> 	struct gendisk *disk;
> -	struct request_queue *queue;
> 	struct blk_mq_tag_set tag_set;
> 	spinlock_t lock;
> };
> @@ -892,10 +891,9 @@ static int ubd_add(int n, char **error_out)
> 		err = PTR_ERR(disk);
> 		goto out_cleanup_tags;
> 	}
> -	ubd_dev->queue = disk->queue;
> 
> 	blk_queue_flag_set(QUEUE_FLAG_NONROT, disk->queue);
> -	blk_queue_write_cache(ubd_dev->queue, true, false);
> +	blk_queue_write_cache(disk->queue, true, false);
> 	disk->major = UBD_MAJOR;
> 	disk->first_minor = n << UBD_SHIFT;
> 	disk->minors = 1 << UBD_SHIFT;
> --
> 2.39.2

Reviewed-by: Richard Weinberger <richard@nod.at>

Thanks,
//richard

Re: [PATCH 7/7] ubd: open the backing files in ubd_add

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "hch" <hch@lst.de>
> An: "richard" <richard@nod.at>, "anton ivanov" <anton.ivanov@cambridgegreys.com>, "Johannes Berg"
> <johannes@sipsolutions.net>, "Jens Axboe" <axboe@kernel.dk>
> CC: "linux-um" <linux-um@lists.infradead.org>, "linux-block" <linux-block@vger.kernel.org>
> Gesendet: Donnerstag, 22. Februar 2024 08:24:17
> Betreff: [PATCH 7/7] ubd: open the backing files in ubd_add

> Opening the backing device only when the block device is opened is
> a bit weird as no one configures block devices to not use them.

Agreed. I guess this is a strange optimization from the old days.

> Opend them at add time, close them at remove time and remove the
> now superflous opened counter as remove can simply check for
> disk_openers.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> arch/um/drivers/ubd_kern.c | 58 +++++++++++---------------------------
> 1 file changed, 16 insertions(+), 42 deletions(-)
> 
> diff --git a/arch/um/drivers/ubd_kern.c b/arch/um/drivers/ubd_kern.c
> index 9bf1d6a88bae59..63fc062add708c 100644
> --- a/arch/um/drivers/ubd_kern.c
> +++ b/arch/um/drivers/ubd_kern.c
> @@ -108,8 +108,6 @@ static inline void ubd_set_bit(__u64 bit, unsigned char
> *data)
> static DEFINE_MUTEX(ubd_lock);
> static DEFINE_MUTEX(ubd_mutex); /* replaces BKL, might not be needed */
> 
> -static int ubd_open(struct gendisk *disk, blk_mode_t mode);
> -static void ubd_release(struct gendisk *disk);
> static int ubd_ioctl(struct block_device *bdev, blk_mode_t mode,
> 		     unsigned int cmd, unsigned long arg);
> static int ubd_getgeo(struct block_device *bdev, struct hd_geometry *geo);
> @@ -118,8 +116,6 @@ static int ubd_getgeo(struct block_device *bdev, struct
> hd_geometry *geo);
> 
> static const struct block_device_operations ubd_blops = {
>         .owner		= THIS_MODULE,
> -        .open		= ubd_open,
> -        .release	= ubd_release,
>         .ioctl		= ubd_ioctl,
>         .compat_ioctl	= blkdev_compat_ptr_ioctl,
> 	.getgeo		= ubd_getgeo,
> @@ -152,7 +148,6 @@ struct ubd {
> 	 * backing or the cow file. */
> 	char *file;
> 	char *serial;
> -	int count;
> 	int fd;
> 	__u64 size;
> 	struct openflags boot_openflags;
> @@ -178,7 +173,6 @@ struct ubd {
> #define DEFAULT_UBD { \
> 	.file = 		NULL, \
> 	.serial =		NULL, \
> -	.count =		0, \
> 	.fd =			-1, \
> 	.size =			-1, \
> 	.boot_openflags =	OPEN_FLAGS, \
> @@ -873,6 +867,13 @@ static int ubd_add(int n, char **error_out)
> 		goto out;
> 	}
> 
> +	err = ubd_open_dev(ubd_dev);
> +	if (err) {
> +		pr_err("ubd%c: Can't open \"%s\": errno = %d\n",
> +			'a' + n, ubd_dev->file, -err);
> +		goto out;
> +	}
> +
> 	ubd_dev->size = ROUND_BLOCK(ubd_dev->size);
> 
> 	ubd_dev->tag_set.ops = &ubd_mq_ops;
> @@ -884,7 +885,7 @@ static int ubd_add(int n, char **error_out)
> 
> 	err = blk_mq_alloc_tag_set(&ubd_dev->tag_set);
> 	if (err)
> -		goto out;
> +		goto out_close;
> 
> 	disk = blk_mq_alloc_disk(&ubd_dev->tag_set, &lim, ubd_dev);
> 	if (IS_ERR(disk)) {
> @@ -919,6 +920,8 @@ static int ubd_add(int n, char **error_out)
> 	put_disk(disk);
> out_cleanup_tags:
> 	blk_mq_free_tag_set(&ubd_dev->tag_set);
> +out_close:
> +	ubd_close_dev(ubd_dev);
> out:
> 	return err;
> }
> @@ -1014,13 +1017,14 @@ static int ubd_remove(int n, char **error_out)
> 	if(ubd_dev->file == NULL)
> 		goto out;
> 
> -	/* you cannot remove a open disk */
> -	err = -EBUSY;
> -	if(ubd_dev->count > 0)
> -		goto out;
> -
> 	if (ubd_dev->disk) {
> +		/* you cannot remove a open disk */
> +		err = -EBUSY;
> +		if (disk_openers(ubd_dev->disk))
> +			goto out;
> +
> 		del_gendisk(ubd_dev->disk);
> +		ubd_close_dev(ubd_dev);
> 		put_disk(ubd_dev->disk);
> 	}
> 
> @@ -1143,36 +1147,6 @@ static int __init ubd_driver_init(void){
> 
> device_initcall(ubd_driver_init);
> 
> -static int ubd_open(struct gendisk *disk, blk_mode_t mode)
> -{
> -	struct ubd *ubd_dev = disk->private_data;
> -	int err = 0;
> -
> -	mutex_lock(&ubd_mutex);
> -	if(ubd_dev->count == 0){
> -		err = ubd_open_dev(ubd_dev);
> -		if(err){
> -			printk(KERN_ERR "%s: Can't open \"%s\": errno = %d\n",
> -			       disk->disk_name, ubd_dev->file, -err);
> -			goto out;
> -		}
> -	}
> -	ubd_dev->count++;
> -out:
> -	mutex_unlock(&ubd_mutex);
> -	return err;
> -}
> -
> -static void ubd_release(struct gendisk *disk)
> -{
> -	struct ubd *ubd_dev = disk->private_data;
> -
> -	mutex_lock(&ubd_mutex);
> -	if(--ubd_dev->count == 0)
> -		ubd_close_dev(ubd_dev);
> -	mutex_unlock(&ubd_mutex);
> -}
> -
> static void cowify_bitmap(__u64 io_offset, int length, unsigned long *cow_mask,
> 			  __u64 *cow_offset, unsigned long *bitmap,
> 			  __u64 bitmap_offset, unsigned long *bitmap_words,
> --
> 2.39.2

Reviewed-by: Richard Weinberger <richard@nod.at>

Thanks,
//richard

Re: ubd cleanups

[Jens Axboe]

On Thu, 22 Feb 2024 08:24:10 +0100, Christoph Hellwig wrote:
> this series cleans up the uml ubd drivers and gets rid of more blk_queue_*
> calls.  It is a against Jens' for-6.9/block branch.
> 
> Diffstat:
>  ubd_kern.c |  127 +++++++++++++++++++++----------------------------------------
>  1 file changed, 44 insertions(+), 83 deletions(-)
> 
> [...]

Applied, thanks!

[1/7] ubd: remove the ubd_gendisk array
      commit: 32621ad7a7ea4c3add1dd6bd27b62c2b22500d54
[2/7] ubd: remove ubd_disk_register
      commit: 0267e9cac6de0c25ec1b6e3fef4fd8a87a4774a2
[3/7] ubd: move setting the nonrot flag to ubd_add
      commit: b8b364d2af7483ad82975cf35d5201efe1efa29f
[4/7] ubd: move setting the variable queue limits to ubd_add
      commit: 58ebe3e748353960fcc3b2661273737f45d197b1
[5/7] ubd: move set_disk_ro to ubd_add
      commit: 5e4e1ff820e855a328cf2efb795c079f3a58abbe
[6/7] ubd: remove the queue pointer in struct ubd
      commit: f3c17dcc43e207e4fba511e9e63d52f7098ad6f3
[7/7] ubd: open the backing files in ubd_add
      commit: fb5d1d389c9e78d68f1f71f926d6251017579f5b

Best regards,
-- 
Jens Axboe