linux-block.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 00/10] block: first batch of add_disk() error handling conversions
@ 2021-08-27 19:17 Luis Chamberlain
  2021-08-27 19:18 ` [PATCH 01/10] block/brd: add error handling support for add_disk() Luis Chamberlain
                   ` (10 more replies)
  0 siblings, 11 replies; 16+ messages in thread
From: Luis Chamberlain @ 2021-08-27 19:17 UTC (permalink / raw)
  To: axboe, colyli, kent.overstreet, kbusch, sagi, vishal.l.verma,
	dan.j.williams, dave.jiang, ira.weiny, konrad.wilk, roger.pau,
	boris.ostrovsky, jgross, sstabellini, minchan, ngupta,
	senozhatsky
  Cc: xen-devel, nvdimm, linux-nvme, linux-bcache, linux-block,
	linux-kernel, Luis Chamberlain

This is my second batch of driver conversions to use add_disk() error
handling. Please review and let me know if you spot any issues. This is
part of a larger effort to covert all drivers over to use the new
add_disk() error handling. The entire work can be found on my branch
dedicated for this work [0]

[0] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20210827-for-axboe-add-disk-error-handling-next-2nd

Luis Chamberlain (10):
  block/brd: add error handling support for add_disk()
  bcache: add error handling support for add_disk()
  nvme-multipath: add error handling support for add_disk()
  nvdimm/btt: do not call del_gendisk() if not needed
  nvdimm/btt: use goto error labels on btt_blk_init()
  nvdimm/btt: add error handling support for add_disk()
  nvdimm/blk: avoid calling del_gendisk() on early failures
  nvdimm/blk: add error handling support for add_disk()
  xen-blkfront: add error handling support for add_disk()
  zram: add error handling support for add_disk()

 drivers/block/brd.c           | 10 ++++++++--
 drivers/block/xen-blkfront.c  |  8 +++++++-
 drivers/block/zram/zram_drv.c |  6 +++++-
 drivers/md/bcache/super.c     | 17 ++++++++++++-----
 drivers/nvdimm/blk.c          | 21 +++++++++++++++------
 drivers/nvdimm/btt.c          | 24 +++++++++++++++---------
 drivers/nvme/host/multipath.c | 10 +++++++---
 7 files changed, 69 insertions(+), 27 deletions(-)

-- 
2.30.2


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 01/10] block/brd: add error handling support for add_disk()
  2021-08-27 19:17 [PATCH 00/10] block: first batch of add_disk() error handling conversions Luis Chamberlain
@ 2021-08-27 19:18 ` Luis Chamberlain
  2021-08-27 19:18 ` [PATCH 02/10] bcache: " Luis Chamberlain
                   ` (9 subsequent siblings)
  10 siblings, 0 replies; 16+ messages in thread
From: Luis Chamberlain @ 2021-08-27 19:18 UTC (permalink / raw)
  To: axboe, colyli, kent.overstreet, kbusch, sagi, vishal.l.verma,
	dan.j.williams, dave.jiang, ira.weiny, konrad.wilk, roger.pau,
	boris.ostrovsky, jgross, sstabellini, minchan, ngupta,
	senozhatsky
  Cc: xen-devel, nvdimm, linux-nvme, linux-bcache, linux-block,
	linux-kernel, 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.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 drivers/block/brd.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/block/brd.c b/drivers/block/brd.c
index 58ec167aa018..c2bf4946f4e3 100644
--- a/drivers/block/brd.c
+++ b/drivers/block/brd.c
@@ -372,6 +372,7 @@ static int brd_alloc(int i)
 	struct brd_device *brd;
 	struct gendisk *disk;
 	char buf[DISK_NAME_LEN];
+	int err = -ENOMEM;
 
 	brd = kzalloc(sizeof(*brd), GFP_KERNEL);
 	if (!brd)
@@ -410,14 +411,19 @@ static int brd_alloc(int i)
 	/* Tell the block layer that this is not a rotational device */
 	blk_queue_flag_set(QUEUE_FLAG_NONROT, disk->queue);
 	blk_queue_flag_clear(QUEUE_FLAG_ADD_RANDOM, disk->queue);
-	add_disk(disk);
+	err = add_disk(disk);
+	if (err)
+		goto out_cleanup_disk;
+
 	list_add_tail(&brd->brd_list, &brd_devices);
 
 	return 0;
 
+out_cleanup_disk:
+	blk_cleanup_disk(disk);
 out_free_dev:
 	kfree(brd);
-	return -ENOMEM;
+	return err;
 }
 
 static void brd_probe(dev_t dev)
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 02/10] bcache: add error handling support for add_disk()
  2021-08-27 19:17 [PATCH 00/10] block: first batch of add_disk() error handling conversions Luis Chamberlain
  2021-08-27 19:18 ` [PATCH 01/10] block/brd: add error handling support for add_disk() Luis Chamberlain
@ 2021-08-27 19:18 ` Luis Chamberlain
  2021-08-29  7:50   ` Coly Li
  2021-08-27 19:18 ` [PATCH 03/10] nvme-multipath: " Luis Chamberlain
                   ` (8 subsequent siblings)
  10 siblings, 1 reply; 16+ messages in thread
From: Luis Chamberlain @ 2021-08-27 19:18 UTC (permalink / raw)
  To: axboe, colyli, kent.overstreet, kbusch, sagi, vishal.l.verma,
	dan.j.williams, dave.jiang, ira.weiny, konrad.wilk, roger.pau,
	boris.ostrovsky, jgross, sstabellini, minchan, ngupta,
	senozhatsky
  Cc: xen-devel, nvdimm, linux-nvme, linux-bcache, linux-block,
	linux-kernel, 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.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 drivers/md/bcache/super.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index f2874c77ff79..f0c32cdd6594 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -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)
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 03/10] nvme-multipath: add error handling support for add_disk()
  2021-08-27 19:17 [PATCH 00/10] block: first batch of add_disk() error handling conversions Luis Chamberlain
  2021-08-27 19:18 ` [PATCH 01/10] block/brd: add error handling support for add_disk() Luis Chamberlain
  2021-08-27 19:18 ` [PATCH 02/10] bcache: " Luis Chamberlain
@ 2021-08-27 19:18 ` Luis Chamberlain
  2021-08-27 20:29   ` Keith Busch
  2021-08-27 19:18 ` [PATCH 04/10] nvdimm/btt: do not call del_gendisk() if not needed Luis Chamberlain
                   ` (7 subsequent siblings)
  10 siblings, 1 reply; 16+ messages in thread
From: Luis Chamberlain @ 2021-08-27 19:18 UTC (permalink / raw)
  To: axboe, colyli, kent.overstreet, kbusch, sagi, vishal.l.verma,
	dan.j.williams, dave.jiang, ira.weiny, konrad.wilk, roger.pau,
	boris.ostrovsky, jgross, sstabellini, minchan, ngupta,
	senozhatsky
  Cc: xen-devel, nvdimm, linux-nvme, linux-bcache, linux-block,
	linux-kernel, 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.

Since we now can tell for sure when a disk was added, move
setting the bit NVME_NSHEAD_DISK_LIVE only when we did
add the disk successfully.

Nothing to do here as the cleanup is done elsewhere.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 drivers/nvme/host/multipath.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index 37ce3e8b1db2..f95643629fdb 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -479,13 +479,17 @@ int nvme_mpath_alloc_disk(struct nvme_ctrl *ctrl, struct nvme_ns_head *head)
 static void nvme_mpath_set_live(struct nvme_ns *ns)
 {
 	struct nvme_ns_head *head = ns->head;
+	int rc;
 
 	if (!head->disk)
 		return;
 
-	if (!test_and_set_bit(NVME_NSHEAD_DISK_LIVE, &head->flags)) {
-		device_add_disk(&head->subsys->dev, head->disk,
-				nvme_ns_id_attr_groups);
+	if (!test_bit(NVME_NSHEAD_DISK_LIVE, &head->flags)) {
+		rc = device_add_disk(&head->subsys->dev, head->disk,
+				     nvme_ns_id_attr_groups);
+		if (rc)
+			return;
+		set_bit(NVME_NSHEAD_DISK_LIVE, &head->flags);
 		nvme_add_ns_head_cdev(head);
 	}
 
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 04/10] nvdimm/btt: do not call del_gendisk() if not needed
  2021-08-27 19:17 [PATCH 00/10] block: first batch of add_disk() error handling conversions Luis Chamberlain
                   ` (2 preceding siblings ...)
  2021-08-27 19:18 ` [PATCH 03/10] nvme-multipath: " Luis Chamberlain
@ 2021-08-27 19:18 ` Luis Chamberlain
  2021-08-27 19:18 ` [PATCH 05/10] nvdimm/btt: use goto error labels on btt_blk_init() Luis Chamberlain
                   ` (6 subsequent siblings)
  10 siblings, 0 replies; 16+ messages in thread
From: Luis Chamberlain @ 2021-08-27 19:18 UTC (permalink / raw)
  To: axboe, colyli, kent.overstreet, kbusch, sagi, vishal.l.verma,
	dan.j.williams, dave.jiang, ira.weiny, konrad.wilk, roger.pau,
	boris.ostrovsky, jgross, sstabellini, minchan, ngupta,
	senozhatsky
  Cc: xen-devel, nvdimm, linux-nvme, linux-bcache, linux-block,
	linux-kernel, Luis Chamberlain

We know we don't need del_gendisk() if we haven't added
the disk, so just skip it. This should fix a bug on older
kernels, as del_gendisk() became able to deal with
disks not added only recently, after the patch titled
"block: add flag for add_disk() completion notation".

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 drivers/nvdimm/btt.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c
index 92dec4952297..3fd1bdb9fc05 100644
--- a/drivers/nvdimm/btt.c
+++ b/drivers/nvdimm/btt.c
@@ -1538,7 +1538,6 @@ static int btt_blk_init(struct btt *btt)
 		int rc = nd_integrity_init(btt->btt_disk, btt_meta_size(btt));
 
 		if (rc) {
-			del_gendisk(btt->btt_disk);
 			blk_cleanup_disk(btt->btt_disk);
 			return rc;
 		}
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 05/10] nvdimm/btt: use goto error labels on btt_blk_init()
  2021-08-27 19:17 [PATCH 00/10] block: first batch of add_disk() error handling conversions Luis Chamberlain
                   ` (3 preceding siblings ...)
  2021-08-27 19:18 ` [PATCH 04/10] nvdimm/btt: do not call del_gendisk() if not needed Luis Chamberlain
@ 2021-08-27 19:18 ` Luis Chamberlain
  2021-08-27 19:18 ` [PATCH 06/10] nvdimm/btt: add error handling support for add_disk() Luis Chamberlain
                   ` (5 subsequent siblings)
  10 siblings, 0 replies; 16+ messages in thread
From: Luis Chamberlain @ 2021-08-27 19:18 UTC (permalink / raw)
  To: axboe, colyli, kent.overstreet, kbusch, sagi, vishal.l.verma,
	dan.j.williams, dave.jiang, ira.weiny, konrad.wilk, roger.pau,
	boris.ostrovsky, jgross, sstabellini, minchan, ngupta,
	senozhatsky
  Cc: xen-devel, nvdimm, linux-nvme, linux-bcache, linux-block,
	linux-kernel, Luis Chamberlain

This will make it easier to share common error paths.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 drivers/nvdimm/btt.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c
index 3fd1bdb9fc05..275704d80109 100644
--- a/drivers/nvdimm/btt.c
+++ b/drivers/nvdimm/btt.c
@@ -1520,10 +1520,11 @@ static int btt_blk_init(struct btt *btt)
 {
 	struct nd_btt *nd_btt = btt->nd_btt;
 	struct nd_namespace_common *ndns = nd_btt->ndns;
+	int rc = -ENOMEM;
 
 	btt->btt_disk = blk_alloc_disk(NUMA_NO_NODE);
 	if (!btt->btt_disk)
-		return -ENOMEM;
+		goto out;
 
 	nvdimm_namespace_disk_name(ndns, btt->btt_disk->disk_name);
 	btt->btt_disk->first_minor = 0;
@@ -1535,19 +1536,23 @@ static int btt_blk_init(struct btt *btt)
 	blk_queue_flag_set(QUEUE_FLAG_NONROT, btt->btt_disk->queue);
 
 	if (btt_meta_size(btt)) {
-		int rc = nd_integrity_init(btt->btt_disk, btt_meta_size(btt));
-
-		if (rc) {
-			blk_cleanup_disk(btt->btt_disk);
-			return rc;
-		}
+		rc = nd_integrity_init(btt->btt_disk, btt_meta_size(btt));
+		if (rc)
+			goto out_cleanup_disk;
 	}
+
 	set_capacity(btt->btt_disk, btt->nlba * btt->sector_size >> 9);
 	device_add_disk(&btt->nd_btt->dev, btt->btt_disk, NULL);
+
 	btt->nd_btt->size = btt->nlba * (u64)btt->sector_size;
 	nvdimm_check_and_set_ro(btt->btt_disk);
 
 	return 0;
+
+out_cleanup_disk:
+	blk_cleanup_disk(btt->btt_disk);
+out:
+	return rc;
 }
 
 static void btt_blk_cleanup(struct btt *btt)
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 06/10] nvdimm/btt: add error handling support for add_disk()
  2021-08-27 19:17 [PATCH 00/10] block: first batch of add_disk() error handling conversions Luis Chamberlain
                   ` (4 preceding siblings ...)
  2021-08-27 19:18 ` [PATCH 05/10] nvdimm/btt: use goto error labels on btt_blk_init() Luis Chamberlain
@ 2021-08-27 19:18 ` Luis Chamberlain
  2021-08-27 19:18 ` [PATCH 07/10] nvdimm/blk: avoid calling del_gendisk() on early failures Luis Chamberlain
                   ` (4 subsequent siblings)
  10 siblings, 0 replies; 16+ messages in thread
From: Luis Chamberlain @ 2021-08-27 19:18 UTC (permalink / raw)
  To: axboe, colyli, kent.overstreet, kbusch, sagi, vishal.l.verma,
	dan.j.williams, dave.jiang, ira.weiny, konrad.wilk, roger.pau,
	boris.ostrovsky, jgross, sstabellini, minchan, ngupta,
	senozhatsky
  Cc: xen-devel, nvdimm, linux-nvme, linux-bcache, linux-block,
	linux-kernel, 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.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 drivers/nvdimm/btt.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c
index 275704d80109..abdac4b7769f 100644
--- a/drivers/nvdimm/btt.c
+++ b/drivers/nvdimm/btt.c
@@ -1542,7 +1542,9 @@ static int btt_blk_init(struct btt *btt)
 	}
 
 	set_capacity(btt->btt_disk, btt->nlba * btt->sector_size >> 9);
-	device_add_disk(&btt->nd_btt->dev, btt->btt_disk, NULL);
+	rc = device_add_disk(&btt->nd_btt->dev, btt->btt_disk, NULL);
+	if (rc)
+		goto out_cleanup_disk;
 
 	btt->nd_btt->size = btt->nlba * (u64)btt->sector_size;
 	nvdimm_check_and_set_ro(btt->btt_disk);
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 07/10] nvdimm/blk: avoid calling del_gendisk() on early failures
  2021-08-27 19:17 [PATCH 00/10] block: first batch of add_disk() error handling conversions Luis Chamberlain
                   ` (5 preceding siblings ...)
  2021-08-27 19:18 ` [PATCH 06/10] nvdimm/btt: add error handling support for add_disk() Luis Chamberlain
@ 2021-08-27 19:18 ` Luis Chamberlain
  2021-08-27 19:18 ` [PATCH 08/10] nvdimm/blk: add error handling support for add_disk() Luis Chamberlain
                   ` (3 subsequent siblings)
  10 siblings, 0 replies; 16+ messages in thread
From: Luis Chamberlain @ 2021-08-27 19:18 UTC (permalink / raw)
  To: axboe, colyli, kent.overstreet, kbusch, sagi, vishal.l.verma,
	dan.j.williams, dave.jiang, ira.weiny, konrad.wilk, roger.pau,
	boris.ostrovsky, jgross, sstabellini, minchan, ngupta,
	senozhatsky
  Cc: xen-devel, nvdimm, linux-nvme, linux-bcache, linux-block,
	linux-kernel, Luis Chamberlain

If nd_integrity_init() fails we'd get del_gendisk() called,
but that's not correct as we should only call that if we're
done with device_add_disk(). Fix this by providing unwinding
prior to the devm call being registered and moving the devm
registration to the very end.

This should fix calling del_gendisk() if nd_integrity_init()
fails. I only spotted this issue through code inspection. It
does not fix any real world bug.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 drivers/nvdimm/blk.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/nvdimm/blk.c b/drivers/nvdimm/blk.c
index 088d3dd6f6fa..591fa1f86f1e 100644
--- a/drivers/nvdimm/blk.c
+++ b/drivers/nvdimm/blk.c
@@ -240,6 +240,7 @@ static int nsblk_attach_disk(struct nd_namespace_blk *nsblk)
 	resource_size_t available_disk_size;
 	struct gendisk *disk;
 	u64 internal_nlba;
+	int rc;
 
 	internal_nlba = div_u64(nsblk->size, nsblk_internal_lbasize(nsblk));
 	available_disk_size = internal_nlba * nsblk_sector_size(nsblk);
@@ -256,20 +257,26 @@ static int nsblk_attach_disk(struct nd_namespace_blk *nsblk)
 	blk_queue_logical_block_size(disk->queue, nsblk_sector_size(nsblk));
 	blk_queue_flag_set(QUEUE_FLAG_NONROT, disk->queue);
 
-	if (devm_add_action_or_reset(dev, nd_blk_release_disk, disk))
-		return -ENOMEM;
-
 	if (nsblk_meta_size(nsblk)) {
-		int rc = nd_integrity_init(disk, nsblk_meta_size(nsblk));
+		rc = nd_integrity_init(disk, nsblk_meta_size(nsblk));
 
 		if (rc)
-			return rc;
+			goto out_before_devm_err;
 	}
 
 	set_capacity(disk, available_disk_size >> SECTOR_SHIFT);
 	device_add_disk(dev, disk, NULL);
+
+	/* nd_blk_release_disk() is called if this fails */
+	if (devm_add_action_or_reset(dev, nd_blk_release_disk, disk))
+		return -ENOMEM;
+
 	nvdimm_check_and_set_ro(disk);
 	return 0;
+
+out_before_devm_err:
+	blk_cleanup_disk(disk);
+	return rc;
 }
 
 static int nd_blk_probe(struct device *dev)
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 08/10] nvdimm/blk: add error handling support for add_disk()
  2021-08-27 19:17 [PATCH 00/10] block: first batch of add_disk() error handling conversions Luis Chamberlain
                   ` (6 preceding siblings ...)
  2021-08-27 19:18 ` [PATCH 07/10] nvdimm/blk: avoid calling del_gendisk() on early failures Luis Chamberlain
@ 2021-08-27 19:18 ` Luis Chamberlain
  2021-08-27 19:18 ` [PATCH 09/10] xen-blkfront: " Luis Chamberlain
                   ` (2 subsequent siblings)
  10 siblings, 0 replies; 16+ messages in thread
From: Luis Chamberlain @ 2021-08-27 19:18 UTC (permalink / raw)
  To: axboe, colyli, kent.overstreet, kbusch, sagi, vishal.l.verma,
	dan.j.williams, dave.jiang, ira.weiny, konrad.wilk, roger.pau,
	boris.ostrovsky, jgross, sstabellini, minchan, ngupta,
	senozhatsky
  Cc: xen-devel, nvdimm, linux-nvme, linux-bcache, linux-block,
	linux-kernel, 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.

Since nvdimm/blk uses devm we just need to move the devm
registration towards the end. And in hindsight, that seems
to also provide a fix given del_gendisk() should not be
called unless the disk was already added via add_disk().
The probably of that issue happening is low though, like
OOM while calling devm_add_action(), so the fix is minor.

We manually unwind in case of add_disk() failure prior
to the devm registration.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 drivers/nvdimm/blk.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/nvdimm/blk.c b/drivers/nvdimm/blk.c
index 591fa1f86f1e..9f1eb41404ac 100644
--- a/drivers/nvdimm/blk.c
+++ b/drivers/nvdimm/blk.c
@@ -265,7 +265,9 @@ static int nsblk_attach_disk(struct nd_namespace_blk *nsblk)
 	}
 
 	set_capacity(disk, available_disk_size >> SECTOR_SHIFT);
-	device_add_disk(dev, disk, NULL);
+	rc = device_add_disk(dev, disk, NULL);
+	if (rc)
+		goto out_before_devm_err;
 
 	/* nd_blk_release_disk() is called if this fails */
 	if (devm_add_action_or_reset(dev, nd_blk_release_disk, disk))
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 09/10] xen-blkfront: add error handling support for add_disk()
  2021-08-27 19:17 [PATCH 00/10] block: first batch of add_disk() error handling conversions Luis Chamberlain
                   ` (7 preceding siblings ...)
  2021-08-27 19:18 ` [PATCH 08/10] nvdimm/blk: add error handling support for add_disk() Luis Chamberlain
@ 2021-08-27 19:18 ` Luis Chamberlain
  2021-08-27 19:18 ` [PATCH 10/10] zram: " Luis Chamberlain
  2021-08-27 19:19 ` [PATCH 00/10] block: first batch of add_disk() error handling conversions Luis Chamberlain
  10 siblings, 0 replies; 16+ messages in thread
From: Luis Chamberlain @ 2021-08-27 19:18 UTC (permalink / raw)
  To: axboe, colyli, kent.overstreet, kbusch, sagi, vishal.l.verma,
	dan.j.williams, dave.jiang, ira.weiny, konrad.wilk, roger.pau,
	boris.ostrovsky, jgross, sstabellini, minchan, ngupta,
	senozhatsky
  Cc: xen-devel, nvdimm, linux-nvme, linux-bcache, linux-block,
	linux-kernel, 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.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 drivers/block/xen-blkfront.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index 715bfa8aca7f..9fe28af5f6d9 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -2343,7 +2343,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;
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 10/10] zram: add error handling support for add_disk()
  2021-08-27 19:17 [PATCH 00/10] block: first batch of add_disk() error handling conversions Luis Chamberlain
                   ` (8 preceding siblings ...)
  2021-08-27 19:18 ` [PATCH 09/10] xen-blkfront: " Luis Chamberlain
@ 2021-08-27 19:18 ` Luis Chamberlain
  2021-08-27 19:19 ` [PATCH 00/10] block: first batch of add_disk() error handling conversions Luis Chamberlain
  10 siblings, 0 replies; 16+ messages in thread
From: Luis Chamberlain @ 2021-08-27 19:18 UTC (permalink / raw)
  To: axboe, colyli, kent.overstreet, kbusch, sagi, vishal.l.verma,
	dan.j.williams, dave.jiang, ira.weiny, konrad.wilk, roger.pau,
	boris.ostrovsky, jgross, sstabellini, minchan, ngupta,
	senozhatsky
  Cc: xen-devel, nvdimm, linux-nvme, linux-bcache, linux-block,
	linux-kernel, 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.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 drivers/block/zram/zram_drv.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index fcaf2750f68f..d5b343c2bc96 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -1953,7 +1953,9 @@ static int zram_add(void)
 		blk_queue_max_write_zeroes_sectors(zram->disk->queue, UINT_MAX);
 
 	blk_queue_flag_set(QUEUE_FLAG_STABLE_WRITES, zram->disk->queue);
-	device_add_disk(NULL, zram->disk, zram_disk_attr_groups);
+	ret = device_add_disk(NULL, zram->disk, zram_disk_attr_groups);
+	if (ret)
+		goto out_cleanup_disk;
 
 	strlcpy(zram->compressor, default_compressor, sizeof(zram->compressor));
 
@@ -1961,6 +1963,8 @@ static int zram_add(void)
 	pr_info("Added device: %s\n", zram->disk->disk_name);
 	return device_id;
 
+out_cleanup_disk:
+	blk_cleanup_disk(zram->disk);
 out_free_idr:
 	idr_remove(&zram_index_idr, device_id);
 out_free_dev:
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH 00/10] block: first batch of add_disk() error handling conversions
  2021-08-27 19:17 [PATCH 00/10] block: first batch of add_disk() error handling conversions Luis Chamberlain
                   ` (9 preceding siblings ...)
  2021-08-27 19:18 ` [PATCH 10/10] zram: " Luis Chamberlain
@ 2021-08-27 19:19 ` Luis Chamberlain
  10 siblings, 0 replies; 16+ messages in thread
From: Luis Chamberlain @ 2021-08-27 19:19 UTC (permalink / raw)
  To: axboe, colyli, kent.overstreet, kbusch, sagi, vishal.l.verma,
	dan.j.williams, dave.jiang, ira.weiny, konrad.wilk, roger.pau,
	boris.ostrovsky, jgross, sstabellini, minchan, ngupta,
	senozhatsky
  Cc: xen-devel, nvdimm, linux-nvme, linux-bcache, linux-block, linux-kernel


Botched the subject. Sorry. this is the *second* batch :)

  Luis

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 03/10] nvme-multipath: add error handling support for add_disk()
  2021-08-27 19:18 ` [PATCH 03/10] nvme-multipath: " Luis Chamberlain
@ 2021-08-27 20:29   ` Keith Busch
  2021-08-30 21:08     ` Luis Chamberlain
  0 siblings, 1 reply; 16+ messages in thread
From: Keith Busch @ 2021-08-27 20:29 UTC (permalink / raw)
  To: Luis Chamberlain
  Cc: axboe, colyli, kent.overstreet, sagi, vishal.l.verma,
	dan.j.williams, dave.jiang, ira.weiny, konrad.wilk, roger.pau,
	boris.ostrovsky, jgross, sstabellini, minchan, ngupta,
	senozhatsky, xen-devel, nvdimm, linux-nvme, linux-bcache,
	linux-block, linux-kernel

On Fri, Aug 27, 2021 at 12:18:02PM -0700, Luis Chamberlain wrote:
> @@ -479,13 +479,17 @@ int nvme_mpath_alloc_disk(struct nvme_ctrl *ctrl, struct nvme_ns_head *head)
>  static void nvme_mpath_set_live(struct nvme_ns *ns)
>  {
>  	struct nvme_ns_head *head = ns->head;
> +	int rc;
>  
>  	if (!head->disk)
>  		return;
>  
> -	if (!test_and_set_bit(NVME_NSHEAD_DISK_LIVE, &head->flags)) {
> -		device_add_disk(&head->subsys->dev, head->disk,
> -				nvme_ns_id_attr_groups);
> +	if (!test_bit(NVME_NSHEAD_DISK_LIVE, &head->flags)) {

This should still be test_and_set_bit() because it is protecting against
two nvme paths simultaneously calling device_add_disk() on the same
namespace head.

> +		rc = device_add_disk(&head->subsys->dev, head->disk,
> +				     nvme_ns_id_attr_groups);
> +		if (rc)
> +			return;
> +		set_bit(NVME_NSHEAD_DISK_LIVE, &head->flags);
>  		nvme_add_ns_head_cdev(head);
>  	}

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 02/10] bcache: add error handling support for add_disk()
  2021-08-27 19:18 ` [PATCH 02/10] bcache: " Luis Chamberlain
@ 2021-08-29  7:50   ` Coly Li
  0 siblings, 0 replies; 16+ messages in thread
From: Coly Li @ 2021-08-29  7:50 UTC (permalink / raw)
  To: Luis Chamberlain
  Cc: xen-devel, nvdimm, linux-nvme, linux-bcache, linux-block,
	linux-kernel, axboe, kent.overstreet, kbusch, sagi,
	vishal.l.verma, dan.j.williams, dave.jiang, ira.weiny,
	konrad.wilk, roger.pau, boris.ostrovsky, jgross, sstabellini,
	minchan, ngupta, senozhatsky

On 8/28/21 3:18 AM, Luis Chamberlain wrote:
> 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.
>
> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>

Acked-by: Coly Li <colyli@suse.de>

Thanks.

> ---
>   drivers/md/bcache/super.c | 17 ++++++++++++-----
>   1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> index f2874c77ff79..f0c32cdd6594 100644
> --- a/drivers/md/bcache/super.c
> +++ b/drivers/md/bcache/super.c
> @@ -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)


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 03/10] nvme-multipath: add error handling support for add_disk()
  2021-08-27 20:29   ` Keith Busch
@ 2021-08-30 21:08     ` Luis Chamberlain
  0 siblings, 0 replies; 16+ messages in thread
From: Luis Chamberlain @ 2021-08-30 21:08 UTC (permalink / raw)
  To: Keith Busch
  Cc: axboe, colyli, kent.overstreet, sagi, vishal.l.verma,
	dan.j.williams, dave.jiang, ira.weiny, konrad.wilk, roger.pau,
	boris.ostrovsky, jgross, sstabellini, minchan, ngupta,
	senozhatsky, xen-devel, nvdimm, linux-nvme, linux-bcache,
	linux-block, linux-kernel

On Fri, Aug 27, 2021 at 01:29:32PM -0700, Keith Busch wrote:
> On Fri, Aug 27, 2021 at 12:18:02PM -0700, Luis Chamberlain wrote:
> > @@ -479,13 +479,17 @@ int nvme_mpath_alloc_disk(struct nvme_ctrl *ctrl, struct nvme_ns_head *head)
> >  static void nvme_mpath_set_live(struct nvme_ns *ns)
> >  {
> >  	struct nvme_ns_head *head = ns->head;
> > +	int rc;
> >  
> >  	if (!head->disk)
> >  		return;
> >  
> > -	if (!test_and_set_bit(NVME_NSHEAD_DISK_LIVE, &head->flags)) {
> > -		device_add_disk(&head->subsys->dev, head->disk,
> > -				nvme_ns_id_attr_groups);
> > +	if (!test_bit(NVME_NSHEAD_DISK_LIVE, &head->flags)) {
> 
> This should still be test_and_set_bit() because it is protecting against
> two nvme paths simultaneously calling device_add_disk() on the same
> namespace head.

Interesting, I'll add a comment as well, as this was not clear with the drive
by effort.

  Luis

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 00/10] block: first batch of add_disk() error handling conversions
@ 2021-08-23 20:29 Luis Chamberlain
  0 siblings, 0 replies; 16+ messages in thread
From: Luis Chamberlain @ 2021-08-23 20:29 UTC (permalink / raw)
  To: axboe, martin.petersen, jejb, kbusch, sagi, adrian.hunter,
	beanhuo, ulf.hansson, avri.altman, swboyd, agk, snitzer, josef
  Cc: hch, hare, bvanassche, ming.lei, linux-scsi, linux-nvme,
	linux-mmc, dm-devel, nbd, linux-block, linux-kernel,
	Luis Chamberlain

There's a total of 70 pending patches in my queue which transform
drivers over to use the new add_disk() error handling. Now that
Jens has merged the core components what is left are all the other
driver conversions. A bit of these changes are helpers to make that
easier to do.

I'm going to split the driver conversions into batches, and
this first batch are drivers which should be of high priority
to consider.

Should this get merged, I'll chug on with the next batch, and
so on with batches of 10 each, until we tackle last the wonderful
world of floppy drivers.

I've put together a git tree based on Jen's for-5.15/block branch
which holds all of my pending changes, in case anyone wants to
take a peak.

[0] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20210823-for-axboe-add-disk-error-handling-next

Luis Chamberlain (10):
  scsi/sd: use blk_cleanup_queue() insted of put_disk()
  scsi/sd: add error handling support for add_disk()
  scsi/sr: use blk_cleanup_disk() instead of put_disk()
  scsi/sr: add error handling support for add_disk()
  nvme: add error handling support for add_disk()
  mmc/core/block: add error handling support for add_disk()
  md: add error handling support for add_disk()
  dm: add add_disk() error handling
  loop: add error handling support for add_disk()
  nbd: add error handling support for add_disk()

 drivers/block/loop.c     |  9 ++++++++-
 drivers/block/nbd.c      |  6 +++++-
 drivers/md/dm.c          | 16 +++++++++++-----
 drivers/md/md.c          |  7 ++++++-
 drivers/mmc/core/block.c |  4 +++-
 drivers/nvme/host/core.c | 10 +++++++++-
 drivers/scsi/sd.c        |  8 ++++++--
 drivers/scsi/sr.c        |  7 +++++--
 8 files changed, 53 insertions(+), 14 deletions(-)

-- 
2.30.2


^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2021-08-30 21:09 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-27 19:17 [PATCH 00/10] block: first batch of add_disk() error handling conversions Luis Chamberlain
2021-08-27 19:18 ` [PATCH 01/10] block/brd: add error handling support for add_disk() Luis Chamberlain
2021-08-27 19:18 ` [PATCH 02/10] bcache: " Luis Chamberlain
2021-08-29  7:50   ` Coly Li
2021-08-27 19:18 ` [PATCH 03/10] nvme-multipath: " Luis Chamberlain
2021-08-27 20:29   ` Keith Busch
2021-08-30 21:08     ` Luis Chamberlain
2021-08-27 19:18 ` [PATCH 04/10] nvdimm/btt: do not call del_gendisk() if not needed Luis Chamberlain
2021-08-27 19:18 ` [PATCH 05/10] nvdimm/btt: use goto error labels on btt_blk_init() Luis Chamberlain
2021-08-27 19:18 ` [PATCH 06/10] nvdimm/btt: add error handling support for add_disk() Luis Chamberlain
2021-08-27 19:18 ` [PATCH 07/10] nvdimm/blk: avoid calling del_gendisk() on early failures Luis Chamberlain
2021-08-27 19:18 ` [PATCH 08/10] nvdimm/blk: add error handling support for add_disk() Luis Chamberlain
2021-08-27 19:18 ` [PATCH 09/10] xen-blkfront: " Luis Chamberlain
2021-08-27 19:18 ` [PATCH 10/10] zram: " Luis Chamberlain
2021-08-27 19:19 ` [PATCH 00/10] block: first batch of add_disk() error handling conversions Luis Chamberlain
  -- strict thread matches above, loose matches on Subject: below --
2021-08-23 20:29 Luis Chamberlain

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).