Re: [PATCH] mm/filemap: clear page cache marks once when removing a folio

[Jan Kara]

On Fri 30-10-26 01:52:38, Qiliang Yuan wrote:
> xas_store() clears the marks of an entry before it replaces the entry
> with NULL.
> 
> page_cache_delete() calls xas_init_marks() after xas_store() whether it
> stored a shadow entry or NULL, so a folio removed without a shadow, as
> POSIX_FADV_DONTNEED and drop_caches do, has its marks cleared twice.
> With 4K folios that is about 6% of the time it takes to drop clean page
> cache.
> 
> Call xas_init_marks() only after storing a shadow entry, whose marks
> xas_store() leaves alone. Dropping 8 GiB of 4K folios, median of five
> runs, averaged over two boots of each kernel, in a 32 vCPU VM:
> 
>                           before    after
>   POSIX_FADV_DONTNEED    1517 ms  1423 ms
>   drop_caches            1515 ms  1422 ms
> 
> Signed-off-by: Qiliang Yuan <odys.yuan@xxxxxxxxx>

Correct but please add a short explanation before the condition like:

	/* If xas_store() didn't clear marks, do it now */

With that feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

> ---
>  mm/filemap.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/filemap.c b/mm/filemap.c
> index b74bc1e5015c6..a0c959e74533e 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -140,7 +140,8 @@ static void page_cache_delete(struct address_space *mapping,
>  	VM_BUG_ON_FOLIO(!folio_test_locked(folio), folio);
>  
>  	xas_store(&xas, shadow);
> -	xas_init_marks(&xas);
> +	if (shadow)
> +		xas_init_marks(&xas);
>  
>  	folio->mapping = NULL;
>  	/* Leave folio->index set: truncation lookup relies upon it */
> 
> ---
> base-commit: d89e1d186ec9271bf78e62771779e9fc03bf0fc3
> change-id: 20261030-bug-mm-filemap-init-marks-once-b83c6aa67bab
> 
> Best regards,
> -- 
> Qiliang Yuan <odys.yuan@xxxxxxxxx>
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR