diff mbox series

[1/3] block: Fix handling of tasks without ioprio in ioprio_get(2)

Message ID 20220601145110.18162-1-jack@suse.cz (mailing list archive)
State New, archived
Headers show
Series block: Fix IO priority mess | expand

Commit Message

Jan Kara June 1, 2022, 2:51 p.m. UTC 
ioprio_get(2) can be asked to return the best IO priority from several
tasks (IOPRIO_WHO_PGRP, IOPRIO_WHO_USER). Currently the call treats
tasks without set IO priority as having priority
IOPRIO_CLASS_BE/IOPRIO_BE_NORM however this does not really reflect the
IO priority the task will get (which depends on task's nice value) and
with the following fix it will not even match returned IO priority for a
single task. So fix IO priority comparison to treat unset IO priority as
the lowest possible one. This way we will return IOPRIO_CLASS_NONE
priority only if none of the considered tasks has explicitely set IO
priority, otherwise we return the highest set IO priority. This changes
userspace visible behavior but hopefully the results are clearer and
nothing breaks.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 block/ioprio.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Comments

Damien Le Moal June 1, 2022, 3:11 p.m. UTC | #1 
On 2022/06/01 23:51, Jan Kara wrote:
> ioprio_get(2) can be asked to return the best IO priority from several
> tasks (IOPRIO_WHO_PGRP, IOPRIO_WHO_USER). Currently the call treats
> tasks without set IO priority as having priority
> IOPRIO_CLASS_BE/IOPRIO_BE_NORM however this does not really reflect the
> IO priority the task will get (which depends on task's nice value) and
> with the following fix it will not even match returned IO priority for a
> single task. So fix IO priority comparison to treat unset IO priority as
> the lowest possible one. This way we will return IOPRIO_CLASS_NONE
> priority only if none of the considered tasks has explicitely set IO
> priority, otherwise we return the highest set IO priority. This changes
> userspace visible behavior but hopefully the results are clearer and
> nothing breaks.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  block/ioprio.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/block/ioprio.c b/block/ioprio.c
> index 2fe068fcaad5..62890391fc80 100644
> --- a/block/ioprio.c
> +++ b/block/ioprio.c
> @@ -157,10 +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;
> +		return bprio;

bprio may not be valid...

>  	if (!ioprio_valid(bprio))
> -		bprio = IOPRIO_DEFAULT;
> -
> +		return aprio;
>  	return min(aprio, bprio);
>  }
>
Jan Kara June 1, 2022, 3:23 p.m. UTC | #2 
On Thu 02-06-22 00:11:29, Damien Le Moal wrote:
> On 2022/06/01 23:51, Jan Kara wrote:
> > ioprio_get(2) can be asked to return the best IO priority from several
> > tasks (IOPRIO_WHO_PGRP, IOPRIO_WHO_USER). Currently the call treats
> > tasks without set IO priority as having priority
> > IOPRIO_CLASS_BE/IOPRIO_BE_NORM however this does not really reflect the
> > IO priority the task will get (which depends on task's nice value) and
> > with the following fix it will not even match returned IO priority for a
> > single task. So fix IO priority comparison to treat unset IO priority as
> > the lowest possible one. This way we will return IOPRIO_CLASS_NONE
> > priority only if none of the considered tasks has explicitely set IO
> > priority, otherwise we return the highest set IO priority. This changes
> > userspace visible behavior but hopefully the results are clearer and
> > nothing breaks.
> > 
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> >  block/ioprio.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/block/ioprio.c b/block/ioprio.c
> > index 2fe068fcaad5..62890391fc80 100644
> > --- a/block/ioprio.c
> > +++ b/block/ioprio.c
> > @@ -157,10 +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;
> > +		return bprio;
> 
> bprio may not be valid...

Yes, bprio may be from IOPRIO_CLASS_NONE as well and IMHO that is a
desirable return in that case. ioprio_valid() is IMHO a bit of a misnomer
because IOPRIO_CLASS_NONE is a valid class and can be even set by
userspace. It actually means, set IO priority based on task's CPU priority.
But lets first settle on the desired meaning of ioprio in the discussion
over patch 3/3. How we should behave in this case is a detail we can sort
out after the general meaning is clear.

								Honza
Damien Le Moal June 2, 2022, 12:56 a.m. UTC | #3 
On 2022/06/02 0:23, Jan Kara wrote:
> On Thu 02-06-22 00:11:29, Damien Le Moal wrote:
>> On 2022/06/01 23:51, Jan Kara wrote:
>>> ioprio_get(2) can be asked to return the best IO priority from several
>>> tasks (IOPRIO_WHO_PGRP, IOPRIO_WHO_USER). Currently the call treats
>>> tasks without set IO priority as having priority
>>> IOPRIO_CLASS_BE/IOPRIO_BE_NORM however this does not really reflect the
>>> IO priority the task will get (which depends on task's nice value) and
>>> with the following fix it will not even match returned IO priority for a
>>> single task. So fix IO priority comparison to treat unset IO priority as
>>> the lowest possible one. This way we will return IOPRIO_CLASS_NONE
>>> priority only if none of the considered tasks has explicitely set IO
>>> priority, otherwise we return the highest set IO priority. This changes
>>> userspace visible behavior but hopefully the results are clearer and
>>> nothing breaks.
>>>
>>> Signed-off-by: Jan Kara <jack@suse.cz>
>>> ---
>>>  block/ioprio.c | 5 ++---
>>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/block/ioprio.c b/block/ioprio.c
>>> index 2fe068fcaad5..62890391fc80 100644
>>> --- a/block/ioprio.c
>>> +++ b/block/ioprio.c
>>> @@ -157,10 +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;
>>> +		return bprio;
>>
>> bprio may not be valid...
> 
> Yes, bprio may be from IOPRIO_CLASS_NONE as well and IMHO that is a
> desirable return in that case. ioprio_valid() is IMHO a bit of a misnomer
> because IOPRIO_CLASS_NONE is a valid class and can be even set by
> userspace. It actually means, set IO priority based on task's CPU priority.
> But lets first settle on the desired meaning of ioprio in the discussion
> over patch 3/3. How we should behave in this case is a detail we can sort
> out after the general meaning is clear.

Sounds all good to me.

> 
> 								Honza
diff mbox series

Patch

diff --git a/block/ioprio.c b/block/ioprio.c
index 2fe068fcaad5..62890391fc80 100644
--- a/block/ioprio.c
+++ b/block/ioprio.c
@@ -157,10 +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;
+		return bprio;
 	if (!ioprio_valid(bprio))
-		bprio = IOPRIO_DEFAULT;
-
+		return aprio;
 	return min(aprio, bprio);
 }