block: add error handling for *add_disk*()
Subsystems (2)
v2 cover block: add error handling for *add_disk*()
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(-)
Signature · 1 line
2.27.0
PATCH v2 1/6 block: refcount the request_queue early in __device_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, 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.
--- block/genhd.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)
block/genhd.c +9 -9
@@ -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); }
Signature · 1 line
2.27.0
Re: [PATCH v2 1/6] block: refcount the request_queue early in __device_add_disk()
Looks good,
PATCH v2 2/6 block: move disk announce work from register_disk() to a helper
In reply to Luis ChamberlainThis 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.
--- block/genhd.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
block/genhd.c +10 -4
@@ -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,
Signature · 1 line
2.27.0
Re: [PATCH v2 2/6] block: move disk announce work from register_disk() to a helper
Quoted 7 lines · 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.
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 2/6] block: move disk announce work from register_disk() to a helper
Quoted 5 lines · Christoph Hellwig
Quoted 4 lines · 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.
Quoted 7 lines
Quoted 3 lines
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.
PATCH v2 3/6 block: move disk invalidation from del_gendisk() into a helper
In reply to Luis ChamberlainMove 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.
--- block/genhd.c | 47 ++++++++++++++++++++++++++--------------------- 1 file changed, 26 insertions(+), 21 deletions(-)
block/genhd.c +26 -21
@@ -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);
Signature · 1 line
2.27.0
Re: [PATCH v2 3/6] block: move disk invalidation from del_gendisk() into a helper
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.
block/blk-integrity.c +2 -0
@@ -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);
block/disk-events.c +2 -1
@@ -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); } }
block/genhd.c +51 -73
165 lines not on this page · raw message
PATCH v2 4/6 block: move disk unregistration work from del_gendisk() to a helper
In reply to Luis ChamberlainThere 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().
--- block/genhd.c | 44 ++++++++++++++++++++++++++------------------ 1 file changed, 26 insertions(+), 18 deletions(-)
block/genhd.c +26 -18
@@ -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);
Signature · 1 line
2.27.0
PATCH v2 5/6 block: add initial error handling for *add_disk()* and friends
In reply to Luis ChamberlainThis 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.
--- 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(-)
block/blk-integrity.c +8 -4
@@ -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
@@ -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);
block/blk.h +4 -3
@@ -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);
block/disk-events.c +5 -3
@@ -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)
block/genhd.c +85 -33
200 lines not on this page · raw message
include/linux/genhd.h +7 -7
@@ -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);
Signature · 1 line
2.27.0
Re: [PATCH v2 5/6] block: add initial error handling for *add_disk()* and friends
include/linux/genhd.h static inline dev_t disk_devt(struct gendisk *disk) · line 219
Quoted 11 lines
include/linux/genhd.h +5
-4-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.
PATCH v2 6/6 block: skip queue if NULL on blk_cleanup_queue()
In reply to Luis ChamberlainNow 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.
--- block/blk-core.c | 3 +++ 1 file changed, 3 insertions(+)
block/blk-core.c +3 -0
@@ -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 */
Signature · 1 line
2.27.0
Re: [PATCH v2 6/6] block: skip queue if NULL on blk_cleanup_queue()
Quoted 10 lines · 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 cannotblk_get_queue(), and so may be NULL. Whenblk_cleanup_disk()is called in this caseblk_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 6/6] block: skip queue if NULL on blk_cleanup_queue()
Quoted 9 lines · Christoph Hellwig
Quoted 8 lines · 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 cannotblk_get_queue(), and so may be NULL. Whenblk_cleanup_disk()is called in this caseblk_cleanup_queue()will crash with a null dereference.
Quoted 4 lines
Quoted 2 lines
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_diskwhenadd_diskfails?
Yes, they can open code things if they wish as well, but when possible yes.
Quoted 4 lines
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_diskyet in any way I think (and I plan to fix this up ASAP).
Have an example in mind?
Luis
Re: [PATCH v2 6/6] block: skip queue if NULL on blk_cleanup_queue()
Quoted 6 lines · Luis Chamberlain
Quoted 4 lines
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_diskyet 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.
---
drivers/nvme/host/core.c +13 -20
@@ -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()
Quoted 6 lines · Christoph Hellwig
Quoted 5 lines · Luis Chamberlain
Quoted 4 lines
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_diskyet in any way I think (and I plan to fix this up ASAP).
Quoted 6 lines
Quoted 1 line
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
Quoted 4 lines · Luis Chamberlain
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.
Quoted 8 lines · Christoph Hellwig
Quoted 4 lines · Luis Chamberlain
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