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

[Damien Le Moal <dlemoal@kernel.org>]

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
>  #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.


-- 
Damien Le Moal
Western Digital Research