[PATCH v2 0/6] block: add error handling for *add_disk*()

[PATCH v2 0/6] block: add error handling for *add_disk*()

[Luis Chamberlain]

This v2 has the following changes:

  - rebases onto a fresh linux-next which includes Christoph's
    latest cleanups
  - dropping error injection as I inspect ebpf and other
    alternatives, yet I still tested this series with that
    patch and the only change needed was the last one.
  - adds a new patch, the last one,  to adjust to our
    preference now to always wish for users to call a cleanup
    like blk_cleanup_disk() when add_disk() fails.
  - dropped driver conversion

Although I've dropped driver conversion at this point I've
converted all drivers over, but that series is about 80
patches... and so should be dealt with after this basic core
work is reviewed and merged.

Luis Chamberlain (6):
  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
  block: skip queue if NULL on blk_cleanup_queue()

 block/blk-core.c      |   3 +
 block/blk-integrity.c |  12 ++-
 block/blk-sysfs.c     |   5 +-
 block/blk.h           |   7 +-
 block/disk-events.c   |   8 +-
 block/genhd.c         | 229 +++++++++++++++++++++++++++---------------
 include/linux/genhd.h |  14 +--
 7 files changed, 180 insertions(+), 98 deletions(-)

-- 
2.27.0

[PATCH v2 1/6] 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, so that
we can simply bail right away on error. No point in doing any
work on the queue if its invalid. Otherwise we'd also be
unwinding all the work we had done towards the very end.

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

diff --git a/block/genhd.c b/block/genhd.c
index af4d2ab4a633..4dd6db3618f2 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -464,6 +464,15 @@ static void __device_add_disk(struct device *parent, struct gendisk *disk,
 {
 	int ret;
 
+	/*
+	 * Take an extra ref on queue which will be put on disk_release()
+	 * so that it sticks around as long as @disk is there.
+	 */
+	if (blk_get_queue(disk->queue))
+		set_bit(GD_QUEUE_REF, &disk->state);
+	else
+		WARN_ON_ONCE(1);
+
 	/*
 	 * The disk queue should now be all set with enough information about
 	 * the device for the elevator code to pick an adequate default
@@ -528,15 +537,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.
-	 */
-	if (blk_get_queue(disk->queue))
-		set_bit(GD_QUEUE_REF, &disk->state);
-	else
-		WARN_ON_ONCE(1);
-
 	disk_add_events(disk);
 	blk_integrity_add(disk);
 }
-- 
2.27.0

[PATCH v2 2/6] 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 4dd6db3618f2..520e23b25ed5 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -390,6 +390,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)
 {
@@ -433,10 +442,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.27.0

[PATCH v2 3/6] 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 | 47 ++++++++++++++++++++++++++---------------------
 1 file changed, 26 insertions(+), 21 deletions(-)

diff --git a/block/genhd.c b/block/genhd.c
index 520e23b25ed5..e5535879d629 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -399,6 +399,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)
 {
@@ -606,27 +631,7 @@ void del_gendisk(struct gendisk *disk)
 
 	set_capacity(disk, 0);
 
-	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.27.0

[PATCH v2 4/6] 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 partition 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 | 44 ++++++++++++++++++++++++++------------------
 1 file changed, 26 insertions(+), 18 deletions(-)

diff --git a/block/genhd.c b/block/genhd.c
index e5535879d629..b84ba22eed39 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -399,6 +399,25 @@ static void disk_announce(struct gendisk *disk)
 	disk_uevent(disk, KOBJ_ADD);
 }
 
+static void unregister_disk_partitions(struct gendisk *disk)
+{
+	mutex_lock(&disk->open_mutex);
+	disk->flags &= ~GENHD_FL_UP;
+	blk_drop_partitions(disk);
+	mutex_unlock(&disk->open_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);
+}
+
 static void disk_invalidate(struct gendisk *disk)
 {
 	if (!(disk->flags & GENHD_FL_HIDDEN)) {
@@ -424,6 +443,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)
 {
@@ -614,24 +639,7 @@ void del_gendisk(struct gendisk *disk)
 
 	blk_integrity_del(disk);
 	disk_del_events(disk);
-
-	mutex_lock(&disk->open_mutex);
-	disk->flags &= ~GENHD_FL_UP;
-	blk_drop_partitions(disk);
-	mutex_unlock(&disk->open_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_invalidate(disk);
+	unregister_disk(disk);
 }
 EXPORT_SYMBOL(del_gendisk);
 
-- 
2.27.0

[PATCH v2 5/6] 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           |   7 +--
 block/disk-events.c   |   8 +--
 block/genhd.c         | 118 ++++++++++++++++++++++++++++++------------
 include/linux/genhd.h |  14 ++---
 6 files changed, 112 insertions(+), 52 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 370d83c18057..7fd53bf3f8ad 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;
 
 	blk_queue_update_readahead(q);
 
diff --git a/block/blk.h b/block/blk.h
index 4b885c0f6708..e24273ebc4bd 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)
 {
@@ -360,7 +361,7 @@ int bio_add_hw_page(struct request_queue *q, struct bio *bio,
 
 struct request_queue *blk_alloc_queue(int node_id);
 
-void disk_alloc_events(struct gendisk *disk);
+int disk_alloc_events(struct gendisk *disk);
 void disk_add_events(struct gendisk *disk);
 void disk_del_events(struct gendisk *disk);
 void disk_release_events(struct gendisk *disk);
diff --git a/block/disk-events.c b/block/disk-events.c
index a75931ff5da4..0262784be34c 100644
--- a/block/disk-events.c
+++ b/block/disk-events.c
@@ -410,17 +410,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.
  */
-void disk_alloc_events(struct gendisk *disk)
+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);
@@ -432,6 +432,8 @@ void disk_alloc_events(struct gendisk *disk)
 	INIT_DELAYED_WORK(&ev->dwork, disk_events_workfn);
 
 	disk->ev = ev;
+
+	return 0;
 }
 
 void disk_add_events(struct gendisk *disk)
diff --git a/block/genhd.c b/block/genhd.c
index b84ba22eed39..c6c9c196ff27 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -449,8 +449,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;
@@ -466,15 +467,21 @@ 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 does this
+		 * inderectly with put_disk(), or typically more often with
+		 * just blk_cleanup_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;
 	}
 
 	/*
@@ -489,7 +496,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);
@@ -498,8 +505,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;
 }
 
 /**
@@ -512,22 +530,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)
 {
 	int ret;
 
 	/*
 	 * 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_disk() on error from this function.
 	 */
-	if (blk_get_queue(disk->queue))
-		set_bit(GD_QUEUE_REF, &disk->state);
-	else
-		WARN_ON_ONCE(1);
+	if (WARN_ON_ONCE(!blk_get_queue(disk->queue))) {
+		disk->queue = NULL;
+		return -ESHUTDOWN;
+	}
+
+	set_bit(GD_QUEUE_REF, &disk->state);
 
 	/*
 	 * The disk queue should now be all set with enough information about
@@ -546,7 +566,8 @@ static void __device_add_disk(struct device *parent, struct gendisk *disk,
 	 * and all partitions from the extended dev_t space.
 	 */
 	if (disk->major) {
-		WARN_ON(!disk->minors);
+		if (WARN_ON(!disk->minors))
+			return -EINVAL;
 
 		if (disk->minors > DISK_MAX_PARTS) {
 			pr_err("block: can't allocate more than %d partitions\n",
@@ -554,12 +575,13 @@ static void __device_add_disk(struct device *parent, struct gendisk *disk,
 			disk->minors = DISK_MAX_PARTS;
 		}
 	} else {
-		WARN_ON(disk->minors);
+		if (WARN_ON(disk->minors))
+			return -EINVAL;
 
 		ret = blk_alloc_ext_minor();
 		if (ret < 0) {
 			WARN_ON(1);
-			return;
+			return ret;
 		}
 		disk->major = BLOCK_EXT_MAJOR;
 		disk->first_minor = MINOR(ret);
@@ -568,7 +590,9 @@ static void __device_add_disk(struct device *parent, struct gendisk *disk,
 
 	disk->flags |= GENHD_FL_UP;
 
-	disk_alloc_events(disk);
+	ret = disk_alloc_events(disk);
+	if (ret)
+		goto exit_free_ext_minor;
 
 	if (disk->flags & GENHD_FL_HIDDEN) {
 		/*
@@ -585,29 +609,57 @@ static void __device_add_disk(struct device *parent, struct gendisk *disk,
 		dev->devt = MKDEV(disk->major, disk->first_minor);
 		ret = bdi_register(bdi, "%u:%u",
 				   disk->major, disk->first_minor);
-		WARN_ON(ret);
+		if (WARN_ON(ret))
+			goto exit_disk_release_events;
 		bdi_set_owner(bdi, dev);
 		bdev_add(disk->part0, dev->devt);
 	}
-	register_disk(parent, disk, groups);
-	if (register_queue)
-		blk_register_queue(disk);
+	ret = register_disk(parent, disk, groups);
+	if (ret)
+		goto exit_unregister_bdi;
+
+	if (register_queue) {
+		ret = blk_register_queue(disk);
+		if (ret)
+			goto exit_unregister_disk;
+	}
 
 	disk_add_events(disk);
-	blk_integrity_add(disk);
+
+	ret = blk_integrity_add(disk);
+	if (ret)
+		goto exit_del_events;
+
+	return 0;
+exit_del_events:
+	disk_del_events(disk);
+exit_unregister_disk:
+	unregister_disk(disk);
+	return ret;
+
+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_free_ext_minor:
+	if (disk->major == BLOCK_EXT_MAJOR)
+		blk_free_ext_minor(disk->first_minor);
+
+	return ret;
 }
 
-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);
 
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index 13b34177cc85..51f27b9b38b5 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -210,16 +210,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.27.0

[PATCH v2 6/6] block: skip queue if NULL on blk_cleanup_queue()

[Luis Chamberlain]

Now that error handling for add_disk*() calls is added, we must
accept a common form for when errors are detected on the the
add_disk*() calls, and that is to call blk_cleanup_disk() on
error always. One of the corner cases possible is a driver bug
where the queue is already gone and we cannot blk_get_queue(),
and so may be NULL. When blk_cleanup_disk() is called in this
case blk_cleanup_queue() will crash with a null dereference.

Make this an accepted condition and just skip it. This allows us
to also test for it safely with error injection.

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

diff --git a/block/blk-core.c b/block/blk-core.c
index 04477697ee4b..156f7d3b4bd9 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -372,6 +372,9 @@ void blk_cleanup_queue(struct request_queue *q)
 	/* cannot be called from atomic context */
 	might_sleep();
 
+	if (!q)
+		return;
+
 	WARN_ON_ONCE(blk_queue_registered(q));
 
 	/* mark @q DYING, no new request or merges will be allowed afterwards */
-- 
2.27.0

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

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

[Christoph Hellwig]

On Wed, Jul 14, 2021 at 09:55:27PM -0700, 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.

On it's own I don't really see the point of moving two function
calls and a comment into a separate helper, but I'll see where this
series goes..

Re: [PATCH v2 6/6] block: skip queue if NULL on blk_cleanup_queue()

[Christoph Hellwig]

On Wed, Jul 14, 2021 at 09:55:31PM -0700, Luis Chamberlain wrote:
> Now that error handling for add_disk*() calls is added, we must
> accept a common form for when errors are detected on the the
> add_disk*() calls, and that is to call blk_cleanup_disk() on
> error always. One of the corner cases possible is a driver bug
> where the queue is already gone and we cannot blk_get_queue(),
> and so may be NULL. When blk_cleanup_disk() is called in this
> case blk_cleanup_queue() will crash with a null dereference.
> 
> Make this an accepted condition and just skip it. This allows us
> to also test for it safely with error injection.

So you plan to call blk_cleanup_disk when add_disk fails?

For all drivers using blk_alloc_disk/blk_mq_alloc_disk there should
always be a queue.  The others ones aren't ready to handle errors
from add_disk yet in any way I think (and I plan to fix this up
ASAP).

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

[Christoph Hellwig]

On Thu, Jul 15, 2021 at 08:05:41AM +0100, Christoph Hellwig wrote:
> On Wed, Jul 14, 2021 at 09:55:27PM -0700, 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.
> 
> On it's own I don't really see the point of moving two function
> calls and a comment into a separate helper, but I'll see where this
> series goes..

Looking at the whole context I still can't see a good reason for
this helper.

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

[Christoph Hellwig]

On Wed, Jul 14, 2021 at 09:55:25PM -0700, Luis Chamberlain wrote:
> Although I've dropped driver conversion at this point I've
> converted all drivers over, but that series is about 80
> patches... and so should be dealt with after this basic core
> work is reviewed and merged.

I think we need at least a few sample conversions to show how
you intend this to be used.

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

[Christoph Hellwig]

I don't really like many of these helpers as they are rather
confusing.  Below is a patch ontop of your whole series that
massages it into something I find much easier to follow.
The big caveats are:

 - this moves the bdi link creation earlier, which is safe and probably
   preferable. (should go into a prep patch).
 - del_gendisk now needs to cope with not set up events and integrity
   kobject

I also have a bunch of changes to del_gendisk which might require
backporting, let me try to get those out ASAP.

diff --git a/block/blk-integrity.c b/block/blk-integrity.c
index e46f47f2dec9..3135adab2e7c 100644
--- a/block/blk-integrity.c
+++ b/block/blk-integrity.c
@@ -446,6 +446,8 @@ int blk_integrity_add(struct gendisk *disk)
 
 void blk_integrity_del(struct gendisk *disk)
 {
+	if (!disk->integrity_kobj.state_initialized)
+		return;
 	kobject_uevent(&disk->integrity_kobj, KOBJ_REMOVE);
 	kobject_del(&disk->integrity_kobj);
 	kobject_put(&disk->integrity_kobj);
diff --git a/block/disk-events.c b/block/disk-events.c
index 0262784be34c..af7d7249eefa 100644
--- a/block/disk-events.c
+++ b/block/disk-events.c
@@ -458,7 +458,8 @@ void disk_del_events(struct gendisk *disk)
 		disk_block_events(disk);
 
 		mutex_lock(&disk_events_mutex);
-		list_del_init(&disk->ev->node);
+		if (!list_empty(&disk->ev->node))
+			list_del_init(&disk->ev->node);
 		mutex_unlock(&disk_events_mutex);
 	}
 }
diff --git a/block/genhd.c b/block/genhd.c
index c6c9c196ff27..0c8c71d78536 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -390,68 +390,8 @@ 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 unregister_disk_partitions(struct gendisk *disk)
-{
-	mutex_lock(&disk->open_mutex);
-	disk->flags &= ~GENHD_FL_UP;
-	blk_drop_partitions(disk);
-	mutex_unlock(&disk->open_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);
-}
-
-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 unregister_disk(struct gendisk *disk)
-{
-	unregister_disk_partitions(disk);
-	disk_invalidate(disk);
-}
-
-static int __must_check register_disk(struct device *parent,
-				      struct gendisk *disk,
-				      const struct attribute_group **groups)
+static int register_disk(struct device *parent, struct gendisk *disk,
+		const struct attribute_group **groups)
 {
 	struct device *ddev = disk_to_dev(disk);
 	int err;
@@ -498,20 +438,22 @@ static int __must_check register_disk(struct device *parent,
 	if (disk->flags & GENHD_FL_HIDDEN)
 		return 0;
 
-	disk_scan_partitions(disk);
-	disk_announce(disk);
-
 	if (disk->queue->backing_dev_info->dev) {
 		err = sysfs_create_link(&ddev->kobj,
-			  &disk->queue->backing_dev_info->dev->kobj,
-			  "bdi");
+			  &disk->queue->backing_dev_info->dev->kobj, "bdi");
 		if (WARN_ON(err))
 			goto exit_del_block_depr;
 	}
+
+	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);
+
 	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:
@@ -614,6 +556,7 @@ static int __device_add_disk(struct device *parent, struct gendisk *disk,
 		bdi_set_owner(bdi, dev);
 		bdev_add(disk->part0, dev->devt);
 	}
+
 	ret = register_disk(parent, disk, groups);
 	if (ret)
 		goto exit_unregister_bdi;
@@ -628,13 +571,11 @@ static int __device_add_disk(struct device *parent, struct gendisk *disk,
 
 	ret = blk_integrity_add(disk);
 	if (ret)
-		goto exit_del_events;
+		goto exit_unregister_disk;
 
 	return 0;
-exit_del_events:
-	disk_del_events(disk);
 exit_unregister_disk:
-	unregister_disk(disk);
+	del_gendisk(disk);
 	return ret;
 
 exit_unregister_bdi:
@@ -691,7 +632,44 @@ void del_gendisk(struct gendisk *disk)
 
 	blk_integrity_del(disk);
 	disk_del_events(disk);
-	unregister_disk(disk);
+
+	mutex_lock(&disk->open_mutex);
+	disk->flags &= ~GENHD_FL_UP;
+	blk_drop_partitions(disk);
+	mutex_unlock(&disk->open_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);
+
+	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));
 }
 EXPORT_SYMBOL(del_gendisk);
 

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

[Christoph Hellwig]

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

Nit: please drop the pointless externs and avoid > 80 character lines.

Re: [PATCH v2 6/6] block: skip queue if NULL on blk_cleanup_queue()

[Luis Chamberlain]

On Thu, Jul 15, 2021 at 08:11:57AM +0100, Christoph Hellwig wrote:
> On Wed, Jul 14, 2021 at 09:55:31PM -0700, Luis Chamberlain wrote:
> > Now that error handling for add_disk*() calls is added, we must
> > accept a common form for when errors are detected on the the
> > add_disk*() calls, and that is to call blk_cleanup_disk() on
> > error always. One of the corner cases possible is a driver bug
> > where the queue is already gone and we cannot blk_get_queue(),
> > and so may be NULL. When blk_cleanup_disk() is called in this
> > case blk_cleanup_queue() will crash with a null dereference.
> > 
> > Make this an accepted condition and just skip it. This allows us
> > to also test for it safely with error injection.
> 
> So you plan to call blk_cleanup_disk when add_disk fails?

Yes, they can open code things if they wish as well, but when possible yes.

> For all drivers using blk_alloc_disk/blk_mq_alloc_disk there should
> always be a queue.  The others ones aren't ready to handle errors
> from add_disk yet in any way I think (and I plan to fix this up
> ASAP).

Have an example in mind?

  Luis

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

[Luis Chamberlain]

On Thu, Jul 15, 2021 at 08:23:00AM +0100, Christoph Hellwig wrote:
> On Wed, Jul 14, 2021 at 09:55:25PM -0700, Luis Chamberlain wrote:
> > Although I've dropped driver conversion at this point I've
> > converted all drivers over, but that series is about 80
> > patches... and so should be dealt with after this basic core
> > work is reviewed and merged.
> 
> I think we need at least a few sample conversions to show how
> you intend this to be used.

I'll send a few out, but I'll send them as separate groups. There
are 5 groups of further changes as part of the driver conversion:

  1) trivial changes: requires a blk_cleanup_disk() on error added
  2) Fix uses of GENHD_FL_UP
  3) Fix uses of GENHD_FL_UP for del_gendisk() with a block helper
  4) make probe on blk_request_module() return an error, so to
     take advantage of the add_disk() on probe calls. Drivers which
     benefit:
     - ataflop
     - brd
     - floppy
     - loop
     - scsi/sd
  5) Once all drivers are converted, add __must_check() to add_disk()
     and friends

There are so many patches I think it makes sense to only post a few
for each group, except maybe the 4th group, that requires probe
change to go in one full sweep. I've only build tested what fits
in my architecture so far, waiting on 0-day complaints reports to
fix the rest. So only the first first group will go as PATCH form
for now.

  Luis

Re: [PATCH v2 6/6] block: skip queue if NULL on blk_cleanup_queue()

[Christoph Hellwig]

On Thu, Jul 15, 2021 at 12:07:26PM -0700, Luis Chamberlain wrote:
> > For all drivers using blk_alloc_disk/blk_mq_alloc_disk there should
> > always be a queue.  The others ones aren't ready to handle errors
> > from add_disk yet in any way I think (and I plan to fix this up
> > ASAP).
> 
> Have an example in mind?

The only ones left are nvme, dasd and scsi.  NVMe is trivial (attached),
dasd needs a little more work, I need to send up a WIP to the
maintainers.  scsi is the real problem and will require a fair amount
of work.

---
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 11779be42186..35da956acef6 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3726,9 +3726,14 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid,
 	if (!ns)
 		goto out_free_id;
 
-	ns->queue = blk_mq_init_queue(ctrl->tagset);
-	if (IS_ERR(ns->queue))
+	disk = blk_mq_alloc_disk(ctrl->tagset, ns);
+	if (IS_ERR(disk))
 		goto out_free_ns;
+	disk->fops = &nvme_bdev_ops;
+	disk->private_data = ns;
+
+	ns->disk = disk;
+	ns->queue = disk->queue;
 
 	if (ctrl->opts && ctrl->opts->data_digest)
 		blk_queue_flag_set(QUEUE_FLAG_STABLE_WRITES, ns->queue);
@@ -3737,20 +3742,12 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid,
 	if (ctrl->ops->flags & NVME_F_PCI_P2PDMA)
 		blk_queue_flag_set(QUEUE_FLAG_PCI_P2PDMA, ns->queue);
 
-	ns->queue->queuedata = ns;
 	ns->ctrl = ctrl;
 	kref_init(&ns->kref);
 
 	if (nvme_init_ns_head(ns, nsid, ids, id->nmic & NVME_NS_NMIC_SHARED))
-		goto out_free_queue;
-
-	disk = alloc_disk_node(0, node);
-	if (!disk)
-		goto out_unlink_ns;
+		goto out_cleanup_disk;
 
-	disk->fops = &nvme_bdev_ops;
-	disk->private_data = ns;
-	disk->queue = ns->queue;
 	/*
 	 * Without the multipath code enabled, multiple controller per
 	 * subsystems are visible as devices and thus we cannot use the
@@ -3759,15 +3756,14 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid,
 	if (!nvme_mpath_set_disk_name(ns, disk->disk_name, &disk->flags))
 		sprintf(disk->disk_name, "nvme%dn%d", ctrl->instance,
 			ns->head->instance);
-	ns->disk = disk;
 
 	if (nvme_update_ns_info(ns, id))
-		goto out_put_disk;
+		goto out_unlink_ns;
 
 	if ((ctrl->quirks & NVME_QUIRK_LIGHTNVM) && id->vs[0] == 0x1) {
 		if (nvme_nvm_register(ns, disk->disk_name, node)) {
 			dev_warn(ctrl->device, "LightNVM init failure\n");
-			goto out_put_disk;
+			goto out_unlink_ns;
 		}
 	}
 
@@ -3786,10 +3782,7 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid,
 	kfree(id);
 
 	return;
- out_put_disk:
-	/* prevent double queue cleanup */
-	ns->disk->queue = NULL;
-	put_disk(ns->disk);
+
  out_unlink_ns:
 	mutex_lock(&ctrl->subsys->lock);
 	list_del_rcu(&ns->siblings);
@@ -3797,8 +3790,8 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid,
 		list_del_init(&ns->head->entry);
 	mutex_unlock(&ctrl->subsys->lock);
 	nvme_put_ns_head(ns->head);
- out_free_queue:
-	blk_cleanup_queue(ns->queue);
+ out_cleanup_disk:
+	blk_cleanup_disk(disk);
  out_free_ns:
 	kfree(ns);
  out_free_id:

Re: [PATCH v2 6/6] block: skip queue if NULL on blk_cleanup_queue()

[Luis Chamberlain]

On Mon, Jul 19, 2021 at 10:50:13AM +0100, Christoph Hellwig wrote:
> On Thu, Jul 15, 2021 at 12:07:26PM -0700, Luis Chamberlain wrote:
> > > For all drivers using blk_alloc_disk/blk_mq_alloc_disk there should
> > > always be a queue.  The others ones aren't ready to handle errors
> > > from add_disk yet in any way I think (and I plan to fix this up
> > > ASAP).
> > 
> > Have an example in mind?
> 
> The only ones left are nvme, dasd and scsi.  NVMe is trivial (attached),
> dasd needs a little more work, I need to send up a WIP to the
> maintainers.  scsi is the real problem and will require a fair amount
> of work.

Alright, I'll wait until these changes are merged before sending those
driver's conversions. Without this patch though, there's no way to test
the error injection in case the driver was buggy and removed the queue
by mistake. Recall my first patch moved that check to the beginning too.

So we either skip that error injection test ... or not sure what else to do.

 Luis