block: remove test of io priority level

[PATCH] block: remove test of io priority level

Posted by Aaron Lu 9 months, 2 weeks ago

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@h3c.com>
Cc: Damien Le Moal <dlemoal@kernel.org>
Signed-off-by: Aaron Lu <ziqianlu@bytedance.com>
---
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.

 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;
-- 
2.39.5

Re: [PATCH] block: remove test of io priority level

Posted by Damien Le Moal 9 months, 2 weeks ago

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@h3c.com>
> Cc: Damien Le Moal <dlemoal@kernel.org>
> Signed-off-by: Aaron Lu <ziqianlu@bytedance.com>
> ---
> 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 ?

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


>  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;


-- 
Damien Le Moal
Western Digital Research

Re: [External] Re: [PATCH] block: remove test of io priority level

Posted by Aaron Lu 9 months, 2 weeks ago

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@h3c.com>
> > Cc: Damien Le Moal <dlemoal@kernel.org>
> > Signed-off-by: Aaron Lu <ziqianlu@bytedance.com>
> > ---
> > 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;

Re: [External] Re: [PATCH] block: remove test of io priority level

Posted by Damien Le Moal 9 months, 2 weeks ago

On 4/29/25 20:44, Aaron Lu wrote:
> 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@h3c.com>
>>> Cc: Damien Le Moal <dlemoal@kernel.org>
>>> Signed-off-by: Aaron Lu <ziqianlu@bytedance.com>
>>> ---
>>> 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.

OK. I understand the "confusing" now :)

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

Yes, looks much better !


-- 
Damien Le Moal
Western Digital Research