[PATCH v1 0/8] block: add error handling for *add_disk*()

[PATCH v1 0/8] block: add error handling for *add_disk*()

[Luis Chamberlain]

This series bumps from RFC to PATCH form for two reasons, one is that I
have run quite a bit of full blktests tests on this without finding
regressions. Second is that the only feedback from the first RFC was
from Bart asking for error injection support, and this series adds just
that.

Lemme know if you've spotted something stupid. You can find these
patches my linux-next 20210427-block-fixes-v8 branch. Yes, it is
using a slightly older linux-next, so I can respin, its just that
there were quite a bit of linux-next release which were *really*
*fscked* up lately, so I had to stick at least something slightly
stable.

https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20210427-block-fixes-v8

Luis Chamberlain (8):
  block: refcount the request_queue early in __device_add_disk()
  block: move disk announce work from register_disk() to a helper
  block: move disk invalidation from del_gendisk() into a helper
  block: move disk unregistration work from del_gendisk() to a helper
  block: add initial error handling for *add_disk()* and friends
  loop: add error handling support for add_disk()
  null_blk: add error handling support for add_disk()
  block: add add_disk() failure injection support

 .../fault-injection/fault-injection.rst       |  23 ++
 block/Makefile                                |   1 +
 block/blk-core.c                              |   1 +
 block/blk-integrity.c                         |  12 +-
 block/blk-sysfs.c                             |   5 +-
 block/blk.h                                   |  60 +++-
 block/failure-injection.c                     |  54 +++
 block/genhd.c                                 | 331 ++++++++++++------
 drivers/block/loop.c                          |   7 +-
 drivers/block/null_blk/main.c                 |   9 +-
 include/linux/genhd.h                         |  14 +-
 lib/Kconfig.debug                             |  13 +
 12 files changed, 414 insertions(+), 116 deletions(-)
 create mode 100644 block/failure-injection.c

-- 
2.30.2

[PATCH v1 1/8] block: refcount the request_queue early in __device_add_disk()

[Luis Chamberlain]

We refcount the request_queue right now towards the end of the
__device_add_disk(), however when we add error handling on this
function we'll want to refcount the request_queue first, to help
make less complicated changes on drivers on their error paths.

For instance, today a driver may call add_disk without error handling
but still handle other errors:

int foo_block_add(...)
{
	...
	queue = blk_mq_init_queue(...);
	...
	disk->queue = queue;
	disk = alloc_disk(...);
	if (!disk)
		goto out_free_queue;
	...
        add_disk(disk);
	...
        return 0;

out_free_queue:
        blk_cleanup_queue(queue);
	/* Note: we never call put_disk() as add_disk() never failed */
	...
}

We want drivers to cleanup with put_disk() on the error path if
add_disk() fails. However, calling blk_cleanup_queue() will already
put the queue, and so the last put_disk() on the error path will
be extra. This can be simplified later if once error handling is
added to __device_add_disk(), if refcounting the request_queue
fails right away on __device_add_disk() we just return early and
set disk->NULL for the driver. That would ensure driver error
paths chug on with their error paths, and all they'd need to
expand with is the missing put_disk().

The collateral evolution for adding error paths for add_disk() becomes
larger with the alternative of replacing the blk_cleanup_queue() with
a put_disk(). We'd still need to sprinkle then some blk_cleanup_queue()
calls on the driver paths up above prior to add_disk(). And how would
we know we reached a part of add_disk() which did refcount then?

A related commit is 5a0ec388ef0 ("pktcdvd: Fix pkt_setup_dev() error
path") which *had* to take the approach of removing the blk_cleanup_queue()
because otherwise the driver crashes.

Moving this to the top ensure our future error path can easily just
handle this itself. For instance, if it was not able to refcount the
request_queue it can disk->queue to NULL, that way allowing a
blk_cleanup_queue() call followed but a put_disk(). And if the
refcount was incremented, we'd still be able to keep the same error
path of blk_cleanup_queue() followed by put_disk().

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 block/genhd.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/block/genhd.c b/block/genhd.c
index 39ca97b0edc6..51dff87c4756 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -504,6 +504,12 @@ static void __device_add_disk(struct device *parent, struct gendisk *disk,
 	dev_t devt;
 	int retval;
 
+	/*
+	 * Take an extra ref on queue which will be put on disk_release()
+	 * so that it sticks around as long as @disk is there.
+	 */
+	WARN_ON_ONCE(!blk_get_queue(disk->queue));
+
 	/*
 	 * The disk queue should now be all set with enough information about
 	 * the device for the elevator code to pick an adequate default
@@ -556,12 +562,6 @@ static void __device_add_disk(struct device *parent, struct gendisk *disk,
 	if (register_queue)
 		blk_register_queue(disk);
 
-	/*
-	 * Take an extra ref on queue which will be put on disk_release()
-	 * so that it sticks around as long as @disk is there.
-	 */
-	WARN_ON_ONCE(!blk_get_queue(disk->queue));
-
 	disk_add_events(disk);
 	blk_integrity_add(disk);
 }
-- 
2.30.2

[PATCH v1 2/8] block: move disk announce work from register_disk() to a helper

[Luis Chamberlain]

This moves quite a bit of code which does one thing into a helper.
We currently do not check for errors but we may decide that might
be desirable later.

This also makes the code easier to read.

This change has no functional changes.

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

diff --git a/block/genhd.c b/block/genhd.c
index 51dff87c4756..484cda981b4e 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -429,6 +429,15 @@ static void disk_scan_partitions(struct gendisk *disk)
 		blkdev_put(bdev, FMODE_READ);
 }
 
+static void disk_announce(struct gendisk *disk)
+{
+	struct device *ddev = disk_to_dev(disk);
+
+	/* announce the disk and partitions after all partitions are created */
+	dev_set_uevent_suppress(ddev, 0);
+	disk_uevent(disk, KOBJ_ADD);
+}
+
 static void register_disk(struct device *parent, struct gendisk *disk,
 			  const struct attribute_group **groups)
 {
@@ -472,10 +481,7 @@ static void register_disk(struct device *parent, struct gendisk *disk,
 		return;
 
 	disk_scan_partitions(disk);
-
-	/* announce the disk and partitions after all partitions are created */
-	dev_set_uevent_suppress(ddev, 0);
-	disk_uevent(disk, KOBJ_ADD);
+	disk_announce(disk);
 
 	if (disk->queue->backing_dev_info->dev) {
 		err = sysfs_create_link(&ddev->kobj,
-- 
2.30.2

[PATCH v1 3/8] block: move disk invalidation from del_gendisk() into a helper

[Luis Chamberlain]

Move the disk / partition invalidation into a helper. This will make
reading del_gendisk easier to read, in preparation for adding support
to add error handling later on register_disk() and to later share more
code with del_gendisk.

This change has no functional changes.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 block/genhd.c | 48 ++++++++++++++++++++++++++----------------------
 1 file changed, 26 insertions(+), 22 deletions(-)

diff --git a/block/genhd.c b/block/genhd.c
index 484cda981b4e..40a34981f9e2 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -438,6 +438,31 @@ static void disk_announce(struct gendisk *disk)
 	disk_uevent(disk, KOBJ_ADD);
 }
 
+static void disk_invalidate(struct gendisk *disk)
+{
+	if (!(disk->flags & GENHD_FL_HIDDEN)) {
+		sysfs_remove_link(&disk_to_dev(disk)->kobj, "bdi");
+
+		/*
+		 * Unregister bdi before releasing device numbers (as they can
+		 * get reused and we'd get clashes in sysfs).
+		 */
+		bdi_unregister(disk->queue->backing_dev_info);
+	}
+
+	blk_unregister_queue(disk);
+
+	kobject_put(disk->part0->bd_holder_dir);
+	kobject_put(disk->slave_dir);
+
+	part_stat_set_all(disk->part0, 0);
+	disk->part0->bd_stamp = 0;
+	if (!sysfs_deprecated)
+		sysfs_remove_link(block_depr, dev_name(disk_to_dev(disk)));
+	pm_runtime_set_memalloc_noio(disk_to_dev(disk), false);
+	device_del(disk_to_dev(disk));
+}
+
 static void register_disk(struct device *parent, struct gendisk *disk,
 			  const struct attribute_group **groups)
 {
@@ -614,7 +639,6 @@ void del_gendisk(struct gendisk *disk)
 
 	blk_integrity_del(disk);
 	disk_del_events(disk);
-
 	/*
 	 * Block lookups of the disk until all bdevs are unhashed and the
 	 * disk is marked as dead (GENHD_FL_UP cleared).
@@ -638,27 +662,7 @@ void del_gendisk(struct gendisk *disk)
 	disk->flags &= ~GENHD_FL_UP;
 	up_write(&bdev_lookup_sem);
 
-	if (!(disk->flags & GENHD_FL_HIDDEN)) {
-		sysfs_remove_link(&disk_to_dev(disk)->kobj, "bdi");
-
-		/*
-		 * Unregister bdi before releasing device numbers (as they can
-		 * get reused and we'd get clashes in sysfs).
-		 */
-		bdi_unregister(disk->queue->backing_dev_info);
-	}
-
-	blk_unregister_queue(disk);
-
-	kobject_put(disk->part0->bd_holder_dir);
-	kobject_put(disk->slave_dir);
-
-	part_stat_set_all(disk->part0, 0);
-	disk->part0->bd_stamp = 0;
-	if (!sysfs_deprecated)
-		sysfs_remove_link(block_depr, dev_name(disk_to_dev(disk)));
-	pm_runtime_set_memalloc_noio(disk_to_dev(disk), false);
-	device_del(disk_to_dev(disk));
+	disk_invalidate(disk);
 }
 EXPORT_SYMBOL(del_gendisk);
 
-- 
2.30.2

[PATCH v1 4/8] block: move disk unregistration work from del_gendisk() to a helper

[Luis Chamberlain]

There is quite a bit of code on del_gendisk() which relates to
unregistering the disk, using register_disk() as an counter.
Move all this code into a helper instead of re-writing our own,
which we'll need later to handle errors on add_disk().

Since disk unregistrationa also deals with parition unregistration,
provide a halper for that as well, as we'll later need this when
adding error handling for add_disk().

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 block/genhd.c | 56 +++++++++++++++++++++++++++++----------------------
 1 file changed, 32 insertions(+), 24 deletions(-)

diff --git a/block/genhd.c b/block/genhd.c
index 40a34981f9e2..baa68192ebb3 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -438,6 +438,31 @@ static void disk_announce(struct gendisk *disk)
 	disk_uevent(disk, KOBJ_ADD);
 }
 
+static void unregister_disk_partitions(struct gendisk *disk)
+{
+	/*
+	 * Block lookups of the disk until all bdevs are unhashed and the
+	 * disk is marked as dead (GENHD_FL_UP cleared).
+	 */
+	down_write(&bdev_lookup_sem);
+
+	mutex_lock(&disk->part0->bd_mutex);
+	blk_drop_partitions(disk);
+	mutex_unlock(&disk->part0->bd_mutex);
+	fsync_bdev(disk->part0);
+	__invalidate_device(disk->part0, true);
+
+	/*
+	 * Unhash the bdev inode for this device so that it can't be looked
+	 * up any more even if openers still hold references to it.
+	 */
+	remove_inode_hash(disk->part0->bd_inode);
+
+	set_capacity(disk, 0);
+	disk->flags &= ~GENHD_FL_UP;
+	up_write(&bdev_lookup_sem);
+}
+
 static void disk_invalidate(struct gendisk *disk)
 {
 	if (!(disk->flags & GENHD_FL_HIDDEN)) {
@@ -463,6 +488,12 @@ static void disk_invalidate(struct gendisk *disk)
 	device_del(disk_to_dev(disk));
 }
 
+static void unregister_disk(struct gendisk *disk)
+{
+	unregister_disk_partitions(disk);
+	disk_invalidate(disk);
+}
+
 static void register_disk(struct device *parent, struct gendisk *disk,
 			  const struct attribute_group **groups)
 {
@@ -639,30 +670,7 @@ void del_gendisk(struct gendisk *disk)
 
 	blk_integrity_del(disk);
 	disk_del_events(disk);
-	/*
-	 * Block lookups of the disk until all bdevs are unhashed and the
-	 * disk is marked as dead (GENHD_FL_UP cleared).
-	 */
-	down_write(&bdev_lookup_sem);
-
-	mutex_lock(&disk->part0->bd_mutex);
-	blk_drop_partitions(disk);
-	mutex_unlock(&disk->part0->bd_mutex);
-
-	fsync_bdev(disk->part0);
-	__invalidate_device(disk->part0, true);
-
-	/*
-	 * Unhash the bdev inode for this device so that it can't be looked
-	 * up any more even if openers still hold references to it.
-	 */
-	remove_inode_hash(disk->part0->bd_inode);
-
-	set_capacity(disk, 0);
-	disk->flags &= ~GENHD_FL_UP;
-	up_write(&bdev_lookup_sem);
-
-	disk_invalidate(disk);
+	unregister_disk(disk);
 }
 EXPORT_SYMBOL(del_gendisk);
 
-- 
2.30.2

[PATCH v1 5/8] block: add initial error handling for *add_disk()* and friends

[Luis Chamberlain]

This adds error handling to the *add_disk*() callers and the functions
it depends on. This is initial work as drivers are not converted. That
is separate work.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 block/blk-integrity.c |  12 ++--
 block/blk-sysfs.c     |   5 +-
 block/blk.h           |   5 +-
 block/genhd.c         | 152 +++++++++++++++++++++++++++++-------------
 include/linux/genhd.h |  14 ++--
 5 files changed, 127 insertions(+), 61 deletions(-)

diff --git a/block/blk-integrity.c b/block/blk-integrity.c
index 410da060d1f5..e46f47f2dec9 100644
--- a/block/blk-integrity.c
+++ b/block/blk-integrity.c
@@ -431,13 +431,17 @@ void blk_integrity_unregister(struct gendisk *disk)
 }
 EXPORT_SYMBOL(blk_integrity_unregister);
 
-void blk_integrity_add(struct gendisk *disk)
+int blk_integrity_add(struct gendisk *disk)
 {
-	if (kobject_init_and_add(&disk->integrity_kobj, &integrity_ktype,
-				 &disk_to_dev(disk)->kobj, "%s", "integrity"))
-		return;
+	int ret;
+
+	ret = kobject_init_and_add(&disk->integrity_kobj, &integrity_ktype,
+				 &disk_to_dev(disk)->kobj, "%s", "integrity");
+	if (ret)
+		return ret;
 
 	kobject_uevent(&disk->integrity_kobj, KOBJ_ADD);
+	return 0;
 }
 
 void blk_integrity_del(struct gendisk *disk)
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index e03bedf180ab..8a0978a0623c 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -862,9 +862,10 @@ int blk_register_queue(struct gendisk *disk)
 	if (WARN_ON(!q))
 		return -ENXIO;
 
-	WARN_ONCE(blk_queue_registered(q),
+	if (WARN_ONCE(blk_queue_registered(q),
 		  "%s is registering an already registered queue\n",
-		  kobject_name(&dev->kobj));
+		  kobject_name(&dev->kobj)))
+		return -ENXIO;
 
 	/*
 	 * SCSI probing may synchronously create and destroy a lot of
diff --git a/block/blk.h b/block/blk.h
index 8b3591aee0a5..01ec7aba8d70 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -132,7 +132,7 @@ static inline bool integrity_req_gap_front_merge(struct request *req,
 				bip_next->bip_vec[0].bv_offset);
 }
 
-void blk_integrity_add(struct gendisk *);
+int blk_integrity_add(struct gendisk *);
 void blk_integrity_del(struct gendisk *);
 #else /* CONFIG_BLK_DEV_INTEGRITY */
 static inline bool blk_integrity_merge_rq(struct request_queue *rq,
@@ -166,8 +166,9 @@ static inline bool bio_integrity_endio(struct bio *bio)
 static inline void bio_integrity_free(struct bio *bio)
 {
 }
-static inline void blk_integrity_add(struct gendisk *disk)
+static inline int blk_integrity_add(struct gendisk *disk)
 {
+	return 0;
 }
 static inline void blk_integrity_del(struct gendisk *disk)
 {
diff --git a/block/genhd.c b/block/genhd.c
index baa68192ebb3..eafcd256fc6f 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -37,8 +37,8 @@ static DEFINE_IDA(ext_devt_ida);
 
 static void disk_check_events(struct disk_events *ev,
 			      unsigned int *clearing_ptr);
-static void disk_alloc_events(struct gendisk *disk);
-static void disk_add_events(struct gendisk *disk);
+static int disk_alloc_events(struct gendisk *disk);
+static int disk_add_events(struct gendisk *disk);
 static void disk_del_events(struct gendisk *disk);
 static void disk_release_events(struct gendisk *disk);
 
@@ -494,8 +494,9 @@ static void unregister_disk(struct gendisk *disk)
 	disk_invalidate(disk);
 }
 
-static void register_disk(struct device *parent, struct gendisk *disk,
-			  const struct attribute_group **groups)
+static int __must_check register_disk(struct device *parent,
+				      struct gendisk *disk,
+				      const struct attribute_group **groups)
 {
 	struct device *ddev = disk_to_dev(disk);
 	int err;
@@ -511,15 +512,20 @@ static void register_disk(struct device *parent, struct gendisk *disk,
 		WARN_ON(ddev->groups);
 		ddev->groups = groups;
 	}
-	if (device_add(ddev))
-		return;
+	err = device_add(ddev);
+	if (err) {
+		/*
+		 * We don't put_device(ddev), the driver is responsible to
+		 * issue the last put_device(ddev), however it instead uses
+		 * put_disk().
+		 */
+		return err;
+	}
 	if (!sysfs_deprecated) {
 		err = sysfs_create_link(block_depr, &ddev->kobj,
 					kobject_name(&ddev->kobj));
-		if (err) {
-			device_del(ddev);
-			return;
-		}
+		if (err)
+			goto exit_del_device;
 	}
 
 	/*
@@ -534,7 +540,7 @@ static void register_disk(struct device *parent, struct gendisk *disk,
 	disk->slave_dir = kobject_create_and_add("slaves", &ddev->kobj);
 
 	if (disk->flags & GENHD_FL_HIDDEN)
-		return;
+		return 0;
 
 	disk_scan_partitions(disk);
 	disk_announce(disk);
@@ -543,8 +549,19 @@ static void register_disk(struct device *parent, struct gendisk *disk,
 		err = sysfs_create_link(&ddev->kobj,
 			  &disk->queue->backing_dev_info->dev->kobj,
 			  "bdi");
-		WARN_ON(err);
+		if (WARN_ON(err))
+			goto exit_del_block_depr;
 	}
+	return 0;
+
+exit_del_block_depr:
+	unregister_disk_partitions(disk);
+	if (!sysfs_deprecated)
+		sysfs_remove_link(block_depr, dev_name(disk_to_dev(disk)));
+exit_del_device:
+	device_del(ddev);
+
+	return err;
 }
 
 /**
@@ -557,20 +574,24 @@ static void register_disk(struct device *parent, struct gendisk *disk,
  * This function registers the partitioning information in @disk
  * with the kernel.
  *
- * FIXME: error handling
  */
-static void __device_add_disk(struct device *parent, struct gendisk *disk,
-			      const struct attribute_group **groups,
-			      bool register_queue)
+static int __device_add_disk(struct device *parent, struct gendisk *disk,
+			     const struct attribute_group **groups,
+			     bool register_queue)
 {
 	dev_t devt;
 	int retval;
 
 	/*
 	 * Take an extra ref on queue which will be put on disk_release()
-	 * so that it sticks around as long as @disk is there.
+	 * so that it sticks around as long as @disk is there. The driver
+	 * must call blk_cleanup_queue() and then put_disk() on error from
+	 * this function.
 	 */
-	WARN_ON_ONCE(!blk_get_queue(disk->queue));
+	if (WARN_ON_ONCE(!blk_get_queue(disk->queue))) {
+		disk->queue = NULL;
+		return -ESHUTDOWN;
+	}
 
 	/*
 	 * The disk queue should now be all set with enough information about
@@ -585,21 +606,24 @@ static void __device_add_disk(struct device *parent, struct gendisk *disk,
 	 * be accompanied with EXT_DEVT flag.  Make sure all
 	 * parameters make sense.
 	 */
-	WARN_ON(disk->minors && !(disk->major || disk->first_minor));
-	WARN_ON(!disk->minors &&
-		!(disk->flags & (GENHD_FL_EXT_DEVT | GENHD_FL_HIDDEN)));
+	if (WARN_ON(disk->minors && !(disk->major || disk->first_minor)))
+		return -EINVAL;
+	if (WARN_ON(!disk->minors &&
+		    !(disk->flags & (GENHD_FL_EXT_DEVT | GENHD_FL_HIDDEN))))
+		return -EINVAL;
 
 	disk->flags |= GENHD_FL_UP;
 
 	retval = blk_alloc_devt(disk->part0, &devt);
-	if (retval) {
-		WARN_ON(1);
-		return;
-	}
+	if (WARN_ON(retval))
+		return retval;
+
 	disk->major = MAJOR(devt);
 	disk->first_minor = MINOR(devt);
 
-	disk_alloc_events(disk);
+	retval = disk_alloc_events(disk);
+	if (retval)
+		goto exit_blk_free_devt;
 
 	if (disk->flags & GENHD_FL_HIDDEN) {
 		/*
@@ -611,34 +635,62 @@ static void __device_add_disk(struct device *parent, struct gendisk *disk,
 	} else {
 		struct backing_dev_info *bdi = disk->queue->backing_dev_info;
 		struct device *dev = disk_to_dev(disk);
-		int ret;
 
 		/* Register BDI before referencing it from bdev */
 		dev->devt = devt;
-		ret = bdi_register(bdi, "%u:%u", MAJOR(devt), MINOR(devt));
-		WARN_ON(ret);
+		retval = bdi_register(bdi, "%u:%u", MAJOR(devt), MINOR(devt));
+		if (WARN_ON(retval))
+			goto exit_disk_release_events;
 		bdi_set_owner(bdi, dev);
 		bdev_add(disk->part0, devt);
 	}
-	register_disk(parent, disk, groups);
-	if (register_queue)
-		blk_register_queue(disk);
+	retval = register_disk(parent, disk, groups);
+	if (retval)
+		goto exit_unregister_bdi;
+
+	if (register_queue) {
+		retval = blk_register_queue(disk);
+		if (retval)
+			goto exit_unregister_disk;
+	}
 
-	disk_add_events(disk);
-	blk_integrity_add(disk);
+	retval = disk_add_events(disk);
+	if (retval)
+		goto exit_unregister_disk;
+
+	retval = blk_integrity_add(disk);
+	if (retval)
+		goto exit_del_events;
+
+	return 0;
+exit_del_events:
+	disk_del_events(disk);
+exit_unregister_disk:
+	unregister_disk(disk);
+	return retval;
+
+exit_unregister_bdi:
+	if (disk->queue && !(disk->flags & GENHD_FL_HIDDEN))
+		bdi_unregister(disk->queue->backing_dev_info);
+exit_disk_release_events:
+	disk_release_events(disk);
+exit_blk_free_devt:
+	blk_free_devt(disk_devt(disk));
+
+	return retval;
 }
 
-void device_add_disk(struct device *parent, struct gendisk *disk,
-		     const struct attribute_group **groups)
+int device_add_disk(struct device *parent, struct gendisk *disk,
+		    const struct attribute_group **groups)
 
 {
-	__device_add_disk(parent, disk, groups, true);
+	return __device_add_disk(parent, disk, groups, true);
 }
 EXPORT_SYMBOL(device_add_disk);
 
-void device_add_disk_no_queue_reg(struct device *parent, struct gendisk *disk)
+int device_add_disk_no_queue_reg(struct device *parent, struct gendisk *disk)
 {
-	__device_add_disk(parent, disk, NULL, false);
+	return __device_add_disk(parent, disk, NULL, false);
 }
 EXPORT_SYMBOL(device_add_disk_no_queue_reg);
 
@@ -1817,17 +1869,17 @@ module_param_cb(events_dfl_poll_msecs, &disk_events_dfl_poll_msecs_param_ops,
 /*
  * disk_{alloc|add|del|release}_events - initialize and destroy disk_events.
  */
-static void disk_alloc_events(struct gendisk *disk)
+static int disk_alloc_events(struct gendisk *disk)
 {
 	struct disk_events *ev;
 
 	if (!disk->fops->check_events || !disk->events)
-		return;
+		return 0;
 
 	ev = kzalloc(sizeof(*ev), GFP_KERNEL);
 	if (!ev) {
 		pr_warn("%s: failed to initialize events\n", disk->disk_name);
-		return;
+		return -ENOMEM;
 	}
 
 	INIT_LIST_HEAD(&ev->node);
@@ -1839,17 +1891,23 @@ static void disk_alloc_events(struct gendisk *disk)
 	INIT_DELAYED_WORK(&ev->dwork, disk_events_workfn);
 
 	disk->ev = ev;
+
+	return 0;
 }
 
-static void disk_add_events(struct gendisk *disk)
+static int disk_add_events(struct gendisk *disk)
 {
-	/* FIXME: error handling */
-	if (sysfs_create_files(&disk_to_dev(disk)->kobj, disk_events_attrs) < 0)
+	int ret;
+
+	ret = sysfs_create_files(&disk_to_dev(disk)->kobj, disk_events_attrs);
+	if (ret < 0) {
 		pr_warn("%s: failed to create sysfs files for events\n",
 			disk->disk_name);
+		return ret;
+	}
 
 	if (!disk->ev)
-		return;
+		return 0;
 
 	mutex_lock(&disk_events_mutex);
 	list_add_tail(&disk->ev->node, &disk_events);
@@ -1860,6 +1918,8 @@ static void disk_add_events(struct gendisk *disk)
 	 * unblock kicks it into action.
 	 */
 	__disk_unblock_events(disk, true);
+
+	return 0;
 }
 
 static void disk_del_events(struct gendisk *disk)
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index 7e9660ea967d..5a23d97ccb90 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -205,16 +205,16 @@ static inline dev_t disk_devt(struct gendisk *disk)
 void disk_uevent(struct gendisk *disk, enum kobject_action action);
 
 /* block/genhd.c */
-extern void device_add_disk(struct device *parent, struct gendisk *disk,
-			    const struct attribute_group **groups);
-static inline void add_disk(struct gendisk *disk)
+extern int device_add_disk(struct device *parent, struct gendisk *disk,
+			   const struct attribute_group **groups);
+static inline int add_disk(struct gendisk *disk)
 {
-	device_add_disk(NULL, disk, NULL);
+	return device_add_disk(NULL, disk, NULL);
 }
-extern void device_add_disk_no_queue_reg(struct device *parent, struct gendisk *disk);
-static inline void add_disk_no_queue_reg(struct gendisk *disk)
+extern int device_add_disk_no_queue_reg(struct device *parent, struct gendisk *disk);
+static inline int add_disk_no_queue_reg(struct gendisk *disk)
 {
-	device_add_disk_no_queue_reg(NULL, disk);
+	return device_add_disk_no_queue_reg(NULL, disk);
 }
 
 extern void del_gendisk(struct gendisk *gp);
-- 
2.30.2

[PATCH v1 6/8] loop: add error handling support for add_disk()

[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/loop.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index d58d68f3c7cd..a22d8c985bf3 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -2170,10 +2170,15 @@ static int loop_add(struct loop_device **l, int i)
 	disk->private_data	= lo;
 	disk->queue		= lo->lo_queue;
 	sprintf(disk->disk_name, "loop%d", i);
-	add_disk(disk);
+	err = add_disk(disk);
+	if (err)
+		goto out_put_disk;
+
 	*l = lo;
 	return lo->lo_number;
 
+out_put_disk:
+	put_disk(lo->lo_disk);
 out_free_queue:
 	blk_cleanup_queue(lo->lo_queue);
 out_cleanup_tags:
-- 
2.30.2

[PATCH v1 7/8] null_blk: add error handling support for add_disk()

[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/null_blk/main.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
index 5f006d9e1472..2346d1292b26 100644
--- a/drivers/block/null_blk/main.c
+++ b/drivers/block/null_blk/main.c
@@ -1699,6 +1699,7 @@ static int init_driver_queues(struct nullb *nullb)
 
 static int null_gendisk_register(struct nullb *nullb)
 {
+	int ret;
 	sector_t size = ((sector_t)nullb->dev->size * SZ_1M) >> SECTOR_SHIFT;
 	struct gendisk *disk;
 
@@ -1719,13 +1720,17 @@ static int null_gendisk_register(struct nullb *nullb)
 	strncpy(disk->disk_name, nullb->disk_name, DISK_NAME_LEN);
 
 	if (nullb->dev->zoned) {
-		int ret = null_register_zoned_dev(nullb);
+		ret = null_register_zoned_dev(nullb);
 
 		if (ret)
 			return ret;
 	}
 
-	add_disk(disk);
+	ret = add_disk(disk);
+	if (ret) {
+		put_disk(disk);
+		return ret;
+	}
 	return 0;
 }
 
-- 
2.30.2

[PATCH v1 8/8] block: add add_disk() failure injection support

[Luis Chamberlain]

For a long time we have lived without any error handling
on the add_disk() error path. Now that we have some initial
error handling, add error injection support for its path so
that we can test it and ensure we don't regress this path
moving forward.

This only adds runtime code *iff* the new bool CONFIG_FAIL_ADD_DISK is
enabled in your kernel. If you don't have this enabled this provides
no new functional. When CONFIG_FAIL_ADD_DISK is disabled the new routine
blk_should_fail_add_disk() ends up being transformed to if (false), and
so the compiler should optimize these out as dead code producing no
new effective binary changes.

Failure injection lets us configure at boot how often we want a failure
to take place by specifying the interval, the probability, and when needed
a size constraint. We don't need to test for size constraints for
add_disk() and so ignore that part of error injection. Although testing
early boot failures with add_disk() failures might be useful we don't
to make add_disk() fail every time as otherwise we wouldn't be able to
boot. So enabling add_disk() error injection requires a second post
boot step where you specify where in the add_disk() code path you want
to enable failure injection for. This lets us verify correctness of
the different error handling parts of add_disk(), while also allowing
a respective blktests test to grow dynamically in case the add_disk()
paths grows.

We currently enable 11 code paths on add_disk() which can fail
and we can test for:

	# ls -1 /sys/kernel/debug/block/config_fail_add_disk/
	alloc_devt
	alloc_events
	bdi_register
	device_add
	disk_add_events
	get_queue
	integrity_add
	register_disk
	register_queue
	sysfs_bdi_link
	sysfs_depr_link

If you want to modify the configuration of fail_add_disk dynamically
at boot, you can enable CONFIG_FAULT_INJECTION_DEBUG_FS. If you've
enabled CONFIG_FAIL_ADD_DISK you will see these knobs:

	# ls -1 /sys/kernel/debug/block/fail_add_disk/
	interval
	probability
	space
	task-filter
	times
	verbose
	verbose_ratelimit_burst
	verbose_ratelimit_interval_ms

Suggested-by: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 .../fault-injection/fault-injection.rst       | 23 ++++++++
 block/Makefile                                |  1 +
 block/blk-core.c                              |  1 +
 block/blk.h                                   | 55 ++++++++++++++++++
 block/failure-injection.c                     | 54 ++++++++++++++++++
 block/genhd.c                                 | 57 +++++++++++++++++++
 lib/Kconfig.debug                             | 13 +++++
 7 files changed, 204 insertions(+)
 create mode 100644 block/failure-injection.c

diff --git a/Documentation/fault-injection/fault-injection.rst b/Documentation/fault-injection/fault-injection.rst
index 31ecfe44e5b4..129f41b9a581 100644
--- a/Documentation/fault-injection/fault-injection.rst
+++ b/Documentation/fault-injection/fault-injection.rst
@@ -24,6 +24,29 @@ Available fault injection capabilities
 
   injects futex deadlock and uaddr fault errors.
 
+- fail_add_disk
+
+  allows error injection to the block layer's add_disk() call path. Enabling
+  this at boot doesn't immediately force errors, you have to then select post
+  boot where in the add_disk() code path you want a failure to be triggered.
+  The following code paths are supported:
+
+  # ls -1 /sys/kernel/debug/block/config_fail_add_disk/
+  alloc_devt
+  alloc_events
+  bdi_register
+  device_add
+  disk_add_events
+  get_queue
+  integrity_add
+  register_disk
+  register_queue
+  sysfs_bdi_link
+  sysfs_depr_link
+
+  If you enable CONFIG_FAULT_INJECTION_DEBUG_FS the fail_add_disk failure
+  injection parameters are placed under /sys/kernel/debug/block/fail_add_disk/
+
 - fail_make_request
 
   injects disk IO errors on devices permitted by setting
diff --git a/block/Makefile b/block/Makefile
index 8d841f5f986f..1589bffe2528 100644
--- a/block/Makefile
+++ b/block/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_BLOCK) := bio.o elevator.o blk-core.o blk-sysfs.o \
 			blk-mq-sysfs.o blk-mq-cpumap.o blk-mq-sched.o ioctl.o \
 			genhd.o ioprio.o badblocks.o partitions/ blk-rq-qos.o
 
+obj-$(CONFIG_FAIL_ADD_DISK)	+= failure-injection.o
 obj-$(CONFIG_BOUNCE)		+= bounce.o
 obj-$(CONFIG_BLK_SCSI_REQUEST)	+= scsi_ioctl.o
 obj-$(CONFIG_BLK_DEV_BSG)	+= bsg.o
diff --git a/block/blk-core.c b/block/blk-core.c
index 9bcdae93f6d4..657da4ae7d35 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1797,6 +1797,7 @@ int __init blk_dev_init(void)
 			sizeof(struct request_queue), 0, SLAB_PANIC, NULL);
 
 	blk_debugfs_root = debugfs_create_dir("block", NULL);
+	blk_init_add_disk_fail();
 
 	return 0;
 }
diff --git a/block/blk.h b/block/blk.h
index 01ec7aba8d70..b92972433f86 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -344,6 +344,61 @@ static inline void blk_queue_free_zone_bitmaps(struct request_queue *q) {}
 static inline void blk_queue_clear_zone_settings(struct request_queue *q) {}
 #endif
 
+#ifdef CONFIG_FAIL_ADD_DISK
+
+/**
+ * struct blk_add_disk_fail - configuration for add disk failure injection
+ *
+ * You can enable add_disk() failure injection on boot, but the add_disk()
+ * path is quite large. In order to support allowing configuration of where
+ * exactly on that add_disk() we want to fail, we use a debugfs directory.
+
+ * By default enabling add_disk() failure on boot won't produce any failures,
+ * after boot you must enable at least one of the debugfs knobs. Kernel warnings
+ * which would typically happen on the block layer are supressed when using
+ * error injection.
+ *
+ * @get_queue: mimics a failure on the first blk_get_queue() at the beginning of
+ *	the add_disk() call chain.
+ * @alloc_devt: mimics a failure as if blk_alloc_devt() had failed
+ * @alloc_events: mimics failure as if disk_alloc_events() had failed
+ * @bdi_register: mimics failure as if bdi_register() had failed
+ * @register_disk: mimics failure as if register_disk() had failed
+ * @device_add: mimics failure as if the core device_add() call had failed
+ * @sysfs_depr_link: mimics failure as if creating the now deprecated sysfs
+ *	device link with sysfs_create_link() had failed.
+ * @sysfs_bdi_link: mimics failure as if creating the bdi link with
+ *	sysfs_create_link() had failed
+ * @register_queue: mimics failure on blk_register_queue()
+ * @disk_add_events: mimics failure on disk_add_events()
+ * @integrity_add: mimics failure on blk_integrity_add() at the very end
+ *	of the add_disk() call chain.
+ */
+struct blk_config_add_disk_fail {
+	bool get_queue;
+	bool alloc_devt;
+	bool alloc_events;
+	bool bdi_register;
+	bool register_disk;
+	bool device_add;
+	bool sysfs_depr_link;
+	bool sysfs_bdi_link;
+	bool register_queue;
+	bool disk_add_events;
+	bool integrity_add;
+};
+
+extern struct blk_config_add_disk_fail blk_config_add_disk_fail;
+
+void blk_init_add_disk_fail(void);
+#define blk_should_fail_add_disk(path_name) \
+	__blk_should_fail_add_disk(blk_config_add_disk_fail.path_name)
+int __blk_should_fail_add_disk(bool evaluate);
+#else
+static inline void blk_init_add_disk_fail(void) {}
+#define blk_should_fail_add_disk(path_name) (false)
+#endif
+
 int blk_alloc_devt(struct block_device *part, dev_t *devt);
 void blk_free_devt(dev_t devt);
 char *disk_name(struct gendisk *hd, int partno, char *buf);
diff --git a/block/failure-injection.c b/block/failure-injection.c
new file mode 100644
index 000000000000..ba7717192eaa
--- /dev/null
+++ b/block/failure-injection.c
@@ -0,0 +1,54 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/fault-inject.h>
+
+#include "blk.h"
+
+static DECLARE_FAULT_ATTR(fail_add_disk);
+struct blk_config_add_disk_fail blk_config_add_disk_fail;
+
+static int __init setup_fail_add_disk(char *str)
+{
+	return setup_fault_attr(&fail_add_disk, str);
+}
+
+__setup("fail_blk_add_disk=", setup_fail_add_disk);
+
+struct dentry *config_fail_add_disk;
+
+void blk_init_add_disk_fail(void)
+{
+	fault_create_debugfs_attr("fail_add_disk", blk_debugfs_root, &fail_add_disk);
+	config_fail_add_disk = debugfs_create_dir("config_fail_add_disk", blk_debugfs_root);
+
+	debugfs_create_bool("get_queue", 0600, config_fail_add_disk,
+			    &blk_config_add_disk_fail.get_queue);
+	debugfs_create_bool("alloc_devt", 0600, config_fail_add_disk,
+			    &blk_config_add_disk_fail.alloc_devt);
+	debugfs_create_bool("alloc_events", 0600, config_fail_add_disk,
+			    &blk_config_add_disk_fail.alloc_events);
+	debugfs_create_bool("bdi_register", 0600, config_fail_add_disk,
+			    &blk_config_add_disk_fail.bdi_register);
+	debugfs_create_bool("register_disk", 0600, config_fail_add_disk,
+			    &blk_config_add_disk_fail.register_disk);
+	debugfs_create_bool("device_add", 0600, config_fail_add_disk,
+			    &blk_config_add_disk_fail.device_add);
+	debugfs_create_bool("sysfs_depr_link", 0600, config_fail_add_disk,
+			    &blk_config_add_disk_fail.sysfs_depr_link);
+	debugfs_create_bool("sysfs_bdi_link", 0600, config_fail_add_disk,
+			    &blk_config_add_disk_fail.sysfs_bdi_link);
+	debugfs_create_bool("register_queue", 0600, config_fail_add_disk,
+			    &blk_config_add_disk_fail.register_queue);
+	debugfs_create_bool("disk_add_events", 0600, config_fail_add_disk,
+			    &blk_config_add_disk_fail.disk_add_events);
+	debugfs_create_bool("integrity_add", 0600, config_fail_add_disk,
+			    &blk_config_add_disk_fail.integrity_add);
+}
+
+int __blk_should_fail_add_disk(bool evaluate)
+{
+	if (!evaluate)
+		return 0;
+
+	return should_fail(&fail_add_disk, 0);
+}
diff --git a/block/genhd.c b/block/genhd.c
index eafcd256fc6f..27b4b446cf3e 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -24,6 +24,7 @@
 #include <linux/log2.h>
 #include <linux/pm_runtime.h>
 #include <linux/badblocks.h>
+#include <linux/fault-inject.h>
 
 #include "blk.h"
 
@@ -512,6 +513,10 @@ static int __must_check register_disk(struct device *parent,
 		WARN_ON(ddev->groups);
 		ddev->groups = groups;
 	}
+
+	if (blk_should_fail_add_disk(device_add))
+		return -ENOMEM;
+
 	err = device_add(ddev);
 	if (err) {
 		/*
@@ -521,7 +526,14 @@ static int __must_check register_disk(struct device *parent,
 		 */
 		return err;
 	}
+
+
 	if (!sysfs_deprecated) {
+		if (blk_should_fail_add_disk(sysfs_depr_link)) {
+			err = -ENOMEM;
+			goto exit_del_device;
+		}
+
 		err = sysfs_create_link(block_depr, &ddev->kobj,
 					kobject_name(&ddev->kobj));
 		if (err)
@@ -546,6 +558,11 @@ static int __must_check register_disk(struct device *parent,
 	disk_announce(disk);
 
 	if (disk->queue->backing_dev_info->dev) {
+		if (blk_should_fail_add_disk(sysfs_bdi_link)) {
+			err = -ENOMEM;
+			goto exit_del_block_depr;
+		}
+
 		err = sysfs_create_link(&ddev->kobj,
 			  &disk->queue->backing_dev_info->dev->kobj,
 			  "bdi");
@@ -582,6 +599,11 @@ static int __device_add_disk(struct device *parent, struct gendisk *disk,
 	dev_t devt;
 	int retval;
 
+	if (blk_should_fail_add_disk(get_queue)) {
+		disk->queue = NULL;
+		return -ESHUTDOWN;
+	}
+
 	/*
 	 * Take an extra ref on queue which will be put on disk_release()
 	 * so that it sticks around as long as @disk is there. The driver
@@ -614,6 +636,9 @@ static int __device_add_disk(struct device *parent, struct gendisk *disk,
 
 	disk->flags |= GENHD_FL_UP;
 
+	if (blk_should_fail_add_disk(alloc_devt))
+		return -EINVAL;
+
 	retval = blk_alloc_devt(disk->part0, &devt);
 	if (WARN_ON(retval))
 		return retval;
@@ -621,6 +646,11 @@ static int __device_add_disk(struct device *parent, struct gendisk *disk,
 	disk->major = MAJOR(devt);
 	disk->first_minor = MINOR(devt);
 
+	if (blk_should_fail_add_disk(alloc_events)) {
+		retval = -ENOMEM;
+		goto exit_blk_free_devt;
+	}
+
 	retval = disk_alloc_events(disk);
 	if (retval)
 		goto exit_blk_free_devt;
@@ -638,26 +668,53 @@ static int __device_add_disk(struct device *parent, struct gendisk *disk,
 
 		/* Register BDI before referencing it from bdev */
 		dev->devt = devt;
+
+		if (blk_should_fail_add_disk(bdi_register)) {
+			retval = -ENOMEM;
+			goto exit_disk_release_events;
+		}
+
 		retval = bdi_register(bdi, "%u:%u", MAJOR(devt), MINOR(devt));
 		if (WARN_ON(retval))
 			goto exit_disk_release_events;
 		bdi_set_owner(bdi, dev);
 		bdev_add(disk->part0, devt);
 	}
+
+	if (blk_should_fail_add_disk(register_disk)) {
+		retval = -ENOMEM;
+		goto exit_unregister_bdi;
+	}
+
 	retval = register_disk(parent, disk, groups);
 	if (retval)
 		goto exit_unregister_bdi;
 
+	if (blk_should_fail_add_disk(register_queue)) {
+		retval = -ENOMEM;
+		goto exit_unregister_disk;
+	}
+
 	if (register_queue) {
 		retval = blk_register_queue(disk);
 		if (retval)
 			goto exit_unregister_disk;
 	}
 
+	if (blk_should_fail_add_disk(disk_add_events)) {
+		retval = -ENOMEM;
+		goto exit_unregister_disk;
+	}
+
 	retval = disk_add_events(disk);
 	if (retval)
 		goto exit_unregister_disk;
 
+	if (blk_should_fail_add_disk(integrity_add)) {
+		retval = -ENOMEM;
+		goto exit_del_events;
+	}
+
 	retval = blk_integrity_add(disk);
 	if (retval)
 		goto exit_del_events;
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index d1467658361f..4fccc0fad190 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1917,6 +1917,19 @@ config FAULT_INJECTION_USERCOPY
 	  Provides fault-injection capability to inject failures
 	  in usercopy functions (copy_from_user(), get_user(), ...).
 
+config FAIL_ADD_DISK
+	bool "Fault-injection capability for add_disk() callers"
+	depends on FAULT_INJECTION && BLOCK
+	help
+	  Provide fault-injection capability for the add_disk() block layer
+	  call path. This allows the kernel to provide error injection when
+	  the add_disk() call is made. You would use something like blktests
+	  test against this or just load the null_blk driver. This only
+	  enables the error injection functionality. To use it you must
+	  configure which path you want to trigger on error on using debugfs
+	  under /sys/kernel/debug/block/config_fail_add_disk/. By default
+	  all of these are disabled.
+
 config FAIL_MAKE_REQUEST
 	bool "Fault-injection capability for disk IO"
 	depends on FAULT_INJECTION && BLOCK
-- 
2.30.2

Re: [PATCH v1 0/8] block: add error handling for *add_disk*()

[Christoph Hellwig]

Just FYI, I have a large series to clean up a lot of the
disk/request_queue allocation and cleanup.  Which should help
with actually adding add_disk error handling to drivers, and has
some as far as I can tell minor context conflicts with your work.

Re: [PATCH v1 1/8] block: refcount the request_queue early in __device_add_disk()

[Hannes Reinecke]

On 5/12/21 8:46 AM, Luis Chamberlain wrote:
> We refcount the request_queue right now towards the end of the
> __device_add_disk(), however when we add error handling on this
> function we'll want to refcount the request_queue first, to help
> make less complicated changes on drivers on their error paths.
> 
> For instance, today a driver may call add_disk without error handling
> but still handle other errors:
> 
> int foo_block_add(...)
> {
> 	...
> 	queue = blk_mq_init_queue(...);
> 	...
> 	disk->queue = queue;
> 	disk = alloc_disk(...);
> 	if (!disk)
> 		goto out_free_queue;
> 	...
>          add_disk(disk);
> 	...
>          return 0;
> 
> out_free_queue:
>          blk_cleanup_queue(queue);
> 	/* Note: we never call put_disk() as add_disk() never failed */
> 	...
> }
> 
> We want drivers to cleanup with put_disk() on the error path if
> add_disk() fails. However, calling blk_cleanup_queue() will already
> put the queue, and so the last put_disk() on the error path will
> be extra. This can be simplified later if once error handling is
> added to __device_add_disk(), if refcounting the request_queue
> fails right away on __device_add_disk() we just return early and
> set disk->NULL for the driver. That would ensure driver error
> paths chug on with their error paths, and all they'd need to
> expand with is the missing put_disk().
> 
> The collateral evolution for adding error paths for add_disk() becomes
> larger with the alternative of replacing the blk_cleanup_queue() with
> a put_disk(). We'd still need to sprinkle then some blk_cleanup_queue()
> calls on the driver paths up above prior to add_disk(). And how would
> we know we reached a part of add_disk() which did refcount then?
> 
> A related commit is 5a0ec388ef0 ("pktcdvd: Fix pkt_setup_dev() error
> path") which *had* to take the approach of removing the blk_cleanup_queue()
> because otherwise the driver crashes.
> 
> Moving this to the top ensure our future error path can easily just
> handle this itself. For instance, if it was not able to refcount the
> request_queue it can disk->queue to NULL, that way allowing a
> blk_cleanup_queue() call followed but a put_disk(). And if the
> refcount was incremented, we'd still be able to keep the same error
> path of blk_cleanup_queue() followed by put_disk().
> 
> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> ---
>   block/genhd.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH v1 2/8] block: move disk announce work from register_disk() to a helper

[Hannes Reinecke]

On 5/12/21 8:46 AM, Luis Chamberlain wrote:
> This moves quite a bit of code which does one thing into a helper.
> We currently do not check for errors but we may decide that might
> be desirable later.
> 
> This also makes the code easier to read.
> 
> This change has no functional changes.
> 
> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> ---
>   block/genhd.c | 14 ++++++++++----
>   1 file changed, 10 insertions(+), 4 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH v1 3/8] block: move disk invalidation from del_gendisk() into a helper

[Hannes Reinecke]

On 5/12/21 8:46 AM, Luis Chamberlain wrote:
> Move the disk / partition invalidation into a helper. This will make
> reading del_gendisk easier to read, in preparation for adding support
> to add error handling later on register_disk() and to later share more
> code with del_gendisk.
> 
> This change has no functional changes.
> 
> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> ---
>   block/genhd.c | 48 ++++++++++++++++++++++++++----------------------
>   1 file changed, 26 insertions(+), 22 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH v1 4/8] block: move disk unregistration work from del_gendisk() to a helper

[Hannes Reinecke]

On 5/12/21 8:46 AM, Luis Chamberlain wrote:
> There is quite a bit of code on del_gendisk() which relates to
> unregistering the disk, using register_disk() as an counter.
> Move all this code into a helper instead of re-writing our own,
> which we'll need later to handle errors on add_disk().
> 
> Since disk unregistrationa also deals with parition unregistration,
> provide a halper for that as well, as we'll later need this when
> adding error handling for add_disk().
> 
> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> ---
>   block/genhd.c | 56 +++++++++++++++++++++++++++++----------------------
>   1 file changed, 32 insertions(+), 24 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH v1 5/8] block: add initial error handling for *add_disk()* and friends

[Hannes Reinecke]

On 5/12/21 8:46 AM, Luis Chamberlain wrote:
> This adds error handling to the *add_disk*() callers and the functions
> it depends on. This is initial work as drivers are not converted. That
> is separate work.
> 
> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> ---
>   block/blk-integrity.c |  12 ++--
>   block/blk-sysfs.c     |   5 +-
>   block/blk.h           |   5 +-
>   block/genhd.c         | 152 +++++++++++++++++++++++++++++-------------
>   include/linux/genhd.h |  14 ++--
>   5 files changed, 127 insertions(+), 61 deletions(-)
> 
> diff --git a/block/blk-integrity.c b/block/blk-integrity.c
> index 410da060d1f5..e46f47f2dec9 100644
> --- a/block/blk-integrity.c
> +++ b/block/blk-integrity.c
> @@ -431,13 +431,17 @@ void blk_integrity_unregister(struct gendisk *disk)
>   }
>   EXPORT_SYMBOL(blk_integrity_unregister);
>   
> -void blk_integrity_add(struct gendisk *disk)
> +int blk_integrity_add(struct gendisk *disk)
>   {
> -	if (kobject_init_and_add(&disk->integrity_kobj, &integrity_ktype,
> -				 &disk_to_dev(disk)->kobj, "%s", "integrity"))
> -		return;
> +	int ret;
> +
> +	ret = kobject_init_and_add(&disk->integrity_kobj, &integrity_ktype,
> +				 &disk_to_dev(disk)->kobj, "%s", "integrity");
> +	if (ret)
> +		return ret;
>   
>   	kobject_uevent(&disk->integrity_kobj, KOBJ_ADD);
> +	return 0;
>   }
>   
>   void blk_integrity_del(struct gendisk *disk)
> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
> index e03bedf180ab..8a0978a0623c 100644
> --- a/block/blk-sysfs.c
> +++ b/block/blk-sysfs.c
> @@ -862,9 +862,10 @@ int blk_register_queue(struct gendisk *disk)
>   	if (WARN_ON(!q))
>   		return -ENXIO;
>   
> -	WARN_ONCE(blk_queue_registered(q),
> +	if (WARN_ONCE(blk_queue_registered(q),
>   		  "%s is registering an already registered queue\n",
> -		  kobject_name(&dev->kobj));
> +		  kobject_name(&dev->kobj)))
> +		return -ENXIO;
>   
>   	/*
>   	 * SCSI probing may synchronously create and destroy a lot of
> diff --git a/block/blk.h b/block/blk.h
> index 8b3591aee0a5..01ec7aba8d70 100644
> --- a/block/blk.h
> +++ b/block/blk.h
> @@ -132,7 +132,7 @@ static inline bool integrity_req_gap_front_merge(struct request *req,
>   				bip_next->bip_vec[0].bv_offset);
>   }
>   
> -void blk_integrity_add(struct gendisk *);
> +int blk_integrity_add(struct gendisk *);
>   void blk_integrity_del(struct gendisk *);
>   #else /* CONFIG_BLK_DEV_INTEGRITY */
>   static inline bool blk_integrity_merge_rq(struct request_queue *rq,
> @@ -166,8 +166,9 @@ static inline bool bio_integrity_endio(struct bio *bio)
>   static inline void bio_integrity_free(struct bio *bio)
>   {
>   }
> -static inline void blk_integrity_add(struct gendisk *disk)
> +static inline int blk_integrity_add(struct gendisk *disk)
>   {
> +	return 0;
>   }
>   static inline void blk_integrity_del(struct gendisk *disk)
>   {
> diff --git a/block/genhd.c b/block/genhd.c
> index baa68192ebb3..eafcd256fc6f 100644
> --- a/block/genhd.c
> +++ b/block/genhd.c
> @@ -37,8 +37,8 @@ static DEFINE_IDA(ext_devt_ida);
>   
>   static void disk_check_events(struct disk_events *ev,
>   			      unsigned int *clearing_ptr);
> -static void disk_alloc_events(struct gendisk *disk);
> -static void disk_add_events(struct gendisk *disk);
> +static int disk_alloc_events(struct gendisk *disk);
> +static int disk_add_events(struct gendisk *disk);
>   static void disk_del_events(struct gendisk *disk);
>   static void disk_release_events(struct gendisk *disk);
>   
> @@ -494,8 +494,9 @@ static void unregister_disk(struct gendisk *disk)
>   	disk_invalidate(disk);
>   }
>   
> -static void register_disk(struct device *parent, struct gendisk *disk,
> -			  const struct attribute_group **groups)
> +static int __must_check register_disk(struct device *parent,
> +				      struct gendisk *disk,
> +				      const struct attribute_group **groups)
>   {
>   	struct device *ddev = disk_to_dev(disk);
>   	int err;
> @@ -511,15 +512,20 @@ static void register_disk(struct device *parent, struct gendisk *disk,
>   		WARN_ON(ddev->groups);
>   		ddev->groups = groups;
>   	}
> -	if (device_add(ddev))
> -		return;
> +	err = device_add(ddev);
> +	if (err) {
> +		/*
> +		 * We don't put_device(ddev), the driver is responsible to
> +		 * issue the last put_device(ddev), however it instead uses
> +		 * put_disk().
> +		 */
> +		return err;
> +	}
>   	if (!sysfs_deprecated) {
>   		err = sysfs_create_link(block_depr, &ddev->kobj,
>   					kobject_name(&ddev->kobj));
> -		if (err) {
> -			device_del(ddev);
> -			return;
> -		}
> +		if (err)
> +			goto exit_del_device;
>   	}
>   
>   	/*
> @@ -534,7 +540,7 @@ static void register_disk(struct device *parent, struct gendisk *disk,
>   	disk->slave_dir = kobject_create_and_add("slaves", &ddev->kobj);
>   
>   	if (disk->flags & GENHD_FL_HIDDEN)
> -		return;
> +		return 0;
>   
>   	disk_scan_partitions(disk);
>   	disk_announce(disk);
> @@ -543,8 +549,19 @@ static void register_disk(struct device *parent, struct gendisk *disk,
>   		err = sysfs_create_link(&ddev->kobj,
>   			  &disk->queue->backing_dev_info->dev->kobj,
>   			  "bdi");
> -		WARN_ON(err);
> +		if (WARN_ON(err))
> +			goto exit_del_block_depr;
>   	}
> +	return 0;
> +
> +exit_del_block_depr:
> +	unregister_disk_partitions(disk);
> +	if (!sysfs_deprecated)
> +		sysfs_remove_link(block_depr, dev_name(disk_to_dev(disk)));
> +exit_del_device:
> +	device_del(ddev);
> +
> +	return err;
>   }
>   
>   /**
> @@ -557,20 +574,24 @@ static void register_disk(struct device *parent, struct gendisk *disk,
>    * This function registers the partitioning information in @disk
>    * with the kernel.
>    *
> - * FIXME: error handling
>    */
> -static void __device_add_disk(struct device *parent, struct gendisk *disk,
> -			      const struct attribute_group **groups,
> -			      bool register_queue)
> +static int __device_add_disk(struct device *parent, struct gendisk *disk,
> +			     const struct attribute_group **groups,
> +			     bool register_queue)
>   {
>   	dev_t devt;
>   	int retval;
>   
>   	/*
>   	 * Take an extra ref on queue which will be put on disk_release()
> -	 * so that it sticks around as long as @disk is there.
> +	 * so that it sticks around as long as @disk is there. The driver
> +	 * must call blk_cleanup_queue() and then put_disk() on error from
> +	 * this function.
>   	 */
> -	WARN_ON_ONCE(!blk_get_queue(disk->queue));
> +	if (WARN_ON_ONCE(!blk_get_queue(disk->queue))) {
> +		disk->queue = NULL;

Please don't.
We don't set disk->queue here in this function, and we don't know what 
the caller will do with that value.
If we start setting it to NULL here we'll need to audit all drivers to 
ensure they don't look at disk->queue after calling this function.
Plus it's good coding practice to only clear fields which you set in the 
first place, which is not the case here.

> +		return -ESHUTDOWN;
> +	}
>   
>   	/*
>   	 * The disk queue should now be all set with enough information about
> @@ -585,21 +606,24 @@ static void __device_add_disk(struct device *parent, struct gendisk *disk,
>   	 * be accompanied with EXT_DEVT flag.  Make sure all
>   	 * parameters make sense.
>   	 */
> -	WARN_ON(disk->minors && !(disk->major || disk->first_minor));
> -	WARN_ON(!disk->minors &&
> -		!(disk->flags & (GENHD_FL_EXT_DEVT | GENHD_FL_HIDDEN)));
> +	if (WARN_ON(disk->minors && !(disk->major || disk->first_minor)))
> +		return -EINVAL;
> +	if (WARN_ON(!disk->minors &&
> +		    !(disk->flags & (GENHD_FL_EXT_DEVT | GENHD_FL_HIDDEN))))
> +		return -EINVAL;
>   
>   	disk->flags |= GENHD_FL_UP;
>   
>   	retval = blk_alloc_devt(disk->part0, &devt);
> -	if (retval) {
> -		WARN_ON(1);
> -		return;
> -	}
> +	if (WARN_ON(retval))

Don't we need to reset GENHD_FL_UP here?

> +		return retval;
> +
>   	disk->major = MAJOR(devt);
>   	disk->first_minor = MINOR(devt);
>   
> -	disk_alloc_events(disk);
> +	retval = disk_alloc_events(disk);
> +	if (retval)
> +		goto exit_blk_free_devt;
>   
>   	if (disk->flags & GENHD_FL_HIDDEN) {
>   		/*
> @@ -611,34 +635,62 @@ static void __device_add_disk(struct device *parent, struct gendisk *disk,
>   	} else {
>   		struct backing_dev_info *bdi = disk->queue->backing_dev_info;
>   		struct device *dev = disk_to_dev(disk);
> -		int ret;
>   
>   		/* Register BDI before referencing it from bdev */
>   		dev->devt = devt;
> -		ret = bdi_register(bdi, "%u:%u", MAJOR(devt), MINOR(devt));
> -		WARN_ON(ret);
> +		retval = bdi_register(bdi, "%u:%u", MAJOR(devt), MINOR(devt));
> +		if (WARN_ON(retval))
> +			goto exit_disk_release_events;
>   		bdi_set_owner(bdi, dev);
>   		bdev_add(disk->part0, devt);
>   	}
> -	register_disk(parent, disk, groups);
> -	if (register_queue)
> -		blk_register_queue(disk);
> +	retval = register_disk(parent, disk, groups);
> +	if (retval)
> +		goto exit_unregister_bdi;
> +
> +	if (register_queue) {
> +		retval = blk_register_queue(disk);
> +		if (retval)
> +			goto exit_unregister_disk;
> +	}
>   
> -	disk_add_events(disk);
> -	blk_integrity_add(disk);
> +	retval = disk_add_events(disk);
> +	if (retval)
> +		goto exit_unregister_disk;
> +
> +	retval = blk_integrity_add(disk);
> +	if (retval)
> +		goto exit_del_events;
> +
> +	return 0;
> +exit_del_events:
> +	disk_del_events(disk);
> +exit_unregister_disk:
> +	unregister_disk(disk);
> +	return retval;
> +
> +exit_unregister_bdi:
> +	if (disk->queue && !(disk->flags & GENHD_FL_HIDDEN))
> +		bdi_unregister(disk->queue->backing_dev_info);
> +exit_disk_release_events:
> +	disk_release_events(disk);
> +exit_blk_free_devt:
> +	blk_free_devt(disk_devt(disk));
> +

Same here: In the error path we might end up with the GENHD_FL_UP being 
set. I would expected that flag to be unset once we return with an error 
here.

> +	return retval;
>   }
>   
> -void device_add_disk(struct device *parent, struct gendisk *disk,
> -		     const struct attribute_group **groups)
> +int device_add_disk(struct device *parent, struct gendisk *disk,
> +		    const struct attribute_group **groups)
>   
>   {
> -	__device_add_disk(parent, disk, groups, true);
> +	return __device_add_disk(parent, disk, groups, true);
>   }
>   EXPORT_SYMBOL(device_add_disk);
>   
> -void device_add_disk_no_queue_reg(struct device *parent, struct gendisk *disk)
> +int device_add_disk_no_queue_reg(struct device *parent, struct gendisk *disk)
>   {
> -	__device_add_disk(parent, disk, NULL, false);
> +	return __device_add_disk(parent, disk, NULL, false);
>   }
>   EXPORT_SYMBOL(device_add_disk_no_queue_reg);
>   
> @@ -1817,17 +1869,17 @@ module_param_cb(events_dfl_poll_msecs, &disk_events_dfl_poll_msecs_param_ops,
>   /*
>    * disk_{alloc|add|del|release}_events - initialize and destroy disk_events.
>    */
> -static void disk_alloc_events(struct gendisk *disk)
> +static int disk_alloc_events(struct gendisk *disk)
>   {
>   	struct disk_events *ev;
>   
>   	if (!disk->fops->check_events || !disk->events)
> -		return;
> +		return 0;
>   
>   	ev = kzalloc(sizeof(*ev), GFP_KERNEL);
>   	if (!ev) {
>   		pr_warn("%s: failed to initialize events\n", disk->disk_name);
> -		return;
> +		return -ENOMEM;
>   	}
>   
>   	INIT_LIST_HEAD(&ev->node);
> @@ -1839,17 +1891,23 @@ static void disk_alloc_events(struct gendisk *disk)
>   	INIT_DELAYED_WORK(&ev->dwork, disk_events_workfn);
>   
>   	disk->ev = ev;
> +
> +	return 0;
>   }
>   
> -static void disk_add_events(struct gendisk *disk)
> +static int disk_add_events(struct gendisk *disk)
>   {
> -	/* FIXME: error handling */
> -	if (sysfs_create_files(&disk_to_dev(disk)->kobj, disk_events_attrs) < 0)
> +	int ret;
> +
> +	ret = sysfs_create_files(&disk_to_dev(disk)->kobj, disk_events_attrs);
> +	if (ret < 0) {
>   		pr_warn("%s: failed to create sysfs files for events\n",
>   			disk->disk_name);
> +		return ret;
> +	}
>   
>   	if (!disk->ev)
> -		return;
> +		return 0;
>   
>   	mutex_lock(&disk_events_mutex);
>   	list_add_tail(&disk->ev->node, &disk_events);
> @@ -1860,6 +1918,8 @@ static void disk_add_events(struct gendisk *disk)
>   	 * unblock kicks it into action.
>   	 */
>   	__disk_unblock_events(disk, true);
> +
> +	return 0;
>   }
>   
>   static void disk_del_events(struct gendisk *disk)
> diff --git a/include/linux/genhd.h b/include/linux/genhd.h
> index 7e9660ea967d..5a23d97ccb90 100644
> --- a/include/linux/genhd.h
> +++ b/include/linux/genhd.h
> @@ -205,16 +205,16 @@ static inline dev_t disk_devt(struct gendisk *disk)
>   void disk_uevent(struct gendisk *disk, enum kobject_action action);
>   
>   /* block/genhd.c */
> -extern void device_add_disk(struct device *parent, struct gendisk *disk,
> -			    const struct attribute_group **groups);
> -static inline void add_disk(struct gendisk *disk)
> +extern int device_add_disk(struct device *parent, struct gendisk *disk,
> +			   const struct attribute_group **groups);
> +static inline int add_disk(struct gendisk *disk)
>   {
> -	device_add_disk(NULL, disk, NULL);
> +	return device_add_disk(NULL, disk, NULL);
>   }
> -extern void device_add_disk_no_queue_reg(struct device *parent, struct gendisk *disk);
> -static inline void add_disk_no_queue_reg(struct gendisk *disk)
> +extern int device_add_disk_no_queue_reg(struct device *parent, struct gendisk *disk);
> +static inline int add_disk_no_queue_reg(struct gendisk *disk)
>   {
> -	device_add_disk_no_queue_reg(NULL, disk);
> +	return device_add_disk_no_queue_reg(NULL, disk);
>   }
>   
>   extern void del_gendisk(struct gendisk *gp);
> 

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH v1 6/8] loop: add error handling support for add_disk()

[Hannes Reinecke]

On 5/12/21 8:46 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.
> 
> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> ---
>   drivers/block/loop.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index d58d68f3c7cd..a22d8c985bf3 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -2170,10 +2170,15 @@ static int loop_add(struct loop_device **l, int i)
>   	disk->private_data	= lo;
>   	disk->queue		= lo->lo_queue;
>   	sprintf(disk->disk_name, "loop%d", i);
> -	add_disk(disk);
> +	err = add_disk(disk);
> +	if (err)
> +		goto out_put_disk;
> +
>   	*l = lo;
>   	return lo->lo_number;
>   
> +out_put_disk:
> +	put_disk(lo->lo_disk);
>   out_free_queue:
>   	blk_cleanup_queue(lo->lo_queue);
>   out_cleanup_tags:
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH v1 7/8] null_blk: add error handling support for add_disk()

[Hannes Reinecke]

On 5/12/21 8:46 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.
> 
> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> ---
>   drivers/block/null_blk/main.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
> index 5f006d9e1472..2346d1292b26 100644
> --- a/drivers/block/null_blk/main.c
> +++ b/drivers/block/null_blk/main.c
> @@ -1699,6 +1699,7 @@ static int init_driver_queues(struct nullb *nullb)
>   
>   static int null_gendisk_register(struct nullb *nullb)
>   {
> +	int ret;
>   	sector_t size = ((sector_t)nullb->dev->size * SZ_1M) >> SECTOR_SHIFT;
>   	struct gendisk *disk;
>   
> @@ -1719,13 +1720,17 @@ static int null_gendisk_register(struct nullb *nullb)
>   	strncpy(disk->disk_name, nullb->disk_name, DISK_NAME_LEN);
>   
>   	if (nullb->dev->zoned) {
> -		int ret = null_register_zoned_dev(nullb);
> +		ret = null_register_zoned_dev(nullb);
>   
>   		if (ret)
>   			return ret;
>   	}
>   
> -	add_disk(disk);
> +	ret = add_disk(disk);
> +	if (ret) {

unregister_zoned_device() ?

> +		put_disk(disk);
> +		return ret;
> +	}
>   	return 0;
>   }
>   
> Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH v1 8/8] block: add add_disk() failure injection support

[Hannes Reinecke]

On 5/12/21 8:46 AM, Luis Chamberlain wrote:
> For a long time we have lived without any error handling
> on the add_disk() error path. Now that we have some initial
> error handling, add error injection support for its path so
> that we can test it and ensure we don't regress this path
> moving forward.
> 
> This only adds runtime code *iff* the new bool CONFIG_FAIL_ADD_DISK is
> enabled in your kernel. If you don't have this enabled this provides
> no new functional. When CONFIG_FAIL_ADD_DISK is disabled the new routine
> blk_should_fail_add_disk() ends up being transformed to if (false), and
> so the compiler should optimize these out as dead code producing no
> new effective binary changes.
> 
> Failure injection lets us configure at boot how often we want a failure
> to take place by specifying the interval, the probability, and when needed
> a size constraint. We don't need to test for size constraints for
> add_disk() and so ignore that part of error injection. Although testing
> early boot failures with add_disk() failures might be useful we don't
> to make add_disk() fail every time as otherwise we wouldn't be able to
> boot. So enabling add_disk() error injection requires a second post
> boot step where you specify where in the add_disk() code path you want
> to enable failure injection for. This lets us verify correctness of
> the different error handling parts of add_disk(), while also allowing
> a respective blktests test to grow dynamically in case the add_disk()
> paths grows.
> 
> We currently enable 11 code paths on add_disk() which can fail
> and we can test for:
> 
> 	# ls -1 /sys/kernel/debug/block/config_fail_add_disk/
> 	alloc_devt
> 	alloc_events
> 	bdi_register
> 	device_add
> 	disk_add_events
> 	get_queue
> 	integrity_add
> 	register_disk
> 	register_queue
> 	sysfs_bdi_link
> 	sysfs_depr_link
> 
> If you want to modify the configuration of fail_add_disk dynamically
> at boot, you can enable CONFIG_FAULT_INJECTION_DEBUG_FS. If you've
> enabled CONFIG_FAIL_ADD_DISK you will see these knobs:
> 
> 	# ls -1 /sys/kernel/debug/block/fail_add_disk/
> 	interval
> 	probability
> 	space
> 	task-filter
> 	times
> 	verbose
> 	verbose_ratelimit_burst
> 	verbose_ratelimit_interval_ms
> 
> Suggested-by: Bart Van Assche <bvanassche@acm.org>
> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> ---
>   .../fault-injection/fault-injection.rst       | 23 ++++++++
>   block/Makefile                                |  1 +
>   block/blk-core.c                              |  1 +
>   block/blk.h                                   | 55 ++++++++++++++++++
>   block/failure-injection.c                     | 54 ++++++++++++++++++
>   block/genhd.c                                 | 57 +++++++++++++++++++
>   lib/Kconfig.debug                             | 13 +++++
>   7 files changed, 204 insertions(+)
>   create mode 100644 block/failure-injection.c
> 
[ .. ]
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index d1467658361f..4fccc0fad190 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -1917,6 +1917,19 @@ config FAULT_INJECTION_USERCOPY
>   	  Provides fault-injection capability to inject failures
>   	  in usercopy functions (copy_from_user(), get_user(), ...).
>   
> +config FAIL_ADD_DISK
> +	bool "Fault-injection capability for add_disk() callers"
> +	depends on FAULT_INJECTION && BLOCK
> +	help
> +	  Provide fault-injection capability for the add_disk() block layer
> +	  call path. This allows the kernel to provide error injection when
> +	  the add_disk() call is made. You would use something like blktests
> +	  test against this or just load the null_blk driver. This only
> +	  enables the error injection functionality. To use it you must
> +	  configure which path you want to trigger on error on using debugfs
> +	  under /sys/kernel/debug/block/config_fail_add_disk/. By default
> +	  all of these are disabled.
> +
>   config FAIL_MAKE_REQUEST
>   	bool "Fault-injection capability for disk IO"
>   	depends on FAULT_INJECTION && BLOCK
> 

Hmm. Not a fan of this approach.

Having to have a separate piece of code just to test individual 
functions, _and_ having to place hooks in the code to _simulate_ a 
failure seems rather fragile to me.

I would have vastly preferred if we could to this via generic tools like 
ebpf or livepatching.
Also I'm worried that this approach doesn't really scale; taken to 
extremes we would have to add duplicate calls to each and every function 
for full error injection, essentially double the size of the code just 
on the off-chance that someone wants to do error injection.

So I'd rather delegate the topic of error injection to a more general 
discussion (LSF springs to mind ...), and then agree on a framework 
which is suitable for every function.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH v1 7/8] null_blk: add error handling support for add_disk()

[Luis Chamberlain]

On Wed, May 12, 2021 at 05:16:39PM +0200, Hannes Reinecke wrote:
> On 5/12/21 8:46 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.
> > 
> > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> > ---
> >   drivers/block/null_blk/main.c | 9 +++++++--
> >   1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
> > index 5f006d9e1472..2346d1292b26 100644
> > --- a/drivers/block/null_blk/main.c
> > +++ b/drivers/block/null_blk/main.c
> > @@ -1699,6 +1699,7 @@ static int init_driver_queues(struct nullb *nullb)
> >   static int null_gendisk_register(struct nullb *nullb)
> >   {
> > +	int ret;
> >   	sector_t size = ((sector_t)nullb->dev->size * SZ_1M) >> SECTOR_SHIFT;
> >   	struct gendisk *disk;
> > @@ -1719,13 +1720,17 @@ static int null_gendisk_register(struct nullb *nullb)
> >   	strncpy(disk->disk_name, nullb->disk_name, DISK_NAME_LEN);
> >   	if (nullb->dev->zoned) {
> > -		int ret = null_register_zoned_dev(nullb);
> > +		ret = null_register_zoned_dev(nullb);
> >   		if (ret)
> >   			return ret;
> >   	}
> > -	add_disk(disk);
> > +	ret = add_disk(disk);
> > +	if (ret) {
> 
> unregister_zoned_device() ?

That function does not exist, do you mean null_free_zoned_dev()? If so
that is done by the caller.

  Luis

Re: [PATCH v1 8/8] block: add add_disk() failure injection support

[Luis Chamberlain]

On Wed, May 12, 2021 at 05:22:48PM +0200, Hannes Reinecke wrote:
> On 5/12/21 8:46 AM, Luis Chamberlain wrote:
> > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> > index d1467658361f..4fccc0fad190 100644
> > --- a/lib/Kconfig.debug
> > +++ b/lib/Kconfig.debug
> > @@ -1917,6 +1917,19 @@ config FAULT_INJECTION_USERCOPY
> >   	  Provides fault-injection capability to inject failures
> >   	  in usercopy functions (copy_from_user(), get_user(), ...).
> > +config FAIL_ADD_DISK
> > +	bool "Fault-injection capability for add_disk() callers"
> > +	depends on FAULT_INJECTION && BLOCK
> > +	help
> > +	  Provide fault-injection capability for the add_disk() block layer
> > +	  call path. This allows the kernel to provide error injection when
> > +	  the add_disk() call is made. You would use something like blktests
> > +	  test against this or just load the null_blk driver. This only
> > +	  enables the error injection functionality. To use it you must
> > +	  configure which path you want to trigger on error on using debugfs
> > +	  under /sys/kernel/debug/block/config_fail_add_disk/. By default
> > +	  all of these are disabled.
> > +
> >   config FAIL_MAKE_REQUEST
> >   	bool "Fault-injection capability for disk IO"
> >   	depends on FAULT_INJECTION && BLOCK
> > 
> 
> Hmm. Not a fan of this approach.
> 
> Having to have a separate piece of code just to test individual functions,
> _and_ having to place hooks in the code to _simulate_ a failure seems rather
> fragile to me.
> 
> I would have vastly preferred if we could to this via generic tools like
> ebpf or livepatching.

Agreed. Now, we would then need a place to dump these as well. I guess
blktets would be it for the block layer... and fstests for fs. If done
with livepatching it would take a long time, consider the time added for
probing modules just for a new fault injection for a few routines...
how many modules.. and time.

ebpf maybe. Someone is going to have to try it.

Another possibility is kunit, and I think the tests would be faster.
However maintained boiler place would still be needed.

> Also I'm worried that this approach doesn't really scale; taken to extremes
> we would have to add duplicate calls to each and every function for full
> error injection, essentially double the size of the code just on the
> off-chance that someone wants to do error injection.

Indeed. What would be better is to have the ability to get this for
free and programatically enable knobs. Now fault-injection has some
ability to fail on functions dynamically but I haven't tested that.
Reason I didn't go with that is we want certain functions to fail but
*only* in certain context, not all the time for every caller. This
approach was safer and specific to the block layer, and in fact
only applicable to the add_disk() path.

> So I'd rather delegate the topic of error injection to a more general
> discussion (LSF springs to mind ...), and then agree on a framework which is
> suitable for every function.

Or we just get cranking and produce proof of concepts to compare and
contrast later. At least I hope this patch and the respective blktests
patches suffice to help demo what we need to test.

  Luis

Re: [PATCH v1 7/8] null_blk: add error handling support for add_disk()

[Hannes Reinecke]

On 5/12/21 6:47 PM, Luis Chamberlain wrote:
> On Wed, May 12, 2021 at 05:16:39PM +0200, Hannes Reinecke wrote:
>> On 5/12/21 8:46 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.
>>>
>>> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
>>> ---
>>>    drivers/block/null_blk/main.c | 9 +++++++--
>>>    1 file changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
>>> index 5f006d9e1472..2346d1292b26 100644
>>> --- a/drivers/block/null_blk/main.c
>>> +++ b/drivers/block/null_blk/main.c
>>> @@ -1699,6 +1699,7 @@ static int init_driver_queues(struct nullb *nullb)
>>>    static int null_gendisk_register(struct nullb *nullb)
>>>    {
>>> +	int ret;
>>>    	sector_t size = ((sector_t)nullb->dev->size * SZ_1M) >> SECTOR_SHIFT;
>>>    	struct gendisk *disk;
>>> @@ -1719,13 +1720,17 @@ static int null_gendisk_register(struct nullb *nullb)
>>>    	strncpy(disk->disk_name, nullb->disk_name, DISK_NAME_LEN);
>>>    	if (nullb->dev->zoned) {
>>> -		int ret = null_register_zoned_dev(nullb);
>>> +		ret = null_register_zoned_dev(nullb);
>>>    		if (ret)
>>>    			return ret;
>>>    	}
>>> -	add_disk(disk);
>>> +	ret = add_disk(disk);
>>> +	if (ret) {
>>
>> unregister_zoned_device() ?
> 
> That function does not exist, do you mean null_free_zoned_dev()? If so
> that is done by the caller.
> 
What I intended to say is that you are calling 
'null_register_zoned_dev()' at one point, but don't call a cleanup 
function if there is an error later in the path, leaving the caller to 
guess whether null_register_zoned_dev() has been called or not.
So we should call the cleanup function here, too.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH v1 7/8] null_blk: add error handling support for add_disk()

[Luis Chamberlain]

On Wed, May 12, 2021 at 07:12:03PM +0200, Hannes Reinecke wrote:
> On 5/12/21 6:47 PM, Luis Chamberlain wrote:
> > On Wed, May 12, 2021 at 05:16:39PM +0200, Hannes Reinecke wrote:
> > > On 5/12/21 8:46 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.
> > > > 
> > > > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> > > > ---
> > > >    drivers/block/null_blk/main.c | 9 +++++++--
> > > >    1 file changed, 7 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
> > > > index 5f006d9e1472..2346d1292b26 100644
> > > > --- a/drivers/block/null_blk/main.c
> > > > +++ b/drivers/block/null_blk/main.c
> > > > @@ -1699,6 +1699,7 @@ static int init_driver_queues(struct nullb *nullb)
> > > >    static int null_gendisk_register(struct nullb *nullb)
> > > >    {
> > > > +	int ret;
> > > >    	sector_t size = ((sector_t)nullb->dev->size * SZ_1M) >> SECTOR_SHIFT;
> > > >    	struct gendisk *disk;
> > > > @@ -1719,13 +1720,17 @@ static int null_gendisk_register(struct nullb *nullb)
> > > >    	strncpy(disk->disk_name, nullb->disk_name, DISK_NAME_LEN);
> > > >    	if (nullb->dev->zoned) {
> > > > -		int ret = null_register_zoned_dev(nullb);
> > > > +		ret = null_register_zoned_dev(nullb);
> > > >    		if (ret)
> > > >    			return ret;
> > > >    	}
> > > > -	add_disk(disk);
> > > > +	ret = add_disk(disk);
> > > > +	if (ret) {
> > > 
> > > unregister_zoned_device() ?
> > 
> > That function does not exist, do you mean null_free_zoned_dev()? If so
> > that is done by the caller.
> > 
> What I intended to say is that you are calling 'null_register_zoned_dev()'
> at one point, but don't call a cleanup function if there is an error later
> in the path, leaving the caller to guess whether null_register_zoned_dev()
> has been called or not.
> So we should call the cleanup function here, too.

The cleanup for zone stuff is done on the caller.

  Luis

Re: [PATCH v1 7/8] null_blk: add error handling support for add_disk()

[Hannes Reinecke]

On 5/12/21 7:20 PM, Luis Chamberlain wrote:
> On Wed, May 12, 2021 at 07:12:03PM +0200, Hannes Reinecke wrote:
>> On 5/12/21 6:47 PM, Luis Chamberlain wrote:
>>> On Wed, May 12, 2021 at 05:16:39PM +0200, Hannes Reinecke wrote:
>>>> On 5/12/21 8:46 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.
>>>>>
>>>>> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
>>>>> ---
>>>>>     drivers/block/null_blk/main.c | 9 +++++++--
>>>>>     1 file changed, 7 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
>>>>> index 5f006d9e1472..2346d1292b26 100644
>>>>> --- a/drivers/block/null_blk/main.c
>>>>> +++ b/drivers/block/null_blk/main.c
>>>>> @@ -1699,6 +1699,7 @@ static int init_driver_queues(struct nullb *nullb)
>>>>>     static int null_gendisk_register(struct nullb *nullb)
>>>>>     {
>>>>> +	int ret;
>>>>>     	sector_t size = ((sector_t)nullb->dev->size * SZ_1M) >> SECTOR_SHIFT;
>>>>>     	struct gendisk *disk;
>>>>> @@ -1719,13 +1720,17 @@ static int null_gendisk_register(struct nullb *nullb)
>>>>>     	strncpy(disk->disk_name, nullb->disk_name, DISK_NAME_LEN);
>>>>>     	if (nullb->dev->zoned) {
>>>>> -		int ret = null_register_zoned_dev(nullb);
>>>>> +		ret = null_register_zoned_dev(nullb);
>>>>>     		if (ret)
>>>>>     			return ret;
>>>>>     	}
>>>>> -	add_disk(disk);
>>>>> +	ret = add_disk(disk);
>>>>> +	if (ret) {
>>>>
>>>> unregister_zoned_device() ?
>>>
>>> That function does not exist, do you mean null_free_zoned_dev()? If so
>>> that is done by the caller.
>>>
>> What I intended to say is that you are calling 'null_register_zoned_dev()'
>> at one point, but don't call a cleanup function if there is an error later
>> in the path, leaving the caller to guess whether null_register_zoned_dev()
>> has been called or not.
>> So we should call the cleanup function here, too.
> 
> The cleanup for zone stuff is done on the caller.
> 
My point being: how does he know?
The zone stuff might or might not be initialized.
Why not do the cleanup action in the error path of this function?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH v1 8/8] block: add add_disk() failure injection support

[Hannes Reinecke]

On 5/12/21 6:56 PM, Luis Chamberlain wrote:
> On Wed, May 12, 2021 at 05:22:48PM +0200, Hannes Reinecke wrote:
[ .. ]
>> So I'd rather delegate the topic of error injection to a more general
>> discussion (LSF springs to mind ...), and then agree on a framework which is
>> suitable for every function.
> 
> Or we just get cranking and produce proof of concepts to compare and
> contrast later. At least I hope this patch and the respective blktests
> patches suffice to help demo what we need to test.
> 
Yeah; I know; 'tis a hard topic.
(Will we have another ALPSS this year? Would be ideal to discuss things 
there. Christoph?)

What I would love to see is to facilitate kernel live patching for 
testing, blanking out the function body under test and just return 
specific error codes.
That would be _ideal_ for testing, as we don't have to modify the kernel 
at all, we just need to compile it with the appropriate compiler options 
for live patching.
And then we should be able compile modules for the functions to test, 
load the module, do the test, remove the module, and everything would be 
back to normal again.

I know, but one can dream ...

But maybe I can trick you in trying it ... hmm?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH v1 7/8] null_blk: add error handling support for add_disk()

[Luis Chamberlain]

On Wed, May 12, 2021 at 07:28:16PM +0200, Hannes Reinecke wrote:
> On 5/12/21 7:20 PM, Luis Chamberlain wrote:
> > On Wed, May 12, 2021 at 07:12:03PM +0200, Hannes Reinecke wrote:
> > > On 5/12/21 6:47 PM, Luis Chamberlain wrote:
> > > > On Wed, May 12, 2021 at 05:16:39PM +0200, Hannes Reinecke wrote:
> > > > > On 5/12/21 8:46 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.
> > > > > > 
> > > > > > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> > > > > > ---
> > > > > >     drivers/block/null_blk/main.c | 9 +++++++--
> > > > > >     1 file changed, 7 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
> > > > > > index 5f006d9e1472..2346d1292b26 100644
> > > > > > --- a/drivers/block/null_blk/main.c
> > > > > > +++ b/drivers/block/null_blk/main.c
> > > > > > @@ -1699,6 +1699,7 @@ static int init_driver_queues(struct nullb *nullb)
> > > > > >     static int null_gendisk_register(struct nullb *nullb)
> > > > > >     {
> > > > > > +	int ret;
> > > > > >     	sector_t size = ((sector_t)nullb->dev->size * SZ_1M) >> SECTOR_SHIFT;
> > > > > >     	struct gendisk *disk;
> > > > > > @@ -1719,13 +1720,17 @@ static int null_gendisk_register(struct nullb *nullb)
> > > > > >     	strncpy(disk->disk_name, nullb->disk_name, DISK_NAME_LEN);
> > > > > >     	if (nullb->dev->zoned) {
> > > > > > -		int ret = null_register_zoned_dev(nullb);
> > > > > > +		ret = null_register_zoned_dev(nullb);
> > > > > >     		if (ret)
> > > > > >     			return ret;
> > > > > >     	}
> > > > > > -	add_disk(disk);
> > > > > > +	ret = add_disk(disk);
> > > > > > +	if (ret) {
> > > > > 
> > > > > unregister_zoned_device() ?
> > > > 
> > > > That function does not exist, do you mean null_free_zoned_dev()? If so
> > > > that is done by the caller.
> > > > 
> > > What I intended to say is that you are calling 'null_register_zoned_dev()'
> > > at one point, but don't call a cleanup function if there is an error later
> > > in the path, leaving the caller to guess whether null_register_zoned_dev()
> > > has been called or not.
> > > So we should call the cleanup function here, too.
> > 
> > The cleanup for zone stuff is done on the caller.
> > 
> My point being: how does he know?

The person who is maintainer of the code would.

> The zone stuff might or might not be initialized.
> Why not do the cleanup action in the error path of this function?

Because its not currently allocated in that function, its done in
the caller function.

  Luis