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@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxx
