block: reviewed add_disk() error handling set

PATCH Applied Luis Chamberlain 3 participants 16 messages Oct 15, 2021 – Oct 21, 2021

Subsystems (10)

SCSI Block layer Memory technology devices (MTD) Device mapper +6 more subsystems

lore

Share & tools

lore

Luis Chamberlain

author

2021-10-15 23:30 UTC #0 lore raw

cover block: reviewed add_disk() error handling set

Jens,

I had last split up patches into 7 groups, but at this point now most changes are merged except a few more drivers. Instead of creating a new patch set for each of the 7 groups I'm creating 3 new groups of patches now:

* This set, for which we already have an Acked-by or Reviewed-by tag, it would be nice to get clarification of driver maintainers want these to go through you or if a the maintainers want to pick these changes up themselves.

* A second set will deal with patches which have no reviews done for them yet

* The last set deals with the __register_blkdev() change and the __must_check change which ensures we don't let in new drivers which don't deal with error handling.

If you're a maintainer of any of the below patches and wish for them to go through Jens' tree directly now would be a good time to say so or you can just pick the patch up yourself.

Luis Chamberlain (9):
scsi/sd: add error handling support for add_disk() scsi/sr: add error handling support for add_disk()
dm: add add_disk() error handling
bcache: add error handling support for add_disk()
xen-blkfront: add error handling support for add_disk() m68k/emu/nfblock: add error handling support for add_disk() um/drivers/ubd_kern: add error handling support for add_disk()
rnbd: add error handling support for add_disk()
mtd: add add_disk() error handling

 arch/m68k/emu/nfblock.c       |  9 +++++++--
 arch/um/drivers/ubd_kern.c    | 13 +++++++++----
 drivers/block/rnbd/rnbd-clt.c | 13 +++++++++----
 drivers/block/xen-blkfront.c  |  8 +++++++-
 drivers/md/bcache/super.c     | 17 ++++++++++++-----
 drivers/md/dm.c               |  4 +++-
 drivers/mtd/mtd_blkdevs.c     |  6 +++++-
 drivers/scsi/sd.c             |  8 +++++++-
 drivers/scsi/sr.c             |  7 ++++++-
 9 files changed, 65 insertions(+), 20 deletions(-)

Signature · 1 line

2.30.2

Luis Chamberlain

author

2021-10-15 23:30 UTC #1 lore raw

PATCH 1/9 scsi/sd: add error handling support for add_disk()

We never checked for errors on add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling.

As with the error handling for device_add() we follow the same logic and just put the device so that cleanup is done via the scsi_disk_release().

Reviewed-by Christoph Hellwig <[email protected]>

Signed-off-by Luis Chamberlain <[email protected]>

---
 drivers/scsi/sd.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

drivers/scsi/sd.c +7 -1

index a646d27df681..d69f2e626e76 100644
@@ -3457,7 +3457,13 @@ static int sd_probe(struct device *dev)
 		pm_runtime_set_autosuspend_delay(dev,
 			sdp->host->hostt->rpm_autosuspend_delay);
 	}
-	device_add_disk(dev, gd, NULL);
+
+	error = device_add_disk(dev, gd, NULL);
+	if (error) {
+		put_device(&sdkp->dev);
+		goto out;
+	}
+
 	if (sdkp->capacity)
 		sd_dif_config_host(sdkp);

Signature · 1 line

2.30.2

Martin K. Petersen

Acked-by on v1 1/9

2021-10-17 02:51 UTC #2 lore raw

Re: [PATCH 1/9] scsi/sd: add error handling support for add_disk()

Luis,

Quoted 6 lines

We never checked for errors on add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling.
As with the error handling for device_add() we follow the same logic and just put the device so that cleanup is done via the scsi_disk_release().

Acked-by Martin K. Petersen <[email protected]>

Signature · 1 line

Martin K. Petersen	Oracle Linux Engineering

Luis Chamberlain

author

2021-10-18 20:31 UTC #3 lore raw

Re: [PATCH 1/9] scsi/sd: add error handling support for add_disk()

Quoted 11 lines · Martin K. Petersen

Luis,
Quoted 6 lines
We never checked for errors on add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling.
As with the error handling for device_add() we follow the same logic and just put the device so that cleanup is done via the scsi_disk_release().
Acked-by Martin K. Petersen <[email protected]>

Thanks, would you like Jens to pick this up and the other scsi/sr patch or are you taking it through your tree?

Luis

Martin K. Petersen

2021-10-19 02:52 UTC #4 lore raw

Re: [PATCH 1/9] scsi/sd: add error handling support for add_disk()

Luis,

Quoted 2 lines

Thanks, would you like Jens to pick this up and the other scsi/sr patch or are you taking it through your tree?

Didn't think I had the relevant add_disk() patch in my baseline tree but it turns out I do. So I queued them up.

Thanks!

Signature · 1 line

Martin K. Petersen	Oracle Linux Engineering

Luis Chamberlain

author

2021-10-15 23:30 UTC #5 lore raw

PATCH 2/9 scsi/sr: add error handling support for add_disk()

In reply to Luis Chamberlain

We never checked for errors on add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling.

Just put the cdrom kref and have the unwinding be done by sr_kref_release().

Reviewed-by Christoph Hellwig <[email protected]>

Signed-off-by Luis Chamberlain <[email protected]>

---
 drivers/scsi/sr.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

drivers/scsi/sr.c +6 -1

index 115f7ef7a5de..4e5515848fa6 100644
@@ -728,7 +728,12 @@ static int sr_probe(struct device *dev)
 	dev_set_drvdata(dev, cd);
 	disk->flags |= GENHD_FL_REMOVABLE;
 	sr_revalidate_disk(cd);
-	device_add_disk(&sdev->sdev_gendev, disk, NULL);
+
+	error = device_add_disk(&sdev->sdev_gendev, disk, NULL);
+	if (error) {
+		kref_put(&cd->kref, sr_kref_release);
+		goto fail;
+	}
 
 	sdev_printk(KERN_DEBUG, sdev,
 		    "Attached scsi CD-ROM %s\n", cd->cdi.name);

Signature · 1 line

2.30.2

Martin K. Petersen

Acked-by on v1 2/9

2021-10-17 02:53 UTC #6 lore raw

Re: [PATCH 2/9] scsi/sr: add error handling support for add_disk()

Luis,

Quoted 5 lines

We never checked for errors on add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling.
Just put the cdrom kref and have the unwinding be done by sr_kref_release().

Acked-by Martin K. Petersen <[email protected]>

Signature · 1 line

Martin K. Petersen	Oracle Linux Engineering

Luis Chamberlain

author

2021-10-15 23:30 UTC #7 lore raw

PATCH 3/9 dm: add add_disk() error handling

In reply to Luis Chamberlain

We never checked for errors on add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling.

There are two calls to dm_setup_md_queue() which can fail then, one on dm_early_create() and we can easily see that the error path there calls dm_destroy in the error path. The other use case is on the ioctl table_load case. If that fails userspace needs to call the DM_DEV_REMOVE_CMD to cleanup the state - similar to any other failure.

Reviewed-by Hannes Reinecke <[email protected]>

Signed-off-by Luis Chamberlain <[email protected]>

---
 drivers/md/dm.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

drivers/md/dm.c +3 -1

index 245fa4153306..6d3265ed37c0 100644
@@ -2086,7 +2086,9 @@ int dm_setup_md_queue(struct mapped_device *md, struct dm_table *t)
 	if (r)
 		return r;
 
-	add_disk(md->disk);
+	r = add_disk(md->disk);
+	if (r)
+		return r;
 
 	r = dm_sysfs_init(md);
 	if (r) {

Signature · 1 line

2.30.2

Luis Chamberlain

author

2021-10-15 23:30 UTC #8 lore raw

PATCH 4/9 bcache: add error handling support for add_disk()

In reply to Luis Chamberlain

We never checked for errors on add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling.

This driver doesn't do any unwinding with blk_cleanup_disk() even on errors after add_disk() and so we follow that tradition.

Acked-by Coly Li <[email protected]>

Signed-off-by Luis Chamberlain <[email protected]>

---
 drivers/md/bcache/super.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

drivers/md/bcache/super.c +12 -5

index f2874c77ff79..f0c32cdd6594 100644
@@ -1082,7 +1082,9 @@ int bch_cached_dev_run(struct cached_dev *dc)
 		closure_sync(&cl);
 	}
 
-	add_disk(d->disk);
+	ret = add_disk(d->disk);
+	if (ret)
+		goto out;
 	bd_link_disk_holder(dc->bdev, dc->disk.disk);
 	/*
 	 * won't show up in the uevent file, use udevadm monitor -e instead

@@ -1534,10 +1536,11 @@ static void flash_dev_flush(struct closure *cl)
 
 static int flash_dev_run(struct cache_set *c, struct uuid_entry *u)
 {
+	int err = -ENOMEM;
 	struct bcache_device *d = kzalloc(sizeof(struct bcache_device),
 					  GFP_KERNEL);
 	if (!d)
-		return -ENOMEM;
+		goto err_ret;
 
 	closure_init(&d->cl, NULL);
 	set_closure_fn(&d->cl, flash_dev_flush, system_wq);

@@ -1551,9 +1554,12 @@ static int flash_dev_run(struct cache_set *c, struct uuid_entry *u)
 	bcache_device_attach(d, c, u - c->uuids);
 	bch_sectors_dirty_init(d);
 	bch_flash_dev_request_init(d);
-	add_disk(d->disk);
+	err = add_disk(d->disk);
+	if (err)
+		goto err;
 
-	if (kobject_add(&d->kobj, &disk_to_dev(d->disk)->kobj, "bcache"))
+	err = kobject_add(&d->kobj, &disk_to_dev(d->disk)->kobj, "bcache");
+	if (err)
 		goto err;
 
 	bcache_device_link(d, c, "volume");

@@ -1567,7 +1573,8 @@ static int flash_dev_run(struct cache_set *c, struct uuid_entry *u)
 	return 0;
 err:
 	kobject_put(&d->kobj);
-	return -ENOMEM;
+err_ret:
+	return err;
 }
 
 static int flash_devs_run(struct cache_set *c)

Signature · 1 line

2.30.2

Luis Chamberlain

author

2021-10-15 23:30 UTC #9 lore raw

PATCH 5/9 xen-blkfront: add error handling support for add_disk()

In reply to Luis Chamberlain

We never checked for errors on device_add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling. The function xlvbd_alloc_gendisk() typically does the unwinding on error on allocating the disk and creating the tag, but since all that error handling was stuffed inside xlvbd_alloc_gendisk() we must repeat the tag free'ing as well.

We set the info->rq to NULL to ensure blkif_free() doesn't crash on blk_mq_stop_hw_queues() on device_add_disk() error as the queue will be long gone by then.

Reviewed-by Juergen Gross <[email protected]>

Signed-off-by Luis Chamberlain <[email protected]>

---
 drivers/block/xen-blkfront.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

drivers/block/xen-blkfront.c +7 -1

index df0deb927760..8e3983e456f3 100644
@@ -2386,7 +2386,13 @@ static void blkfront_connect(struct blkfront_info *info)
 	for_each_rinfo(info, rinfo, i)
 		kick_pending_request_queues(rinfo);
 
-	device_add_disk(&info->xbdev->dev, info->gd, NULL);
+	err = device_add_disk(&info->xbdev->dev, info->gd, NULL);
+	if (err) {
+		blk_cleanup_disk(info->gd);
+		blk_mq_free_tag_set(&info->tag_set);
+		info->rq = NULL;
+		goto fail;
+	}
 
 	info->is_ready = 1;
 	return;

Signature · 1 line

2.30.2

Luis Chamberlain

author

2021-10-15 23:30 UTC #10 lore raw

PATCH 6/9 m68k/emu/nfblock: add error handling support for add_disk()

In reply to Luis Chamberlain

We never checked for errors on add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling.

Reviewed-by Geert Uytterhoeven <[email protected]>

Acked-by Geert Uytterhoeven <[email protected]>

Signed-off-by Luis Chamberlain <[email protected]>

---
 arch/m68k/emu/nfblock.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

arch/m68k/emu/nfblock.c +7 -2

index 9a8394e96388..4de5a6087034 100644
@@ -100,6 +100,7 @@ static int __init nfhd_init_one(int id, u32 blocks, u32 bsize)
 {
 	struct nfhd_device *dev;
 	int dev_id = id - NFHD_DEV_OFFSET;
+	int err = -ENOMEM;
 
 	pr_info("nfhd%u: found device with %u blocks (%u bytes)\n", dev_id,
 		blocks, bsize);

@@ -130,16 +131,20 @@ static int __init nfhd_init_one(int id, u32 blocks, u32 bsize)
 	sprintf(dev->disk->disk_name, "nfhd%u", dev_id);
 	set_capacity(dev->disk, (sector_t)blocks * (bsize / 512));
 	blk_queue_logical_block_size(dev->disk->queue, bsize);
-	add_disk(dev->disk);
+	err = add_disk(dev->disk);
+	if (err)
+		goto out_cleanup_disk;
 
 	list_add_tail(&dev->list, &nfhd_list);
 
 	return 0;
 
+out_cleanup_disk:
+	blk_cleanup_disk(dev->disk);
 free_dev:
 	kfree(dev);
 out:
-	return -ENOMEM;
+	return err;
 }
 
 static int __init nfhd_init(void)

Signature · 1 line

2.30.2

Luis Chamberlain

author

2021-10-15 23:30 UTC #11 lore raw

PATCH 7/9 um/drivers/ubd_kern: add error handling support for add_disk()

In reply to Luis Chamberlain

We never checked for errors on add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling.

ubd_disk_register() never returned an error, so just fix that now and let the caller handle the error condition.

Reviewed-by Gabriel Krisman Bertazi <[email protected]>

Signed-off-by Luis Chamberlain <[email protected]>

---
 arch/um/drivers/ubd_kern.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

arch/um/drivers/ubd_kern.c +9 -4

index fefd343412c7..69d2d0049a61 100644
@@ -855,8 +855,8 @@ static const struct attribute_group *ubd_attr_groups[] = {
 	NULL,
 };
 
-static void ubd_disk_register(int major, u64 size, int unit,
-			      struct gendisk *disk)
+static int ubd_disk_register(int major, u64 size, int unit,
+			     struct gendisk *disk)
 {
 	disk->major = major;
 	disk->first_minor = unit << UBD_SHIFT;

@@ -873,7 +873,7 @@ static void ubd_disk_register(int major, u64 size, int unit,
 
 	disk->private_data = &ubd_devs[unit];
 	disk->queue = ubd_devs[unit].queue;
-	device_add_disk(&ubd_devs[unit].pdev.dev, disk, ubd_attr_groups);
+	return device_add_disk(&ubd_devs[unit].pdev.dev, disk, ubd_attr_groups);
 }
 
 #define ROUND_BLOCK(n) ((n + (SECTOR_SIZE - 1)) & (-SECTOR_SIZE))

@@ -920,10 +920,15 @@ static int ubd_add(int n, char **error_out)
 	blk_queue_write_cache(ubd_dev->queue, true, false);
 	blk_queue_max_segments(ubd_dev->queue, MAX_SG);
 	blk_queue_segment_boundary(ubd_dev->queue, PAGE_SIZE - 1);
-	ubd_disk_register(UBD_MAJOR, ubd_dev->size, n, disk);
+	err = ubd_disk_register(UBD_MAJOR, ubd_dev->size, n, disk);
+	if (err)
+		goto out_cleanup_disk;
+
 	ubd_gendisk[n] = disk;
 	return 0;
 
+out_cleanup_disk:
+	blk_cleanup_disk(disk);
 out_cleanup_tags:
 	blk_mq_free_tag_set(&ubd_dev->tag_set);
 out:

Signature · 1 line

2.30.2

Luis Chamberlain

author

2021-10-15 23:30 UTC #12 lore raw

PATCH 8/9 rnbd: add error handling support for add_disk()

In reply to Luis Chamberlain

We never checked for errors on add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling.

Acked-by Jack Wang <[email protected]>

Signed-off-by Luis Chamberlain <[email protected]>

---
 drivers/block/rnbd/rnbd-clt.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

drivers/block/rnbd/rnbd-clt.c +9 -4

index 5864c9b46cb9..3b78dc55a9a2 100644
@@ -1384,8 +1384,10 @@ static void setup_request_queue(struct rnbd_clt_dev *dev)
 	blk_queue_write_cache(dev->queue, dev->wc, dev->fua);
 }
 
-static void rnbd_clt_setup_gen_disk(struct rnbd_clt_dev *dev, int idx)
+static int rnbd_clt_setup_gen_disk(struct rnbd_clt_dev *dev, int idx)
 {
+	int err;
+
 	dev->gd->major		= rnbd_client_major;
 	dev->gd->first_minor	= idx << RNBD_PART_BITS;
 	dev->gd->minors		= 1 << RNBD_PART_BITS;

@@ -1410,7 +1412,11 @@ static void rnbd_clt_setup_gen_disk(struct rnbd_clt_dev *dev, int idx)
 
 	if (!dev->rotational)
 		blk_queue_flag_set(QUEUE_FLAG_NONROT, dev->queue);
-	add_disk(dev->gd);
+	err = add_disk(dev->gd);
+	if (err)
+		blk_cleanup_disk(dev->gd);
+
+	return err;
 }
 
 static int rnbd_client_setup_device(struct rnbd_clt_dev *dev)

@@ -1426,8 +1432,7 @@ static int rnbd_client_setup_device(struct rnbd_clt_dev *dev)
 	rnbd_init_mq_hw_queues(dev);
 
 	setup_request_queue(dev);
-	rnbd_clt_setup_gen_disk(dev, idx);
-	return 0;
+	return rnbd_clt_setup_gen_disk(dev, idx);
 }
 
 static struct rnbd_clt_dev *init_dev(struct rnbd_clt_session *sess,

Signature · 1 line

2.30.2

Luis Chamberlain

author

2021-10-15 23:30 UTC #13 lore raw

PATCH 9/9 mtd: add add_disk() error handling

In reply to Luis Chamberlain

We never checked for errors on add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling.

Acked-by Miquel Raynal <[email protected]>

Signed-off-by Luis Chamberlain <[email protected]>

---
 drivers/mtd/mtd_blkdevs.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

drivers/mtd/mtd_blkdevs.c +5 -1

index b8ae1ec14e17..4eaba6f4ec68 100644
@@ -384,7 +384,9 @@ int add_mtd_blktrans_dev(struct mtd_blktrans_dev *new)
 	if (new->readonly)
 		set_disk_ro(gd, 1);
 
-	device_add_disk(&new->mtd->dev, gd, NULL);
+	ret = device_add_disk(&new->mtd->dev, gd, NULL);
+	if (ret)
+		goto out_cleanup_disk;
 
 	if (new->disk_attributes) {
 		ret = sysfs_create_group(&disk_to_dev(gd)->kobj,

@@ -393,6 +395,8 @@ int add_mtd_blktrans_dev(struct mtd_blktrans_dev *new)
 	}
 	return 0;
 
+out_cleanup_disk:
+	blk_cleanup_disk(new->disk);
 out_free_tag_set:
 	blk_mq_free_tag_set(new->tag_set);
 out_kfree_tag_set:

Signature · 1 line

2.30.2

Martin K. Petersen

Applied

2021-10-21 03:42 UTC #14 lore raw

In reply to Luis Chamberlain

Quoted 8 lines · Luis Chamberlain

Jens,
I had last split up patches into 7 groups, but at this point now most changes are merged except a few more drivers. Instead of creating a new patch set for each of the 7 groups I'm creating 3 new groups of patches now:
[...]

Applied to 5.16/scsi-queue, thanks!

[1/9] scsi/sd: add error handling support for add_disk()

      https://git.kernel.org/mkp/scsi/c/2a7a891f4c40
[2/9] scsi/sr: add error handling support for add_disk()
      https://git.kernel.org/mkp/scsi/c/e9d658c2175b

Signature · 1 line

Martin K. Petersen	Oracle Linux Engineering

Jens Axboe

maintainer Applied

2021-10-21 15:01 UTC #15 lore raw

Re: (subset) [PATCH 0/9] block: reviewed add_disk() error handling set

In reply to Luis Chamberlain

Quoted 8 lines · Luis Chamberlain

Jens,
I had last split up patches into 7 groups, but at this point now most changes are merged except a few more drivers. Instead of creating a new patch set for each of the 7 groups I'm creating 3 new groups of patches now:
[...]

Applied, thanks!

[3/9] dm: add add_disk() error handling

      commit: e7089f65dd51afeda5eb760506b5950d95f9ec29
[4/9] bcache: add error handling support for add_disk()
      commit: 2961c3bbcaec0ed7fb7b9a465b3796f37f2294e5
[5/9] xen-blkfront: add error handling support for add_disk()
      commit: 293a7c528803321479593d42d0898bb5a9769db1
[6/9] m68k/emu/nfblock: add error handling support for add_disk()
      commit: 21fd880d3da7564bab68979417cab7408e4f9642
[7/9] um/drivers/ubd_kern: add error handling support for add_disk()
      commit: 66638f163a2b5c5b462ca38525129b14a20117eb
[8/9] rnbd: add error handling support for add_disk()
      commit: 2e9e31bea01997450397d64da43b6675e0adb9e3
[9/9] mtd: add add_disk() error handling
      commit: 83b863f4a3f0de4ece7802d9121fed0c3e64145f

Best regards,

Signature · 1 line

Jens Axboe