On Fri 17-06-22 09:04:34, Damien Le Moal wrote:
> 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
You make it sound here like IOPRIO_WHO_PGRP would be some different type of
IO priority. For record it is not, there's just one IO priority per task,
if you set ioprio with IOPRIO_WHO_PGRP, it will just iterate all the tasks
in PGRP and set IO priority for each task. After my patches,
ioprio_get(IOPRIO_WHO_PGRPIO) will return the best of the effective IO
priorities of tasks within PGRP. Before my patch it was doing the same but
if IO priority was unset for some task it considered it to be CLASS_BE,4.
> 2) If IOPRIO_WHO_USER is not set, ioprio_get(IOPRIO_WHO_USER) will also
> return the effective priority.
This is the same as above. Just the calls iterate over all tasks of the
given user...
> 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)
I want to go back to returning IOPRIO_CLASS_NONE for tasks with unset IO
priority.
> 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.
I also don't find (1) and (2) too important. (3) is IMHO somewhat important
and I think that the reason why nobody complained about the change there is
because your change is relatively new so it didn't propagate yet to any
widely used distro kernel...
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
