On 6/16/22 20:23, Jan Kara wrote: > On Thu 16-06-22 12:15:25, Damien Le Moal wrote: >> On 6/16/22 01:16, Jan Kara wrote: >>> Currently, IO priority set in task's IO context is not reflected in the >>> bio->bi_ioprio for most IO (only io_uring and direct IO set it). This >>> results in odd results where process is submitting some bios with one >>> priority and other bios with a different (unset) priority and due to >>> differing priorities bios cannot be merged. Make sure bio->bi_ioprio is >>> always set on bio submission. >>> >>> Signed-off-by: Jan Kara <jack@xxxxxxx> >>> --- >>> block/blk-mq.c | 6 ++++++ >>> 1 file changed, 6 insertions(+) >>> >>> diff --git a/block/blk-mq.c b/block/blk-mq.c >>> index e17d822e6051..e976f696babc 100644 >>> --- a/block/blk-mq.c >>> +++ b/block/blk-mq.c >>> @@ -2793,7 +2793,13 @@ static inline struct request *blk_mq_get_cached_request(struct request_queue *q, >>> static void bio_set_ioprio(struct bio *bio) >>> { >>> + unsigned short ioprio_class; >>> + >>> blkcg_set_ioprio(bio); >> >> Shouldn't this function set the default using the below "if" code ? >> >>> + ioprio_class = IOPRIO_PRIO_CLASS(bio->bi_ioprio); >>> + /* Nobody set ioprio so far? Initialize it based on task's nice value */ >> >> I do not think that the ioprio_class variable is useful. >> This can be: >> >> if (IOPRIO_PRIO_CLASS(bio->bi_ioprio) == IOPRIO_CLASS_NONE) >> bio->bi_ioprio = get_current_ioprio(); > > You are right. Fixed. What about moving this inside blkcg_set_ioprio() ? bio_set_ioprio() would then not be needed at all and blkcg_set_ioprio() called directly ? > >>> + 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! > > Honza -- Damien Le Moal Western Digital Research