Hitting BUG_ON trap in read_pages() - : [PATCH v2] mm: Optimise put_pages_list()

Hitting BUG_ON trap in read_pages() - : [PATCH v2] mm: Optimise put_pages_list()

[Namjae Jeon]

Hi Matthew,

This patch is hitting BUG_ON trap in read_pages() when running
xfstests for cifs.
There seems to be a same issue with other filesystems using .readpages ?
Could you please take a look ?

[ 1007.298234] ------------[ cut here ]------------
[ 1007.298237] kernel BUG at mm/readahead.c:151!
[ 1007.298244] invalid opcode: 0000 [#1] PREEMPT SMP PTI
[ 1007.298248] CPU: 4 PID: 35188 Comm: fio Not tainted 5.15.0+ #2
[ 1007.298250] Hardware name: ASUSTeK COMPUTER INC. Z10PA-D8
Series/Z10PA-D8 Series, BIOS 3801 08/23/2019
[ 1007.298252] RIP: 0010:read_pages+0x247/0x250
[ 1007.298259] Code: 07 48 c1 e8 33 83 e0 07 83 f8 04 75 c4 48 8b 47
08 8b 40 68 83 e8 01 83 f8 01 77 b5 e8 b2 26 00 00 eb b4 e8 fb 25 00
00 eb ad <0f> 0b 0f 0b e8 10 0f 9b 00 0f 1f 44 00 00 55 48 89 e5 41 57
41 56
[ 1007.298261] RSP: 0018:ffffa3ae21ea7a98 EFLAGS: 00010287
[ 1007.298263] RAX: ffffcc5dc5e21c08 RBX: ffffa3ae21ea7c40 RCX: 0000000000000000
[ 1007.298265] RDX: ffffcc5dc5e21c00 RSI: 0000000000000000 RDI: ffffa3ae21ea7a98
[ 1007.298267] RBP: ffffa3ae21ea7af0 R08: 0000000000000000 R09: ffffa3ae21ea7960
[ 1007.298268] R10: ffff9474afb37598 R11: 0000000000000000 R12: ffffa3ae21ea7b38
[ 1007.298269] R13: 0000000000000000 R14: ffffffffc0de2220 R15: ffff94719dc4c328
[ 1007.298271] FS:  00007fc94d3f5880(0000) GS:ffff9474afb00000(0000)
knlGS:0000000000000000
[ 1007.298273] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 1007.298274] CR2: 00005635fb61c000 CR3: 000000013b2ac006 CR4: 00000000001706e0
[ 1007.298276] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 1007.298277] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 1007.298279] Call Trace:
[ 1007.298280]  <TASK>
[ 1007.298283]  page_cache_ra_unbounded+0x16b/0x230
[ 1007.298287]  do_page_cache_ra+0x3d/0x40
[ 1007.298290]  force_page_cache_ra+0x7c/0xb0
[ 1007.298293]  page_cache_sync_ra+0x3e/0xd0
[ 1007.298295]  filemap_get_pages+0xe5/0x770
[ 1007.298297]  ? __switch_to_asm+0x36/0x70
[ 1007.298303]  filemap_read+0xd0/0x3e0
[ 1007.298305]  ? preempt_count_add+0x74/0xc0
[ 1007.298310]  ? kvfree+0x28/0x30
[ 1007.298316]  ? cifs_aio_ctx_release+0xd1/0xe0 [cifs]
[ 1007.298362]  generic_file_read_iter+0xf0/0x160
[ 1007.298365]  cifs_strict_readv+0xea/0x100 [cifs]
[ 1007.298391]  new_sync_read+0x113/0x1a0
[ 1007.298395]  vfs_read+0xfe/0x1a0
[ 1007.298398]  ksys_read+0x67/0xe0
[ 1007.298401]  __x64_sys_read+0x1a/0x20
[ 1007.298404]  do_syscall_64+0x3b/0xc0
[ 1007.298408]  entry_SYSCALL_64_after_hwframe+0x44/0xae
[ 1007.298412] RIP: 0033:0x7fc956d1a36c

2021-10-08 4:21 GMT+09:00, Matthew Wilcox (Oracle) <willy@infradead.org>:
> Instead of calling put_page() one page at a time, pop pages off
> the list if their refcount was too high and pass the remainder to
> put_unref_page_list().  This should be a speed improvement, but I have
> no measurements to support that.  Current callers do not care about
> performance, but I hope to add some which do.
>
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
> v2:
>  - Handle compound pages (Mel)
>  - Comment why we don't need to handle PageLRU
>  - Added call to __ClearPageWaiters(), matching that in release_pages()
>
>  mm/swap.c | 23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/mm/swap.c b/mm/swap.c
> index af3cad4e5378..9f334d503fd2 100644
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -134,18 +134,27 @@ EXPORT_SYMBOL(__put_page);
>   * put_pages_list() - release a list of pages
>   * @pages: list of pages threaded on page->lru
>   *
> - * Release a list of pages which are strung together on page.lru.
> Currently
> - * used by read_cache_pages() and related error recovery code.
> + * Release a list of pages which are strung together on page.lru.
>   */
>  void put_pages_list(struct list_head *pages)
>  {
> -	while (!list_empty(pages)) {
> -		struct page *victim;
> +	struct page *page, *next;
>
> -		victim = lru_to_page(pages);
> -		list_del(&victim->lru);
> -		put_page(victim);
> +	list_for_each_entry_safe(page, next, pages, lru) {
> +		if (!put_page_testzero(page)) {
> +			list_del(&page->lru);
> +			continue;
> +		}
> +		if (PageHead(page)) {
> +			list_del(&page->lru);
> +			__put_compound_page(page);
> +			continue;
> +		}
> +		/* Cannot be PageLRU because it's passed to us using the lru */
> +		__ClearPageWaiters(page);
>  	}
> +
> +	free_unref_page_list(pages);
>  }
>  EXPORT_SYMBOL(put_pages_list);
>
> --
> 2.32.0
>
>

Re: Hitting BUG_ON trap in read_pages() - : [PATCH v2] mm: Optimise put_pages_list()

[Matthew Wilcox]

On Tue, Nov 09, 2021 at 07:45:47PM +0900, Namjae Jeon wrote:
> Hi Matthew,
> 
> This patch is hitting BUG_ON trap in read_pages() when running
> xfstests for cifs.
> There seems to be a same issue with other filesystems using .readpages ?

The real fix, of course, is to migrate away from using ->readpages ;-)
I think both 9p and nfs are going away this cycle.  CIFS really needs
to move to using the netfs interfaces.

> Could you please take a look ?

Please try this patch:

While free_unref_page_list() puts pages onto the CPU local LRU list, it
does not remove them from the list they were passed in on.  That makes
the list_head appear to be non-empty, and would lead to various corruption
problems if we didn't have an assertion that the list was empty.

Reinitialise the list after calling free_unref_page_list() to avoid
this problem.

Fixes: 988c69f1bc23 ("mm: optimise put_pages_list()")
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

diff --git a/mm/swap.c b/mm/swap.c
index 1841c24682f8..e8c9dc6d0377 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -156,6 +156,7 @@ void put_pages_list(struct list_head *pages)
 	}
 
 	free_unref_page_list(pages);
+	INIT_LIST_HEAD(pages);
 }
 EXPORT_SYMBOL(put_pages_list);
 

Re: Hitting BUG_ON trap in read_pages() - : [PATCH v2] mm: Optimise put_pages_list()

[Namjae Jeon]

2021-11-09 22:34 GMT+09:00, Matthew Wilcox <willy@infradead.org>:
> On Tue, Nov 09, 2021 at 07:45:47PM +0900, Namjae Jeon wrote:
>> Hi Matthew,
>>
>> This patch is hitting BUG_ON trap in read_pages() when running
>> xfstests for cifs.
>> There seems to be a same issue with other filesystems using .readpages ?
>
> The real fix, of course, is to migrate away from using ->readpages ;-)
> I think both 9p and nfs are going away this cycle.  CIFS really needs
> to move to using the netfs interfaces.
Okay.
>
>> Could you please take a look ?
>
> Please try this patch:
Work fine.
>
> While free_unref_page_list() puts pages onto the CPU local LRU list, it
> does not remove them from the list they were passed in on.  That makes
> the list_head appear to be non-empty, and would lead to various corruption
> problems if we didn't have an assertion that the list was empty.
>
> Reinitialise the list after calling free_unref_page_list() to avoid
> this problem.
>
> Fixes: 988c69f1bc23 ("mm: optimise put_pages_list()")
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Tested-by: Namjae Jeon <linkinjeon@kernel.org>

Thanks!
>
> diff --git a/mm/swap.c b/mm/swap.c
> index 1841c24682f8..e8c9dc6d0377 100644
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -156,6 +156,7 @@ void put_pages_list(struct list_head *pages)
>  	}
>
>  	free_unref_page_list(pages);
> +	INIT_LIST_HEAD(pages);
>  }
>  EXPORT_SYMBOL(put_pages_list);
>
>