From: Eric Farman <farman@linux.ibm.com>
To: Damien Le Moal <damien.lemoal@wdc.com>,
linux-block@vger.kernel.org, Jens Axboe <axboe@kernel.dk>,
linux-scsi@vger.kernel.org,
"Martin K . Petersen" <martin.petersen@oracle.com>,
dm-devel@redhat.com, Mike Snitzer <snitzer@redhat.com>
Cc: Ming Lei <ming.lei@redhat.com>,
Christian Borntraeger <borntraeger@de.ibm.com>
Subject: Re: [PATCH v5 5/7] block: Delay default elevator initialization
Date: Tue, 1 Oct 2019 16:46:06 -0400 [thread overview]
Message-ID: <9355c25f-61d7-b290-7d60-552ef4206e8c@linux.ibm.com> (raw)
In-Reply-To: <20190905095135.26026-6-damien.lemoal@wdc.com>
On 9/5/19 5:51 AM, 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 for most drivers. The device type and elevator required features
> are not set yet, preventing to correctly select 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 adding the boolean elevator_init argument to
> blk_mq_init_allocated_queue() to control the execution of
> elevator_init_mq(). Two cases exist:
> 1) elevator_init = false is used for calls to
> blk_mq_init_allocated_queue() within blk_mq_init_queue(). In this
> case, a call to elevator_init_mq() is added to __device_add_disk(),
> resulting in the delayed initialization of the queue elevator
> after the device driver finished probing the device information. This
> effectively allows elevator_init_mq() access to more information
> about the device.
> 2) elevator_init = true preserves the current behavior of initializing
> the elevator directly from blk_mq_init_allocated_queue(). This case
> is used for the special request based DM devices where the device
> gendisk is created before the queue initialization and device
> information (e.g. queue limits) is already known when the queue
> initialization is executed.
>
> Additionally, 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 calling blk_mq_init_sched() in elevator_init_mq().
>
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
Coincidentally, I had been looking into a problem that is fixed in
5.4-rc1 by this patch. Thanks for that!
The problem was a delay during boot of a KVM guest with virtio-scsi
devices (or hotplug of such a device to a guest) in recent releases,
especially when virtio-scsi is configured as a module. The symptoms
look like:
[ 0.975315] virtio_blk virtio2: [vda] 1803060 4096-byte logical
blocks (7.39 GB/6.88 GiB)
[ 0.977859] scsi host0: Virtio SCSI HBA
[ 0.980339] scsi 0:0:0:0: Direct-Access QEMU QEMU HARDDISK
2.5+ PQ: 0 ANSI: 5
[ 0.981685] vda:VOL1/ 0XA906: vda1
[ 0.988253] alg: No test for crc32be (crc32be-vx)
...stall...
[ 24.544920] sd 0:0:0:0: Power-on or device reset occurred
[ 24.545176] sd 0:0:0:0: Attached scsi generic sg0 type 0
[ 24.545292] sd 0:0:0:0: [sda] 385 512-byte logical blocks: (197
kB/193 KiB)
[ 24.545368] sd 0:0:0:0: [sda] Write Protect is off
[ 24.545416] sd 0:0:0:0: [sda] Mode Sense: 63 00 00 08
[ 24.545456] sd 0:0:0:0: [sda] Write cache: enabled, read cache:
enabled, doesn't support DPO or FUA
[ 24.547033] sd 0:0:0:0: [sda] Attached SCSI disk
I debugged this down to the same behavior described/fixed back in 3.18
by commit 17497acbdce9 ("blk-mq, percpu_ref: start q->mq_usage_counter
in atomic mode"), and for the same reason. The delay starts occurring
as soon as q->q_usage_counter is converted to percpu for the one LUN tha
twas found, while scsi_scan_channel() is still working on its loop of
mostly non-existent devices. Exactly when this problem started
re-occuring is not certain to me, though I did see this problem with 5.2
on linux-stable.
When I run with a 5.3 kernel, the problem is easily reproducible. So I
bisected between 5.3 and 5.4-rc1, and got here. Cherry-picking this
patch on top of 5.3 cleans up the boot/hotplug process and removes any
stall. Any chance this could be cc'd to stable? Any data someone wants
to see behavioral changes?
Thanks,
Eric
> ---
> block/blk-mq.c | 12 +++++++++---
> block/elevator.c | 7 +++++++
> block/genhd.c | 9 +++++++++
> drivers/md/dm-rq.c | 2 +-
> include/linux/blk-mq.h | 3 ++-
> 5 files changed, 28 insertions(+), 5 deletions(-)
>
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index ee4caf0c0807..240416057f28 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -2689,7 +2689,11 @@ struct request_queue *blk_mq_init_queue(struct blk_mq_tag_set *set)
> if (!uninit_q)
> return ERR_PTR(-ENOMEM);
>
> - q = blk_mq_init_allocated_queue(set, uninit_q);
> + /*
> + * Initialize the queue without an elevator. device_add_disk() will do
> + * the initialization.
> + */
> + q = blk_mq_init_allocated_queue(set, uninit_q, false);
> if (IS_ERR(q))
> blk_cleanup_queue(uninit_q);
>
> @@ -2840,7 +2844,8 @@ static unsigned int nr_hw_queues(struct blk_mq_tag_set *set)
> }
>
> struct request_queue *blk_mq_init_allocated_queue(struct blk_mq_tag_set *set,
> - struct request_queue *q)
> + struct request_queue *q,
> + bool elevator_init)
> {
> /* mark the queue as mq asap */
> q->mq_ops = set->ops;
> @@ -2902,7 +2907,8 @@ 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);
> + if (elevator_init)
> + elevator_init_mq(q);
>
> return q;
>
> 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..26b31fcae217 100644
> --- a/block/genhd.c
> +++ b/block/genhd.c
> @@ -695,6 +695,15 @@ 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 if one is needed, that is, for devices requesting queue
> + * registration.
> + */
> + if (register_queue)
> + 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.
> diff --git a/drivers/md/dm-rq.c b/drivers/md/dm-rq.c
> index 21d5c1784d0c..3f8577e2c13b 100644
> --- a/drivers/md/dm-rq.c
> +++ b/drivers/md/dm-rq.c
> @@ -563,7 +563,7 @@ int dm_mq_init_request_queue(struct mapped_device *md, struct dm_table *t)
> if (err)
> goto out_kfree_tag_set;
>
> - q = blk_mq_init_allocated_queue(md->tag_set, md->queue);
> + q = blk_mq_init_allocated_queue(md->tag_set, md->queue, true);
> if (IS_ERR(q)) {
> err = PTR_ERR(q);
> goto out_tag_set;
> diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
> index 62a3bb715899..0bf056de5cc3 100644
> --- a/include/linux/blk-mq.h
> +++ b/include/linux/blk-mq.h
> @@ -248,7 +248,8 @@ enum {
>
> struct request_queue *blk_mq_init_queue(struct blk_mq_tag_set *);
> struct request_queue *blk_mq_init_allocated_queue(struct blk_mq_tag_set *set,
> - struct request_queue *q);
> + struct request_queue *q,
> + bool elevator_init);
> struct request_queue *blk_mq_init_sq_queue(struct blk_mq_tag_set *set,
> const struct blk_mq_ops *ops,
> unsigned int queue_depth,
>
next prev parent reply other threads:[~2019-10-01 20:46 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-09-05 9:51 [PATCH v5 0/7] Elevator cleanups and improvements Damien Le Moal
2019-09-05 9:51 ` [PATCH v5 1/7] block: Cleanup elevator_init_mq() use Damien Le Moal
2019-09-05 9:51 ` [PATCH v5 2/7] block: Change elevator_init_mq() to always succeed Damien Le Moal
2019-09-05 9:51 ` [PATCH v5 3/7] block: Introduce elevator features Damien Le Moal
2019-09-05 9:51 ` [PATCH v5 4/7] block: Improve default elevator selection Damien Le Moal
2019-09-05 9:51 ` [PATCH v5 5/7] block: Delay default elevator initialization Damien Le Moal
2019-10-01 20:46 ` Eric Farman [this message]
2019-10-16 18:51 ` Eric Farman
2019-10-17 2:39 ` Damien Le Moal
2019-10-17 13:37 ` Eric Farman
2019-09-05 9:51 ` [PATCH v5 6/7] block: Set ELEVATOR_F_ZBD_SEQ_WRITE for nullblk zoned disks Damien Le Moal
2019-09-05 9:51 ` [PATCH v5 7/7] sd: Set ELEVATOR_F_ZBD_SEQ_WRITE for ZBC disks Damien Le Moal
2019-09-06 0:31 ` [PATCH v5 0/7] Elevator cleanups and improvements Ming Lei
2019-09-06 1:53 ` Jens Axboe
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=9355c25f-61d7-b290-7d60-552ef4206e8c@linux.ibm.com \
--to=farman@linux.ibm.com \
--cc=axboe@kernel.dk \
--cc=borntraeger@de.ibm.com \
--cc=damien.lemoal@wdc.com \
--cc=dm-devel@redhat.com \
--cc=linux-block@vger.kernel.org \
--cc=linux-scsi@vger.kernel.org \
--cc=martin.petersen@oracle.com \
--cc=ming.lei@redhat.com \
--cc=snitzer@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).