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
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(-)
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
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(-)
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
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(-)
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
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.
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 <[email protected]>
> ---
> block/genhd.c | 14 ++++++++++----
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
Reviewed-by: Hannes Reinecke <[email protected]>
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
[email protected] +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
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 <[email protected]>
> ---
> block/genhd.c | 48 ++++++++++++++++++++++++++----------------------
> 1 file changed, 26 insertions(+), 22 deletions(-)
>
Reviewed-by: Hannes Reinecke <[email protected]>
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
[email protected] +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
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 <[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(-)
>
> 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
[email protected] +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer