Re: [PATCH] xfs: more lockdep whackamole with kmem_alloc*

[Brian Foster <bfoster@redhat.com>]

On Tue, May 26, 2020 at 09:34:57AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Dave Airlie reported the following lockdep complaint:
> 
> >  ======================================================
> >  WARNING: possible circular locking dependency detected
> >  5.7.0-0.rc5.20200515git1ae7efb38854.1.fc33.x86_64 #1 Not tainted
> >  ------------------------------------------------------
> >  kswapd0/159 is trying to acquire lock:
> >  ffff9b38d01a4470 (&xfs_nondir_ilock_class){++++}-{3:3},
> >  at: xfs_ilock+0xde/0x2c0 [xfs]
> >
> >  but task is already holding lock:
> >  ffffffffbbb8bd00 (fs_reclaim){+.+.}-{0:0}, at:
> >  __fs_reclaim_acquire+0x5/0x30
> >
> >  which lock already depends on the new lock.
> >
> >
> >  the existing dependency chain (in reverse order) is:
> >
> >  -> #1 (fs_reclaim){+.+.}-{0:0}:
> >         fs_reclaim_acquire+0x34/0x40
> >         __kmalloc+0x4f/0x270
> >         kmem_alloc+0x93/0x1d0 [xfs]
> >         kmem_alloc_large+0x4c/0x130 [xfs]
> >         xfs_attr_copy_value+0x74/0xa0 [xfs]
> >         xfs_attr_get+0x9d/0xc0 [xfs]
> >         xfs_get_acl+0xb6/0x200 [xfs]
> >         get_acl+0x81/0x160
> >         posix_acl_xattr_get+0x3f/0xd0
> >         vfs_getxattr+0x148/0x170
> >         getxattr+0xa7/0x240
> >         path_getxattr+0x52/0x80
> >         do_syscall_64+0x5c/0xa0
> >         entry_SYSCALL_64_after_hwframe+0x49/0xb3
> >
> >  -> #0 (&xfs_nondir_ilock_class){++++}-{3:3}:
> >         __lock_acquire+0x1257/0x20d0
> >         lock_acquire+0xb0/0x310
> >         down_write_nested+0x49/0x120
> >         xfs_ilock+0xde/0x2c0 [xfs]
> >         xfs_reclaim_inode+0x3f/0x400 [xfs]
> >         xfs_reclaim_inodes_ag+0x20b/0x410 [xfs]
> >         xfs_reclaim_inodes_nr+0x31/0x40 [xfs]
> >         super_cache_scan+0x190/0x1e0
> >         do_shrink_slab+0x184/0x420
> >         shrink_slab+0x182/0x290
> >         shrink_node+0x174/0x680
> >         balance_pgdat+0x2d0/0x5f0
> >         kswapd+0x21f/0x510
> >         kthread+0x131/0x150
> >         ret_from_fork+0x3a/0x50
> >
> >  other info that might help us debug this:
> >
> >   Possible unsafe locking scenario:
> >
> >         CPU0                    CPU1
> >         ----                    ----
> >    lock(fs_reclaim);
> >                                 lock(&xfs_nondir_ilock_class);
> >                                 lock(fs_reclaim);
> >    lock(&xfs_nondir_ilock_class);
> >
> >   *** DEADLOCK ***
> >
> >  4 locks held by kswapd0/159:
> >   #0: ffffffffbbb8bd00 (fs_reclaim){+.+.}-{0:0}, at:
> >  __fs_reclaim_acquire+0x5/0x30
> >   #1: ffffffffbbb7cef8 (shrinker_rwsem){++++}-{3:3}, at:
> >  shrink_slab+0x115/0x290
> >   #2: ffff9b39f07a50e8
> >  (&type->s_umount_key#56){++++}-{3:3}, at: super_cache_scan+0x38/0x1e0
> >   #3: ffff9b39f077f258
> >  (&pag->pag_ici_reclaim_lock){+.+.}-{3:3}, at:
> >  xfs_reclaim_inodes_ag+0x82/0x410 [xfs]
> 
> This is a known false positive because inodes cannot simultaneously be
> getting reclaimed and the target of a getxattr operation, but lockdep
> doesn't know that.  We can (selectively) shut up lockdep until either
> it gets smarter or we change inode reclaim not to require the ILOCK by
> applying a stupid GFP_NOLOCKDEP bandaid.
> 
> Reported-by: Dave Airlie <airlied@gmail.com>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> Tested-by: Dave Airlie <airlied@gmail.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/kmem.h                 |    6 +++++-
>  fs/xfs/libxfs/xfs_attr_leaf.c |    2 +-
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/kmem.h b/fs/xfs/kmem.h
> index fc87ea9f6843..34cbcfde9228 100644
> --- a/fs/xfs/kmem.h
> +++ b/fs/xfs/kmem.h
> @@ -19,6 +19,7 @@ typedef unsigned __bitwise xfs_km_flags_t;
>  #define KM_NOFS		((__force xfs_km_flags_t)0x0004u)
>  #define KM_MAYFAIL	((__force xfs_km_flags_t)0x0008u)
>  #define KM_ZERO		((__force xfs_km_flags_t)0x0010u)
> +#define KM_NOLOCKDEP	((__force xfs_km_flags_t)0x0020u)
>  
>  /*
>   * We use a special process flag to avoid recursive callbacks into
> @@ -30,7 +31,7 @@ kmem_flags_convert(xfs_km_flags_t flags)
>  {
>  	gfp_t	lflags;
>  
> -	BUG_ON(flags & ~(KM_NOFS|KM_MAYFAIL|KM_ZERO));
> +	BUG_ON(flags & ~(KM_NOFS | KM_MAYFAIL | KM_ZERO | KM_NOLOCKDEP));
>  
>  	lflags = GFP_KERNEL | __GFP_NOWARN;
>  	if (flags & KM_NOFS)
> @@ -49,6 +50,9 @@ kmem_flags_convert(xfs_km_flags_t flags)
>  	if (flags & KM_ZERO)
>  		lflags |= __GFP_ZERO;
>  
> +	if (flags & KM_NOLOCKDEP)
> +		lflags |= __GFP_NOLOCKDEP;
> +
>  	return lflags;
>  }
>  
> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> index f3d18a1f5b20..2f7e89e4be3e 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -488,7 +488,7 @@ xfs_attr_copy_value(
>  	}
>  
>  	if (!args->value) {
> -		args->value = kmem_alloc_large(valuelen, 0);
> +		args->value = kmem_alloc_large(valuelen, KM_NOLOCKDEP);
>  		if (!args->value)
>  			return -ENOMEM;
>  	}
>