Re: [PATCH V5 11/12] xfs: Set tp->t_firstblock only once during a transaction's lifetime

[Chandan Babu R <chandanrlinux@gmail.com>]

On Tuesday 6 October 2020 9:56:29 AM IST Darrick J. Wong wrote:
> On Sat, Oct 03, 2020 at 11:26:32AM +0530, Chandan Babu R wrote:
> > tp->t_firstblock is supposed to hold the first fs block allocated by the
> > transaction. There are two cases in the current code base where
> > tp->t_firstblock is assigned a value unconditionally. This commit makes
> > sure that we assign to tp->t_firstblock only if its current value is
> > NULLFSBLOCK.
> 
> Do we hit this currently?  This seems like a regression fix, since I'm
> guessing you hit this fairly soon after adding the next patch and
> twisting the "shatter everything" debug knob it establishes?  And if
> you can hit it there, you could hit this on a severely fragmented fs?

I came across this when I was trying to understand the code flow w.r.t
xfs_bmap_btalloc() => xfs_alloc_vextent() => etc. I noticed that if a
transaction does the following,

1. Satisfy the first allocation request from AG X.
2. Satisfy the second allocation request from AG X+1, since say the second
   allocation request was for a larger minlen value.

... A new space allocation request with minlen equal to what was issued in
step 1 could fail (even though AG X could still have minlen free space)
because step 2 ended up updating tp->t_firstblock to a block from AG X+1 and
hence AG X could never be scanned for free blocks even though the transaction
holds a lock on the corresponding AGF.

This behaviour is most likely true when the "alloc minlen" debug knob
(introduced in the next patch) is enabled. However I didn't execute any
workload on a severly fragmented fs to actually see this behaviour on a
mounted filesystem.

> 
> --D
> 
> > 
> > Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
> > ---
> >  fs/xfs/libxfs/xfs_bmap.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> > index 51c2d2690f05..5156cbd476f2 100644
> > --- a/fs/xfs/libxfs/xfs_bmap.c
> > +++ b/fs/xfs/libxfs/xfs_bmap.c
> > @@ -724,7 +724,8 @@ xfs_bmap_extents_to_btree(
> >  	 */
> >  	ASSERT(tp->t_firstblock == NULLFSBLOCK ||
> >  	       args.agno >= XFS_FSB_TO_AGNO(mp, tp->t_firstblock));
> > -	tp->t_firstblock = args.fsbno;
> > +	if (tp->t_firstblock == NULLFSBLOCK)
> > +		tp->t_firstblock = args.fsbno;
> >  	cur->bc_ino.allocated++;
> >  	ip->i_d.di_nblocks++;
> >  	xfs_trans_mod_dquot_byino(tp, ip, XFS_TRANS_DQ_BCOUNT, 1L);
> > @@ -875,7 +876,8 @@ xfs_bmap_local_to_extents(
> >  	/* Can't fail, the space was reserved. */
> >  	ASSERT(args.fsbno != NULLFSBLOCK);
> >  	ASSERT(args.len == 1);
> > -	tp->t_firstblock = args.fsbno;
> > +	if (tp->t_firstblock == NULLFSBLOCK)
> > +		tp->t_firstblock = args.fsbno;
> >  	error = xfs_trans_get_buf(tp, args.mp->m_ddev_targp,
> >  			XFS_FSB_TO_DADDR(args.mp, args.fsbno),
> >  			args.mp->m_bsize, 0, &bp);
> 


-- 
chandan