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@xxxxxxxxxx> Thanks! -Dave. -- Dave Chinner david@xxxxxxxxxxxxx
