| Message ID | 20201003055633.9379-12-chandanrlinux@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Bail out if transaction can cause extent count to overflow | expand |
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? --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); > -- > 2.28.0 >
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); >
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);
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. Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com> --- fs/xfs/libxfs/xfs_bmap.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)