Re: [PATCH 1/2] xfs: annotate grabbing the realtime bitmap/summary locks in growfs

From: Chandan Babu R <chandanrlinux@gmail.com>
To: "Darrick J. Wong" <darrick.wong@oracle.com>
Cc: linux-xfs@vger.kernel.org, hch@lst.de
Subject: Re: [PATCH 1/2] xfs: annotate grabbing the realtime bitmap/summary locks in growfs
Date: Tue, 13 Oct 2020 12:33:47 +0530	[thread overview]
Message-ID: <33389409.c8MGsxpk1H@garuda> (raw)
In-Reply-To: <160235126770.1384192.7924916439728391885.stgit@magnolia>

On Saturday 10 October 2020 11:04:27 PM IST Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Use XFS_ILOCK_RT{BITMAP,SUM} to annotate grabbing the rt bitmap and
> summary locks when we grow the realtime volume, just like we do most
> everywhere else.  This shuts up lockdep warnings about grabbing the
> ILOCK class of locks recursively:
> 
> ============================================
> WARNING: possible recursive locking detected
> 5.9.0-rc4-djw #rc4 Tainted: G           O
> --------------------------------------------
> xfs_growfs/4841 is trying to acquire lock:
> ffff888035acc230 (&xfs_nondir_ilock_class){++++}-{3:3}, at: xfs_ilock+0xac/0x1a0 [xfs]
> 
> but task is already holding lock:
> ffff888035acedb0 (&xfs_nondir_ilock_class){++++}-{3:3}, at: xfs_ilock+0xac/0x1a0 [xfs]
> 
> other info that might help us debug this:
>  Possible unsafe locking scenario:
> 
>        CPU0
>        ----
>   lock(&xfs_nondir_ilock_class);
>   lock(&xfs_nondir_ilock_class);
> 
>  *** DEADLOCK ***
> 
>  May be due to missing lock nesting notation
>

The changes in the patch are correct since it uses different lockdep
subclasses for rt bitmap and rt summary inodes.

Reviewed-by: Chandan Babu R <chandanrlinux@gmail.com>

> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/xfs_rtalloc.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
> index f9119ba3e9d0..ede1baf31413 100644
> --- a/fs/xfs/xfs_rtalloc.c
> +++ b/fs/xfs/xfs_rtalloc.c
> @@ -1024,7 +1024,7 @@ xfs_growfs_rt(
>  		/*
>  		 * Lock out other callers by grabbing the bitmap inode lock.
>  		 */
> -		xfs_ilock(mp->m_rbmip, XFS_ILOCK_EXCL);
> +		xfs_ilock(mp->m_rbmip, XFS_ILOCK_EXCL | XFS_ILOCK_RTBITMAP);
>  		xfs_trans_ijoin(tp, mp->m_rbmip, XFS_ILOCK_EXCL);
>  		/*
>  		 * Update the bitmap inode's size ondisk and incore.  We need
> @@ -1038,7 +1038,7 @@ xfs_growfs_rt(
>  		/*
>  		 * Get the summary inode into the transaction.
>  		 */
> -		xfs_ilock(mp->m_rsumip, XFS_ILOCK_EXCL);
> +		xfs_ilock(mp->m_rsumip, XFS_ILOCK_EXCL | XFS_ILOCK_RTSUM);
>  		xfs_trans_ijoin(tp, mp->m_rsumip, XFS_ILOCK_EXCL);
>  		/*
>  		 * Update the summary inode's size.  We need to update the
> 
> 

-- 
chandan