xfs: lock bitmap/summary inodes in xfs_rtbuf_get()

PATCH In review Eric Sandeen 3 participants 4 messages Jan 30, 2016 – Feb 1, 2016 XFS filesystem

Share & tools

Eric Sandeen

author

2016-01-30 03:26 UTC #0 lore raw

PATCH 1/1 xfs: lock bitmap/summary inodes in xfs_rtbuf_get()

Commit eef334e added an ASSERT that the inode was locked in some way in xfs_bmapi_read(), but on realtime paths through xfs_rtbuf_get() this isn't the case; fix that.

Reported-by Ross Zwisler <[email protected]>

Signed-off-by Eric Sandeen <[email protected]>

---

I think we need the data_map_shared gyrations here, but not certain...

fs/xfs/libxfs/xfs_rtbitmap.c +3 -0

index 9b59ffa..e6da0b2 100644
@@ -57,11 +57,14 @@ xfs_rtbuf_get(
 	xfs_inode_t	*ip;		/* bitmap or summary inode */
 	xfs_bmbt_irec_t	map;
 	int		nmap = 1;
+	int		lock_mode;
 	int		error;		/* error value */
 
 	ip = issum ? mp->m_rsumip : mp->m_rbmip;
 
+	lock_mode = xfs_ilock_data_map_shared(ip);
 	error = xfs_bmapi_read(ip, block, 1, &map, &nmap, XFS_DATA_FORK);
+	xfs_iunlock(ip, lock_mode);
 	if (error)
 		return error;

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Dave Chinner

2016-01-30 21:07 UTC #1 lore raw

Quoted 16 lines · Eric Sandeen

Commit eef334e added an ASSERT that the inode was locked in some way in xfs_bmapi_read(), but on realtime paths through xfs_rtbuf_get() this isn't the case; fix that.
Reported-by Ross Zwisler <[email protected]>
Signed-off-by Eric Sandeen <[email protected]>
---
I think we need the data_map_shared gyrations here, but not certain...
fs/xfs/libxfs/xfs_rtbitmap.c +0 -0
index 9b59ffa..e6da0b2 100644
@@ -57,11 +57,14 @@ xfs_rtbuf_get(
 	xfs_inode_t	*ip;		/* bitmap or summary inode */

fs/xfs/libxfs/xfs_rtbitmap.c xfs_rtbuf_get( · line 67

Quoted 10 lines

fs/xfs/libxfs/xfs_rtbitmap.c +3 -0

 	xfs_bmbt_irec_t	map;
 	int		nmap = 1;
+	int		lock_mode;
 	int		error;		/* error value */
 
 	ip = issum ? mp->m_rsumip : mp->m_rbmip;
 
+	lock_mode = xfs_ilock_data_map_shared(ip);
 	error = xfs_bmapi_read(ip, block, 1, &map, &nmap, XFS_DATA_FORK);
+	xfs_iunlock(ip, lock_mode);

I've looked into this recently and didn't think up a simple answer to the problem. I didn't spend much time on it because it's nowhere near the top of my priority list because it only affects debug kernels as the summary inode is effectively protected by the bitmap inode exclusion during allocation.

That said, the above change is not safe because xfs_rtbuf_get() can be called with the bitmap inode lock already held. e.g:

xfs_bmap_rtalloc

    xfs_ilock(mp->m_rbmip, XFS_ILOCK_EXCL);   << locks bitmap
    xfs_rtallocate_extent
      xfs_rtallocate_extent_exact
	xfs_rtcheck_range
	  xfs_rtbuf_get(issum = 0)
	    xfs_ilock_data_map_shared(mp->m_rbmip)
	    <self deadlock>

The issue here is that the summary inode is not locked early on in the transaction, so modifications are done with it unlocked.

xfs_bmap_rtalloc
xfs_ilock(mp->m_rbmip, XFS_ILOCK_EXCL); << locks bitmap
xfs_rtallocate_extent
xfs_rtallocate_extent_size
xfs_rtget_summary
xfs_rtmodify_summary_int
xfs_rtbuf_get
xfs_bmapi_read(summary inode) << unlocked summary

The only path through which the summary inode is locked for modification is the growfs path (xfs_rtcopy_summary()), so all the other paths that modify/access the summary inode also need to be locked at a higher level before calling into the summary functions.

Cheers,

Dave.

Signature · 7 lines

Dave Chinner
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Eric Sandeen

2016-01-30 21:53 UTC #2 lore raw

OK, thanks Dave. It seemed like it was probably too simple…

Quoted 60 lines · Dave Chinner

Quoted 18 lines · Eric Sandeen
Commit eef334e added an ASSERT that the inode was locked in some way in xfs_bmapi_read(), but on realtime paths through xfs_rtbuf_get() this isn't the case; fix that.
Reported-by Ross Zwisler <[email protected]>
Signed-off-by Eric Sandeen <[email protected]>
---
I think we need the data_map_shared gyrations here, but not certain...
fs/xfs/libxfs/xfs_rtbitmap.c +0 -0
index 9b59ffa..e6da0b2 100644
@@ -57,11 +57,14 @@ xfs_rtbuf_get(
   xfs_inode_t    *ip;        /* bitmap or summary inode */
fs/xfs/libxfs/xfs_rtbitmap.c xfs_rtbuf_get( · line 67
Quoted 10 lines
fs/xfs/libxfs/xfs_rtbitmap.c +3 -0
   xfs_bmbt_irec_t    map;
   int        nmap = 1;
+    int        lock_mode;
   int        error;        /* error value */
 
   ip = issum ? mp->m_rsumip : mp->m_rbmip;
 
+    lock_mode = xfs_ilock_data_map_shared(ip);
   error = xfs_bmapi_read(ip, block, 1, &map, &nmap, XFS_DATA_FORK);
+    xfs_iunlock(ip, lock_mode);
I've looked into this recently and didn't think up a simple answer to the problem. I didn't spend much time on it because it's nowhere near the top of my priority list because it only affects debug kernels as the summary inode is effectively protected by the bitmap inode exclusion during allocation.
That said, the above change is not safe because xfs_rtbuf_get() can be called with the bitmap inode lock already held. e.g:
xfs_bmap_rtalloc
xfs_ilock(mp->m_rbmip, XFS_ILOCK_EXCL); << locks bitmap
xfs_rtallocate_extent
xfs_rtallocate_extent_exact
xfs_rtcheck_range
xfs_rtbuf_get(issum = 0)
xfs_ilock_data_map_shared(mp->m_rbmip)
<self deadlock>
The issue here is that the summary inode is not locked early on in the transaction, so modifications are done with it unlocked.
xfs_bmap_rtalloc
xfs_ilock(mp->m_rbmip, XFS_ILOCK_EXCL); << locks bitmap
xfs_rtallocate_extent
xfs_rtallocate_extent_size
xfs_rtget_summary
xfs_rtmodify_summary_int
xfs_rtbuf_get
xfs_bmapi_read(summary inode) << unlocked summary

Quoted 17 lines

The only path through which the summary inode is locked for modification is the growfs path (xfs_rtcopy_summary()), so all the other paths that modify/access the summary inode also need to be locked at a higher level before calling into the summary functions.
Cheers,
Dave.
Signature · 7 lines
Dave Chinner
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Dave Chinner

2016-02-01 22:05 UTC #3 lore raw

Quoted 1 line · Eric Sandeen

OK, thanks Dave. It seemed like it was probably too simple…

Yeah, and the simple fix (lock in xfs_bmap_rtalloc()) uncovers further issues w.r.t. logging - the buffers are not stamped with a type, so log recovery will not categorise them correctly.

And, I suspect, log recovery might have problems with the bitmap/smmary buffers as they don't have magic numbers in them. More work to be done here...

Cheers,

Dave.

Signature · 7 lines

Dave Chinner
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs