linux-kernel - Re: [PATCH v2] readahead: Correct the start and size in ondemand

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d6957a20-db31-d6ac-8822-003bdb9cd747@intel.com>
Date:   Tue, 4 Jul 2023 09:41:23 +0800
From:   "Yin, Fengwei" <fengwei.yin@...el.com>
To:     Mike Kravetz <mike.kravetz@...cle.com>
CC:     <akpm@...ux-foundation.org>, <willy@...radead.org>,
        <ackerleytng@...gle.com>, <linux-fsdevel@...r.kernel.org>,
        <linux-mm@...ck.org>, <linux-kernel@...r.kernel.org>,
        <oliver.sang@...el.com>
Subject: Re: [PATCH v2] readahead: Correct the start and size in
 ondemand_readahead()



On 7/4/2023 2:49 AM, Mike Kravetz wrote:
> On 06/28/23 12:43, Yin Fengwei wrote:
>> The commit
>> 9425c591e06a ("page cache: fix page_cache_next/prev_miss off by one")
>> updated the page_cache_next_miss() to return the index beyond
>> range.
>>
>> But it breaks the start/size of ra in ondemand_readahead() because
>> the offset by one is accumulated to readahead_index. As a consequence,
>> not best readahead order is picked.
>>
>> Tracing of the order parameter of filemap_alloc_folio() showed:
>>      page order    : count     distribution
>>         0          : 892073   |                                        |
>>         1          : 0        |                                        |
>>         2          : 65120457 |****************************************|
>>         3          : 32914005 |********************                    |
>>         4          : 33020991 |********************                    |
>> with 9425c591e06a9.
>>
>> With parent commit:
>>      page order    : count     distribution
>>         0          : 3417288  |****                                    |
>>         1          : 0        |                                        |
>>         2          : 877012   |*                                       |
>>         3          : 288      |                                        |
>>         4          : 5607522  |*******                                 |
>>         5          : 29974228 |****************************************|
>>
>> Fix the issue by removing the offset by one when page_cache_next_miss()
>> returns no gaps in the range.
>>
>> After the fix:
>>     page order     : count     distribution
>>         0          : 2598561  |***                                     |
>>         1          : 0        |                                        |
>>         2          : 687739   |                                        |
>>         3          : 288      |                                        |
>>         4          : 207210   |                                        |
>>         5          : 32628260 |****************************************|
>>
> 
> Thank you for your detailed analysis!
> 
> When the regression was initially discovered, I sent a patch to revert
> commit 9425c591e06a.  Andrew has picked up this change.  And, Andrew has
> also picked up this patch.
Oh. I didn't notice that you sent revert patch. My understanding is that
commit 9425c591e06a is a good change.

> 
> I have not verified yet, but I suspect that this patch is going to cause
> a regression because it depends on the behavior of page_cache_next_miss
> in 9425c591e06a which has been reverted.
Yes. If the 9425c591e06a was reverted, this patch could introduce regression.
Which fixing do you prefer? reverting 9425c591e06a or this patch? Then we
can suggest to Andrew to take it.


Regards
Yin, Fengwei