Re: [PATCH v2] block: BFQ default for single queue devices

Oleksandr Natalenko
2018-10-15T14:32:53Z

Hi.

On 15.10.2018 16:10, Linus Walleij wrote:
> This sets BFQ as the default scheduler for single queue
> block devices (nr_hw_queues == 1) if it is available. This
> affects notably MMC/SD-cards but also UBI and the loopback
> device.
>
> I have been running it for a while without any negative
> effects on my pet systems and I want some wider testing
> so let's throw it out there and see what people say.
> Admittedly my use cases are limited. I need to keep this
> patch around for my personal needs anyway.
>
> We take special care to avoid using BFQ on zoned devices
> (in particular SMR, shingled magnetic recording devices)
> as these currently require mq-deadline to group writes
> together.
>
> I have opted against introducing any default scheduler
> through Kconfig as the mq-deadline enforcement for
> zoned devices has to be done at runtime anyways and
> too many config options will make things confusing.
>
> My argument for setting a default policy in the kernel
> as opposed to user space is the "reasonable defaults"
> type, analogous to how we have one default CPU scheduling
> policy (CFS) that make most sense for most tasks, and
> how automatic process group scheduling happens in most
> distributions without userspace involvement. The BFQ
> scheduling policy makes most sense for single hardware
> queue devices and many embedded systems will not have
> the clever userspace tools (such as udev) to make an
> educated choice of scheduling policy. Defaults should be
> those that make most sense for the hardware.
>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Paolo Valente <paolo.valente@linaro.org>
> Cc: Jens Axboe <axboe@kernel.dk>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Richard Weinberger <richard@nod.at>
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Bart Van Assche <bvanassche@acm.org>
> Cc: Jan Kara <jack@suse.cz>
> Cc: Artem Bityutskiy <dedekind1@gmail.com>
> Cc: Christoph Hellwig <hch@infradead.org>
> Cc: Alan Cox <gnomes@lxorguk.ukuu.org.uk>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Damien Le Moal <Damien.LeMoal@wdc.com>
> Cc: Johannes Thumshirn <jthumshirn@suse.de>
> Cc: Oleksandr Natalenko <oleksandr@natalenko.name>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - Add a quirk so that devices with zoned writes are forced
> to use the deadline scheduler, this is necessary since only
> that scheduler supports zoned writes.
> - There is a summary article in LWN for subscribers:
> https://lwn.net/Articles/767987/
> ---
> block/elevator.c | 22 ++++++++++++++++++----
> 1 file changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/block/elevator.c b/block/elevator.c
> index 8fdcd64ae12e..6e6048ca3471 100644
> --- a/block/elevator.c
> +++ b/block/elevator.c
> @@ -948,13 +948,16 @@ int elevator_switch_mq(struct request_queue *q,
> }
>
> /*
> - * For blk-mq devices, we default to using mq-deadline, if available,
> for single
> - * queue devices. If deadline isn't available OR we have multiple
> queues,
> - * default to "none".
> + * For blk-mq devices, we default to using:
> + * - "none" for multiqueue devices (nr_hw_queues != 1)
> + * - "bfq", if available, for single queue devices
> + * - "mq-deadline" if "bfq" is not available for single queue devices
> + * - "none" for single queue devices as well as last resort
> */
> int elevator_init_mq(struct request_queue *q)
> {
> struct elevator_type *e;
> + const char *policy;
> int err = 0;
>
> if (q->nr_hw_queues != 1)
> @@ -968,7 +971,18 @@ int elevator_init_mq(struct request_queue *q)
> if (unlikely(q->elevator))
> goto out_unlock;
>
> - e = elevator_get(q, "mq-deadline", false);
> + /*
> + * Zoned devices must use a deadline scheduler because currently
> + * that is the only scheduler respecting zoned writes.
> + */
> + if (blk_queue_is_zoned(q))
> + policy = "mq-deadline";
> + else if (IS_ENABLED(CONFIG_IOSCHED_BFQ))
> + policy = "bfq";
> + else
> + policy = "mq-deadline";

If more rules will be needed in the future, shall we just add extra ifs,
or it would be better to craft some struct/table now + policy search
helper?

> +
> + e = elevator_get(q, policy, false);
> if (!e)
> goto out_unlock;

--
Oleksandr Natalenko (post-factum)