Re: [PATCH 4/5] xfs: xfs_defer_capture should absorb remaining block reservation

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Thu, Oct 01, 2020 at 01:32:24PM -0400, Brian Foster wrote:
> On Tue, Sep 29, 2020 at 10:43:38AM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > When xfs_defer_capture extracts the deferred ops and transaction state
> > from a transaction, it should absorb the remaining block reservation so
> > that when we continue the dfops chain, we still have those blocks to
> > use.
> > 
> > This adds the requirement that every log intent item recovery function
> > must be careful to reserve enough blocks to handle both itself and all
> > defer ops that it can queue.  On the other hand, this enables us to do
> > away with the handwaving block estimation nonsense that was going on in
> > xlog_finish_defer_ops.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > Reviewed-by: Dave Chinner <dchinner@redhat.com>
> > ---
> >  fs/xfs/libxfs/xfs_defer.c |    5 +++++
> >  fs/xfs/libxfs/xfs_defer.h |    1 +
> >  fs/xfs/xfs_log_recover.c  |   18 +-----------------
> >  3 files changed, 7 insertions(+), 17 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
> > index 85c371d29e8d..0cceebb390c4 100644
> > --- a/fs/xfs/libxfs/xfs_defer.c
> > +++ b/fs/xfs/libxfs/xfs_defer.c
> > @@ -575,6 +575,10 @@ xfs_defer_ops_capture(
> >  	dfc->dfc_tpflags = tp->t_flags & XFS_TRANS_LOWMODE;
> >  	tp->t_flags &= ~XFS_TRANS_LOWMODE;
> >  
> > +	/* Capture the block reservation along with the dfops. */
> > +	dfc->dfc_blkres = tp->t_blk_res - tp->t_blk_res_used;
> > +	tp->t_blk_res = tp->t_blk_res_used;
> > +
> >  	return dfc;
> >  }
> >  
> > @@ -632,6 +636,7 @@ xfs_defer_ops_continue(
> >  	/* Move captured dfops chain and state to the transaction. */
> >  	list_splice_init(&dfc->dfc_dfops, &tp->t_dfops);
> >  	tp->t_flags |= dfc->dfc_tpflags;
> > +	tp->t_blk_res += dfc->dfc_blkres;
> >  
> >  	kmem_free(dfc);
> >  }
> 
> Seems sane, but I'm curious why we need to modify the transactions
> directly in both of these contexts. Rather than building up and holding
> a growing block reservation across transactions during intent
> processing, could we just sample the unused blocks in the transaction at
> capture time and use that as a resblks parameter when we allocate the
> transaction to continue the chain? Then we at least have some validation
> via the traditional allocation path if we ever screw up the accounting..

Good idea.  I'll also make this patch save the rt block reservation.

--D

> Brian
> 
> > diff --git a/fs/xfs/libxfs/xfs_defer.h b/fs/xfs/libxfs/xfs_defer.h
> > index 3af82ebc1249..b1c7b761afd5 100644
> > --- a/fs/xfs/libxfs/xfs_defer.h
> > +++ b/fs/xfs/libxfs/xfs_defer.h
> > @@ -75,6 +75,7 @@ struct xfs_defer_capture {
> >  	/* Deferred ops state saved from the transaction. */
> >  	struct list_head	dfc_dfops;
> >  	unsigned int		dfc_tpflags;
> > +	unsigned int		dfc_blkres;
> >  };
> >  
> >  /*
> > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> > index 550d0fa8057a..b06c9881a13d 100644
> > --- a/fs/xfs/xfs_log_recover.c
> > +++ b/fs/xfs/xfs_log_recover.c
> > @@ -2439,26 +2439,10 @@ xlog_finish_defer_ops(
> >  {
> >  	struct xfs_defer_capture *dfc, *next;
> >  	struct xfs_trans	*tp;
> > -	int64_t			freeblks;
> > -	uint64_t		resblks;
> >  	int			error = 0;
> >  
> >  	list_for_each_entry_safe(dfc, next, capture_list, dfc_list) {
> > -		/*
> > -		 * We're finishing the defer_ops that accumulated as a result
> > -		 * of recovering unfinished intent items during log recovery.
> > -		 * We reserve an itruncate transaction because it is the
> > -		 * largest permanent transaction type.  Since we're the only
> > -		 * user of the fs right now, take 93% (15/16) of the available
> > -		 * free blocks.  Use weird math to avoid a 64-bit division.
> > -		 */
> > -		freeblks = percpu_counter_sum(&mp->m_fdblocks);
> > -		if (freeblks <= 0)
> > -			return -ENOSPC;
> > -
> > -		resblks = min_t(uint64_t, UINT_MAX, freeblks);
> > -		resblks = (resblks * 15) >> 4;
> > -		error = xfs_trans_alloc(mp, &M_RES(mp)->tr_itruncate, resblks,
> > +		error = xfs_trans_alloc(mp, &M_RES(mp)->tr_itruncate, 0,
> >  				0, XFS_TRANS_RESERVE, &tp);
> >  		if (error)
> >  			return error;
> > 
>