Re: [PATCH 01/30] xfs: Don't allow logging of XFS_ISTALE inodes

[Dave Chinner <david@fromorbit.com>]

On Mon, Jun 01, 2020 at 09:30:52PM -0700, Darrick J. Wong wrote:
> On Tue, Jun 02, 2020 at 07:42:22AM +1000, Dave Chinner wrote:
> > --- a/fs/xfs/xfs_inode.c
> > +++ b/fs/xfs/xfs_inode.c
> > @@ -1740,10 +1740,31 @@ xfs_inactive_ifree(
> >  		return error;
> >  	}
> >  
> > +	/*
> > +	 * We do not hold the inode locked across the entire rolling transaction
> > +	 * here. We only need to hold it for the first transaction that
> > +	 * xfs_ifree() builds, which may mark the inode XFS_ISTALE if the
> > +	 * underlying cluster buffer is freed. Relogging an XFS_ISTALE inode
> > +	 * here breaks the relationship between cluster buffer invalidation and
> > +	 * stale inode invalidation on cluster buffer item journal commit
> > +	 * completion, and can result in leaving dirty stale inodes hanging
> > +	 * around in memory.
> > +	 *
> > +	 * We have no need for serialising this inode operation against other
> > +	 * operations - we freed the inode and hence reallocation is required
> > +	 * and that will serialise on reallocating the space the deferops need
> > +	 * to free. Hence we can unlock the inode on the first commit of
> > +	 * the transaction rather than roll it right through the deferops. This
> > +	 * avoids relogging the XFS_ISTALE inode.
> 
> Hmm.  What defer ops causes a transaction roll?  Is it the EFI that
> frees the inode cluster blocks?

Yeah, xfs_difree_inode_chunk() calls xfs_bmap_add_free() which goes
through the deferops to free the inode chunk extent(s).

I suspect that we can also get xfs_alloc_fix_freelist() deferring
AGFL frees here too. I basically just assume anything that allocates
or frees extents is likely to have at least one deferop as a result
of this....

> > +	 *
> > +	 * We check that xfs_ifree() hasn't grown an internal transaction roll
> > +	 * by asserting that the inode is still locked when it returns.
> > +	 */
> >  	xfs_ilock(ip, XFS_ILOCK_EXCL);
> > -	xfs_trans_ijoin(tp, ip, 0);
> > +	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
> 
> This looks right to me since we should be marking the inode free in the
> first transaction and therefore should not keep it attached to the
> transaction...

*nod*

That was my thinking about the problem, too.

> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

Thanks!

-Dave.
-- 
Dave Chinner
david@fromorbit.com