Re: BUG_ON() in workingset_node_shadows_dec() triggers

[Jan Kara <jack@suse.cz>]

On Wed 05-10-16 11:25:34, Johannes Weiner wrote:
> From 5e948543a814183f04c30b072707300a94deb6c7 Mon Sep 17 00:00:00 2001
> From: Johannes Weiner <hannes@cmpxchg.org>
> Date: Tue, 4 Oct 2016 22:02:08 +0200
> Subject: [PATCH] mm: filemap: don't plant shadow entries without radix
>  tree node
> 
> When the underflow checks were added to workingset_node_shadow_dec(),
> they triggered immediately:

FWIW the patch looks good to me. So you can add:

Reviewed-by: Jan Kara <jack@suse.cz>

And I agree that the games workingset code plays with node->count are
really fragile so it would be better to change that.
								Honza

> 
>   kernel BUG at ./include/linux/swap.h:276!
>   invalid opcode: 0000 [#1] SMP
>   Modules linked in: isofs usb_storage fuse xt_CHECKSUM ipt_MASQUERADE
> nf_nat_masquerade_ipv4 tun nf_conntrack_netbios_ns
> nf_conntrack_broadcast ip6t_REJECT nf_reject_ipv6
>    soundcore wmi acpi_als pinctrl_sunrisepoint kfifo_buf tpm_tis
> industrialio acpi_pad pinctrl_intel tpm_tis_core tpm nfsd auth_rpcgss
> nfs_acl lockd grace sunrpc dm_crypt
>   CPU: 0 PID: 20929 Comm: blkid Not tainted 4.8.0-rc8-00087-gbe67d60ba944 #1
>   Hardware name: System manufacturer System Product Name/Z170-K, BIOS
> 1803 05/06/2016
>   task: ffff8faa93ecd940 task.stack: ffff8faa7f478000
>   RIP: page_cache_tree_insert+0xf1/0x100
>   RSP: 0018:ffff8faa7f47bab0  EFLAGS: 00010046
>   RAX: 0000000000000001 RBX: ffff8faadfaf8c18 RCX: ffff8fa8737b5488
>   RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffff8fa8737b4b48
>   RBP: ffff8faa7f47bae8 R08: 0000000000000012 R09: ffff8fa8737b54b0
>   R10: 0000000000000040 R11: ffff8fa8737b54b0 R12: ffffea000b1ad580
>   R13: 0000000000000000 R14: ffff8faa7f47bb48 R15: ffffea000b1ad580
>   FS:  00007ffba3a61780(0000) GS:ffff8faaf6c00000(0000) knlGS:0000000000000000
>   CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>   CR2: 00007ffba31a5430 CR3: 00000002c6d40000 CR4: 00000000003406f0
>   DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>   DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>   Call Trace:
>     __add_to_page_cache_locked+0x12e/0x270
>     add_to_page_cache_lru+0x4e/0xe0
>     mpage_readpages+0x112/0x1d0
>     blkdev_readpages+0x1d/0x20
>     __do_page_cache_readahead+0x1ad/0x290
>     force_page_cache_readahead+0xaa/0x100
>     page_cache_sync_readahead+0x3f/0x50
>     generic_file_read_iter+0x5af/0x740
>     blkdev_read_iter+0x35/0x40
>     __vfs_read+0xe1/0x130
>     vfs_read+0x96/0x130
>     SyS_read+0x55/0xc0
>     entry_SYSCALL_64_fastpath+0x13/0x8f
>   Code: 03 00 48 8b 5d d8 65 48 33 1c 25 28 00 00 00 44 89 e8 75 19 48
> 83 c4 18 5b 41 5c 41 5d 41 5e 5d c3 0f 0b 41 bd ef ff ff ff eb d7 <0f>
> 0b e8 88 68 ef ff 0f 1f 84 00
>   RIP  page_cache_tree_insert+0xf1/0x100
> 
> This is a long-standing bug in the way shadow entries are accounted in
> the radix tree nodes. The shrinker needs to know when radix tree nodes
> contain only shadow entries, no pages, so node->count is split in half
> to count shadows in the upper bits and pages in the lower bits.
> 
> Unfortunately, the radix tree implementation doesn't know of this and
> assumes all entries are in node->count. When there is a shadow entry
> directly in root->rnode and the tree is later extended, the radix tree
> implementation will copy that entry into the new node and and bump its
> node->count, i.e. increases the page count bits. Once the shadow gets
> removed and we subtract from the upper counter, node->count underflows
> and triggers the warning. Afterwards, without node->count reaching 0
> again, the radix tree node is leaked.
> 
> Limit shadow entries to when we have actual radix tree nodes and can
> count them properly. That means we lose the ability to detect refaults
> from files that had only the first page faulted in at eviction time.
> 
> Fixes: 449dd6984d0e ("mm: keep page cache radix tree nodes in check")
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
> ---
>  include/linux/radix-tree.h |  6 +++---
>  lib/radix-tree.c           | 14 +++-----------
>  mm/filemap.c               | 46 ++++++++++++++++++++++++++++++----------------
>  3 files changed, 36 insertions(+), 30 deletions(-)
> 
> diff --git a/include/linux/radix-tree.h b/include/linux/radix-tree.h
> index 4c45105dece3..52b97db93830 100644
> --- a/include/linux/radix-tree.h
> +++ b/include/linux/radix-tree.h
> @@ -280,9 +280,9 @@ bool __radix_tree_delete_node(struct radix_tree_root *root,
>  			      struct radix_tree_node *node);
>  void *radix_tree_delete_item(struct radix_tree_root *, unsigned long, void *);
>  void *radix_tree_delete(struct radix_tree_root *, unsigned long);
> -struct radix_tree_node *radix_tree_replace_clear_tags(
> -				struct radix_tree_root *root,
> -				unsigned long index, void *entry);
> +void radix_tree_clear_tags(struct radix_tree_root *root,
> +			   struct radix_tree_node *node,
> +			   void **slot);
>  unsigned int radix_tree_gang_lookup(struct radix_tree_root *root,
>  			void **results, unsigned long first_index,
>  			unsigned int max_items);
> diff --git a/lib/radix-tree.c b/lib/radix-tree.c
> index 91f0727e3cad..8e6d552c40dd 100644
> --- a/lib/radix-tree.c
> +++ b/lib/radix-tree.c
> @@ -1583,15 +1583,10 @@ void *radix_tree_delete(struct radix_tree_root *root, unsigned long index)
>  }
>  EXPORT_SYMBOL(radix_tree_delete);
>  
> -struct radix_tree_node *radix_tree_replace_clear_tags(
> -			struct radix_tree_root *root,
> -			unsigned long index, void *entry)
> +void radix_tree_clear_tags(struct radix_tree_root *root,
> +			   struct radix_tree_node *node,
> +			   void **slot)
>  {
> -	struct radix_tree_node *node;
> -	void **slot;
> -
> -	__radix_tree_lookup(root, index, &node, &slot);
> -
>  	if (node) {
>  		unsigned int tag, offset = get_slot_offset(node, slot);
>  		for (tag = 0; tag < RADIX_TREE_MAX_TAGS; tag++)
> @@ -1600,9 +1595,6 @@ struct radix_tree_node *radix_tree_replace_clear_tags(
>  		/* Clear root node tags */
>  		root->gfp_mask &= __GFP_BITS_MASK;
>  	}
> -
> -	radix_tree_replace_slot(slot, entry);
> -	return node;
>  }
>  
>  /**
> diff --git a/mm/filemap.c b/mm/filemap.c
> index c17395825650..5bcadb6471bf 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -169,33 +169,35 @@ static int page_cache_tree_insert(struct address_space *mapping,
>  static void page_cache_tree_delete(struct address_space *mapping,
>  				   struct page *page, void *shadow)
>  {
> -	struct radix_tree_node *node;
>  	int i, nr = PageHuge(page) ? 1 : hpage_nr_pages(page);
>  
>  	VM_BUG_ON_PAGE(!PageLocked(page), page);
>  	VM_BUG_ON_PAGE(PageTail(page), page);
>  	VM_BUG_ON_PAGE(nr != 1 && shadow, page);
>  
> -	if (shadow) {
> -		mapping->nrexceptional += nr;
> -		/*
> -		 * Make sure the nrexceptional update is committed before
> -		 * the nrpages update so that final truncate racing
> -		 * with reclaim does not see both counters 0 at the
> -		 * same time and miss a shadow entry.
> -		 */
> -		smp_wmb();
> -	}
> -	mapping->nrpages -= nr;
> -
>  	for (i = 0; i < nr; i++) {
> -		node = radix_tree_replace_clear_tags(&mapping->page_tree,
> -				page->index + i, shadow);
> +		struct radix_tree_node *node;
> +		void **slot;
> +
> +		__radix_tree_lookup(&mapping->page_tree, page->index + i,
> +				    &node, &slot);
> +
> +		radix_tree_clear_tags(&mapping->page_tree, node, slot);
> +
>  		if (!node) {
>  			VM_BUG_ON_PAGE(nr != 1, page);
> -			return;
> +			/*
> +			 * We need a node to properly account shadow
> +			 * entries. Don't plant any without. XXX
> +			 */
> +			shadow = NULL;
>  		}
>  
> +		radix_tree_replace_slot(slot, shadow);
> +
> +		if (!node)
> +			break;
> +
>  		workingset_node_pages_dec(node);
>  		if (shadow)
>  			workingset_node_shadows_inc(node);
> @@ -219,6 +221,18 @@ static void page_cache_tree_delete(struct address_space *mapping,
>  					&node->private_list);
>  		}
>  	}
> +
> +	if (shadow) {
> +		mapping->nrexceptional += nr;
> +		/*
> +		 * Make sure the nrexceptional update is committed before
> +		 * the nrpages update so that final truncate racing
> +		 * with reclaim does not see both counters 0 at the
> +		 * same time and miss a shadow entry.
> +		 */
> +		smp_wmb();
> +	}
> +	mapping->nrpages -= nr;
>  }
>  
>  /*
> -- 
> 2.10.0
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR