Re: [PATCH] xfs: lock bitmap/summary inodes in xfs_rtbuf_get()

[Dave Chinner <david@xxxxxxxxxxxxx>]

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