Re: [PATCH 2/4] xfs: include inobt buffers in ifree tx log reservation

[Brian Foster <bfoster@redhat.com>]

On Wed, Nov 29, 2017 at 08:38:44AM +1100, Dave Chinner wrote:
> On Tue, Nov 28, 2017 at 08:30:02AM -0500, Brian Foster wrote:
> > On Tue, Nov 28, 2017 at 09:28:57AM +1100, Dave Chinner wrote:
> > > On Mon, Nov 27, 2017 at 03:24:32PM -0500, Brian Foster wrote:
> > > > The tr_ifree transaction handles inode unlinks and inode chunk
> > > > frees. The current transaction calculation does not accurately
> > > > reflect worst case changes to the inode btree, however. The inobt
> > > > portion of the current transaction reservation only covers
> > > > modification of a single inobt buffer (for the particular inode
> > > > record). This is a historical artifact from the days before XFS
> > > > supported full inode chunk removal.
> > > > 
> > > > When support for inode chunk removal was added in commit
> > > > 254f6311ed1b ("Implement deletion of inode clusters in XFS."), the
> > > > additional log reservation required for chunk removal was not added
> > > > correctly. The new reservation only considered the header overhead
> > > > of associated buffers rather than the full contents of the btrees
> > > > and AGF and AGFL buffers affected by the transaction. The
> > > > reservation for the free space btrees was subsequently fixed up in
> > > > commit 5fe6abb82f76 ("Add space for inode and allocation btrees to
> > > > ITRUNCATE log reservation"), but the res. for full inobt joins has
> > > > never been added.
> > > > 
> > > > Update xfs_calc_ifree_reservation() to include a full inobt join in
> > > > the reservation calculation. Refactor the undocumented +2 blocks for
> > > > the AGF and AGFL buffers into the appropriate place so they are
> > > > accounted as sectors (not FSBs) and update the associated comments.
> > > > 
> > > > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > > > ---
> > > >  fs/xfs/libxfs/xfs_trans_resv.c | 11 +++++------
> > > >  1 file changed, 5 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
> > > > index 6bd916bd35e2..4cd7cd1e60da 100644
> > > > --- a/fs/xfs/libxfs/xfs_trans_resv.c
> > > > +++ b/fs/xfs/libxfs/xfs_trans_resv.c
> > > > @@ -490,10 +490,10 @@ xfs_calc_symlink_reservation(
> > > >  /*
> > > >   * In freeing an inode we can modify:
> > > >   *    the inode being freed: inode size
> > > > - *    the super block free inode counter: sector size
> > > > + *    the super block free inode counter, AGF and AGFL: sector size
> > > >   *    the agi hash list and counters: sector size
> > > > - *    the inode btree entry: block size
> > > >   *    the on disk inode before ours in the agi hash list: inode cluster size
> > > > + *    the inode chunk is marked stale (headers only)
> > > >   *    the inode btree: max depth * blocksize
> > > >   *    the allocation btrees: 2 trees * (max depth - 1) * block size
> > > >   *    the finobt (record insertion, removal or modification)
> > > > @@ -504,12 +504,11 @@ xfs_calc_ifree_reservation(
> > > >  {
> > > >  	return XFS_DQUOT_LOGRES(mp) +
> > > >  		xfs_calc_inode_res(mp, 1) +
> > > 
> > >  *    the inode being freed: inode size
> > > 
> > > > -		xfs_calc_buf_res(1, mp->m_sb.sb_sectsize) +
> > > > -		xfs_calc_buf_res(1, XFS_FSB_TO_B(mp, 1)) +
> > > > +		xfs_calc_buf_res(3, mp->m_sb.sb_sectsize) +
> > > 
> > >  *    the super block free inode counter, AGF and AGFL: sector size
> > > 
> > > 		[missing, hidden in calc_iunlink_remove]
> > > 
> > >  *    the agi hash list and counters: sector size
> > > 
> > > >  		xfs_calc_iunlink_remove_reservation(mp) +
> > > 
> > >  *    the on disk inode before ours in the agi hash list: inode cluster size
> > > 
> > > 		[missing]
> > > 
> > >  *    on-disk inode to log the di_next_unlinked: inode cluster size
> > > 
> > > Yes, check the xfs_iunlink_remove() code - we can log two inode
> > > cluster buffers there: the one prior to us in the unlinked list,
> > > and ours to reset the di_next_unlinked pointer to
> > > null. IOWs, xfs_calc_iunlink_remove_reservation() needs to take into
> > > account /2/ inode cluster buffers, not one.
> > > 
> > 
> > I was wondering why this wouldn't be covered by the inode size included
> > above, but looking at the code, I guess we log the agi unlinked changes
> > directly in the cluster buffer. That means we have separate log items
> > with independent reservation consumption between the unlinked list
> > fixups and core inode.
> > 
> > I'll update xfs_calc_iunlink_remove_reservation() to include another
> > cluster and add a comment.
> 
> And, as I mentioned in the other thread,
> xfs_calc_iunlink_add_reservation() needs this fixup, too.
> 

Right, thanks..

> > 
> > > >  		xfs_calc_buf_res(1, 0) +
> > > 
> > > No idea what this is.
> > > 
> > 
> > Same, I didn't want to remove it unless we could identify what it was
> > originally intended for.
> 
> It's only going to be a 128 bytes for a header, so I think we should
> just remove it rather than keep some unknown magic in the
> reservation. If it turns out to be critical, then we need to
> understand exactly where it came from...
> 

Fair enough. We're still adding reservation in the end so I'll drop this
too.

Brian

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html