Re: [PATCH v2] khugepaged: hold invalidate_lock across collapse_file() readahead

[Matthew Wilcox <willy@infradead.org>]

On Sun, Sep 13, 2026 at 11:36:44PM +0700, Nguyen Ngoc Thang wrote:
> @@ -2271,6 +2272,15 @@ static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr,
>  	if (result != SCAN_SUCCEED)
>  		goto out;
>  
> +	/*
> +	 * Take invalidate_lock before any folio lock: the readahead below
> +	 * needs it, and truncate holds it while waiting on folio locks.
> +	 */
> +	if (!is_shmem) {
> +		filemap_invalidate_lock_shared(mapping);
> +		need_unlock = true;
> +	}

I'm not a fan of all this surplus commentary.  And what happens if e
simultaeneously truncate a shmem file and collapse it at the same time?
I know it doesn't use the invalidate lock, but does it go wrong in some
other way?

>  		} else {	/* !is_shmem */
>  			if (!folio || xa_is_value(folio)) {
> +				DEFINE_READAHEAD(ractl, file, &file->f_ra,
> +						  mapping, index);
> +				pgoff_t eof = DIV_ROUND_UP(i_size_read(mapping->host),
> +							    PAGE_SIZE);

Not a fan of the overly long line.

				pgoff_t eof;

				eof = DIV_ROUND_UP(i_size_read(mapping->host),
						PAGE_SIZE);

or we could cache mapping->host in a variable called 'inode'.  We use it
in four places, so that might be best.

>  				xas_unlock_irq(&xas);
> -				page_cache_sync_readahead(mapping, &file->f_ra,
> -							  file, index,
> -							  end - index);
> +				/*
> +				 * invalidate_lock held above; don't retake it.
> +				 * page_cache_ra_unbounded(), unlike the readahead
> +				 * helper this replaces, does not clamp to EOF.
> +				 */

Again, remove this comment.  This explains why we're making the change
in the way that we are and adds absolutely no value to the reader of the
new file.