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@xxxxxxxxx> > > --- > > 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
