[PATCH] xfs: make tr_growdata a permanent transaction

[PATCH] xfs: make tr_growdata a permanent transaction

[Brian Foster]

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

This is motivated by Darrick's recent xfs/104 failure report[1]. Note
that I made the assert a bit more explicit than originally suggested
because I think any transaction that performs block allocation should be
expected to be able to handle arbitrary AGFL fixups. This survives an
fstests auto run without any regressions[2] or assert failures.

Brian

[1] https://marc.info/?l=linux-xfs&m=155537961822223&w=2
[2] Note that I was never able to reproduce the original xfs/104
failure.

 fs/xfs/libxfs/xfs_alloc.c      | 2 ++
 fs/xfs/libxfs/xfs_trans_resv.c | 6 +++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
index bc3367b8b7bb..e4df1866f949 100644
--- a/fs/xfs/libxfs/xfs_alloc.c
+++ b/fs/xfs/libxfs/xfs_alloc.c
@@ -2237,6 +2237,8 @@ xfs_alloc_fix_freelist(
 	xfs_extlen_t		need;	/* total blocks needed in freelist */
 	int			error = 0;
 
+	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)
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);
-- 
2.17.2

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

[Brian Foster]

On Wed, Apr 17, 2019 at 05:36:08AM -0400, Brian Foster wrote:
> 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>
> ---
> 
> This is motivated by Darrick's recent xfs/104 failure report[1]. Note
> that I made the assert a bit more explicit than originally suggested
> because I think any transaction that performs block allocation should be
> expected to be able to handle arbitrary AGFL fixups. This survives an
> fstests auto run without any regressions[2] or assert failures.
> 
> Brian
> 
> [1] https://marc.info/?l=linux-xfs&m=155537961822223&w=2
> [2] Note that I was never able to reproduce the original xfs/104
> failure.
> 
>  fs/xfs/libxfs/xfs_alloc.c      | 2 ++
>  fs/xfs/libxfs/xfs_trans_resv.c | 6 +++++-
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> index bc3367b8b7bb..e4df1866f949 100644
> --- a/fs/xfs/libxfs/xfs_alloc.c
> +++ b/fs/xfs/libxfs/xfs_alloc.c
> @@ -2237,6 +2237,8 @@ xfs_alloc_fix_freelist(
>  	xfs_extlen_t		need;	/* total blocks needed in freelist */
>  	int			error = 0;
>  
> +	ASSERT(tp->t_flags & XFS_TRANS_PERM_LOG_RES);
> +

Naturally, just after sending this I'm reminded I never went back to
actually audit usage of XFS_ALLOC_FLAG_NOSHRINK. :P NOSHRINK is
currently used in userspace (repair) and scrub. The former is in a place
that looks to me like it already uses a permanent transaction. The
latter is not immediately clear to me. It looks like scrub can alloc
either tr_itruncate, which is permanent, or an empty transaction. The
noshrink call is associated with repair, which I think means we'd have
the permanent transaction (?), but it still might make sense to exclude
NOSHRINK callers from failing the assert in principle. Thoughts?

Brian

>  	if (!pag->pagf_init) {
>  		error = xfs_alloc_read_agf(mp, tp, args->agno, flags, &agbp);
>  		if (error)
> 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);
> -- 
> 2.17.2
> 

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

[Darrick J. Wong]

On Wed, Apr 17, 2019 at 05:54:32AM -0400, Brian Foster wrote:
> On Wed, Apr 17, 2019 at 05:36:08AM -0400, Brian Foster wrote:
> > 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>
> > ---
> > 
> > This is motivated by Darrick's recent xfs/104 failure report[1]. Note
> > that I made the assert a bit more explicit than originally suggested
> > because I think any transaction that performs block allocation should be
> > expected to be able to handle arbitrary AGFL fixups. This survives an
> > fstests auto run without any regressions[2] or assert failures.
> > 
> > Brian
> > 
> > [1] https://marc.info/?l=linux-xfs&m=155537961822223&w=2
> > [2] Note that I was never able to reproduce the original xfs/104
> > failure.
> > 
> >  fs/xfs/libxfs/xfs_alloc.c      | 2 ++
> >  fs/xfs/libxfs/xfs_trans_resv.c | 6 +++++-
> >  2 files changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> > index bc3367b8b7bb..e4df1866f949 100644
> > --- a/fs/xfs/libxfs/xfs_alloc.c
> > +++ b/fs/xfs/libxfs/xfs_alloc.c
> > @@ -2237,6 +2237,8 @@ xfs_alloc_fix_freelist(
> >  	xfs_extlen_t		need;	/* total blocks needed in freelist */
> >  	int			error = 0;
> >  
> > +	ASSERT(tp->t_flags & XFS_TRANS_PERM_LOG_RES);
> > +
> 
> Naturally, just after sending this I'm reminded I never went back to
> actually audit usage of XFS_ALLOC_FLAG_NOSHRINK. :P NOSHRINK is
> currently used in userspace (repair) and scrub. The former is in a place
> that looks to me like it already uses a permanent transaction. The
> latter is not immediately clear to me. It looks like scrub can alloc
> either tr_itruncate, which is permanent, or an empty transaction. The
> noshrink call is associated with repair, which I think means we'd have
> the permanent transaction (?), but it still might make sense to exclude
> NOSHRINK callers from failing the assert in principle. Thoughts?

Scrub should never be calling xfs_alloc_fix_freelist since it's a
readonly operation, and repair should never be running with an empty
transaction, so this should be fine kernel-side.

As for xfs_repair... yes, it does call libxfs_trans_alloc_rollable to
get a "permanent" transaction (which is actually tr_itruncate too).  So
this should be fine in xfsprogs too.

--D

> Brian
> 
> >  	if (!pag->pagf_init) {
> >  		error = xfs_alloc_read_agf(mp, tp, args->agno, flags, &agbp);
> >  		if (error)
> > 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);
> > -- 
> > 2.17.2
> > 

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

[Darrick J. Wong]

On Wed, Apr 17, 2019 at 05:36:08AM -0400, Brian Foster wrote:
> 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>
> ---
> 
> This is motivated by Darrick's recent xfs/104 failure report[1]. Note
> that I made the assert a bit more explicit than originally suggested
> because I think any transaction that performs block allocation should be
> expected to be able to handle arbitrary AGFL fixups. This survives an
> fstests auto run without any regressions[2] or assert failures.
> 
> Brian
> 
> [1] https://marc.info/?l=linux-xfs&m=155537961822223&w=2
> [2] Note that I was never able to reproduce the original xfs/104
> failure.
> 
>  fs/xfs/libxfs/xfs_alloc.c      | 2 ++
>  fs/xfs/libxfs/xfs_trans_resv.c | 6 +++++-
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> index bc3367b8b7bb..e4df1866f949 100644
> --- a/fs/xfs/libxfs/xfs_alloc.c
> +++ b/fs/xfs/libxfs/xfs_alloc.c
> @@ -2237,6 +2237,8 @@ xfs_alloc_fix_freelist(
>  	xfs_extlen_t		need;	/* total blocks needed in freelist */
>  	int			error = 0;
>  
> +	ASSERT(tp->t_flags & XFS_TRANS_PERM_LOG_RES);

/me wonders if we ought to have a comment here about why we're asserting
on this? i.e.

/*
 * This function can shrink the AGFL, which uses a deferred op to avoid
 * exceeding transaction reservation (or whatever the original reason
 * was).  Deferred ops require a transaction with a permanent
 * reservation, so check that here.
 */
ASSERT(...);

Send me back a finished comment and I'll add it on its way in; the rest
of the patch looks ok to me.

--D

> +
>  	if (!pag->pagf_init) {
>  		error = xfs_alloc_read_agf(mp, tp, args->agno, flags, &agbp);
>  		if (error)
> 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);
> -- 
> 2.17.2
> 

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

[Brian Foster]

On Wed, Apr 17, 2019 at 07:37:59AM -0700, Darrick J. Wong wrote:
> On Wed, Apr 17, 2019 at 05:36:08AM -0400, Brian Foster wrote:
> > 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>
> > ---
> > 
> > This is motivated by Darrick's recent xfs/104 failure report[1]. Note
> > that I made the assert a bit more explicit than originally suggested
> > because I think any transaction that performs block allocation should be
> > expected to be able to handle arbitrary AGFL fixups. This survives an
> > fstests auto run without any regressions[2] or assert failures.
> > 
> > Brian
> > 
> > [1] https://marc.info/?l=linux-xfs&m=155537961822223&w=2
> > [2] Note that I was never able to reproduce the original xfs/104
> > failure.
> > 
> >  fs/xfs/libxfs/xfs_alloc.c      | 2 ++
> >  fs/xfs/libxfs/xfs_trans_resv.c | 6 +++++-
> >  2 files changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> > index bc3367b8b7bb..e4df1866f949 100644
> > --- a/fs/xfs/libxfs/xfs_alloc.c
> > +++ b/fs/xfs/libxfs/xfs_alloc.c
> > @@ -2237,6 +2237,8 @@ xfs_alloc_fix_freelist(
> >  	xfs_extlen_t		need;	/* total blocks needed in freelist */
> >  	int			error = 0;
> >  
> > +	ASSERT(tp->t_flags & XFS_TRANS_PERM_LOG_RES);
> 
> /me wonders if we ought to have a comment here about why we're asserting
> on this? i.e.
> 

Yep.

> /*
>  * This function can shrink the AGFL, which uses a deferred op to avoid
>  * exceeding transaction reservation (or whatever the original reason
>  * was).  Deferred ops require a transaction with a permanent
>  * reservation, so check that here.
>  */
> ASSERT(...);
> 
> Send me back a finished comment and I'll add it on its way in; the rest
> of the patch looks ok to me.
> 

Though to me a one-liner seems sufficient since this is just an assert.
E.g.,

	/* deferred ops (AGFL block frees) require permanent transactions */
	ASSERT(tp->t_flags & XFS_TRANS_PERM_LOG_RES);

... seems enough to document the intent. Hm?

Brian

> --D
> 
> > +
> >  	if (!pag->pagf_init) {
> >  		error = xfs_alloc_read_agf(mp, tp, args->agno, flags, &agbp);
> >  		if (error)
> > 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);
> > -- 
> > 2.17.2
> > 

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

[Darrick J. Wong]

On Wed, Apr 17, 2019 at 11:33:15AM -0400, Brian Foster wrote:
> On Wed, Apr 17, 2019 at 07:37:59AM -0700, Darrick J. Wong wrote:
> > On Wed, Apr 17, 2019 at 05:36:08AM -0400, Brian Foster wrote:
> > > 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>
> > > ---
> > > 
> > > This is motivated by Darrick's recent xfs/104 failure report[1]. Note
> > > that I made the assert a bit more explicit than originally suggested
> > > because I think any transaction that performs block allocation should be
> > > expected to be able to handle arbitrary AGFL fixups. This survives an
> > > fstests auto run without any regressions[2] or assert failures.
> > > 
> > > Brian
> > > 
> > > [1] https://marc.info/?l=linux-xfs&m=155537961822223&w=2
> > > [2] Note that I was never able to reproduce the original xfs/104
> > > failure.
> > > 
> > >  fs/xfs/libxfs/xfs_alloc.c      | 2 ++
> > >  fs/xfs/libxfs/xfs_trans_resv.c | 6 +++++-
> > >  2 files changed, 7 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> > > index bc3367b8b7bb..e4df1866f949 100644
> > > --- a/fs/xfs/libxfs/xfs_alloc.c
> > > +++ b/fs/xfs/libxfs/xfs_alloc.c
> > > @@ -2237,6 +2237,8 @@ xfs_alloc_fix_freelist(
> > >  	xfs_extlen_t		need;	/* total blocks needed in freelist */
> > >  	int			error = 0;
> > >  
> > > +	ASSERT(tp->t_flags & XFS_TRANS_PERM_LOG_RES);
> > 
> > /me wonders if we ought to have a comment here about why we're asserting
> > on this? i.e.
> > 
> 
> Yep.
> 
> > /*
> >  * This function can shrink the AGFL, which uses a deferred op to avoid
> >  * exceeding transaction reservation (or whatever the original reason
> >  * was).  Deferred ops require a transaction with a permanent
> >  * reservation, so check that here.
> >  */
> > ASSERT(...);
> > 
> > Send me back a finished comment and I'll add it on its way in; the rest
> > of the patch looks ok to me.
> > 
> 
> Though to me a one-liner seems sufficient since this is just an assert.
> E.g.,
> 
> 	/* deferred ops (AGFL block frees) require permanent transactions */
> 	ASSERT(tp->t_flags & XFS_TRANS_PERM_LOG_RES);
> 
> ... seems enough to document the intent. Hm?

Yep.  Comment added.

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> Brian
> 
> > --D
> > 
> > > +
> > >  	if (!pag->pagf_init) {
> > >  		error = xfs_alloc_read_agf(mp, tp, args->agno, flags, &agbp);
> > >  		if (error)
> > > 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);
> > > -- 
> > > 2.17.2
> > > 

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

[Christoph Hellwig]

On Wed, Apr 17, 2019 at 07:37:59AM -0700, Darrick J. Wong wrote:
> 
> /me wonders if we ought to have a comment here about why we're asserting
> on this? i.e.
> /*
>  * This function can shrink the AGFL, which uses a deferred op to avoid
>  * exceeding transaction reservation (or whatever the original reason
>  * was).  Deferred ops require a transaction with a permanent
>  * reservation, so check that here.
>  */
> ASSERT(...);

Yes, I think we should.  Also I think adding this assert should be
a separate commit from the original fix to even more clearly document
the intention.

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

[Darrick J. Wong]

On Mon, Apr 22, 2019 at 11:24:47PM -0700, Christoph Hellwig wrote:
> On Wed, Apr 17, 2019 at 07:37:59AM -0700, Darrick J. Wong wrote:
> > 
> > /me wonders if we ought to have a comment here about why we're asserting
> > on this? i.e.
> > /*
> >  * This function can shrink the AGFL, which uses a deferred op to avoid
> >  * exceeding transaction reservation (or whatever the original reason
> >  * was).  Deferred ops require a transaction with a permanent
> >  * reservation, so check that here.
> >  */
> > ASSERT(...);
> 
> Yes, I think we should.  Also I think adding this assert should be
> a separate commit from the original fix to even more clearly document
> the intention.

<shrug> OK.  I chatted with Brian and we'll just split the assert into a
new patch...  eh, this patch has changed enough I'm going to send both
of them to the list for everyone's perusal.  (I'd already added them to
my tree, but having not pushed to for-next it isn't too late to make
more changes.)

--D

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

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

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