On Tue, Apr 29, 2025 at 07:50:11PM +0900, Damien Le Moal wrote:
> On 4/29/25 17:29, Aaron Lu wrote:
> > Ever since commit eca2040972b4("scsi: block: ioprio: Clean up interface
> > definition"), the io priority level is masked and can no longer be larger
> > than IOPRIO_NR_LEVELS so remove this now useless test.
> >
> > The actual test of io prio level is done in ioprio_value() where any
> > invalid input of class/level/hint will result in an invalid class being
> > passed to the syscall, this is introduced in commit 01584c1e2337("scsi:
> > block: Improve ioprio value validity checks").
> >
> > Reported-by: Kexin Wei <ys.weikexin@xxxxxxx>
> > Cc: Damien Le Moal <dlemoal@xxxxxxxxxx>
> > Signed-off-by: Aaron Lu <ziqianlu@xxxxxxxxxxxxx>
> > ---
> > Kexin reported a LTP/ioprio_set03 case failure, where the test would
> > pass IOPRIO_CLASS_BE with priority level 8 and see if kernel would
> > return error. Turned out she is using an old kernel header where the
> > change introduced in commit 01584c1e2337("scsi: block: Improve ioprio
> > value validity checks") isn't available. During troubleshooting, I find
> > this priority level test confusing and misleading so I think it should
> > be removed.
>
> What is confusing and misleading about the fact that we support only 8 priority
> levels (0 to 7) and should check for it ?
I meant when I'm troubleshooting this LTP issue, I looked at this level
test and had no idea why it didn't work.
> With that said, the test is indeed redundant for the BE and RT class because we
> have:
>
> int ioprio_check_cap(int ioprio)
> {
> int class = IOPRIO_PRIO_CLASS(ioprio);
> int level = IOPRIO_PRIO_LEVEL(ioprio);
>
> And the macro IOPRIO_PRIO_LEVEL() will mask the level value to something between
> 0 and 7, always. So necessarily, level will always be lower than
> IOPRIO_NR_LEVELS. So please reword your commit message to explain that rather
> than describe what a user may or may not use when setting an ioprio field.
No problem. Does something below look OK to you?
"
Ever since commit eca2040972b4("scsi: block: ioprio: Clean up interface
definition"), the macro IOPRIO_PRIO_LEVEL() will mask the level value to
something between 0 and 7 so necessarily, level will always be lower than
IOPRIO_NR_LEVELS.
Remove this obsolete check.
"
> And also simplify the patch:
>
> diff --git a/block/ioprio.c b/block/ioprio.c
> index 73301a261429..f0ee2798539c 100644
> --- a/block/ioprio.c
> +++ b/block/ioprio.c
> @@ -46,12 +46,8 @@ int ioprio_check_cap(int ioprio)
> */
> if (!capable(CAP_SYS_ADMIN) && !capable(CAP_SYS_NICE))
> return -EPERM;
> - fallthrough;
> - /* rt has prio field too */
> - case IOPRIO_CLASS_BE:
> - if (level >= IOPRIO_NR_LEVELS)
> - return -EINVAL;
> break;
> + case IOPRIO_CLASS_BE:
> case IOPRIO_CLASS_IDLE:
> break;
> case IOPRIO_CLASS_NONE:
>
>
Will do.
Thanks,
Aaron
> > block/ioprio.c | 5 +----
> > 1 file changed, 1 insertion(+), 4 deletions(-)
> >
> > diff --git a/block/ioprio.c b/block/ioprio.c
> > index 73301a261429f..60364d3faf800 100644
> > --- a/block/ioprio.c
> > +++ b/block/ioprio.c
> > @@ -46,11 +46,8 @@ int ioprio_check_cap(int ioprio)
> > */
> > if (!capable(CAP_SYS_ADMIN) && !capable(CAP_SYS_NICE))
> > return -EPERM;
> > - fallthrough;
> > - /* rt has prio field too */
> > + break;
> > case IOPRIO_CLASS_BE:
> > - if (level >= IOPRIO_NR_LEVELS)
> > - return -EINVAL;
> > break;
> > case IOPRIO_CLASS_IDLE:
> > break;
