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

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

[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 <ross.zwisler@linux.intel.com>
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

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);
 	if (error)
 		return error;
 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

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

[Dave Chinner]

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@linux.intel.com>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> 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@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

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

[Eric Sandeen]

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

> On Jan 30, 2016, at 3:06 PM, Dave Chinner <david@fromorbit.com> wrote:
> 
>> 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@linux.intel.com>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>> 
>> 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@fromorbit.com
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

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

[Dave Chinner]

On Sat, Jan 30, 2016 at 03:53:48PM -0600, Eric Sandeen wrote:
> 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.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs