Re: [PATCH 1/8] mm: implement cleancache

[Matthew Wilcox <willy@infradead.org>]

On Thu, Oct 09, 2025 at 06:19:44PM -0700, Suren Baghdasaryan wrote:
> +	/*
> +	 * 99% of the time, we don't need to flush the cleancache on the bdev.
> +	 * But, for the strange corners, lets be cautious
> +	 */
> +	cleancache_invalidate_inode(mapping, mapping->host);

Why do we need to pass in both address_space and inode?

> +/*
> + * Backend API
> + *
> + * Cleancache does not touch page reference. Page refcount should be 1 when
> + * page is placed or returned into cleancache and pages obtained from
> + * cleancache will also have their refcount at 1.

I don't like these references to page refcount.  Surely you mean folio
refcount?

> +	help
> +	  Cleancache can be thought of as a page-granularity victim cache
> +	  for clean pages that the kernel's pageframe replacement algorithm
> +	  (PFRA) would like to keep around, but can't since there isn't enough

PFRA seems to be an acronym you've just made up.  Why?

> +struct cleancache_inode {
> +	struct inode *inode;
> +	struct hlist_node hash;
> +	refcount_t ref_count;
> +	struct xarray folios; /* protected by folios.xa_lock */

This is a pointless comment.  All xarrays are protected by their own
xa_lock.

> +static DEFINE_IDR(fs_idr);

No.  The IDR is deprecated.  Use an allocating XArray.

> +/*
> + * Folio attributes:
> + *	folio->_mapcount - pool_id
> + *	folio->mapping - ccinode reference or NULL if folio is unused
> + *	folio->index - file offset

No.  Don't reuse fields for something entirely different.  Put a
properly named field in the union.

> +static void folio_attachment(struct folio *folio, struct cleancache_inode **ccinode,

Unnecessarily long line