diff mbox series

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

Message ID 20201003055633.9379-12-chandanrlinux@gmail.com (mailing list archive)
State New
Headers show
Series Bail out if transaction can cause extent count to overflow | expand

Commit Message

Chandan Babu R Oct. 3, 2020, 5:56 a.m. UTC
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(-)

Comments

Darrick J. Wong Oct. 6, 2020, 4:26 a.m. UTC | #1
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
>
Chandan Babu R Oct. 6, 2020, 5:17 a.m. UTC | #2
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 mbox series

Patch

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);