Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 3 Jul 2023 11:49:28 -0700
From:   Mike Kravetz <mike.kravetz@oracle.com>
To:     Yin Fengwei <fengwei.yin@intel.com>
Cc:     akpm@linux-foundation.org, willy@infradead.org,
        ackerleytng@google.com, linux-fsdevel@vger.kernel.org,
        linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        oliver.sang@intel.com
Subject: Re: [PATCH v2] readahead: Correct the start and size in
 ondemand_readahead()
Message-ID: <20230703184928.GB4378@monkey>
References: <20230628044303.1412624-1-fengwei.yin@intel.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230628044303.1412624-1-fengwei.yin@intel.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?5peIN0rTRekTBui02sj3Mtci0rB/ZyBCGnR7+bqXXrU0ioNYXPMB6lThuP8j?=
 =?us-ascii?Q?vH82NwXqCf4QLKCy/zqeuY5++zGiNieWKQYRv9BcF5HmwFvhJMH1ffGLuNcb?=
 =?us-ascii?Q?mCKAbtbb987fVMNr+IkoDy4Df/PsvDfi7VqwQTaKDJ/geHenr0cFjgSGeaU4?=
 =?us-ascii?Q?u8susv9L96jYbkZOTmIaomxauXH9ZZyysI2ons+/cIzJR6ZaFK3tqtDDFcv7?=
 =?us-ascii?Q?+kAEZevcuAWt/IhX0KPkjJHEIo0PNmJw0Sd4dhFPgos+TGUAUHKSF37dHLsl?=
 =?us-ascii?Q?K/wcz45j2lOt4zPpqEYbXP7irGVaefyiziQLZjq1EwFmGvWPw8Vgo6xHRdld?=
 =?us-ascii?Q?5+ao2v3CDj0vor5tRnzRLmzXhQkSFwhhvH5ljhZ+Afd0pyPAN44Tg6zV6qBn?=
 =?us-ascii?Q?YxSarkvvKxzluSH0uQVeZrGLqPD+Fd6dSxfKqWpQ6lVNEvMgxiHhQtUtYTYQ?=
 =?us-ascii?Q?X+UNJlHeQW3iKYiTO01JGntLIaaIjNVyHTOnABWgUwYtKw5mDDsx1j6FnnXJ?=
 =?us-ascii?Q?C1CAv4QO+xrRLxfuTTnZEcrMoXMTnji4EYtEH3KB0ezr16UEShB3LPSHS3+L?=
 =?us-ascii?Q?fR40lA7S1YEu2QZ+NBJLzaS3mifdJi+ETVX2SMSY2AUF7ehtQNq80VIcyIJO?=
 =?us-ascii?Q?WxXFsbalT/BdczKEeaa9I4/wySc+GVWR7qaGGA3h3gPljb+PYQ2VPQAbI/hL?=
 =?us-ascii?Q?lnqsJSUh7JEawjaAerHjpia+JpMaLIb2hHDtpkouNZPW+IU8kgyiWRIxswUp?=
 =?us-ascii?Q?GLAZLiSinPKha3pkp/SiiL6Ar2zIwYPO01ACsimm0NA9hxlZGw6io6FXMh3T?=
 =?us-ascii?Q?2QLi4+/X/CU4cJk888r8Q9j2GJI6N/oZxwX4dUmT+dNQ8P9tbAQZBFB1GxQs?=
 =?us-ascii?Q?6wxNQi5YkvqXJyIZfHAMDSZ3RbV2d0wpt/lu/epC/KhI7pHiz9d+N9siJ5In?=
 =?us-ascii?Q?otkTzcX/smCL0nu5e0b3M0TUj8NbhSFc079lAV2knx1XIzUN0V8fOKKaJIeB?=
 =?us-ascii?Q?WSPFrPBbH8sx2uYFBeGrL/FgbKe7it0f6BV+igA9yy2Nd9CoGwVGly7w1mg5?=
 =?us-ascii?Q?HKuQfANC+w9e8+SflnEkMJX3hkA00v7qEGHKgKWd9qLiCCnjVHH0xTgdvT8n?=
 =?us-ascii?Q?UwyPLT6y3rqQhiJOPLRdnvIlNmMg5vRprXp7+9cLsgux/cn4z35alDZE4eEq?=
 =?us-ascii?Q?q2u0hqdp0wEfOGm+F1crCdt/yqUlp5XEt9nVUUJxXk0rjVfBIhhbeNYk0KNu?=
 =?us-ascii?Q?TLgbgaCHVnDQQ9I2WvKAl+Ljh7PGqLl73Up23EwC/7lMs7G9/Vrl2r4kn6Yl?=
 =?us-ascii?Q?gLHlqgsSjYNZ1OLvOVgUtDWG44Cij0C+zcBE1jeMgCV+d3tspzOOy+os9P3c?=
 =?us-ascii?Q?ba6wG8/ggccxs59ilSlEz3JlMQu/c4rvpjkkEkAvVTiwGqfa/oBfHStRzFS5?=
 =?us-ascii?Q?77M355RxCfZM7oj9SeyHl0m96fzou7wRqWDWSi2pxjxOuwVmDRVTuga1MQR6?=
 =?us-ascii?Q?weYse1U4pp1gzcm/yzn6FIltU4EFVDvKAvF9ZshKDg2c7HO+EnY72jsT5UOs?=
 =?us-ascii?Q?VDd0Qezzvp4mweJb2OuAETdcTaeciKDk2fqVV9TE2Uc9BJ8irOUNDHLqBGBP?=
 =?us-ascii?Q?wA=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: =?us-ascii?Q?OmqikRl0A9+i8yo4kLfpn52AD2KxMw0LHX2c3dc07gxw3F1lgHPQnF6VSqPg?=
 =?us-ascii?Q?JsnYT+NlfSp+Mfmb5Ohpq0yaGLRPpdTujeinH9ESc7PvnjftWsEfoKvj7mVu?=
 =?us-ascii?Q?4bzUO/Y8DH/klCmnECx+qal9DrSNCUDKUr1rsZn4cwRXEEL66qEqCazQQ8Ra?=
 =?us-ascii?Q?vfyFh5swn8W/K9u0a0m5EyWZq4QEBFA/Zin1oLdBGN8/WKkVVRUkok2tKCYH?=
 =?us-ascii?Q?dk85mqXDYzwAlVodPxrxlksv1L+nGKC+Flg9iiDF39dg+zbCNP6j3KtpafId?=
 =?us-ascii?Q?kELMp16cHukb7MdYCC9oQ4OMgS5/MfBCxRfBzbZ2zD+BsMMYcJReQzsIvYwr?=
 =?us-ascii?Q?EeSAaHar55DQIuxeW1PUnlBZgWcVS2kY9cu8RlKzX0Emkb2IrMLN10j5sGf/?=
 =?us-ascii?Q?IOVmWw7WwKB8S6ObMuX/vGEKT/KPgkQkNlZBAImz1WYaO8aqrFErct3prLAZ?=
 =?us-ascii?Q?eYwdhnOIrDw4hqMmfbooS8dq994SkKxahskFer7UbnESkP7bccLS5U7v0GaD?=
 =?us-ascii?Q?xjUdZuDE8LecW2fVHL9WyVpJILCdSseeraDKwaiYHARapOIyXojb15cxD16H?=
 =?us-ascii?Q?fs/VmUE2oGgA9ebIxBdMrKS2fpXPmKIDkjJYtrM84yZ00aUUqZZAqdbtCASv?=
 =?us-ascii?Q?TzPXuncqwuD2yem3EAJKmBOL89eiXv+rxxrwaRohD+zOakqyf17grwkhTpQS?=
 =?us-ascii?Q?xbN98nXd0eNEzoxF4DRFyMWHoXJTyRsdaOLgABO6YsypTBallvqBoiu8yQBo?=
 =?us-ascii?Q?CPTIc+2J+yscNfyGYewpZdFSYisndoogkE7T8hXF8LZn12v3WifRfeXLmYoP?=
 =?us-ascii?Q?VGbL3zn3MCCDllBU5rrQLesFDaXn2cgcBydxab0mJNiqYijrsTV/UJ0yYlDM?=
 =?us-ascii?Q?04l8rLHruYvox9JZAn7CgP4/HhVtninop9cvvFa5dClGFe3B+uBBj+ARMmu4?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: fcc020ce-373b-4c34-fecf-08db7bf63c72
X-MS-Exchange-CrossTenant-AuthSource: BY5PR10MB4196.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Jul 2023 18:49:31.0366
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 32rNM/usWk32q+tMqFvwkBx+6Keoly2VLwcwCXeYYNuD6GwUiFlI2fnsRxoy6lCBoM4A9PRUaPkDQd4wBSyZKQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS7PR10MB5974
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.254,Aquarius:18.0.957,Hydra:6.0.591,FMLib:17.11.176.26
 definitions=2023-07-03_13,2023-06-30_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 bulkscore=0 suspectscore=0
 malwarescore=0 phishscore=0 mlxscore=0 spamscore=0 mlxlogscore=999
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2305260000
 definitions=main-2307030171
X-Proofpoint-GUID: EDyNAi7spJWkg5jfcmgDlql1C2osEMaF
X-Proofpoint-ORIG-GUID: EDyNAi7spJWkg5jfcmgDlql1C2osEMaF
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H5,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,
        T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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.

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.

Sorry for the delay in responding as I was traveling.
-- 
Mike Kravetz


> Reported-by: kernel test robot <oliver.sang@intel.com>
> Closes: https://lore.kernel.org/oe-lkp/202306211346.1e9ff03e-oliver.sang@intel.com
> Fixes: 9425c591e06a ("page cache: fix page_cache_next/prev_miss off by one")
> Signed-off-by: Yin Fengwei <fengwei.yin@intel.com>
> ---
> Changes from v1:
>   - only removing offset by one when there is no gaps found by
>     page_cache_next_miss()
>   - Update commit message to include the histogram of page order
>     after fix
> 
>  mm/readahead.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/readahead.c b/mm/readahead.c
> index 47afbca1d122..a93af773686f 100644
> --- a/mm/readahead.c
> +++ b/mm/readahead.c
> @@ -614,9 +614,17 @@ static void ondemand_readahead(struct readahead_control *ractl,
>  				max_pages);
>  		rcu_read_unlock();
>  
> -		if (!start || start - index > max_pages)
> +		if (!start || start - index - 1 > max_pages)
>  			return;
>  
> +		/*
> +		 * If no gaps in the range, page_cache_next_miss() returns
> +		 * index beyond range. Adjust it back to make sure
> +		 * ractl->_index is updated correctly later.
> +		 */
> +		if ((start - index - 1) == max_pages)
> +			start--;
> +
>  		ra->start = start;
>  		ra->size = start - index;	/* old async_size */
>  		ra->size += req_size;
> -- 
> 2.39.2
>