Re: [PATCH v2 1/8] mm/khugepaged: separate out windy folio logic from collapse_file

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Critical] mm/khugepaged: unreleased xa_lock in
  prepare_collapse_file_folio()
- [Low] mm/khugepaged: unused file and end fields in struct
  collapse_file_state
--

commit 4417d51ddf77d1023dd61cf9f5007c13774865df
Author: Pedro Falcato <pfalcato@suse.de>

mm/khugepaged: separate out windy folio logic from collapse_file

This commit factors the per-folio preparation and locking logic out of
collapse_file() into a new prepare_collapse_file_folio() helper and
introduces struct collapse_file_state to pass state.

> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index 24eb8281ea3cd..0fe19acf470b7 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -2211,6 +2211,105 @@ static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff)
>  	i_mmap_unlock_read(mapping);
>  }
>  
> +struct collapse_file_state {
> +	/* Inputs */
> +	struct address_space *mapping;
> +	struct file *file;
> +	struct xa_state *xas;
> +	/* folio is in-out; preparing might modify it */
> +	struct folio *folio;
> +	/* collapse end index */
> +	const pgoff_t end;
> +	const bool is_shmem : 1;
> +	bool do_readahead : 1;
> +	bool readahead_done : 1;
> +};

[Severity: Low]
This isn't a bug, but the file and end fields in struct collapse_file_state
are initialized in collapse_file() and never read anywhere.

[ ... ]

> +static enum scan_result
> +prepare_collapse_file_folio(pgoff_t index, struct collapse_file_state *state)
> +{
> +	struct address_space *mapping = state->mapping;
> +	enum scan_result result = SCAN_SUCCEED;
> +	const bool is_shmem = state->is_shmem;
> +	struct folio *folio = state->folio;
> +
> +	if (is_shmem) {

[ ... ]

> +	} else {	/* !is_shmem */
> +		if (!folio || xa_is_value(folio)) {
> +			state->do_readahead = !state->readahead_done;
> +			result = SCAN_FAIL;
> +			goto xa_unlocked;

[ ... ]

> +xa_locked:
> +	xas_unlock_irq(state->xas);
> +xa_unlocked:
> +	state->folio = folio;
> +	return result;
> +}

[Severity: Critical]
Does jumping to xa_unlocked instead of xa_locked here return with
state->xas still locked and IRQs disabled when a non-shmem file range has a
missing or shadow page-cache entry?

In collapse_file(), the !SCAN_SUCCEED path also jumps directly to
xa_unlocked and proceeds to rollback:, where it sleeps while atomic and
deadlocks trying to re-acquire mapping->i_pages.xa_lock during readahead
(or returns with xa_lock held and IRQs disabled if readahead already ran).
This regression is later resolved in the series by commit "mm/khugepaged:
factor out page cache folio reading", but breaks bisection here.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111175121.1328383-1-pfalcato@suse.de?part=1