Re: [PATCH 3/3] xfs: dont treat rt extents beyond EOF as eofblocks to be cleared

["Darrick J. Wong" <djwong@kernel.org>]

On Mon, Jun 27, 2022 at 11:37:31AM +1000, Dave Chinner wrote:
> On Sun, Jun 26, 2022 at 03:04:04PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > On a system with a realtime volume and a 28k realtime extent,
> > generic/491 fails because the test opens a file on a frozen filesystem
> > and closing it causes xfs_release -> xfs_can_free_eofblocks to
> > mistakenly think that the the blocks of the realtime extent beyond EOF
> > are posteof blocks to be freed.  Realtime extents cannot be partially
> > unmapped, so this is pointless.  Worse yet, this triggers posteof
> > cleanup, which stalls on a transaction allocation, which is why the test
> > fails.
> > 
> > Teach the predicate to account for realtime extents properly.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  fs/xfs/xfs_bmap_util.c |    2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > 
> > diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> > index 52be58372c63..85e1a26c92e8 100644
> > --- a/fs/xfs/xfs_bmap_util.c
> > +++ b/fs/xfs/xfs_bmap_util.c
> > @@ -686,6 +686,8 @@ xfs_can_free_eofblocks(
> >  	 * forever.
> >  	 */
> >  	end_fsb = XFS_B_TO_FSB(mp, (xfs_ufsize_t)XFS_ISIZE(ip));
> > +	if (XFS_IS_REALTIME_INODE(ip) && mp->m_sb.sb_rextsize > 1)
> > +		end_fsb = roundup_64(end_fsb, mp->m_sb.sb_rextsize);
> >  	last_fsb = XFS_B_TO_FSB(mp, mp->m_super->s_maxbytes);
> >  	if (last_fsb <= end_fsb)
> >  		return false;
> 
> Ok, that works.
> 
> However, I was looking at xfs_can_free_eofblocks() w.r.t. freeze a
> couple of days ago and wondering why there isn't a freeze/RO state
> check in xfs_can_free_eofblocks(). Shouldn't we have one here so
> that we never try to run xfs_free_eofblocks() on RO/frozen
> filesystems regardless of unexpected state/alignment issues?

I asked myself that question too.  I found three callers of this
predicate:

1. fallocate, which should have obtained freeze protection

2. inodegc, which should never be running when we get to the innermost
freeze protection level

3. xfs_release, which doesn't take freeze protection at all.  Either it
needs to take freeze protection so that xfs_free_eofblocks can't get
stuck in xfs_trans_alloc, or we'd need to modify xfs_trans_alloc to
sb_start_intwrite_trylock

I don't really want to try to add (3) as part of a fix for 5.19, but I
would like to get these fixes merged so I can concentrate on finding and
fixing the file corruption problems that are still present in -rc4.  If
we want to engineer a freeze/ro state check later, we can do that too.

So, can we move ahead with this fix?

--D

> Cheers,
> 
> Dave.
> 
> 
> -- 
> Dave Chinner
> david@fromorbit.com