Re: [PATCH v3 01/18] block: introduce duration-limits priority class

[Niklas Cassel <Niklas.Cassel@wdc.com>]

On Thu, Jan 26, 2023 at 09:24:12AM +0900, Damien Le Moal wrote:
> On 2023/01/26 6:23, Niklas Cassel wrote:
> > On Wed, Jan 25, 2023 at 10:37:52AM -0800, Bart Van Assche wrote:

(snip)

> > If we were to reuse IOPRIO_CLASS_RT, then I guess the best option would be
> > to have something like:
> > 
> > $ cat /sys/block/sdX/device/rt_prio_backend
> > [none] ncq-prio cdl
> 
> No need for this. We can keep the existing ncq_prio_enable and the proposed
> duration_limits/enable sysfs attributes. The user cannot enable both at the same
> time with our patches. So if the user enables ncq_prio_enable, then it will get
> high priority NCQ commands mapping for any level of the RT class. If
> duration_limits/enable is set, then the user will get CDL scheduling of commands
> on the drive.
> 
> But again, the difficulty with this overloading is that we *cannot* implement a
> solid level-based scheduling in IO schedulers because ordering the CDLs in a
> meaningful way is impossible. So BFQ handling of the RT class would likely not
> result in the most ideal scheduling (that would depend heavily on how the CDL
> descriptors are defined on the drive). Hence my reluctance to overload the RT
> class for CDL.

Well, if CDL were to reuse IOPRIO_CLASS_RT, then the user would either have to
disable the IO scheduler, so that lower classdata levels wouldn't be prioritized
over higher classdata levels, or simply use an IO scheduler that does not care
about the classdata level, e.g. mq-deadline.

From ionice man page:

-n, --classdata level
Specify the scheduling class data. This only has an effect if the class accepts
an argument. For realtime and best-effort, 0-7 are valid data (priority levels),
and 0 represents the highest priority level.


I guess it kind of made sense for NCQ priority to piggyback on IOPRIO_CLASS_RT,
since the only thing that libata has to do is to set singular the high prio bit
(so the classdata could still be used for prioritizing IOs on the host side).

However, for CDL, things are not as simple as setting a single bit in the
command, because of all the different descriptors, so we must let the classdata
represent the device side priority level, and not the host side priority level
(as we cannot have both, and I agree with you, it is very hard define an order
between the descriptors.. e.g. should a 20 ms policy 0xf descriptor be ranked
higher or lower than a 20 ms policy 0xd descriptor?).

It's best to let the definition for IOPRIO_CLASS_RT stay the way it always has,
0 represents the highest priority level, 7 the lowest priority level (and we
wouldn't be able to change how the schedulers handle IOPRIO_CLASS_RT anyway).

If we update the man page with a IOPRIO_CLASS_DL entry, we could clearly state
that IO schedulers do not care about the classdata at all for IOPRIO_CLASS_DL
(and that the classdata is solely used to convey a priority state/index to the
device).

So I think this patch is good as it is.

Bart, do you agree? Any chance for a Reviewed-by?


Kind regards,
Niklas