----- Ursprüngliche Mail -----
> Von: "hch" <hch@xxxxxx>
> An: "richard" <richard@xxxxxx>, "anton ivanov" <anton.ivanov@xxxxxxxxxxxxxxxxxx>, "Johannes Berg"
> <johannes@xxxxxxxxxxxxxxxx>, "Jens Axboe" <axboe@xxxxxxxxx>
> CC: "linux-um" <linux-um@xxxxxxxxxxxxxxxxxxx>, "linux-block" <linux-block@xxxxxxxxxxxxxxx>
> 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@xxxxxx>
> ---
> 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@xxxxxx>
Thanks,
//richard
