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

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

On 6/21/22 01:11, 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:
> 
> "If no I/O scheduler has been set for a thread, then by default the I/O
> priority will follow the CPU nice value (setpriority(2)).  In Linux
> kernels before version 2.6.24, once an I/O priority had been set using
> ioprio_set(), there was no way to reset the I/O scheduling behavior to
> the default. Since Linux 2.6.24, specifying ioprio as 0 can be used to
> reset to the default I/O scheduling behavior."
> 
> 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 to avoid
> future surprises. Note that we tweak ioprio_best() to maintain
> ioprio_get(2) behavior and make this commit easily backportable.
> 
> Fixes: e70344c05995 ("block: fix default IO priority handling")
> Signed-off-by: Jan Kara <jack@suse.cz>

Looks good to me.

Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

> ---
>  block/blk-ioc.c        | 2 ++
>  block/ioprio.c         | 4 ++--
>  include/linux/ioprio.h | 2 +-
>  3 files changed, 5 insertions(+), 3 deletions(-)
> 
> 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/block/ioprio.c b/block/ioprio.c
> index 2fe068fcaad5..2a34cbca18ae 100644
> --- a/block/ioprio.c
> +++ b/block/ioprio.c
> @@ -157,9 +157,9 @@ static int get_task_ioprio(struct task_struct *p)
>  int ioprio_best(unsigned short aprio, unsigned short bprio)
>  {
>  	if (!ioprio_valid(aprio))
> -		aprio = IOPRIO_DEFAULT;
> +		aprio = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, IOPRIO_BE_NORM);
>  	if (!ioprio_valid(bprio))
> -		bprio = IOPRIO_DEFAULT;
> +		bprio = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, IOPRIO_BE_NORM);
>  
>  	return min(aprio, bprio);
>  }
> diff --git a/include/linux/ioprio.h b/include/linux/ioprio.h
> index 3f53bc27a19b..3d088a88f832 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)
>  
>  /*
>   * Check that a priority value has a valid class.


-- 
Damien Le Moal
Western Digital Research