On 6/16/22 21:24, Jan Kara wrote:
> On Thu 16-06-22 13:23:03, Jan Kara wrote:
>> On Thu 16-06-22 12:15:25, Damien Le Moal wrote:
>>> On 6/16/22 01:16, Jan Kara wrote:
>>>> + if (ioprio_class == IOPRIO_CLASS_NONE)
>>>> + bio->bi_ioprio = get_current_ioprio();
>>>> }
>>>> /**
>>>
>>> Beside this comment, I am still scratching my head regarding what the user
>>> gets with ioprio_get(). If I understood your patches correctly, the user may
>>> still see IOPRIO_CLASS_NONE ?
>>> For that case, to be in sync with the man page, I thought the returned
>>> ioprio should be the effective one based on the task io nice value, that is,
>>> the value returned by get_current_ioprio(). Am I missing something... ?
>>
>> The trouble with returning "effective ioprio" is that with IOPRIO_WHO_PGRP
>> or IOPRIO_WHO_USER the effective IO priority may be different for different
>> processes considered and it can be also further influenced by blk-ioprio
>> settings. But thinking about it now after things have settled I agree that
>> what you suggests makes more sense. I'll fix that. Thanks for suggestion!
>
> Oh, now I've remembered why I've done it that way. With IOPRIO_WHO_PROCESS
> (which is probably the most used and the best defined variant), we were
> returning IOPRIO_CLASS_NONE if the task didn't have set IO priority until
> commit e70344c05995 ("block: fix default IO priority handling"). So my
> patch was just making behavior of IOPRIO_WHO_PGRP & IOPRIO_WHO_USER
> consistent with the behavior of IOPRIO_WHO_PROCESS. I'd be reluctant to
> change the behavior of IOPRIO_WHO_PROCESS because that has the biggest
> chances for userspace regressions. But perhaps it makes sense to keep
> IOPRIO_WHO_PGRP & IOPRIO_WHO_USER inconsistent with IOPRIO_WHO_PROCESS and
> just use effective IO priority in those two variants. That looks like the
> smallest API change to make things at least somewhat sensible...
Still bit lost. Let me try to summarize your goal:
1) If IOPRIO_WHO_PGRP is not set, ioprio_get(IOPRIO_WHO_PGRP) will return
the effective priority
2) If IOPRIO_WHO_USER is not set, ioprio_get(IOPRIO_WHO_USER) will also
return the effective priority
3) if IOPRIO_WHO_PROCESS is not set, return ? I am lost for this one. Do
you want to go back to IOPRIO_CLASS_NONE ? Keep default (IOPRIO_CLASS_BE)
? Or switch to using the effective IO priority ? Not that the last 2
choices are actually equivalent if the user did not IO nice the process
(the default for the effective IO prio is class BE)
For (1) and (2), I am not sure. Given that my last changes to the ioprio
default did not seem to have bothered anyone (nobody screamed at me :)) I
am tempted to say: any choice is OK. So we should try to get as close as
the man page defined behavior as possible.
--
Damien Le Moal
Western Digital Research
