Re: [PATCH 2/8] mm/khugepaged: factor out page cache folio reading

[Lorenzo Stoakes (ARM)]

On Mon, 20 Jul 2026 15:29:07 +0100, Pedro Falcato <pfalcato@xxxxxxx> wrote:
> The two mechanisms (shmem, filemap) are quite similar and only change in
> subtleties. There is no need to have two variants. Factor this out into
> its own separate function, shared by both paths.

I think this commit message could have some more detail, it's difficult to
figure out what's new, what's moved, etc. so mentioning that and the broader
purpose of why you're doing this (iteratively improving file collapse in
khugepaged) would be nice.

Also there are multiple subtleties in this patch as noted below, none of which
you've described here.

You should describe every bit of logic you're changing compared to the original
and why that's OK.

Or ideally really, separate out the mechnical changes from the logical ones.

> 
> Signed-off-by: Pedro Falcato <pfalcato@xxxxxxx>

It's a little tricky to figure out what's moved, what's new, etc. don't be
afraid to break out into even more commits to help reviewers :)

>
>
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index d4de507ac001..4cc6917a55c7 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -2230,42 +2230,63 @@ struct collapse_file_state {
>  	unsigned int is_shmem : 1;
>  };
>  

It'd be nice to add some comments to these bits!

I noticed that you add them later in the series but it'd be good to add them
earlier and build them out as you go.

> +static struct folio *collapse_read_folio(pgoff_t index, struct collapse_file_state *state)

90 chars here be nice to try to stick to ~80 :) simliar comment to last patch.

Also the name seems to imply you're _always_ reading which isn't the case.

Not sure we need the collapse_ either. Something like 'maybe_read_folio()' with
the state implying what it's used for?

> +{
> +	const bool may_bring_uptodate = state->is_shmem;

It's nice to make this self-documenting, but we could make life easier for
ourselves and avoid the (apparent) inconsistentcy of using this and
state->is_shmem separately by just doing the full check here:

	const bool bring_uptodate = folio && state->is_shmem &&
				    !folio_test_uptodate(folio);

(putting the folio decl before it ofc)

> +	struct folio *folio = state->folio;
> +
> +	if (!folio || xa_is_value(folio) ||
> +	   (may_bring_uptodate && !folio_test_uptodate(folio))) {

This X or Y or (A and B) is confusing, separating out bring_uptodate makes it
easier.

Can make this a guard clause like:

	/* No reason to read the folio. */
	if (folio && !xa_is_value(folio) && !bring_uptodate)
		return folio;

Which becomes its own form of self-documentation.

But I'm concerned whether this is correct.

For the shmem case you're going from (xa_is_value(folio) ||
!folio_test_uptodate(folio)) to (!folio || xa_is_value(folio) ||
!folio_test_uptodate(folio).

Again you do need to justify these changes somewhere.

> +		xas_unlock_irq(state->xas);

Be nice to have a comment as to why we're unlocking this.

> +		if (state->is_shmem) {
> +			/* swap in or instantiate fallocated page */

I know it was in the original code but this comment feels superfluous.

> +			if (shmem_get_folio(state->mapping->host, index, 0,
> +					    &folio, SGP_NOALLOC))
> +				return NULL;

Looking through the logic I see that this returns NULL which gets checked first
below and results in SCAN_FAIL, but it's not obvious when first reading the
patch :)

Argues for more info in commit msg + separation of patches.

> [ ... skip 16 lines ... ]
>  	enum scan_result result = SCAN_SUCCEED;
>  	const int is_shmem = state->is_shmem;
> -	struct folio *folio = state->folio;
> +	struct folio *folio;
> +
> +	folio = collapse_read_folio(index, state);

Again would be nice to have index in state :)

> +	if (!folio)
> +		return SCAN_FAIL;

Previously we always did the LRU drain regardless of whether shmem_get_folio()
or page_cache_sync_readahead() failed or not, now you're not doing that.

If an intentional change, you should explain why.

It'd also be nice to separate out mechnical moves from logic changes, as it's
hard to keep track of the new stuff you're doing vs. the stuff you're just moving around.

> +	if (folio != state->folio) {

Is this always the case when the operation succeeds?

You're going from always doing LRU drain and exit early to - only if folio !=
state->folio which is a logic change AFAICT.

> +		/*
> +		 * collapse_read_folio() got a new folio. This folio is locked
> +		 * and ref'd up. Nothing tricky (dirty, writeback, etc) can
> +		 * happen, so bail now. But before that, drain the local LRU
> +		 * add batch. Otherwise, it's very possible folio_isolate_lru()
> +		 * will not succeed.

Hmm :)

I feel we could collapse this into something more succinct like:

	/*
	 * We read a new folio which is now locked and pinned. Drain so
         * folio_isolate_lru() sees it and bail.
	 */

> +		 */
> +		lru_add_drain();

This is now out of order compared to the code it replaces for the !is_shmem
case. Is that OK?

Another thing that'd be nice to cover in the commit msg, it does need to be
justified.

> [ ... skip 31 lines ... ]
> -			goto xa_unlocked;
> -		} else if (folio_test_dirty(folio)) {
> +		if (folio_test_dirty(folio)) {
>  			/*
>  			 * This page is dirty because it hasn't
>  			 * been flushed since first write.

Th resulting code in prepare_collapse_file_folio() at this point is pretty
horrible, I realise it might be an interim state but I'm sure it could be done
better, for instance separating out the is_shmem vs. !is_shmem logic into helper
functions etc.

Let's definitely try to split this up into more patches so it's easier to follow
on respin please!

Thanks, Lorenzo

-- 
Lorenzo Stoakes (ARM) <ljs@xxxxxxxxxx>