On Fri, Jan 29, 2016 at 09:26:28PM -0600, Eric Sandeen wrote:
> 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 <ross.zwisler@xxxxxxxxxxxxxxx>
> Signed-off-by: Eric Sandeen <sandeen@xxxxxxxxxx>
> ---
>
> I think we need the data_map_shared gyrations here, but not certain...
>
> diff --git a/fs/xfs/libxfs/xfs_rtbitmap.c b/fs/xfs/libxfs/xfs_rtbitmap.c
> index 9b59ffa..e6da0b2 100644
> --- a/fs/xfs/libxfs/xfs_rtbitmap.c
> +++ b/fs/xfs/libxfs/xfs_rtbitmap.c
> @@ -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);
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.
--
Dave Chinner
david@xxxxxxxxxxxxx
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
