linux-kernel.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; 15+ 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] 15+ 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; 15+ 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] 15+ 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; 15+ 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] 15+ 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; 15+ 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] 15+ 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; 15+ 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] 15+ 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; 15+ 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] 15+ 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; 15+ 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] 15+ 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; 15+ 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] 15+ 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; 15+ 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] 15+ 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; 15+ 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] 15+ 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; 15+ 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] 15+ 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; 15+ 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] 15+ 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; 15+ 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] 15+ 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; 15+ 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] 15+ 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; 15+ 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] 15+ messages in thread

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

Thread overview: 15+ 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

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).