Re: [PATCH] xfs: make tr_growdata a permanent transaction

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

On Wed, Apr 17, 2019 at 11:33:15AM -0400, Brian Foster wrote:
> On Wed, Apr 17, 2019 at 07:37:59AM -0700, Darrick J. Wong wrote:
> > On Wed, Apr 17, 2019 at 05:36:08AM -0400, Brian Foster wrote:
> > > The growdata transaction is used by growfs operations to increase
> > > the data size of the filesystem. Part of this sequence involves
> > > extending the size of the last preexisting AG in the fs, if
> > > necessary. This is implemented by freeing the newly available
> > > physical range to the AG.
> > > 
> > > tr_growdata is not a permanent transaction, however, and block
> > > allocation transactions must be permanent to handle deferred frees
> > > of AGFL blocks. If the grow operation extends an existing AG that
> > > requires AGFL fixing, assert failures occur due to a populated dfops
> > > list on a non-permanent transaction and the AGFL free does not
> > > occur. This is reproduced (rarely) by xfs/104.
> > > 
> > > Change tr_growdata to a permanent transaction with a default log
> > > count. This increases initial transaction reservation size, but
> > > growfs is an infrequent and non-performance critical operation and
> > > so should have minimal impact. Also add an assert in the block
> > > allocation path to make this transaction requirement explicit and
> > > obvious to future callers.
> > > 
> > > Reported-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > > ---
> > > 
> > > This is motivated by Darrick's recent xfs/104 failure report[1]. Note
> > > that I made the assert a bit more explicit than originally suggested
> > > because I think any transaction that performs block allocation should be
> > > expected to be able to handle arbitrary AGFL fixups. This survives an
> > > fstests auto run without any regressions[2] or assert failures.
> > > 
> > > Brian
> > > 
> > > [1] https://marc.info/?l=linux-xfs&m=155537961822223&w=2
> > > [2] Note that I was never able to reproduce the original xfs/104
> > > failure.
> > > 
> > >  fs/xfs/libxfs/xfs_alloc.c      | 2 ++
> > >  fs/xfs/libxfs/xfs_trans_resv.c | 6 +++++-
> > >  2 files changed, 7 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> > > index bc3367b8b7bb..e4df1866f949 100644
> > > --- a/fs/xfs/libxfs/xfs_alloc.c
> > > +++ b/fs/xfs/libxfs/xfs_alloc.c
> > > @@ -2237,6 +2237,8 @@ xfs_alloc_fix_freelist(
> > >  	xfs_extlen_t		need;	/* total blocks needed in freelist */
> > >  	int			error = 0;
> > >  
> > > +	ASSERT(tp->t_flags & XFS_TRANS_PERM_LOG_RES);
> > 
> > /me wonders if we ought to have a comment here about why we're asserting
> > on this? i.e.
> > 
> 
> Yep.
> 
> > /*
> >  * This function can shrink the AGFL, which uses a deferred op to avoid
> >  * exceeding transaction reservation (or whatever the original reason
> >  * was).  Deferred ops require a transaction with a permanent
> >  * reservation, so check that here.
> >  */
> > ASSERT(...);
> > 
> > Send me back a finished comment and I'll add it on its way in; the rest
> > of the patch looks ok to me.
> > 
> 
> Though to me a one-liner seems sufficient since this is just an assert.
> E.g.,
> 
> 	/* deferred ops (AGFL block frees) require permanent transactions */
> 	ASSERT(tp->t_flags & XFS_TRANS_PERM_LOG_RES);
> 
> ... seems enough to document the intent. Hm?

Yep.  Comment added.

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

--D

> Brian
> 
> > --D
> > 
> > > +
> > >  	if (!pag->pagf_init) {
> > >  		error = xfs_alloc_read_agf(mp, tp, args->agno, flags, &agbp);
> > >  		if (error)
> > > diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
> > > index f99a7aefe418..83f4ee2afc49 100644
> > > --- a/fs/xfs/libxfs/xfs_trans_resv.c
> > > +++ b/fs/xfs/libxfs/xfs_trans_resv.c
> > > @@ -876,9 +876,13 @@ xfs_trans_resv_calc(
> > >  	resp->tr_sb.tr_logres = xfs_calc_sb_reservation(mp);
> > >  	resp->tr_sb.tr_logcount = XFS_DEFAULT_LOG_COUNT;
> > >  
> > > +	/* growdata requires permanent res; it can free space to the last AG */
> > > +	resp->tr_growdata.tr_logres = xfs_calc_growdata_reservation(mp);
> > > +	resp->tr_growdata.tr_logcount = XFS_DEFAULT_PERM_LOG_COUNT;
> > > +	resp->tr_growdata.tr_logflags |= XFS_TRANS_PERM_LOG_RES;
> > > +
> > >  	/* The following transaction are logged in logical format */
> > >  	resp->tr_ichange.tr_logres = xfs_calc_ichange_reservation(mp);
> > > -	resp->tr_growdata.tr_logres = xfs_calc_growdata_reservation(mp);
> > >  	resp->tr_fsyncts.tr_logres = xfs_calc_swrite_reservation(mp);
> > >  	resp->tr_writeid.tr_logres = xfs_calc_writeid_reservation(mp);
> > >  	resp->tr_attrsetrt.tr_logres = xfs_calc_attrsetrt_reservation(mp);
> > > -- 
> > > 2.17.2
> > >