[PATCH] xfs: make tr_growdata a permanent transaction

[PATCH] xfs: make tr_growdata a permanent transaction

[Darrick J. Wong]

From: Brian Foster <bfoster@redhat.com>

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>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
[darrick: add a comment to the assert]
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_trans_resv.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

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

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

[Brian Foster]

On Tue, Apr 23, 2019 at 08:07:09AM -0700, Darrick J. Wong wrote:
> From: Brian Foster <bfoster@redhat.com>
> 
> 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.
> 

We should drop the last sentence from this commit log too if we push the
assert to its own patch.

Brian

> Reported-by: Darrick J. Wong <darrick.wong@oracle.com>
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> [darrick: add a comment to the assert]
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_trans_resv.c |    6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> 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);

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

[Darrick J. Wong]

On Tue, Apr 23, 2019 at 11:28:27AM -0400, Brian Foster wrote:
> On Tue, Apr 23, 2019 at 08:07:09AM -0700, Darrick J. Wong wrote:
> > From: Brian Foster <bfoster@redhat.com>
> > 
> > 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.
> > 
> 
> We should drop the last sentence from this commit log too if we push the
> assert to its own patch.

Ok, will do.

--D

> 
> Brian
> 
> > Reported-by: Darrick J. Wong <darrick.wong@oracle.com>
> > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> > [darrick: add a comment to the assert]
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/libxfs/xfs_trans_resv.c |    6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > 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);

[PATCH] xfs: assert that we don't enter agfl freeing with a non-permanent transaction

[Darrick J. Wong]

From: Brian Foster <bfoster@redhat.com>

Block allocation requires a permanent transaction for deferred AGFL
frees.  Add an assert in the block allocation path to make explicit and
obvious to future callers the requirement of a transaction with a
permanent reservation.

Reported-by: Darrick J. Wong <darrick.wong@oracle.com>
Signed-off-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
[darrick: split this out from the previous patch per hch request]
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_alloc.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
index 857a53e58b94..a9ff3cf82cce 100644
--- a/fs/xfs/libxfs/xfs_alloc.c
+++ b/fs/xfs/libxfs/xfs_alloc.c
@@ -2243,6 +2243,9 @@ xfs_alloc_fix_freelist(
 	xfs_extlen_t		need;	/* total blocks needed in freelist */
 	int			error = 0;
 
+	/* deferred ops (AGFL block frees) require permanent transactions */
+	ASSERT(tp->t_flags & XFS_TRANS_PERM_LOG_RES);
+
 	if (!pag->pagf_init) {
 		error = xfs_alloc_read_agf(mp, tp, args->agno, flags, &agbp);
 		if (error)