Re: [PATCH v2 03/15] block: Support data lifetime in the I/O priority bitfield

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

On Fri, Oct 06, 2023 at 05:19:52PM +0900, Damien Le Moal wrote:
> On 10/6/23 04:40, Bart Van Assche wrote:
> > The NVMe and SCSI standards define 64 different data lifetimes. Support
> > storing this information in the I/O priority bitfield.
> > 
> > The current allocation of the 16 bits in the I/O priority bitfield is as
> > follows:
> > * 15..13: I/O priority class
> > * 12..6: unused
> > * 5..3: I/O hint (CDL)
> > * 2..0: I/O priority level
> > 
> > This patch changes this into the following:
> > * 15..13: I/O priority class
> > * 12: unused
> > * 11..6: data lifetime
> > * 5..3: I/O hint (CDL)
> > * 2..0: I/O priority level
> > 
> > Cc: Damien Le Moal <dlemoal@kernel.org>
> > Cc: Niklas Cassel <niklas.cassel@wdc.com>
> > Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> > ---
> >  include/uapi/linux/ioprio.h | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/uapi/linux/ioprio.h b/include/uapi/linux/ioprio.h
> > index bee2bdb0eedb..efe9bc450872 100644
> > --- a/include/uapi/linux/ioprio.h
> > +++ b/include/uapi/linux/ioprio.h
> > @@ -71,7 +71,7 @@ enum {
> >   * class and level.
> >   */
> >  #define IOPRIO_HINT_SHIFT		IOPRIO_LEVEL_NR_BITS
> > -#define IOPRIO_HINT_NR_BITS		10
> > +#define IOPRIO_HINT_NR_BITS		3

Can we really redefine this?
This is defined to 10 in released kernels, e.g. kernel v6.5.

Perhaps a better option would be to keep this defined to 10,
and then add new macros that define "specific" IO prio hint "classes".

something like
IOPRIO_HINT_NR_BITS                10

IOPRIO_HINT_CDL
IOPRIO_HINT_CDL_NR_BITS
IOPRIO_HINT_LIFETIME
IOPRIO_HINT_LIFETIME_NR_BITS

lifetime is really a hint, so I think it makes sense for it to be part of
the "IOPRIO_HINT bits".


> >  #define IOPRIO_NR_HINTS			(1 << IOPRIO_HINT_NR_BITS)
> >  #define IOPRIO_HINT_MASK		(IOPRIO_NR_HINTS - 1)
> >  #define IOPRIO_PRIO_HINT(ioprio)	\
> > @@ -102,6 +102,12 @@ enum {
> >  	IOPRIO_HINT_DEV_DURATION_LIMIT_7 = 7,
> >  };
> >  
> > +#define IOPRIO_LIFETIME_SHIFT		(IOPRIO_HINT_SHIFT + IOPRIO_HINT_NR_BITS)
> > +#define IOPRIO_LIFETIME_NR_BITS		6
> > +#define IOPRIO_LIFETIME_MASK		((1u << IOPRIO_LIFETIME_NR_BITS) - 1)
> > +#define IOPRIO_PRIO_LIFETIME(ioprio)					\
> > +	((ioprio >> IOPRIO_LIFETIME_SHIFT) & IOPRIO_LIFETIME_MASK)
> > +
> >  #define IOPRIO_BAD_VALUE(val, max) ((val) < 0 || (val) >= (max))
> 
> I am really not a fan of this. This essentially limits prio hints to CDL, while
> the initial intent was to define the hints as something generic that depend on
> the device features. With your change, we will not be able to support new
> features in the future.
> 
> Your change seem to assume that it makes sense to be able to combine CDL with
> lifetime hints. But does it really ? CDL is of dubious value for solid state
> media and as far as I know, UFS world has not expressed interest. Conversely,
> data lifetime hints do not make much sense for spin rust media where CDL is
> important. So I would say that the combination of CDL and lifetime hints is of
> dubious value.
> 
> Given this, why not simply define the 64 possible lifetime values as plain hint
> values (8 to 71, following 1 to 7 for CDL) ?
> 
> The other question here if you really want to keep the bit separation approach
> is: do we really need up to 64 different lifetime hints ? While the scsi
> standard allows that much, does this many different lifetime make sense in
> practice ? Can we ever think of a usecase that needs more than say 8 different
> liftimes (3 bits) ? If you limit the number of possible lifetime hints to 8,
> then we can keep 4 bits unused in the hint field for future features.

I think the question is: do we ever want to allow ioprio hints to be combined
or not?

If the answer is no, then go ahead and add the life time hints as values
8 to 71.

If the answer is yes, then life time hints need to use unique bits, even if it
might not make sense for CDL and life times to be combined.


Kind regards,
Niklas