| Message ID | 20190904084247.23338-6-damien.lemoal@wdc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Elevator cleanups and improvements | expand |
On 04/09/2019 10:42, Damien Le Moal wrote: > @@ -734,6 +741,7 @@ static void __device_add_disk(struct device *parent, struct gendisk *disk, > exact_match, exact_lock, disk); > } > register_disk(parent, disk, groups); > + > if (register_queue) > blk_register_queue(disk); That hunk looks unrelated, but anyways: Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
On 2019/09/04 17:56, Johannes Thumshirn wrote: > On 04/09/2019 10:42, Damien Le Moal wrote: >> @@ -734,6 +741,7 @@ static void __device_add_disk(struct device *parent, struct gendisk *disk, >> exact_match, exact_lock, disk); >> } >> register_disk(parent, disk, groups); >> + >> if (register_queue) >> blk_register_queue(disk); > > That hunk looks unrelated, but anyways: > Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de> Oops. Yes, did not delete the blank line when I moved elevator_init_mq() call. Jens, should I resend a v4 to fix this ?
On Wed, Sep 04, 2019 at 05:42:45PM +0900, Damien Le Moal wrote: > When elevator_init_mq() is called from blk_mq_init_allocated_queue(), > the only information known about the device is the number of hardware > queues as the block device scan by the device driver is not completed > yet. The device type and the device required features are not set yet, > preventing to correctly choose the default elevator most suitable for > the device. > > This currently affects all multi-queue zoned block devices which default > to the "none" elevator instead of the required "mq-deadline" elevator. > These drives currently include host-managed SMR disks connected to a > smartpqi HBA and null_blk block devices with zoned mode enabled. > Upcoming NVMe Zoned Namespace devices will also be affected. > > Fix this by moving the execution of elevator_init_mq() from > blk_mq_init_allocated_queue() into __device_add_disk() to allow for the > device driver to probe the device characteristics and set attributes > of the device request queue prior to the elevator initialization. > > Also to make sure that the elevator initialization is never done while > requests are in-flight (there should be none when the device driver > calls device_add_disk()), freeze and quiesce the device request queue > before executing blk_mq_init_sched(). > > Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> > --- > block/blk-mq.c | 2 -- > block/elevator.c | 7 +++++++ > block/genhd.c | 8 ++++++++ > 3 files changed, 15 insertions(+), 2 deletions(-) > > diff --git a/block/blk-mq.c b/block/blk-mq.c > index ee4caf0c0807..a37503984206 100644 > --- a/block/blk-mq.c > +++ b/block/blk-mq.c > @@ -2902,8 +2902,6 @@ struct request_queue *blk_mq_init_allocated_queue(struct blk_mq_tag_set *set, > blk_mq_add_queue_tag_set(set, q); > blk_mq_map_swqueue(q); > > - elevator_init_mq(q); > - > return q; > > err_hctxs: > diff --git a/block/elevator.c b/block/elevator.c > index 520d6b224b74..096a670d22d7 100644 > --- a/block/elevator.c > +++ b/block/elevator.c > @@ -712,7 +712,14 @@ void elevator_init_mq(struct request_queue *q) > if (!e) > return; > > + blk_mq_freeze_queue(q); > + blk_mq_quiesce_queue(q); > + > err = blk_mq_init_sched(q, e); > + > + blk_mq_unquiesce_queue(q); > + blk_mq_unfreeze_queue(q); > + > if (err) { > pr_warn("\"%s\" elevator initialization failed, " > "falling back to \"none\"\n", e->elevator_name); > diff --git a/block/genhd.c b/block/genhd.c > index 54f1f0d381f4..7380dd7b2257 100644 > --- a/block/genhd.c > +++ b/block/genhd.c > @@ -695,6 +695,13 @@ static void __device_add_disk(struct device *parent, struct gendisk *disk, > dev_t devt; > int retval; > > + /* > + * The disk queue should now be all set with enough information about > + * the device for the elevator code to pick an adequate default > + * elevator. > + */ > + elevator_init_mq(disk->queue); > + For dm-rq, add_disk_no_queue_reg() is called before blk_mq_init_allocated_queue(). That means this patch actually sets elevator early for dm-rq, and I guess this way may not work as expected since hw/sw queues aren't allocated yet. Thanks, Ming
On 9/4/19 3:02 AM, Damien Le Moal wrote: > On 2019/09/04 17:56, Johannes Thumshirn wrote: >> On 04/09/2019 10:42, Damien Le Moal wrote: >>> @@ -734,6 +741,7 @@ static void __device_add_disk(struct device *parent, struct gendisk *disk, >>> exact_match, exact_lock, disk); >>> } >>> register_disk(parent, disk, groups); >>> + >>> if (register_queue) >>> blk_register_queue(disk); >> >> That hunk looks unrelated, but anyways: >> Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de> > > Oops. Yes, did not delete the blank line when I moved elevator_init_mq() call. > Jens, should I resend a v4 to fix this ? Series looks good to me, I'll just delete this one hunk, not a big deal.
On 2019/09/04 21:57, Jens Axboe wrote: > On 9/4/19 3:02 AM, Damien Le Moal wrote: >> On 2019/09/04 17:56, Johannes Thumshirn wrote: >>> On 04/09/2019 10:42, Damien Le Moal wrote: >>>> @@ -734,6 +741,7 @@ static void __device_add_disk(struct device *parent, struct gendisk *disk, >>>> exact_match, exact_lock, disk); >>>> } >>>> register_disk(parent, disk, groups); >>>> + >>>> if (register_queue) >>>> blk_register_queue(disk); >>> >>> That hunk looks unrelated, but anyways: >>> Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de> >> >> Oops. Yes, did not delete the blank line when I moved elevator_init_mq() call. >> Jens, should I resend a v4 to fix this ? > > Series looks good to me, I'll just delete this one hunk, not a big deal. > Jens, Thanks. But Ming's comment needed to be addressed, which I did in the V4 I just sent out. I removed the white line chunk too. Best regards.
diff --git a/block/blk-mq.c b/block/blk-mq.c index ee4caf0c0807..a37503984206 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -2902,8 +2902,6 @@ struct request_queue *blk_mq_init_allocated_queue(struct blk_mq_tag_set *set, blk_mq_add_queue_tag_set(set, q); blk_mq_map_swqueue(q); - elevator_init_mq(q); - return q; err_hctxs: diff --git a/block/elevator.c b/block/elevator.c index 520d6b224b74..096a670d22d7 100644 --- a/block/elevator.c +++ b/block/elevator.c @@ -712,7 +712,14 @@ void elevator_init_mq(struct request_queue *q) if (!e) return; + blk_mq_freeze_queue(q); + blk_mq_quiesce_queue(q); + err = blk_mq_init_sched(q, e); + + blk_mq_unquiesce_queue(q); + blk_mq_unfreeze_queue(q); + if (err) { pr_warn("\"%s\" elevator initialization failed, " "falling back to \"none\"\n", e->elevator_name); diff --git a/block/genhd.c b/block/genhd.c index 54f1f0d381f4..7380dd7b2257 100644 --- a/block/genhd.c +++ b/block/genhd.c @@ -695,6 +695,13 @@ static void __device_add_disk(struct device *parent, struct gendisk *disk, dev_t devt; int retval; + /* + * The disk queue should now be all set with enough information about + * the device for the elevator code to pick an adequate default + * elevator. + */ + elevator_init_mq(disk->queue); + /* minors == 0 indicates to use ext devt from part0 and should * be accompanied with EXT_DEVT flag. Make sure all * parameters make sense. @@ -734,6 +741,7 @@ static void __device_add_disk(struct device *parent, struct gendisk *disk, exact_match, exact_lock, disk); } register_disk(parent, disk, groups); + if (register_queue) blk_register_queue(disk);
When elevator_init_mq() is called from blk_mq_init_allocated_queue(), the only information known about the device is the number of hardware queues as the block device scan by the device driver is not completed yet. The device type and the device required features are not set yet, preventing to correctly choose the default elevator most suitable for the device. This currently affects all multi-queue zoned block devices which default to the "none" elevator instead of the required "mq-deadline" elevator. These drives currently include host-managed SMR disks connected to a smartpqi HBA and null_blk block devices with zoned mode enabled. Upcoming NVMe Zoned Namespace devices will also be affected. Fix this by moving the execution of elevator_init_mq() from blk_mq_init_allocated_queue() into __device_add_disk() to allow for the device driver to probe the device characteristics and set attributes of the device request queue prior to the elevator initialization. Also to make sure that the elevator initialization is never done while requests are in-flight (there should be none when the device driver calls device_add_disk()), freeze and quiesce the device request queue before executing blk_mq_init_sched(). Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> --- block/blk-mq.c | 2 -- block/elevator.c | 7 +++++++ block/genhd.c | 8 ++++++++ 3 files changed, 15 insertions(+), 2 deletions(-)