Re: [PATCHv2 1/1] block: introduce content activity based ioprio

[Jens Axboe <axboe@kernel.dk>]

On 1/24/24 4:58 AM, Zhaoyang Huang wrote:
> On Wed, Jan 24, 2024 at 5:38?PM Christoph Hellwig <hch@infradead.org> wrote:
>>
>> The I/O priority can be explicitly set by the submitter, task and
>> blkcg arre jut fallbacks.
> Yes. I would like to suggest if it is possible to have this commit
> work as a hint for promoting the priority since it has been proved in
> the verification?

We don't add patches that are wrong just because they provide a
performance benefit for some cases. Down that path lies tech debt to be
cleaned up later. Rather, the feature should be done right from the
start.

>> And as said multiple times now bio_add_page must just treat the page
>> as a physical address container.  It must never look at MM-internal
>> flags.
> The alternative way is to iterate the request;s pages in the scheduler
> which has been refused by Jens in the previous version. Anyway, we can
> find a solution on this.

That approach, or the current one, both have the same layering violation
that Christoph keeps telling you is wrong - you are looking at the page
itself in the IO path. What has been suggested is that the _issuer_ of
the IO, the one that actually deals with pages, is the one that should
be submitting IO at the right priority to begin with.

Your approach tries to hack around the fact that this isn't done, and
hence is introducing a layering violation where the block layer now
needs to look at the page and adjust the priority. If the IO was
submitted with the right priority to begin with, you would not have this
issue at all.

-- 
Jens Axboe