diff mbox series

[4/6] xfs: reduce the absurdly large log reservations

Message ID 164997688838.383881.2386659608282052005.stgit@magnolia (mailing list archive)
State New, archived
Headers show
Series xfs: fix reflink inefficiencies | expand

Commit Message

Darrick J. Wong April 14, 2022, 10:54 p.m. UTC
From: Darrick J. Wong <djwong@kernel.org>

Back in the early days of reflink and rmap development I set the
transaction reservation sizes to be overly generous for rmap+reflink
filesystems, and a little under-generous for rmap-only filesystems.

Since we don't need *eight* transaction rolls to handle three new log
intent items, decrease the logcounts to what we actually need, and amend
the shadow reservation computation function to reflect what we used to
do so that the minimum log size doesn't change.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/libxfs/xfs_trans_resv.c |   88 +++++++++++++++++++++++++++-------------
 fs/xfs/libxfs/xfs_trans_resv.h |    6 ++-
 2 files changed, 64 insertions(+), 30 deletions(-)

Comments

Dave Chinner April 22, 2022, 10:51 p.m. UTC | #1
On Thu, Apr 14, 2022 at 03:54:48PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Back in the early days of reflink and rmap development I set the
> transaction reservation sizes to be overly generous for rmap+reflink
> filesystems, and a little under-generous for rmap-only filesystems.
> 
> Since we don't need *eight* transaction rolls to handle three new log
> intent items, decrease the logcounts to what we actually need, and amend
> the shadow reservation computation function to reflect what we used to
> do so that the minimum log size doesn't change.

Yup, this will make a huge difference to the number of transactions
we can have in flight on reflink/rmap enabled filesystems.

Mostly looks good, some comments about code and comments below.

> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/libxfs/xfs_trans_resv.c |   88 +++++++++++++++++++++++++++-------------
>  fs/xfs/libxfs/xfs_trans_resv.h |    6 ++-
>  2 files changed, 64 insertions(+), 30 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
> index 12d4e451e70e..8d2f04dddb65 100644
> --- a/fs/xfs/libxfs/xfs_trans_resv.c
> +++ b/fs/xfs/libxfs/xfs_trans_resv.c
> @@ -814,36 +814,16 @@ xfs_trans_resv_calc(
>  	struct xfs_mount	*mp,
>  	struct xfs_trans_resv	*resp)
>  {
> -	unsigned int		rmap_maxlevels = mp->m_rmap_maxlevels;
> -
> -	/*
> -	 * In the early days of rmap+reflink, we always set the rmap maxlevels
> -	 * to 9 even if the AG was small enough that it would never grow to
> -	 * that height.  Transaction reservation sizes influence the minimum
> -	 * log size calculation, which influences the size of the log that mkfs
> -	 * creates.  Use the old value here to ensure that newly formatted
> -	 * small filesystems will mount on older kernels.
> -	 */
> -	if (xfs_has_rmapbt(mp) && xfs_has_reflink(mp))
> -		mp->m_rmap_maxlevels = XFS_OLD_REFLINK_RMAP_MAXLEVELS;
> -
>  	/*
>  	 * The following transactions are logged in physical format and
>  	 * require a permanent reservation on space.
>  	 */
>  	resp->tr_write.tr_logres = xfs_calc_write_reservation(mp);
> -	if (xfs_has_reflink(mp))
> -		resp->tr_write.tr_logcount = XFS_WRITE_LOG_COUNT_REFLINK;
> -	else
> -		resp->tr_write.tr_logcount = XFS_WRITE_LOG_COUNT;
> +	resp->tr_write.tr_logcount = XFS_WRITE_LOG_COUNT;
>  	resp->tr_write.tr_logflags |= XFS_TRANS_PERM_LOG_RES;
>  
>  	resp->tr_itruncate.tr_logres = xfs_calc_itruncate_reservation(mp);
> -	if (xfs_has_reflink(mp))
> -		resp->tr_itruncate.tr_logcount =
> -				XFS_ITRUNCATE_LOG_COUNT_REFLINK;
> -	else
> -		resp->tr_itruncate.tr_logcount = XFS_ITRUNCATE_LOG_COUNT;
> +	resp->tr_itruncate.tr_logcount = XFS_ITRUNCATE_LOG_COUNT;
>  	resp->tr_itruncate.tr_logflags |= XFS_TRANS_PERM_LOG_RES;
>  
>  	resp->tr_rename.tr_logres = xfs_calc_rename_reservation(mp);
> @@ -900,10 +880,7 @@ xfs_trans_resv_calc(
>  	resp->tr_growrtalloc.tr_logflags |= XFS_TRANS_PERM_LOG_RES;
>  
>  	resp->tr_qm_dqalloc.tr_logres = xfs_calc_qm_dqalloc_reservation(mp);
> -	if (xfs_has_reflink(mp))
> -		resp->tr_qm_dqalloc.tr_logcount = XFS_WRITE_LOG_COUNT_REFLINK;
> -	else
> -		resp->tr_qm_dqalloc.tr_logcount = XFS_WRITE_LOG_COUNT;
> +	resp->tr_qm_dqalloc.tr_logcount = XFS_WRITE_LOG_COUNT;
>  	resp->tr_qm_dqalloc.tr_logflags |= XFS_TRANS_PERM_LOG_RES;
>  
>  	/*
> @@ -930,8 +907,26 @@ xfs_trans_resv_calc(
>  	resp->tr_growrtzero.tr_logres = xfs_calc_growrtzero_reservation(mp);
>  	resp->tr_growrtfree.tr_logres = xfs_calc_growrtfree_reservation(mp);
>  
> -	/* Put everything back the way it was.  This goes at the end. */
> -	mp->m_rmap_maxlevels = rmap_maxlevels;
> +	/* Add one logcount for BUI items that appear with rmap or reflink. */
> +	if (xfs_has_reflink(mp) || xfs_has_rmapbt(mp)) {
> +		resp->tr_itruncate.tr_logcount++;
> +		resp->tr_write.tr_logcount++;
> +		resp->tr_qm_dqalloc.tr_logcount++;
> +	}
> +
> +	/* Add one logcount for refcount intent items. */
> +	if (xfs_has_reflink(mp)) {
> +		resp->tr_itruncate.tr_logcount++;
> +		resp->tr_write.tr_logcount++;
> +		resp->tr_qm_dqalloc.tr_logcount++;
> +	}
> +
> +	/* Add one logcount for rmap intent items. */
> +	if (xfs_has_rmapbt(mp)) {
> +		resp->tr_itruncate.tr_logcount++;
> +		resp->tr_write.tr_logcount++;
> +		resp->tr_qm_dqalloc.tr_logcount++;
> +	}

This would be much more concisely written as

	count = 0;
	if (xfs_has_reflink(mp) || xfs_has_rmapbt(mp)) {
		count = 2;
		if (xfs_has_reflink(mp) && xfs_has_rmapbt(mp))
			count++;
	}

	resp->tr_itruncate.tr_logcount += count;
	resp->tr_write.tr_logcount += count;
	resp->tr_qm_dqalloc.tr_logcount += count;

>  }
>  
>  /*
> @@ -943,5 +938,42 @@ xfs_trans_resv_calc_logsize(
>  	struct xfs_mount	*mp,
>  	struct xfs_trans_resv	*resp)
>  {
> +	unsigned int		rmap_maxlevels = mp->m_rmap_maxlevels;
> +
> +	ASSERT(resp != M_RES(mp));
> +
> +	/*
> +	 * In the early days of rmap+reflink, we always set the rmap maxlevels
> +	 * to 9 even if the AG was small enough that it would never grow to
> +	 * that height.  Transaction reservation sizes influence the minimum
> +	 * log size calculation, which influences the size of the log that mkfs
> +	 * creates.  Use the old value here to ensure that newly formatted
> +	 * small filesystems will mount on older kernels.
> +	 */
> +	if (xfs_has_rmapbt(mp) && xfs_has_reflink(mp))
> +		mp->m_rmap_maxlevels = XFS_OLD_REFLINK_RMAP_MAXLEVELS;
> +
>  	xfs_trans_resv_calc(mp, resp);
> +
> +	if (xfs_has_reflink(mp)) {
> +		/*
> +		 * In the early days of reflink we set the logcounts absurdly
> +		 * high.

"In the early days of reflink, typical operation log counts were
greatly overestimated"

> +		 */
> +		resp->tr_write.tr_logcount = XFS_WRITE_LOG_COUNT_REFLINK;
> +		resp->tr_itruncate.tr_logcount =
> +				XFS_ITRUNCATE_LOG_COUNT_REFLINK;
> +		resp->tr_qm_dqalloc.tr_logcount = XFS_WRITE_LOG_COUNT_REFLINK;
> +	} else if (xfs_has_rmapbt(mp)) {
> +		/*
> +		 * In the early days of non-reflink rmap we set the logcount
> +		 * too low.

"In the early days of non-reflink rmap the impact of rmap btree
updates on log counts was not taken into account at all."

> +		 */
> +		resp->tr_write.tr_logcount = XFS_WRITE_LOG_COUNT;
> +		resp->tr_itruncate.tr_logcount = XFS_ITRUNCATE_LOG_COUNT;
> +		resp->tr_qm_dqalloc.tr_logcount = XFS_WRITE_LOG_COUNT;
> +	}
> +
> +	/* Put everything back the way it was.  This goes at the end. */
> +	mp->m_rmap_maxlevels = rmap_maxlevels;
>  }

Yeah, so I think this should all go in xfs_log_rlimit.c as it is
specific to the minimum log size calculation.

> diff --git a/fs/xfs/libxfs/xfs_trans_resv.h b/fs/xfs/libxfs/xfs_trans_resv.h
> index 9fa4863f72a4..461859f4a745 100644
> --- a/fs/xfs/libxfs/xfs_trans_resv.h
> +++ b/fs/xfs/libxfs/xfs_trans_resv.h
> @@ -73,7 +73,6 @@ struct xfs_trans_resv {
>  #define	XFS_DEFAULT_LOG_COUNT		1
>  #define	XFS_DEFAULT_PERM_LOG_COUNT	2
>  #define	XFS_ITRUNCATE_LOG_COUNT		2
> -#define	XFS_ITRUNCATE_LOG_COUNT_REFLINK	8
>  #define XFS_INACTIVE_LOG_COUNT		2
>  #define	XFS_CREATE_LOG_COUNT		2
>  #define	XFS_CREATE_TMPFILE_LOG_COUNT	2
> @@ -83,12 +82,15 @@ struct xfs_trans_resv {
>  #define	XFS_LINK_LOG_COUNT		2
>  #define	XFS_RENAME_LOG_COUNT		2
>  #define	XFS_WRITE_LOG_COUNT		2
> -#define	XFS_WRITE_LOG_COUNT_REFLINK	8
>  #define	XFS_ADDAFORK_LOG_COUNT		2
>  #define	XFS_ATTRINVAL_LOG_COUNT		1
>  #define	XFS_ATTRSET_LOG_COUNT		3
>  #define	XFS_ATTRRM_LOG_COUNT		3
>  
> +/* Absurdly high log counts from the early days of reflink.  Do not use. */
> +#define	XFS_ITRUNCATE_LOG_COUNT_REFLINK	8
> +#define	XFS_WRITE_LOG_COUNT_REFLINK	8

/*
 * Original log counts were overestimated in the early days of
 * reflink. These are retained here purely for minimum log size
 * calculations and are not to be used for runtime reservations.
 */

Cheers,

Dave.
Darrick J. Wong April 25, 2022, 11:47 p.m. UTC | #2
On Sat, Apr 23, 2022 at 08:51:52AM +1000, Dave Chinner wrote:
> On Thu, Apr 14, 2022 at 03:54:48PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Back in the early days of reflink and rmap development I set the
> > transaction reservation sizes to be overly generous for rmap+reflink
> > filesystems, and a little under-generous for rmap-only filesystems.
> > 
> > Since we don't need *eight* transaction rolls to handle three new log
> > intent items, decrease the logcounts to what we actually need, and amend
> > the shadow reservation computation function to reflect what we used to
> > do so that the minimum log size doesn't change.
> 
> Yup, this will make a huge difference to the number of transactions
> we can have in flight on reflink/rmap enabled filesystems.
> 
> Mostly looks good, some comments about code and comments below.

Yay!

> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  fs/xfs/libxfs/xfs_trans_resv.c |   88 +++++++++++++++++++++++++++-------------
> >  fs/xfs/libxfs/xfs_trans_resv.h |    6 ++-
> >  2 files changed, 64 insertions(+), 30 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
> > index 12d4e451e70e..8d2f04dddb65 100644
> > --- a/fs/xfs/libxfs/xfs_trans_resv.c
> > +++ b/fs/xfs/libxfs/xfs_trans_resv.c
> > @@ -814,36 +814,16 @@ xfs_trans_resv_calc(
> >  	struct xfs_mount	*mp,
> >  	struct xfs_trans_resv	*resp)
> >  {
> > -	unsigned int		rmap_maxlevels = mp->m_rmap_maxlevels;
> > -
> > -	/*
> > -	 * In the early days of rmap+reflink, we always set the rmap maxlevels
> > -	 * to 9 even if the AG was small enough that it would never grow to
> > -	 * that height.  Transaction reservation sizes influence the minimum
> > -	 * log size calculation, which influences the size of the log that mkfs
> > -	 * creates.  Use the old value here to ensure that newly formatted
> > -	 * small filesystems will mount on older kernels.
> > -	 */
> > -	if (xfs_has_rmapbt(mp) && xfs_has_reflink(mp))
> > -		mp->m_rmap_maxlevels = XFS_OLD_REFLINK_RMAP_MAXLEVELS;
> > -
> >  	/*
> >  	 * The following transactions are logged in physical format and
> >  	 * require a permanent reservation on space.
> >  	 */
> >  	resp->tr_write.tr_logres = xfs_calc_write_reservation(mp);
> > -	if (xfs_has_reflink(mp))
> > -		resp->tr_write.tr_logcount = XFS_WRITE_LOG_COUNT_REFLINK;
> > -	else
> > -		resp->tr_write.tr_logcount = XFS_WRITE_LOG_COUNT;
> > +	resp->tr_write.tr_logcount = XFS_WRITE_LOG_COUNT;
> >  	resp->tr_write.tr_logflags |= XFS_TRANS_PERM_LOG_RES;
> >  
> >  	resp->tr_itruncate.tr_logres = xfs_calc_itruncate_reservation(mp);
> > -	if (xfs_has_reflink(mp))
> > -		resp->tr_itruncate.tr_logcount =
> > -				XFS_ITRUNCATE_LOG_COUNT_REFLINK;
> > -	else
> > -		resp->tr_itruncate.tr_logcount = XFS_ITRUNCATE_LOG_COUNT;
> > +	resp->tr_itruncate.tr_logcount = XFS_ITRUNCATE_LOG_COUNT;
> >  	resp->tr_itruncate.tr_logflags |= XFS_TRANS_PERM_LOG_RES;
> >  
> >  	resp->tr_rename.tr_logres = xfs_calc_rename_reservation(mp);
> > @@ -900,10 +880,7 @@ xfs_trans_resv_calc(
> >  	resp->tr_growrtalloc.tr_logflags |= XFS_TRANS_PERM_LOG_RES;
> >  
> >  	resp->tr_qm_dqalloc.tr_logres = xfs_calc_qm_dqalloc_reservation(mp);
> > -	if (xfs_has_reflink(mp))
> > -		resp->tr_qm_dqalloc.tr_logcount = XFS_WRITE_LOG_COUNT_REFLINK;
> > -	else
> > -		resp->tr_qm_dqalloc.tr_logcount = XFS_WRITE_LOG_COUNT;
> > +	resp->tr_qm_dqalloc.tr_logcount = XFS_WRITE_LOG_COUNT;
> >  	resp->tr_qm_dqalloc.tr_logflags |= XFS_TRANS_PERM_LOG_RES;
> >  
> >  	/*
> > @@ -930,8 +907,26 @@ xfs_trans_resv_calc(
> >  	resp->tr_growrtzero.tr_logres = xfs_calc_growrtzero_reservation(mp);
> >  	resp->tr_growrtfree.tr_logres = xfs_calc_growrtfree_reservation(mp);
> >  
> > -	/* Put everything back the way it was.  This goes at the end. */
> > -	mp->m_rmap_maxlevels = rmap_maxlevels;
> > +	/* Add one logcount for BUI items that appear with rmap or reflink. */
> > +	if (xfs_has_reflink(mp) || xfs_has_rmapbt(mp)) {
> > +		resp->tr_itruncate.tr_logcount++;
> > +		resp->tr_write.tr_logcount++;
> > +		resp->tr_qm_dqalloc.tr_logcount++;
> > +	}
> > +
> > +	/* Add one logcount for refcount intent items. */
> > +	if (xfs_has_reflink(mp)) {
> > +		resp->tr_itruncate.tr_logcount++;
> > +		resp->tr_write.tr_logcount++;
> > +		resp->tr_qm_dqalloc.tr_logcount++;
> > +	}
> > +
> > +	/* Add one logcount for rmap intent items. */
> > +	if (xfs_has_rmapbt(mp)) {
> > +		resp->tr_itruncate.tr_logcount++;
> > +		resp->tr_write.tr_logcount++;
> > +		resp->tr_qm_dqalloc.tr_logcount++;
> > +	}
> 
> This would be much more concisely written as
> 
> 	count = 0;
> 	if (xfs_has_reflink(mp) || xfs_has_rmapbt(mp)) {
> 		count = 2;
> 		if (xfs_has_reflink(mp) && xfs_has_rmapbt(mp))
> 			count++;
> 	}
> 
> 	resp->tr_itruncate.tr_logcount += count;
> 	resp->tr_write.tr_logcount += count;
> 	resp->tr_qm_dqalloc.tr_logcount += count;

I think I'd rather do:

	/*
	 * Add one logcount for BUI items that appear with rmap or reflink,
	 * one logcount for refcount intent items, and one logcount for rmap
	 * intent items.
	 */
	if (xfs_has_reflink(mp) || xfs_has_rmapbt(mp))
		logcount_adj++;
	if (xfs_has_reflink(mp))
		logcount_adj++;
	if (xfs_has_rmapbt(mp))
		logcount_adj++;

	resp->tr_itruncate.tr_logcount += logcount_adj;
	resp->tr_write.tr_logcount += logcount_adj;
	resp->tr_qm_dqalloc.tr_logcount += logcount_adj;

If you don't mind?

> 
> >  }
> >  
> >  /*
> > @@ -943,5 +938,42 @@ xfs_trans_resv_calc_logsize(
> >  	struct xfs_mount	*mp,
> >  	struct xfs_trans_resv	*resp)
> >  {
> > +	unsigned int		rmap_maxlevels = mp->m_rmap_maxlevels;
> > +
> > +	ASSERT(resp != M_RES(mp));
> > +
> > +	/*
> > +	 * In the early days of rmap+reflink, we always set the rmap maxlevels
> > +	 * to 9 even if the AG was small enough that it would never grow to
> > +	 * that height.  Transaction reservation sizes influence the minimum
> > +	 * log size calculation, which influences the size of the log that mkfs
> > +	 * creates.  Use the old value here to ensure that newly formatted
> > +	 * small filesystems will mount on older kernels.
> > +	 */
> > +	if (xfs_has_rmapbt(mp) && xfs_has_reflink(mp))
> > +		mp->m_rmap_maxlevels = XFS_OLD_REFLINK_RMAP_MAXLEVELS;
> > +
> >  	xfs_trans_resv_calc(mp, resp);
> > +
> > +	if (xfs_has_reflink(mp)) {
> > +		/*
> > +		 * In the early days of reflink we set the logcounts absurdly
> > +		 * high.
> 
> "In the early days of reflink, typical operation log counts were
> greatly overestimated"

Fixed.

> > +		 */
> > +		resp->tr_write.tr_logcount = XFS_WRITE_LOG_COUNT_REFLINK;
> > +		resp->tr_itruncate.tr_logcount =
> > +				XFS_ITRUNCATE_LOG_COUNT_REFLINK;
> > +		resp->tr_qm_dqalloc.tr_logcount = XFS_WRITE_LOG_COUNT_REFLINK;
> > +	} else if (xfs_has_rmapbt(mp)) {
> > +		/*
> > +		 * In the early days of non-reflink rmap we set the logcount
> > +		 * too low.
> 
> "In the early days of non-reflink rmap the impact of rmap btree
> updates on log counts was not taken into account at all."

Fixed.

> > +		 */
> > +		resp->tr_write.tr_logcount = XFS_WRITE_LOG_COUNT;
> > +		resp->tr_itruncate.tr_logcount = XFS_ITRUNCATE_LOG_COUNT;
> > +		resp->tr_qm_dqalloc.tr_logcount = XFS_WRITE_LOG_COUNT;
> > +	}
> > +
> > +	/* Put everything back the way it was.  This goes at the end. */
> > +	mp->m_rmap_maxlevels = rmap_maxlevels;
> >  }
> 
> Yeah, so I think this should all go in xfs_log_rlimit.c as it is
> specific to the minimum log size calculation.

Moved.

> > diff --git a/fs/xfs/libxfs/xfs_trans_resv.h b/fs/xfs/libxfs/xfs_trans_resv.h
> > index 9fa4863f72a4..461859f4a745 100644
> > --- a/fs/xfs/libxfs/xfs_trans_resv.h
> > +++ b/fs/xfs/libxfs/xfs_trans_resv.h
> > @@ -73,7 +73,6 @@ struct xfs_trans_resv {
> >  #define	XFS_DEFAULT_LOG_COUNT		1
> >  #define	XFS_DEFAULT_PERM_LOG_COUNT	2
> >  #define	XFS_ITRUNCATE_LOG_COUNT		2
> > -#define	XFS_ITRUNCATE_LOG_COUNT_REFLINK	8
> >  #define XFS_INACTIVE_LOG_COUNT		2
> >  #define	XFS_CREATE_LOG_COUNT		2
> >  #define	XFS_CREATE_TMPFILE_LOG_COUNT	2
> > @@ -83,12 +82,15 @@ struct xfs_trans_resv {
> >  #define	XFS_LINK_LOG_COUNT		2
> >  #define	XFS_RENAME_LOG_COUNT		2
> >  #define	XFS_WRITE_LOG_COUNT		2
> > -#define	XFS_WRITE_LOG_COUNT_REFLINK	8
> >  #define	XFS_ADDAFORK_LOG_COUNT		2
> >  #define	XFS_ATTRINVAL_LOG_COUNT		1
> >  #define	XFS_ATTRSET_LOG_COUNT		3
> >  #define	XFS_ATTRRM_LOG_COUNT		3
> >  
> > +/* Absurdly high log counts from the early days of reflink.  Do not use. */
> > +#define	XFS_ITRUNCATE_LOG_COUNT_REFLINK	8
> > +#define	XFS_WRITE_LOG_COUNT_REFLINK	8
> 
> /*
>  * Original log counts were overestimated in the early days of
>  * reflink. These are retained here purely for minimum log size
>  * calculations and are not to be used for runtime reservations.
>  */

Fixed, thanks. :)

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
Dave Chinner April 26, 2022, 4:25 a.m. UTC | #3
On Mon, Apr 25, 2022 at 04:47:49PM -0700, Darrick J. Wong wrote:
> On Sat, Apr 23, 2022 at 08:51:52AM +1000, Dave Chinner wrote:
> > On Thu, Apr 14, 2022 at 03:54:48PM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > > 
> > > Back in the early days of reflink and rmap development I set the
> > > transaction reservation sizes to be overly generous for rmap+reflink
> > > filesystems, and a little under-generous for rmap-only filesystems.
> > > 
> > > Since we don't need *eight* transaction rolls to handle three new log
> > > intent items, decrease the logcounts to what we actually need, and amend
> > > the shadow reservation computation function to reflect what we used to
> > > do so that the minimum log size doesn't change.
> > 
> > Yup, this will make a huge difference to the number of transactions
> > we can have in flight on reflink/rmap enabled filesystems.
> > 
> > Mostly looks good, some comments about code and comments below.
....
> > > -	/* Put everything back the way it was.  This goes at the end. */
> > > -	mp->m_rmap_maxlevels = rmap_maxlevels;
> > > +	/* Add one logcount for BUI items that appear with rmap or reflink. */
> > > +	if (xfs_has_reflink(mp) || xfs_has_rmapbt(mp)) {
> > > +		resp->tr_itruncate.tr_logcount++;
> > > +		resp->tr_write.tr_logcount++;
> > > +		resp->tr_qm_dqalloc.tr_logcount++;
> > > +	}
> > > +
> > > +	/* Add one logcount for refcount intent items. */
> > > +	if (xfs_has_reflink(mp)) {
> > > +		resp->tr_itruncate.tr_logcount++;
> > > +		resp->tr_write.tr_logcount++;
> > > +		resp->tr_qm_dqalloc.tr_logcount++;
> > > +	}
> > > +
> > > +	/* Add one logcount for rmap intent items. */
> > > +	if (xfs_has_rmapbt(mp)) {
> > > +		resp->tr_itruncate.tr_logcount++;
> > > +		resp->tr_write.tr_logcount++;
> > > +		resp->tr_qm_dqalloc.tr_logcount++;
> > > +	}
> > 
> > This would be much more concisely written as
> > 
> > 	count = 0;
> > 	if (xfs_has_reflink(mp) || xfs_has_rmapbt(mp)) {
> > 		count = 2;
> > 		if (xfs_has_reflink(mp) && xfs_has_rmapbt(mp))
> > 			count++;
> > 	}
> > 
> > 	resp->tr_itruncate.tr_logcount += count;
> > 	resp->tr_write.tr_logcount += count;
> > 	resp->tr_qm_dqalloc.tr_logcount += count;
> 
> I think I'd rather do:
> 
> 	/*
> 	 * Add one logcount for BUI items that appear with rmap or reflink,
> 	 * one logcount for refcount intent items, and one logcount for rmap
> 	 * intent items.
> 	 */
> 	if (xfs_has_reflink(mp) || xfs_has_rmapbt(mp))
> 		logcount_adj++;
> 	if (xfs_has_reflink(mp))
> 		logcount_adj++;
> 	if (xfs_has_rmapbt(mp))
> 		logcount_adj++;
> 
> 	resp->tr_itruncate.tr_logcount += logcount_adj;
> 	resp->tr_write.tr_logcount += logcount_adj;
> 	resp->tr_qm_dqalloc.tr_logcount += logcount_adj;
> 
> If you don't mind?

Sure, that's just as good.

--Dave.
diff mbox series

Patch

diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
index 12d4e451e70e..8d2f04dddb65 100644
--- a/fs/xfs/libxfs/xfs_trans_resv.c
+++ b/fs/xfs/libxfs/xfs_trans_resv.c
@@ -814,36 +814,16 @@  xfs_trans_resv_calc(
 	struct xfs_mount	*mp,
 	struct xfs_trans_resv	*resp)
 {
-	unsigned int		rmap_maxlevels = mp->m_rmap_maxlevels;
-
-	/*
-	 * In the early days of rmap+reflink, we always set the rmap maxlevels
-	 * to 9 even if the AG was small enough that it would never grow to
-	 * that height.  Transaction reservation sizes influence the minimum
-	 * log size calculation, which influences the size of the log that mkfs
-	 * creates.  Use the old value here to ensure that newly formatted
-	 * small filesystems will mount on older kernels.
-	 */
-	if (xfs_has_rmapbt(mp) && xfs_has_reflink(mp))
-		mp->m_rmap_maxlevels = XFS_OLD_REFLINK_RMAP_MAXLEVELS;
-
 	/*
 	 * The following transactions are logged in physical format and
 	 * require a permanent reservation on space.
 	 */
 	resp->tr_write.tr_logres = xfs_calc_write_reservation(mp);
-	if (xfs_has_reflink(mp))
-		resp->tr_write.tr_logcount = XFS_WRITE_LOG_COUNT_REFLINK;
-	else
-		resp->tr_write.tr_logcount = XFS_WRITE_LOG_COUNT;
+	resp->tr_write.tr_logcount = XFS_WRITE_LOG_COUNT;
 	resp->tr_write.tr_logflags |= XFS_TRANS_PERM_LOG_RES;
 
 	resp->tr_itruncate.tr_logres = xfs_calc_itruncate_reservation(mp);
-	if (xfs_has_reflink(mp))
-		resp->tr_itruncate.tr_logcount =
-				XFS_ITRUNCATE_LOG_COUNT_REFLINK;
-	else
-		resp->tr_itruncate.tr_logcount = XFS_ITRUNCATE_LOG_COUNT;
+	resp->tr_itruncate.tr_logcount = XFS_ITRUNCATE_LOG_COUNT;
 	resp->tr_itruncate.tr_logflags |= XFS_TRANS_PERM_LOG_RES;
 
 	resp->tr_rename.tr_logres = xfs_calc_rename_reservation(mp);
@@ -900,10 +880,7 @@  xfs_trans_resv_calc(
 	resp->tr_growrtalloc.tr_logflags |= XFS_TRANS_PERM_LOG_RES;
 
 	resp->tr_qm_dqalloc.tr_logres = xfs_calc_qm_dqalloc_reservation(mp);
-	if (xfs_has_reflink(mp))
-		resp->tr_qm_dqalloc.tr_logcount = XFS_WRITE_LOG_COUNT_REFLINK;
-	else
-		resp->tr_qm_dqalloc.tr_logcount = XFS_WRITE_LOG_COUNT;
+	resp->tr_qm_dqalloc.tr_logcount = XFS_WRITE_LOG_COUNT;
 	resp->tr_qm_dqalloc.tr_logflags |= XFS_TRANS_PERM_LOG_RES;
 
 	/*
@@ -930,8 +907,26 @@  xfs_trans_resv_calc(
 	resp->tr_growrtzero.tr_logres = xfs_calc_growrtzero_reservation(mp);
 	resp->tr_growrtfree.tr_logres = xfs_calc_growrtfree_reservation(mp);
 
-	/* Put everything back the way it was.  This goes at the end. */
-	mp->m_rmap_maxlevels = rmap_maxlevels;
+	/* Add one logcount for BUI items that appear with rmap or reflink. */
+	if (xfs_has_reflink(mp) || xfs_has_rmapbt(mp)) {
+		resp->tr_itruncate.tr_logcount++;
+		resp->tr_write.tr_logcount++;
+		resp->tr_qm_dqalloc.tr_logcount++;
+	}
+
+	/* Add one logcount for refcount intent items. */
+	if (xfs_has_reflink(mp)) {
+		resp->tr_itruncate.tr_logcount++;
+		resp->tr_write.tr_logcount++;
+		resp->tr_qm_dqalloc.tr_logcount++;
+	}
+
+	/* Add one logcount for rmap intent items. */
+	if (xfs_has_rmapbt(mp)) {
+		resp->tr_itruncate.tr_logcount++;
+		resp->tr_write.tr_logcount++;
+		resp->tr_qm_dqalloc.tr_logcount++;
+	}
 }
 
 /*
@@ -943,5 +938,42 @@  xfs_trans_resv_calc_logsize(
 	struct xfs_mount	*mp,
 	struct xfs_trans_resv	*resp)
 {
+	unsigned int		rmap_maxlevels = mp->m_rmap_maxlevels;
+
+	ASSERT(resp != M_RES(mp));
+
+	/*
+	 * In the early days of rmap+reflink, we always set the rmap maxlevels
+	 * to 9 even if the AG was small enough that it would never grow to
+	 * that height.  Transaction reservation sizes influence the minimum
+	 * log size calculation, which influences the size of the log that mkfs
+	 * creates.  Use the old value here to ensure that newly formatted
+	 * small filesystems will mount on older kernels.
+	 */
+	if (xfs_has_rmapbt(mp) && xfs_has_reflink(mp))
+		mp->m_rmap_maxlevels = XFS_OLD_REFLINK_RMAP_MAXLEVELS;
+
 	xfs_trans_resv_calc(mp, resp);
+
+	if (xfs_has_reflink(mp)) {
+		/*
+		 * In the early days of reflink we set the logcounts absurdly
+		 * high.
+		 */
+		resp->tr_write.tr_logcount = XFS_WRITE_LOG_COUNT_REFLINK;
+		resp->tr_itruncate.tr_logcount =
+				XFS_ITRUNCATE_LOG_COUNT_REFLINK;
+		resp->tr_qm_dqalloc.tr_logcount = XFS_WRITE_LOG_COUNT_REFLINK;
+	} else if (xfs_has_rmapbt(mp)) {
+		/*
+		 * In the early days of non-reflink rmap we set the logcount
+		 * too low.
+		 */
+		resp->tr_write.tr_logcount = XFS_WRITE_LOG_COUNT;
+		resp->tr_itruncate.tr_logcount = XFS_ITRUNCATE_LOG_COUNT;
+		resp->tr_qm_dqalloc.tr_logcount = XFS_WRITE_LOG_COUNT;
+	}
+
+	/* Put everything back the way it was.  This goes at the end. */
+	mp->m_rmap_maxlevels = rmap_maxlevels;
 }
diff --git a/fs/xfs/libxfs/xfs_trans_resv.h b/fs/xfs/libxfs/xfs_trans_resv.h
index 9fa4863f72a4..461859f4a745 100644
--- a/fs/xfs/libxfs/xfs_trans_resv.h
+++ b/fs/xfs/libxfs/xfs_trans_resv.h
@@ -73,7 +73,6 @@  struct xfs_trans_resv {
 #define	XFS_DEFAULT_LOG_COUNT		1
 #define	XFS_DEFAULT_PERM_LOG_COUNT	2
 #define	XFS_ITRUNCATE_LOG_COUNT		2
-#define	XFS_ITRUNCATE_LOG_COUNT_REFLINK	8
 #define XFS_INACTIVE_LOG_COUNT		2
 #define	XFS_CREATE_LOG_COUNT		2
 #define	XFS_CREATE_TMPFILE_LOG_COUNT	2
@@ -83,12 +82,15 @@  struct xfs_trans_resv {
 #define	XFS_LINK_LOG_COUNT		2
 #define	XFS_RENAME_LOG_COUNT		2
 #define	XFS_WRITE_LOG_COUNT		2
-#define	XFS_WRITE_LOG_COUNT_REFLINK	8
 #define	XFS_ADDAFORK_LOG_COUNT		2
 #define	XFS_ATTRINVAL_LOG_COUNT		1
 #define	XFS_ATTRSET_LOG_COUNT		3
 #define	XFS_ATTRRM_LOG_COUNT		3
 
+/* Absurdly high log counts from the early days of reflink.  Do not use. */
+#define	XFS_ITRUNCATE_LOG_COUNT_REFLINK	8
+#define	XFS_WRITE_LOG_COUNT_REFLINK	8
+
 void xfs_trans_resv_calc_logsize(struct xfs_mount *mp,
 		struct xfs_trans_resv *resp);
 void xfs_trans_resv_calc(struct xfs_mount *mp, struct xfs_trans_resv *resp);