block/dm: allow DM to defer blk_register_queue() until ready
Subsystems (2)
v4 cover block/dm: allow DM to defer blk_register_queue() until ready
Hi Jens,
I think this set is now ready for you to pick up for 4.16.
Hannes, I ran with your review feedback and think the result is cleaner (avoids frivolously using another QUEUE_FLAG).
Ming, I didn't add your Reviewed-by to "block: allow gendisk's request_queue registration to be deferred" because it changed enough, albeit for the better, that it is best for you to review again.
Thanks,
Mike
Mike Snitzer (4):
block: only bdi_unregister() in del_gendisk() if !GENHD_FL_HIDDEN
block: use queue_lock when clearing QUEUE_FLAG_REGISTERED in blk_unregister_queue
block: allow gendisk's request_queue registration to be deferred
dm: fix awkward and incomplete request_queue initialization
block/blk-sysfs.c | 12 ++++++++---- block/genhd.c | 23 +++++++++++++++++++---- drivers/md/dm-core.h | 2 -- drivers/md/dm-rq.c | 11 ----------- drivers/md/dm.c | 41 ++++++++++++++++++++++------------------- include/linux/genhd.h | 5 +++++ 6 files changed, 54 insertions(+), 40 deletions(-)
Signature · 1 line
2.15.0
PATCH v4 1/4 block: only bdi_unregister() in del_gendisk() if !GENHD_FL_HIDDEN
device_add_disk() will only call bdi_register_owner() if !GENHD_FL_HIDDEN, so it follows that del_gendisk() should only call bdi_unregister() if !GENHD_FL_HIDDEN.
Found with code inspection. bdi_unregister() won't do any harm if bdi_register_owner() wasn't used but best to avoid the unnecessary call to bdi_unregister().
--- block/genhd.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
block/genhd.c +2 -1
@@ -725,7 +725,8 @@ void del_gendisk(struct gendisk *disk)* 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); +if (!(disk->flags &GENHD_FL_HIDDEN )) +bdi_unregister (disk->queue->backing_dev_info);blk_unregister_queue (disk); }else {WARN_ON (1 );
Signature · 1 line
2.15.0
PATCH v4 2/4 block: use queue_lock when clearing QUEUE_FLAG_REGISTERED in blk_unregister_queue
In reply to Mike Snitzerblk_unregister_queue() must protect against any modifications of q->queue_flags (not just those performed in blk-sysfs.c). Therefore q->queue_lock needs to be used rather than q->sysfs_lock.
--- block/blk-sysfs.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
block/blk-sysfs.c +2 -2
@@ -929,9 +929,9 @@ void blk_unregister_queue(struct gendisk *disk)if (WARN_ON (!q))return ; -mutex_lock (&q->sysfs_lock); +spin_lock_irq (q->queue_lock);queue_flag_clear_unlocked (QUEUE_FLAG_REGISTERED , q); -mutex_unlock (&q->sysfs_lock); +spin_unlock_irq (q->queue_lock);wbt_exit (q);
Signature · 1 line
2.15.0
Re: [for-4.16 PATCH v4 2/4] block: use queue_lock when clearing QUEUE_FLAG_REGISTERED in blk_unregister_queue
Quoted 17 lines · Mike Snitzer
blk_unregister_queue()must protect against any modifications ofq->queue_flags(not just those performed inblk-sysfs.c). Thereforeq->queue_lockneeds to be used rather thanq->sysfs_lock.Fixes e9a823fb34a8b ("block: fix warning when I/O elevator is changed as request_queue is being removed")Cc [email protected] # 4.14+Reported-by Bart Van Assche <[email protected]>Signed-off-by Mike Snitzer <[email protected]>--- block/blk-sysfs.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)block/blk-sysfs.c +0
-0
block/blk-sysfs.c void blk_unregister_queue(struct gendisk *disk) · line 934
Quoted 8 lines
block/blk-sysfs.c +2
-2@@ -929,9 +929,9 @@ void blk_unregister_queue(struct gendisk *disk)if (WARN_ON (!q))return ; -mutex_lock (&q->sysfs_lock); +spin_lock_irq (q->queue_lock);queue_flag_clear_unlocked (QUEUE_FLAG_REGISTERED , q); -mutex_unlock (&q->sysfs_lock); +spin_unlock_irq (q->queue_lock);
Hello Mike,
The function name queue_flag_clear_unlocked() means "clear a queue flag without holding the queue lock". So at least to me the above code is confusing. Please consider to change queue_flag_clear_unlocked() into queue_flag_clear().
Thanks,
Bart.
Re: [for-4.16 PATCH v4 2/4] block: use queue_lock when clearing QUEUE_FLAG_REGISTERED in blk_unregister_queue
Quoted 29 lines · Bart Van Assche
Quoted 17 lines · Mike Snitzer
blk_unregister_queue()must protect against any modifications ofq->queue_flags(not just those performed inblk-sysfs.c). Thereforeq->queue_lockneeds to be used rather thanq->sysfs_lock.Fixes e9a823fb34a8b ("block: fix warning when I/O elevator is changed as request_queue is being removed")Cc [email protected] # 4.14+Reported-by Bart Van Assche <[email protected]>Signed-off-by Mike Snitzer <[email protected]>--- block/blk-sysfs.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)block/blk-sysfs.c +0
-0block/blk-sysfs.c void blk_unregister_queue(struct gendisk *disk) · line 934
Quoted 8 lines
block/blk-sysfs.c +2
-2@@ -929,9 +929,9 @@ void blk_unregister_queue(struct gendisk *disk)if (WARN_ON (!q))return ; -mutex_lock (&q->sysfs_lock); +spin_lock_irq (q->queue_lock);queue_flag_clear_unlocked (QUEUE_FLAG_REGISTERED , q); -mutex_unlock (&q->sysfs_lock); +spin_unlock_irq (q->queue_lock);Hello Mike,
Quoted 3 lines
The function name
queue_flag_clear_unlocked()means "clear a queue flag without holding the queue lock". So at least to me the above code is confusing. Please consider to changequeue_flag_clear_unlocked()intoqueue_flag_clear().
If Jens would like to change it when applying the patch to his tree that is fine by me.
But as you know, it doesn't matter:queue_flag_clear() just has extra queue_lockdep_assert_held(q);
So I see no reason to respin this patch for this. Especially when you consider patch 3 replaces it with queue_flag_test_and_clear() -- and no it isn't a problem for stable@ to carry on using queue_flag_clear_unlocked
Mike
Re: [for-4.16 PATCH v4 2/4] block: use queue_lock when clearing QUEUE_FLAG_REGISTERED in blk_unregister_queue
In reply to Mike SnitzerQuoted 17 lines · Mike Snitzer
blk_unregister_queue()must protect against any modifications ofq->queue_flags(not just those performed inblk-sysfs.c). Thereforeq->queue_lockneeds to be used rather thanq->sysfs_lock.Fixes e9a823fb34a8b ("block: fix warning when I/O elevator is changed as request_queue is being removed")Cc [email protected] # 4.14+Reported-by Bart Van Assche <[email protected]>Signed-off-by Mike Snitzer <[email protected]>--- block/blk-sysfs.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)block/blk-sysfs.c +0
-0
block/blk-sysfs.c void blk_unregister_queue(struct gendisk *disk) · line 936
Quoted 10 lines
block/blk-sysfs.c +2
-2@@ -929,9 +929,9 @@ void blk_unregister_queue(struct gendisk *disk)if (WARN_ON (!q))return ; -mutex_lock (&q->sysfs_lock); +spin_lock_irq (q->queue_lock);queue_flag_clear_unlocked (QUEUE_FLAG_REGISTERED , q); -mutex_unlock (&q->sysfs_lock); +spin_unlock_irq (q->queue_lock);wbt_exit (q);
Hi Mike,
This change may not be correct, since at least e9a823fb34a8b depends on q->sysfs_lock to sync between testing the flag in __elevator_change() and clearing it here.
Thanks,
Ming
Re: [for-4.16 PATCH v4 2/4] block: use queue_lock when clearing QUEUE_FLAG_REGISTERED in blk_unregister_queue
Quoted 27 lines · Ming Lei
Quoted 26 lines · Mike Snitzer
blk_unregister_queue()must protect against any modifications ofq->queue_flags(not just those performed inblk-sysfs.c). Thereforeq->queue_lockneeds to be used rather thanq->sysfs_lock.Fixes e9a823fb34a8b ("block: fix warning when I/O elevator is changed as request_queue is being removed")Cc [email protected] # 4.14+Reported-by Bart Van Assche <[email protected]>Signed-off-by Mike Snitzer <[email protected]>--- block/blk-sysfs.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)block/blk-sysfs.c +2
-2@@ -929,9 +929,9 @@ void blk_unregister_queue(struct gendisk *disk)if (WARN_ON (!q))return ; -mutex_lock (&q->sysfs_lock); +spin_lock_irq (q->queue_lock);queue_flag_clear_unlocked (QUEUE_FLAG_REGISTERED , q); -mutex_unlock (&q->sysfs_lock); +spin_unlock_irq (q->queue_lock);
Quoted 7 lines
block/blk-sysfs.c void blk_unregister_queue(struct gendisk *disk) · line 936
Quoted 1 line
block/blk-sysfs.c +0
-0wbt_exit (q);Hi Mike,
This change may not be correct, since at least
e9a823fb34a8bdepends onq->sysfs_lockto sync between testing the flag in__elevator_change()and clearing it here.
The header for commit e9a823fb34a8b says:
To fix this warning, we can check the QUEUE_FLAG_REGISTERED flag when changing the elevator and use the request_queue's sysfs_lock to serialize between clearing the flag and the elevator testing the flag.
The reality is sysfs_lock isn't needed to serialize between blk_unregister_queue() clearing the flag and __elevator_change() testing the flag.
The original commit e9a823fb34a8b is pretty conflated. "conflated" because the resource being protected isn't the queue_flags (it is the 'queue' kobj).
I'll respin v5 of this patchset to fix this up first, and then apply the changes I _really_ need to land (DM queue initialization fix).
And then I'm going to slowly step away from block core and _not_ allow myself to be tripped up, or baited, by historic block core issues for a while... ;)
Thanks,
Mike
Re: [for-4.16 PATCH v4 2/4] block: use queue_lock when clearing QUEUE_FLAG_REGISTERED in blk_unregister_queue
Quoted 38 lines · Mike Snitzer
Quoted 27 lines · Ming Lei
Quoted 26 lines · Mike Snitzer
blk_unregister_queue()must protect against any modifications ofq->queue_flags(not just those performed inblk-sysfs.c). Thereforeq->queue_lockneeds to be used rather thanq->sysfs_lock.Fixes e9a823fb34a8b ("block: fix warning when I/O elevator is changed as request_queue is being removed")Cc [email protected] # 4.14+Reported-by Bart Van Assche <[email protected]>Signed-off-by Mike Snitzer <[email protected]>--- block/blk-sysfs.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)block/blk-sysfs.c +2
-2@@ -929,9 +929,9 @@ void blk_unregister_queue(struct gendisk *disk)if (WARN_ON (!q))return ; -mutex_lock (&q->sysfs_lock); +spin_lock_irq (q->queue_lock);queue_flag_clear_unlocked (QUEUE_FLAG_REGISTERED , q); -mutex_unlock (&q->sysfs_lock); +spin_unlock_irq (q->queue_lock);Quoted 7 lines
block/blk-sysfs.c void blk_unregister_queue(struct gendisk *disk) · line 936
Quoted 1 line
block/blk-sysfs.c +0
-0wbt_exit (q);Hi Mike,
This change may not be correct, since at least
e9a823fb34a8bdepends onq->sysfs_lockto sync between testing the flag in__elevator_change()and clearing it here.
Quoted 8 lines
The header for commit
e9a823fb34a8bsays:
To fix this warning, we can check theQUEUE_FLAG_REGISTEREDflag when changing the elevator and use the request_queue'ssysfs_lockto serialize between clearing the flag and the elevator testing the flag.The reality is
sysfs_lockisn't needed to serialize betweenblk_unregister_queue()clearing the flag and__elevator_change()testing the flag.
Without holding sysfs_lock, __elevator_change() may just be called after q->kobj is deleted from blk_unregister_queue(), then the warning of 'WARNING: CPU: 3 PID: 14075 at lib/kobject.c:244 kobject_add_internal+0x103/0x2d0' can be triggered again.
BTW, __elevator_change() is always run with holding sysfs_lock.
Thanks,
Ming
Re: [for-4.16 PATCH v4 2/4] block: use queue_lock when clearing QUEUE_FLAG_REGISTERED in blk_unregister_queue
Quoted 48 lines · Ming Lei
Quoted 38 lines · Mike Snitzer
Quoted 27 lines · Ming Lei
Quoted 26 lines · Mike Snitzer
blk_unregister_queue()must protect against any modifications ofq->queue_flags(not just those performed inblk-sysfs.c). Thereforeq->queue_lockneeds to be used rather thanq->sysfs_lock.Fixes e9a823fb34a8b ("block: fix warning when I/O elevator is changed as request_queue is being removed")Cc [email protected] # 4.14+Reported-by Bart Van Assche <[email protected]>Signed-off-by Mike Snitzer <[email protected]>--- block/blk-sysfs.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)block/blk-sysfs.c +2
-2@@ -929,9 +929,9 @@ void blk_unregister_queue(struct gendisk *disk)if (WARN_ON (!q))return ; -mutex_lock (&q->sysfs_lock); +spin_lock_irq (q->queue_lock);queue_flag_clear_unlocked (QUEUE_FLAG_REGISTERED , q); -mutex_unlock (&q->sysfs_lock); +spin_unlock_irq (q->queue_lock);Quoted 7 lines
block/blk-sysfs.c void blk_unregister_queue(struct gendisk *disk) · line 936
Quoted 1 line
block/blk-sysfs.c +0
-0wbt_exit (q);Hi Mike,
This change may not be correct, since at least
e9a823fb34a8bdepends onq->sysfs_lockto sync between testing the flag in__elevator_change()and clearing it here.Quoted 8 lines
The header for commit
e9a823fb34a8bsays:
To fix this warning, we can check theQUEUE_FLAG_REGISTEREDflag when changing the elevator and use the request_queue'ssysfs_lockto serialize between clearing the flag and the elevator testing the flag.The reality is
sysfs_lockisn't needed to serialize betweenblk_unregister_queue()clearing the flag and__elevator_change()testing the flag.
Quoted 6 lines
Without holding
sysfs_lock,__elevator_change()may just be called afterq->kobjis deleted fromblk_unregister_queue(), then the warning of 'WARNING: CPU: 3 PID: 14075 atlib/kobject.c:244kobject_add_internal+0x103/0x2d0' can be triggered again.BTW,
__elevator_change()is always run with holdingsysfs_lock.
Yes, I'm well aware. Please see v5's PATCH 02/04 which is inbound now.
Thanks,
Mike
PATCH v4 3/4 block: allow gendisk's request_queue registration to be deferred
In reply to Mike SnitzerSince I can remember DM has forced the block layer to allow the allocation and initialization of the request_queue to be distinct operations. Reason for this is block/genhd.c:add_disk() has requires that the request_queue (and associated bdi) be tied to the gendisk before add_disk() is called -- because add_disk() also deals with exposing the request_queue via blk_register_queue().
DM's dynamic creation of arbitrary device types (and associated request_queue types) requires the DM device's gendisk be available so that DM table loads can establish a master/slave relationship with subordinate devices that are referenced by loaded DM tables -- using bd_link_disk_holder(). But until these DM tables, and their associated subordinate devices, are known DM cannot know what type of request_queue it needs -- nor what its queue_limits should be.
This chicken and egg scenario has created all manner of problems for DM and, at times, the block layer.
Summary of changes:
- Add blk_add_disk_no_queue_reg() and add_disk_no_queue_reg() variant that drivers may use to add a disk without also calling blk_register_queue(). Driver must call blk_register_queue() once its request_queue is fully initialized.
- Return early from blk_unregister_queue() if QUEUE_FLAG_REGISTERED is not set. It won't be set if driver used add_disk_no_queue_reg() but driver encounters an error and must del_gendisk() before calling blk_register_queue().
- Export blk_register_queue().
These changes allow DM to use add_disk_no_queue_reg() to anchor its gendisk as the "master" for master/slave relationships DM must establish with subordinate devices referenced in DM tables that get loaded. Once all "slave" devices for a DM device are known its request_queue can be properly initialized and then advertised via sysfs -- important improvement being that no request_queue resource initialization performed by blk_register_queue() is missed for DM devices anymore.
--- block/blk-sysfs.c | 8 ++++++-- block/genhd.c | 20 +++++++++++++++++--- include/linux/genhd.h | 5 +++++ 3 files changed, 28 insertions(+), 5 deletions(-)
block/blk-sysfs.c +6 -2
@@ -921,6 +921,7 @@ int blk_register_queue(struct gendisk *disk)mutex_unlock (&q->sysfs_lock);return ret; } +EXPORT_SYMBOL_GPL (blk_register_queue);void blk_unregister_queue (struct gendisk *disk) {
@@ -930,12 +931,15 @@ void blk_unregister_queue(struct gendisk *disk)return ;spin_lock_irq (q->queue_lock); -queue_flag_clear_unlocked (QUEUE_FLAG_REGISTERED , q); +if (!queue_flag_test_and_clear (QUEUE_FLAG_REGISTERED , q)) { +/* Return early if disk->queue was never registered. */ +spin_unlock_irq (q->queue_lock); +return ; + }spin_unlock_irq (q->queue_lock);wbt_exit (q); -if (q->mq_ops)blk_mq_unregister_dev (disk_to_dev (disk), q);
block/genhd.c +17 -3
@@ -629,7 +629,7 @@ static void register_disk(struct device *parent, struct gendisk *disk) }/** -* device_add_disk - add partitioning information to kernel list +* device_add_disk_no_queue_reg - add partitioning information to kernel list * @parent: parent device for the disk * @disk: per-device partitioning information *
@@ -638,7 +638,7 @@ static void register_disk(struct device *parent, struct gendisk *disk)* * FIXME: error handling */ -void device_add_disk (struct device *parent,struct gendisk *disk) +void device_add_disk_no_queue_reg (struct device *parent,struct gendisk *disk) {dev_t devt;int retval;
@@ -682,7 +682,6 @@ void device_add_disk(struct device *parent, struct gendisk *disk) exact_match, exact_lock, disk); }register_disk (parent, disk); -blk_register_queue (disk);/* * Take an extra ref on queue which will be put on disk_release()
@@ -693,6 +692,21 @@ void device_add_disk(struct device *parent, struct gendisk *disk)disk_add_events (disk);blk_integrity_add (disk); } +EXPORT_SYMBOL (device_add_disk_no_queue_reg); + +/** +* device_add_disk - add disk information to kernel list +* @parent: parent device for the disk +* @disk: per-device disk information +* +* Adds partitioning information and also registers the +* associated request_queue to @disk. +*/ +void device_add_disk (struct device *parent,struct gendisk *disk) +{ +device_add_disk_no_queue_reg (parent, disk); +blk_register_queue (disk); +}EXPORT_SYMBOL (device_add_disk);void del_gendisk (struct gendisk *disk)
include/linux/genhd.h +5 -0
@@ -395,6 +395,11 @@ static inline void add_disk(struct gendisk *disk) {device_add_disk (NULL , disk); } +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) +{ +device_add_disk_no_queue_reg (NULL , disk); +}extern void del_gendisk (struct gendisk *gp);extern struct gendisk *get_gendisk (dev_t dev,int *partno);
Signature · 1 line
2.15.0
Re: [for-4.16 PATCH v4 3/4] block: allow gendisk's request_queue registration to be deferred
Quoted 14 lines · Mike Snitzer
block/genhd.c +1
-2-void device_add_disk (struct device *parent,struct gendisk *disk) +void device_add_disk_no_queue_reg (struct device *parent,struct gendisk *disk) {dev_t devt;int retval;@@ -682,7 +682,6 @@ void device_add_disk(struct device *parent, struct gendisk *disk) exact_match, exact_lock, disk); }register_disk (parent, disk); -blk_register_queue (disk);/* * Take an extra ref on queue which will be put on disk_release()
block/genhd.c void device_add_disk(struct device *parent, struct gendisk *disk) · line 695
Quoted 4 lines
block/genhd.c +1
-0@@ -693,6 +692,21 @@ void device_add_disk(struct device *parent, struct gendisk *disk)disk_add_events (disk);blk_integrity_add (disk); } +EXPORT_SYMBOL (device_add_disk_no_queue_reg);
Hello Mike,
This change can increase the time between the generation of the disk uevent and the registration of the request queue sysfs attributes. Can this cause any udev rules to fail?
Thanks,
Bart.
Re: [for-4.16 PATCH v4 3/4] block: allow gendisk's request_queue registration to be deferred
Quoted 22 lines · Bart Van Assche
Quoted 14 lines · Mike Snitzer
block/genhd.c +1
-2-void device_add_disk (struct device *parent,struct gendisk *disk) +void device_add_disk_no_queue_reg (struct device *parent,struct gendisk *disk) {dev_t devt;int retval;@@ -682,7 +682,6 @@ void device_add_disk(struct device *parent, struct gendisk *disk) exact_match, exact_lock, disk); }register_disk (parent, disk); -blk_register_queue (disk);/* * Take an extra ref on queue which will be put on disk_release() block/genhd.c void device_add_disk(struct device *parent, struct gendisk *disk) · line 695
Quoted 4 lines
block/genhd.c +1
-0@@ -693,6 +692,21 @@ void device_add_disk(struct device *parent, struct gendisk *disk)disk_add_events (disk);blk_integrity_add (disk); } +EXPORT_SYMBOL (device_add_disk_no_queue_reg);Hello Mike,
Quoted 3 lines
This change can increase the time between the generation of the disk uevent and the registration of the request queue sysfs attributes. Can this cause any udev rules to fail?
Certainly not for DM (DM has very sophisticated udev rules that wait for the final dm generated "CHANGE" event before considering a device to be "ready").
But are you asking about non-DM devices? I cannot see, what amounts to, moving blk_register_queue() to the end of device_add_disk() as reason for concern. If it were there technically would already be that race.
Mike
Re: [for-4.16 PATCH v4 3/4] block: allow gendisk's request_queue registration to be deferred
In reply to Mike SnitzerQuoted 148 lines · Mike Snitzer
Since I can remember DM has forced the block layer to allow the allocation and initialization of the
request_queueto be distinct operations. Reason for this is block/genhd.c:add_disk() has requires that therequest_queue(and associated bdi) be tied to the gendisk beforeadd_disk()is called -- becauseadd_disk()also deals with exposing therequest_queueviablk_register_queue().DM's dynamic creation of arbitrary device types (and associated
request_queuetypes) requires the DM device's gendisk be available so that DM table loads can establish a master/slave relationship with subordinate devices that are referenced by loaded DM tables -- usingbd_link_disk_holder(). But until these DM tables, and their associated subordinate devices, are known DM cannot know what type ofrequest_queueit needs -- nor what itsqueue_limitsshould be.This chicken and egg scenario has created all manner of problems for DM and, at times, the block layer.
Summary of changes:
- Add
blk_add_disk_no_queue_reg()andadd_disk_no_queue_reg()variant that drivers may use to add a disk without also callingblk_register_queue(). Driver must callblk_register_queue()once itsrequest_queueis fully initialized.- Return early from
blk_unregister_queue()ifQUEUE_FLAG_REGISTEREDis not set. It won't be set if driver usedadd_disk_no_queue_reg()but driver encounters an error and mustdel_gendisk()before callingblk_register_queue().- Export
blk_register_queue().These changes allow DM to use
add_disk_no_queue_reg()to anchor its gendisk as the "master" for master/slave relationships DM must establish with subordinate devices referenced in DM tables that get loaded. Once all "slave" devices for a DM device are known itsrequest_queuecan be properly initialized and then advertised via sysfs -- important improvement being that norequest_queueresource initialization performed byblk_register_queue()is missed for DM devices anymore.Signed-off-by Mike Snitzer <[email protected]>--- block/blk-sysfs.c | 8 ++++++-- block/genhd.c | 20 +++++++++++++++++--- include/linux/genhd.h | 5 +++++ 3 files changed, 28 insertions(+), 5 deletions(-)block/blk-sysfs.c +6
-2@@ -921,6 +921,7 @@ int blk_register_queue(struct gendisk *disk)mutex_unlock (&q->sysfs_lock);return ret; } +EXPORT_SYMBOL_GPL (blk_register_queue);void blk_unregister_queue (struct gendisk *disk) {@@ -930,12 +931,15 @@ void blk_unregister_queue(struct gendisk *disk)return ;spin_lock_irq (q->queue_lock); -queue_flag_clear_unlocked (QUEUE_FLAG_REGISTERED , q); +if (!queue_flag_test_and_clear (QUEUE_FLAG_REGISTERED , q)) { +/* Return early if disk->queue was never registered. */ +spin_unlock_irq (q->queue_lock); +return ; + }spin_unlock_irq (q->queue_lock);wbt_exit (q); -if (q->mq_ops)blk_mq_unregister_dev (disk_to_dev (disk), q);block/genhd.c +17
-3@@ -629,7 +629,7 @@ static void register_disk(struct device *parent, struct gendisk *disk) }/** -* device_add_disk - add partitioning information to kernel list +* device_add_disk_no_queue_reg - add partitioning information to kernel list * @parent: parent device for the disk * @disk: per-device partitioning information * @@ -638,7 +638,7 @@ static void register_disk(struct device *parent, struct gendisk *disk)* * FIXME: error handling */ -void device_add_disk (struct device *parent,struct gendisk *disk) +void device_add_disk_no_queue_reg (struct device *parent,struct gendisk *disk) {dev_t devt;int retval;@@ -682,7 +682,6 @@ void device_add_disk(struct device *parent, struct gendisk *disk) exact_match, exact_lock, disk); }register_disk (parent, disk); -blk_register_queue (disk);/* * Take an extra ref on queue which will be put on disk_release() @@ -693,6 +692,21 @@ void device_add_disk(struct device *parent, struct gendisk *disk)disk_add_events (disk);blk_integrity_add (disk); } +EXPORT_SYMBOL (device_add_disk_no_queue_reg); + +/** +* device_add_disk - add disk information to kernel list +* @parent: parent device for the disk +* @disk: per-device disk information +* +* Adds partitioning information and also registers the +* associated request_queue to @disk. +*/ +void device_add_disk (struct device *parent,struct gendisk *disk) +{ +device_add_disk_no_queue_reg (parent, disk); +blk_register_queue (disk); +}EXPORT_SYMBOL (device_add_disk);void del_gendisk (struct gendisk *disk)include/linux/genhd.h +5
-0@@ -395,6 +395,11 @@ static inline void add_disk(struct gendisk *disk) {device_add_disk (NULL , disk); } +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) +{ +device_add_disk_no_queue_reg (NULL , disk); +}extern void del_gendisk (struct gendisk *gp);extern struct gendisk *get_gendisk (dev_t dev,int *partno);Signature · 1 line
2.15.0
Looks fine:
Signature · 1 line
Ming
PATCH v4 4/4 dm: fix awkward and incomplete request_queue initialization
In reply to Mike SnitzerDM is now no longer prone to having its request_queue be improperly initialized.
Summary of changes:
- defer DM's blk_register_queue() from add_disk()-time until dm_setup_md_queue() by using add_disk_no_queue_reg() in alloc_dev().
- dm_setup_md_queue() is updated to fully initialize DM's request_queue (_after_ all table loads have occurred and the request_queue's type, features and limits are known).
- various other small improvements that were noticed along the way.
A very welcome side-effect of these changes is DM no longer needs to:
1) backfill the "mq" sysfs entry (because historically DM didn't initialize the request_queue to use blk-mq until _after_ blk_register_queue() was called via add_disk()).
2) call elv_register_queue() to get .request_fn request-based DM device's "queue" exposed in syfs.
In addition, blk-mq debugfs support is now made available because request-based DM's blk-mq request_queue is now properly initialized before dm_setup_md_queue() calls blk_register_queue().
These changes also stave off the need to introduce new DM-specific workarounds in block core, e.g. this proposal:
https://patchwork.kernel.org/patch/10067961/
In the end DM devices should be less unicorn in nature (relative to initialization and availability of block core infrastructure).
--- drivers/md/dm-core.h | 2 -- drivers/md/dm-rq.c | 11 ----------- drivers/md/dm.c | 41 ++++++++++++++++++++++------------------- 3 files changed, 22 insertions(+), 32 deletions(-)
drivers/md/dm-core.h +0 -2
@@ -130,8 +130,6 @@ struct mapped_device {struct srcu_struct io_barrier; }; -void dm_init_md_queue (struct mapped_device *md); -void dm_init_normal_md_queue (struct mapped_device *md);int md_in_flight (struct mapped_device *md);void disable_write_same (struct mapped_device *md);void disable_write_zeroes (struct mapped_device *md);
drivers/md/dm-rq.c +0 -11
@@ -700,7 +700,6 @@ int dm_old_init_request_queue(struct mapped_device *md, struct dm_table *t)/* disable dm_old_request_fn's merge heuristic by default */ md->seq_rq_merge_deadline_usecs =0 ; -dm_init_normal_md_queue (md);blk_queue_softirq_done (md->queue, dm_softirq_done);/* Initialize the request-based DM worker thread */
@@ -713,8 +712,6 @@ int dm_old_init_request_queue(struct mapped_device *md, struct dm_table *t)return error; } -elv_register_queue (md->queue); -return 0 ; }
@@ -810,17 +807,9 @@ int dm_mq_init_request_queue(struct mapped_device *md, struct dm_table *t) err =PTR_ERR (q); goto out_tag_set; } -dm_init_md_queue (md); - -/* backfill 'mq' sysfs registration normally done in blk_register_queue */ - err =blk_mq_register_dev (disk_to_dev (md->disk), q); -if (err) - goto out_cleanup_queue;return 0 ; -out_cleanup_queue: -blk_cleanup_queue (q); out_tag_set:blk_mq_free_tag_set (md->tag_set); out_kfree_tag_set:
drivers/md/dm.c +22 -19
102 lines not on this page · raw message
Signature · 1 line
2.15.0