Re: [PATCH 3/3] block: fix default IO priority handling again

[Damien Le Moal <damien.lemoal@opensource.wdc.com>]

On 2022/06/01 23:51, Jan Kara wrote:
> Commit e70344c05995 ("block: fix default IO priority handling")
> introduced an inconsistency in get_current_ioprio() that tasks without
> IO context return IOPRIO_DEFAULT priority while tasks with freshly
> allocated IO context will return 0 (IOPRIO_CLASS_NONE/0) IO priority.
> Tasks without IO context used to be rare before 5a9d041ba2f6 ("block:
> move io_context creation into where it's needed") but after this commit
> they became common because now only BFQ IO scheduler setups task's IO
> context. Similar inconsistency is there for get_task_ioprio() so this
> inconsistency is now exposed to userspace and userspace will see
> different IO priority for tasks operating on devices with BFQ compared
> to devices without BFQ. Furthemore the changes done by commit
> e70344c05995 change the behavior when no IO priority is set for BFQ IO
> scheduler which is also documented in ioprio_set(2) manpage - namely
> that tasks without set IO priority will use IO priority based on their
> nice value.
> 
> So make sure we default to IOPRIO_CLASS_NONE as used to be the case
> before commit e70344c05995. Also cleanup alloc_io_context() to
> explicitely set this IO priority for the allocated IO context.
> 
> Fixes: e70344c05995 ("block: fix default IO priority handling")
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  block/blk-ioc.c        | 2 ++
>  include/linux/ioprio.h | 2 +-
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/block/blk-ioc.c b/block/blk-ioc.c
> index df9cfe4ca532..63fc02042408 100644
> --- a/block/blk-ioc.c
> +++ b/block/blk-ioc.c
> @@ -247,6 +247,8 @@ static struct io_context *alloc_io_context(gfp_t gfp_flags, int node)
>  	INIT_HLIST_HEAD(&ioc->icq_list);
>  	INIT_WORK(&ioc->release_work, ioc_release_fn);
>  #endif
> +	ioc->ioprio = IOPRIO_DEFAULT;
> +
>  	return ioc;
>  }
>  
> diff --git a/include/linux/ioprio.h b/include/linux/ioprio.h
> index 774bb90ad668..d9dc78a15301 100644
> --- a/include/linux/ioprio.h
> +++ b/include/linux/ioprio.h
> @@ -11,7 +11,7 @@
>  /*
>   * Default IO priority.
>   */
> -#define IOPRIO_DEFAULT	IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, IOPRIO_BE_NORM)
> +#define IOPRIO_DEFAULT	IOPRIO_PRIO_VALUE(IOPRIO_CLASS_NONE, 0)

"man ioprio_set" says:

IOPRIO_CLASS_BE (2)
This is the best-effort scheduling class, which is the default for any process
that hasn't set  a  specific I/O  priority.

Which is why patch e70344c05995 introduced IOPRIO_DEFAULT definition using the
BE class, to have the kernel in sync with the manual.

The different ioprio leading to no BIO merging is definitely a problem but this
patch is not really fixing anything in my opinion. It simply gets back to the
previous "all 0s" ioprio initialization, which do not show the places where we
actually have missing ioprio initialization to IOPRIO_DEFAULT.

Isn't it simply that IOPRIO_DEFAULT should be set as the default for any bio
being allocated (in bio_alloc ?) before it is setup and inherits the user io
priority ? Otherwise, the bio io prio is indeed IOPRIO_CLASS_NONE/0 and changing
IOPRIO_DEFAULT to that value removes the differences you observed.

>  
>  /*
>   * Check that a priority value has a valid class.


-- 
Damien Le Moal
Western Digital Research