On 6/21/22 19:24, Jan Kara wrote:
> Hello,
>
> This is the fourth revision of my patches fixing IO priority handling in the
> block layer.
Thanks for this. I reviewed and this all looks good to me.
Nevertheless, I will give this a spin tomorrow (with ATA drives that have
NCQ priority).
>
> Changes since v3:
> * Added Reviewed-by tags from Damien
> * Fixed build failure without CONFIG_BLOCK
> * Separated refactoring of get_current_ioprio() into a separate patch
>
> Changes since v2:
> * added some comments to better explain things
> * changed handling of ioprio_get(2)
> * a few small tweaks based on Damien's feedback
>
> Original cover letter:
> Recently, I've been looking into 10% regression reported by our performance
> measurement infrastructure in reaim benchmark that was bisected down to
> 5a9d041ba2f6 ("block: move io_context creation into where it's needed"). This
> didn't really make much sense and it took me a while to understand this but the
> culprit is actually in even older commit e70344c05995 ("block: fix default IO
> priority handling") and 5a9d041ba2f6 just made the breakage visible.
> Essentially the problem was that after these commits some IO was queued with IO
> priority class IOPRIO_CLASS_BE while other IO was queued with IOPRIO_CLASS_NONE
> and as a result they could not be merged together resulting in performance
> regression. I think what commit e70344c05995 ("block: fix default IO priority
> handling") did is actually broken not only because of this performance
> regression but because of other reasons as well (see changelog of patch 3/8 for
> details). Besides this problem, there are multiple other inconsistencies in the
> IO priority handling throughout the block stack we have identified when
> discussing this with Damien Le Moal. So this patch set aims at fixing all these
> various issues.
>
> Note that there are a few choices I've made I'm not 100% sold on. In particular
> the conversion of blk-ioprio from rqos is somewhat disputable since it now
> incurs a cost similar to blk-throttle in the bio submission fast path (need to
> load bio->bi_blkg->pd[ioprio_policy.plid]). If people think the removed
> boilerplate code is not worth the cost, I can certainly go via the "additional
> rqos hook" path.
>
> Honza
> Previous versions:
> Link: http://lore.kernel.org/r/20220601132347.13543-1-jack@xxxxxxx # v1
> Link: http://lore.kernel.org/r/20220615160437.5478-1-jack@xxxxxxx # v2
> Link: http://lore.kernel.org/r/20220620160726.19798-1-jack@xxxxxxx # v3
--
Damien Le Moal
Western Digital Research
