Re: [RFC PATCH 1/3] mm: refactor rmap_walk_file() to separate out traversal logic

[Matthew Wilcox <willy@infradead.org>]

On Wed, Jan 08, 2025 at 04:18:40PM +0000, Lorenzo Stoakes wrote:
> +/*
> + * rmap_walk_file - do something to file page using the object-based rmap method
> + * @folio: the folio to be handled
> + * @rwc: control variable according to each walk type
> + * @locked: caller holds relevant rmap lock
> + *
> + * Find all the mappings of a folio using the mapping pointer and the vma chains
> + * contained in the address_space struct it points to.
> + */
> +static void rmap_walk_file(struct folio *folio,
> +		struct rmap_walk_control *rwc, bool locked)
> +{
> +	struct address_space *mapping = folio_mapping(folio);

I'm unconvinced this shouldn't be just folio->mapping.  On the face of
it, we're saying that we're walking a file, and file folios just want
to use folio->mapping.  But let's dig a little deeper.

The folio passed in is locked, so it can't be changed during this call.
In folio_mapping(), folio_test_slab() is guaranteed untrue.
folio_test_swapcache() doesn't seem likely to be true either; unless
it's shmem, it can't be in the swapcache, and if it's shmem and in the
swap cache, it can't be mapped to userspace (they're swizzled back from
the swapcache to the pagecache before being mapped).  And then the
check for PAGE_MAPPING_FLAGS is guaranteed to be untrue (we know it's
not anon/ksm/movable).  So I think this should just be folio->mapping.

> +	/*
> +	 * The page lock not only makes sure that page->mapping cannot
> +	 * suddenly be NULLified by truncation, it makes sure that the
> +	 * structure at mapping cannot be freed and reused yet,
> +	 * so we can safely take mapping->i_mmap_rwsem.
> +	 */

I know you only moved this comment, but please fix it to refer to
folios, not pages.

> +	VM_BUG_ON_FOLIO(!folio_test_locked(folio), folio);
> +
> +	if (!mapping)
> +		return;

Maybe make this a WARN_ON_ONCE?

> +	__rmap_walk_file(folio, mapping, folio_pgoff(folio),
> +			 folio_nr_pages(folio), rwc, locked);

folio_pgoff() can go too.  Just use folio->index.