| Message ID | 20190423150709.GG4676@magnolia (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | xfs: make tr_growdata a permanent transaction | expand |
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);
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);
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);