block: add error handling for *add_disk*()

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 <[email protected]>

---
  block/genhd.c | 12 ++++++------
  1 file changed, 6 insertions(+), 6 deletions(-)

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 <[email protected]>

---
  block/genhd.c | 14 ++++++++++----
  1 file changed, 10 insertions(+), 4 deletions(-)

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 <[email protected]>

---
  block/genhd.c | 48 ++++++++++++++++++++++++++----------------------
  1 file changed, 26 insertions(+), 22 deletions(-)

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 <[email protected]>

---
  block/genhd.c | 56 +++++++++++++++++++++++++++++----------------------
  1 file changed, 32 insertions(+), 24 deletions(-)

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 <[email protected]>

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

block/blk-integrity.c +8 -4

index 410da060d1f5..e46f47f2dec9 100644
@@ -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)

block/blk-sysfs.c +3 -2

index e03bedf180ab..8a0978a0623c 100644
@@ -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

block/blk.h +3 -2

index 8b3591aee0a5..01ec7aba8d70 100644
@@ -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)
  {

block/genhd.c +32 -16

index baa68192ebb3..eafcd256fc6f 100644
@@ -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;
 

block/genhd.c +6 -3

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

block/genhd.c +30 -9

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

block/genhd.c +21 -12

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

include/linux/genhd.h +4 -4

index 7e9660ea967d..5a23d97ccb90 100644
@@ -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);
  }

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 <[email protected]>

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

drivers/block/loop.c +2 -1

index d58d68f3c7cd..a22d8c985bf3 100644
@@ -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)

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 <[email protected]>

---
  drivers/block/null_blk/main.c | 9 +++++++--
  1 file changed, 7 insertions(+), 2 deletions(-)

drivers/block/null_blk/main.c +1 -0

index 5f006d9e1472..2346d1292b26 100644
@@ -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) {

drivers/block/null_blk/main.c +3 -0

+		put_disk(disk);
+		return ret;
+	}
  	return 0;
  }
 

Cheers,

Quoted 24 lines · 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 <[email protected]>

---
  drivers/block/null_blk/main.c | 9 +++++++--
  1 file changed, 7 insertions(+), 2 deletions(-)

drivers/block/null_blk/main.c +2 -1

index 5f006d9e1472..2346d1292b26 100644
@@ -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);

drivers/block/null_blk/main.c static int null_gendisk_register(struct nullb *nullb) · line 1730

Quoted 6 lines

drivers/block/null_blk/main.c +2 -1

  		if (ret)
  			return ret;
  	}
-	add_disk(disk);
+	ret = add_disk(disk);
+	if (ret) {

unregister_zoned_device() ?

Quoted 25 lines · Hannes Reinecke

Quoted 24 lines · 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 <[email protected]>

---
   drivers/block/null_blk/main.c | 9 +++++++--
   1 file changed, 7 insertions(+), 2 deletions(-)

drivers/block/null_blk/main.c +2 -1

index 5f006d9e1472..2346d1292b26 100644
@@ -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);

Quoted 8 lines

drivers/block/null_blk/main.c static int null_gendisk_register(struct nullb *nullb) · line 1730

Quoted 6 lines

drivers/block/null_blk/main.c +2 -1

   		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.

Quoted 35 lines · Luis Chamberlain

Quoted 33 lines · Hannes Reinecke

Quoted 20 lines · 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 <[email protected]>

---
   drivers/block/null_blk/main.c | 9 +++++++--
   1 file changed, 7 insertions(+), 2 deletions(-)

drivers/block/null_blk/main.c +1 -0

index 5f006d9e1472..2346d1292b26 100644
@@ -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;

drivers/block/null_blk/main.c static int null_gendisk_register(struct nullb *nullb) · line 1727

Quoted 10 lines

drivers/block/null_blk/main.c +3 -2

@@ -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() ?

Quoted 3 lines

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.

Quoted 39 lines · Hannes Reinecke

Quoted 38 lines · Luis Chamberlain

Quoted 25 lines · Hannes Reinecke

Quoted 24 lines · 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 <[email protected]>

---
    drivers/block/null_blk/main.c | 9 +++++++--
    1 file changed, 7 insertions(+), 2 deletions(-)

drivers/block/null_blk/main.c +2 -1

index 5f006d9e1472..2346d1292b26 100644
@@ -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);

Quoted 8 lines

drivers/block/null_blk/main.c static int null_gendisk_register(struct nullb *nullb) · line 1730

Quoted 6 lines

drivers/block/null_blk/main.c +2 -1

    		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.

Quoted 5 lines

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.

Quoted 46 lines · Luis Chamberlain

Quoted 44 lines · Hannes Reinecke

Quoted 35 lines · Luis Chamberlain

Quoted 25 lines · Hannes Reinecke

Quoted 24 lines · 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 <[email protected]>

---
    drivers/block/null_blk/main.c | 9 +++++++--
    1 file changed, 7 insertions(+), 2 deletions(-)

drivers/block/null_blk/main.c +2 -1

index 5f006d9e1472..2346d1292b26 100644
@@ -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);

Quoted 8 lines

drivers/block/null_blk/main.c static int null_gendisk_register(struct nullb *nullb) · line 1730

Quoted 6 lines

drivers/block/null_blk/main.c +2 -1

    		if (ret)
    			return ret;
    	}
-	add_disk(disk);
+	ret = add_disk(disk);
+	if (ret) {

unregister_zoned_device() ?

Quoted 3 lines

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.

Quoted 2 lines

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?

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 <[email protected]>

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

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

lib/Kconfig.debug +6 -0

index d1467658361f..4fccc0fad190 100644
@@ -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

Quoted 14 lines · Luis Chamberlain

lib/Kconfig.debug +7 -0

index d1467658361f..4fccc0fad190 100644
@@ -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

lib/Kconfig.debug config FAULT_INJECTION_USERCOPY · line 1935

Quoted 10 lines

lib/Kconfig.debug +6 -0

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

On Wed, May 12, 2021 at 05:22:48PM +0200, Hannes Reinecke wrote:

Quoted 3 lines

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.