Re: [PATCH v4 2/2] mm, thp: bail out early in collapse_file for writeback page

[Matthew Wilcox <willy@infradead.org>]

On Mon, Oct 11, 2021 at 10:22:41AM +0800, Rongwei Wang wrote:
> Currently collapse_file does not explicitly check PG_writeback, instead,
> page_has_private and try_to_release_page are used to filter writeback
> pages. This does not work for xfs with blocksize equal to or larger
> than pagesize, because in such case xfs has no page->private.
> 
> This makes collapse_file bail out early for writeback page. Otherwise,
> xfs end_page_writeback will panic as follows.
> 
> Fixes: eb6ecbed0aa2 ("mm, thp: relax the VM_DENYWRITE constraint on file-backed THPs")

This is the wrong Fixes line.  This same bug exists earlier than this.
Your testing may not show it before then, but if you mmap something
that isn't an executable, you can provoke it.  It should be:

Fixes: 99cb0dbd47a1 ("mm,thp: add read-only THP support for (non-shmem) FS")

(unless there's something I'm missing?)

Also, this should surely have a Cc: stable@vger.kernel.org in the
tags section?  It's a user-visible bug, we want it backported.

> Suggested-by: Yang Shi <shy828301@gmail.com>
> Signed-off-by: Xu Yu <xuyu@linux.alibaba.com>
> Signed-off-by: Rongwei Wang <rongwei.wang@linux.alibaba.com>
> Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> Reviewed-by: Yang Shi <shy828301@gmail.com>
> ---
>  mm/khugepaged.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index 045cc579f724..48de4e1b0783 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -1763,6 +1763,10 @@ static void collapse_file(struct mm_struct *mm,
>  				filemap_flush(mapping);
>  				result = SCAN_FAIL;
>  				goto xa_unlocked;
> +			} else if (PageWriteback(page)) {
> +				xas_unlock_irq(&xas);
> +				result = SCAN_FAIL;
> +				goto xa_unlocked;
>  			} else if (trylock_page(page)) {
>  				get_page(page);
>  				xas_unlock_irq(&xas);
> @@ -1798,7 +1802,8 @@ static void collapse_file(struct mm_struct *mm,
>  			goto out_unlock;
>  		}
>  
> -		if (!is_shmem && PageDirty(page)) {
> +		if (!is_shmem && (PageDirty(page) ||
> +				  PageWriteback(page))) {
>  			/*
>  			 * khugepaged only works on read-only fd, so this
>  			 * page is dirty because it hasn't been flushed
> -- 
> 2.27.0
>