block: first batch of add_disk() error handling conversions

We never checked for errors on add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling.

Reviewed-by Christoph Hellwig <[email protected]>

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

---
  drivers/scsi/sd.c | 6 +++++-
  1 file changed, 5 insertions(+), 1 deletion(-)

We never checked for errors on add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling.

Reviewed-by Christoph Hellwig <[email protected]>

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

---
 drivers/scsi/sd.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

drivers/scsi/sd.c +0 -0

index 610ebba0d66e..8c1273fff23e 100644

Quoted 15 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.

Reviewed-by Christoph Hellwig <[email protected]>

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

---
 drivers/scsi/sd.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

drivers/scsi/sd.c +0 -0

index 610ebba0d66e..8c1273fff23e 100644

drivers/scsi/sd.c static int sd_probe(struct device *dev) · line 3494

Quoted 9 lines

drivers/scsi/sd.c +5 -1

@@ -3487,7 +3487,11 @@ static int sd_probe(struct device *dev)
 		pm_runtime_set_autosuspend_delay(dev,
 			sdp->host->hostt->rpm_autosuspend_delay);
 	}
-	device_add_disk(dev, gd, NULL);
+
+	error = device_add_disk(dev, gd, NULL);
+	if (error)
+		goto out_free_index;
+

The error handling is actually wrong, see

	https://lore.kernel.org/linux-scsi/[email protected]/T/#t

Maybe you can base on that patch.

We never checked for errors on add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling.

Reviewed-by Christoph Hellwig <[email protected]>

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

---
  drivers/scsi/sr.c | 5 ++++-
  1 file changed, 4 insertions(+), 1 deletion(-)

We never checked for errors on add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling.

Reviewed-by Christoph Hellwig <[email protected]>

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

---
 drivers/scsi/sr.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

drivers/scsi/sr.c +0 -0

index 2942a4ec9bdd..72fd21844367 100644

Quoted 10 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.

Reviewed-by Christoph Hellwig <[email protected]>

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

---
 drivers/scsi/sr.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

drivers/scsi/sr.c static int sr_probe(struct device *dev) · line 785

Quoted 13 lines

drivers/scsi/sr.c +4 -1

index 2942a4ec9bdd..72fd21844367 100644
@@ -779,7 +779,10 @@ static int sr_probe(struct device *dev)
 	dev_set_drvdata(dev, cd);
 	disk->flags |= GENHD_FL_REMOVABLE;
 	sr_revalidate_disk(cd);
-	device_add_disk(&sdev->sdev_gendev, disk, NULL);
+
+	error = device_add_disk(&sdev->sdev_gendev, disk, NULL);
+	if (error)
+		goto fail_minor;

You don't undo register_cdrom(), maybe you can use kref_put(&cd->kref, sr_kref_release); to simplify the error handling.

We never checked for errors on add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling.

Reviewed-by Christoph Hellwig <[email protected]>

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

---
  drivers/nvme/host/core.c | 9 ++++++++-
  1 file changed, 8 insertions(+), 1 deletion(-)

drivers/nvme/host/core.c +3 -1

index 8679a108f571..687d3be563a3 100644
@@ -3763,7 +3763,9 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid,
 
  	nvme_get_ctrl(ctrl);
 
-	device_add_disk(ctrl->device, ns->disk, nvme_ns_id_attr_groups);
+	if (device_add_disk(ctrl->device, ns->disk, nvme_ns_id_attr_groups))
+		goto out_cleanup_ns_from_list;
+
  	if (!nvme_ns_head_multipath(ns->head))
  		nvme_add_ns_cdev(ns);
 

@@ -3773,6 +3775,11 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid,
 

I would rather turn this around, and call 'nvme_put_ctrl()' after removing the namespace from the list. But it's probably more a style issue, come to think of it.

We never checked for errors on add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling.

The caller only cleanups the disk if we pass on an allocated md but on error we return return ERR_PTR(ret), and so we must do all the unwinding ourselves.

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

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

drivers/mmc/core/block.c +6 -2

index 6a15fdf6e5f2..9b2856aa6231 100644
@@ -2453,9 +2453,14 @@ static struct mmc_blk_data *mmc_blk_alloc_req(struct mmc_card *card,
        /* used in ->open, must be set before add_disk: */
        if (area_type == MMC_BLK_DATA_AREA_MAIN)
                dev_set_drvdata(&card->dev, md);
-       device_add_disk(md->parent, md->disk, mmc_disk_attr_groups);
+       ret = device_add_disk(md->parent, md->disk, mmc_disk_attr_groups);
+       if (ret)
+               goto err_cleanup_queue;
        return md;
 
+ err_cleanup_queue:
+       blk_cleanup_queue(md->disk->queue);
+       blk_mq_free_tag_set(&md->queue.tag_set);
  err_kfree:
        kfree(md);
  out:
--

2.30.2

We never checked for errors on add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling.

We just do the unwinding of what was not done before, and are sure to unlock prior to bailing.

Reviewed-by Christoph Hellwig <[email protected]>

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

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

We never checked for errors on add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling.

There are two calls to dm_setup_md_queue() which can fail then, one on dm_early_create() and we can easily see that the error path there calls dm_destroy in the error path. The other use case is on the ioctl table_load case. If that fails userspace needs to call the DM_DEV_REMOVE_CMD to cleanup the state - similar to any other failure.

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

---
  drivers/md/dm.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

We never checked for errors on add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling.

Reviewed-by Christoph Hellwig <[email protected]>

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

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

We never checked for errors on add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling.

Reviewed-by Christoph Hellwig <[email protected]>

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

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

Jens,

I think this first set is ready, but pending review of just two patches:

* mmc/core/block
* dm

All other patches have a respective Reviewed-by tag. The above two patches were integrated back into the series once I understood Christoph's concerns, and adjusted the patch as such.

This goes rebased onto your for-next as of today. If anyone wants to explore the pending full set this is up on my linux-next branch 20210830-for-axboe-add-disk-error-handling-next [0].

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

Luis Chamberlain (8):
scsi/sd: add error handling support for add_disk() scsi/sr: add error handling support for add_disk()
nvme: add error handling support for add_disk() mmc/core/block: add error handling support for add_disk()
md: add error handling support for add_disk()
dm: add add_disk() error handling
loop: add error handling support for add_disk()
nbd: add error handling support for add_disk()

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

Are the changes by add_disk() made atomically against the caller? If there is a moment where "struct block_device_operations"->open can be called when add_disk() might still fail, how is it protected from the kfree() path?