[04/47] xfs: fix locking of the rt bitmap/summary inodes
diff mbox

Message ID 146907698690.25461.7316591529865921141.stgit@birch.djwong.org
State Accepted
Headers show

Commit Message

Darrick J. Wong July 21, 2016, 4:56 a.m. UTC
When we're deleting realtime extents, we need to lock the summary
inode in case we need to update the summary info to prevent an assert
on the rsumip inode lock on a debug kernel.  While we're at it, fix
the locking annotations so that we avoid triggering lockdep warnings.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_bmap.c |    4 +++-
 fs/xfs/xfs_bmap_util.c   |    4 ++--
 2 files changed, 5 insertions(+), 3 deletions(-)

Comments

Brian Foster July 26, 2016, 4:36 p.m. UTC | #1
On Wed, Jul 20, 2016 at 09:56:26PM -0700, Darrick J. Wong wrote:
> When we're deleting realtime extents, we need to lock the summary
> inode in case we need to update the summary info to prevent an assert
> on the rsumip inode lock on a debug kernel.  While we're at it, fix
> the locking annotations so that we avoid triggering lockdep warnings.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---

I haven't tried the rt stuff in quite some time (and even then never
really played with it much). What's the assert that fails?

Brian

>  fs/xfs/libxfs/xfs_bmap.c |    4 +++-
>  fs/xfs/xfs_bmap_util.c   |    4 ++--
>  2 files changed, 5 insertions(+), 3 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 2f2c85c..c5981f4 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -5179,8 +5179,10 @@ xfs_bunmapi(
>  		/*
>  		 * Synchronize by locking the bitmap inode.
>  		 */
> -		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);
> +		xfs_ilock(mp->m_rsumip, XFS_ILOCK_EXCL|XFS_ILOCK_RTSUM);
> +		xfs_trans_ijoin(tp, mp->m_rsumip, XFS_ILOCK_EXCL);
>  	}
>  
>  	extno = 0;
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index cd4a850..998c3e6 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -214,9 +214,9 @@ xfs_bmap_rtalloc(
>  	/*
>  	 * Lock out modifications to both the RT bitmap and summary inodes
>  	 */
> -	xfs_ilock(mp->m_rbmip, XFS_ILOCK_EXCL);
> +	xfs_ilock(mp->m_rbmip, XFS_ILOCK_EXCL|XFS_ILOCK_RTBITMAP);
>  	xfs_trans_ijoin(ap->tp, mp->m_rbmip, XFS_ILOCK_EXCL);
> -	xfs_ilock(mp->m_rsumip, XFS_ILOCK_EXCL);
> +	xfs_ilock(mp->m_rsumip, XFS_ILOCK_EXCL|XFS_ILOCK_RTSUM);
>  	xfs_trans_ijoin(ap->tp, mp->m_rsumip, XFS_ILOCK_EXCL);
>  
>  	/*
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
Darrick J. Wong July 28, 2016, 6:58 p.m. UTC | #2
On Tue, Jul 26, 2016 at 12:36:07PM -0400, Brian Foster wrote:
> On Wed, Jul 20, 2016 at 09:56:26PM -0700, Darrick J. Wong wrote:
> > When we're deleting realtime extents, we need to lock the summary
> > inode in case we need to update the summary info to prevent an assert
> > on the rsumip inode lock on a debug kernel.  While we're at it, fix
> > the locking annotations so that we avoid triggering lockdep warnings.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> 
> I haven't tried the rt stuff in quite some time (and even then never
> really played with it much). What's the assert that fails?

The first assert that I noticed is in xfs_rtfree_extent():

ASSERT(xfs_isilocked(mp->m_rbmip, XFS_ILOCK_EXCL));

I then noticed that we weren't locking the summary inode either, which
triggers this assert in xfs_bmapi_read():

ASSERT(xfs_isilocked(ip, XFS_ILOCK_SHARED|XFS_ILOCK_EXCL));

which has this call stack:

xfs_rtfree_extent -> xfs_rtmodify_summary -> xfs_rtmodify_summary_int ->
xfs_rtbuf_get -> xfs_bmapi_read.

--D

> 
> Brian
> 
> >  fs/xfs/libxfs/xfs_bmap.c |    4 +++-
> >  fs/xfs/xfs_bmap_util.c   |    4 ++--
> >  2 files changed, 5 insertions(+), 3 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> > index 2f2c85c..c5981f4 100644
> > --- a/fs/xfs/libxfs/xfs_bmap.c
> > +++ b/fs/xfs/libxfs/xfs_bmap.c
> > @@ -5179,8 +5179,10 @@ xfs_bunmapi(
> >  		/*
> >  		 * Synchronize by locking the bitmap inode.
> >  		 */
> > -		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);
> > +		xfs_ilock(mp->m_rsumip, XFS_ILOCK_EXCL|XFS_ILOCK_RTSUM);
> > +		xfs_trans_ijoin(tp, mp->m_rsumip, XFS_ILOCK_EXCL);
> >  	}
> >  
> >  	extno = 0;
> > diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> > index cd4a850..998c3e6 100644
> > --- a/fs/xfs/xfs_bmap_util.c
> > +++ b/fs/xfs/xfs_bmap_util.c
> > @@ -214,9 +214,9 @@ xfs_bmap_rtalloc(
> >  	/*
> >  	 * Lock out modifications to both the RT bitmap and summary inodes
> >  	 */
> > -	xfs_ilock(mp->m_rbmip, XFS_ILOCK_EXCL);
> > +	xfs_ilock(mp->m_rbmip, XFS_ILOCK_EXCL|XFS_ILOCK_RTBITMAP);
> >  	xfs_trans_ijoin(ap->tp, mp->m_rbmip, XFS_ILOCK_EXCL);
> > -	xfs_ilock(mp->m_rsumip, XFS_ILOCK_EXCL);
> > +	xfs_ilock(mp->m_rsumip, XFS_ILOCK_EXCL|XFS_ILOCK_RTSUM);
> >  	xfs_trans_ijoin(ap->tp, mp->m_rsumip, XFS_ILOCK_EXCL);
> >  
> >  	/*
> > 
> > _______________________________________________
> > xfs mailing list
> > xfs@oss.sgi.com
> > http://oss.sgi.com/mailman/listinfo/xfs
Christoph Hellwig Aug. 1, 2016, 6:34 a.m. UTC | #3
On Wed, Jul 20, 2016 at 09:56:26PM -0700, Darrick J. Wong wrote:
> When we're deleting realtime extents, we need to lock the summary
> inode in case we need to update the summary info to prevent an assert
> on the rsumip inode lock on a debug kernel.  While we're at it, fix
> the locking annotations so that we avoid triggering lockdep warnings.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Patch
diff mbox

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 2f2c85c..c5981f4 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -5179,8 +5179,10 @@  xfs_bunmapi(
 		/*
 		 * Synchronize by locking the bitmap inode.
 		 */
-		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);
+		xfs_ilock(mp->m_rsumip, XFS_ILOCK_EXCL|XFS_ILOCK_RTSUM);
+		xfs_trans_ijoin(tp, mp->m_rsumip, XFS_ILOCK_EXCL);
 	}
 
 	extno = 0;
diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index cd4a850..998c3e6 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -214,9 +214,9 @@  xfs_bmap_rtalloc(
 	/*
 	 * Lock out modifications to both the RT bitmap and summary inodes
 	 */
-	xfs_ilock(mp->m_rbmip, XFS_ILOCK_EXCL);
+	xfs_ilock(mp->m_rbmip, XFS_ILOCK_EXCL|XFS_ILOCK_RTBITMAP);
 	xfs_trans_ijoin(ap->tp, mp->m_rbmip, XFS_ILOCK_EXCL);
-	xfs_ilock(mp->m_rsumip, XFS_ILOCK_EXCL);
+	xfs_ilock(mp->m_rsumip, XFS_ILOCK_EXCL|XFS_ILOCK_RTSUM);
 	xfs_trans_ijoin(ap->tp, mp->m_rsumip, XFS_ILOCK_EXCL);
 
 	/*