On Fri, 2022-09-23 at 13:17 -0700, Darrick J. Wong wrote:
> On Wed, Sep 21, 2022 at 10:44:45PM -0700,
> allison.henderson@xxxxxxxxxx wrote:
> > From: Allison Henderson <allison.henderson@xxxxxxxxxx>
> >
> > We need to add, remove or modify parent pointer attributes during
> > create/link/unlink/rename operations atomically with the dirents in
> > the
> > parent directories being modified. This means they need to be
> > modified
> > in the same transaction as the parent directories, and so we need
> > to add
> > the required space for the attribute modifications to the
> > transaction
> > reservations.
> >
> > Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> > Signed-off-by: Allison Henderson <allison.henderson@xxxxxxxxxx>
> > ---
> > fs/xfs/libxfs/xfs_trans_resv.c | 135 ++++++++++++++++++++++++++---
> > ----
> > 1 file changed, 106 insertions(+), 29 deletions(-)
> >
> > diff --git a/fs/xfs/libxfs/xfs_trans_resv.c
> > b/fs/xfs/libxfs/xfs_trans_resv.c
> > index 2c4ad6e4bb14..f7799800d556 100644
> > --- a/fs/xfs/libxfs/xfs_trans_resv.c
> > +++ b/fs/xfs/libxfs/xfs_trans_resv.c
> > @@ -19,6 +19,7 @@
> > #include "xfs_trans.h"
> > #include "xfs_qm.h"
> > #include "xfs_trans_space.h"
> > +#include "xfs_attr_item.h"
> >
> > #define _ALLOC true
> > #define _FREE false
> > @@ -421,28 +422,45 @@ xfs_calc_itruncate_reservation_minlogsize(
> > }
> >
> > /*
> > - * In renaming a files we can modify:
> > - * the four inodes involved: 4 * inode size
> > + * In renaming a files we can modify (t1):
> > + * the four inodes involved: 5 * inode size
>
> ...the *five* inodes involved...
>
> Also -- even before parent pointers we could have five inodes
> involved
> in a rename transaction, so I think this change needs to be a
> separate
> bugfix at the start of the series. Rename isn't experimental, so I
> can't let this one slide. :/
I see, ok I will split this one out then
>
> > * the two directory btrees: 2 * (max depth + v2) * dir block
> > size
> > * the two directory bmap btrees: 2 * max depth * block size
> > * And the bmap_finish transaction can free dir and bmap blocks
> > (two sets
> > - * of bmap blocks) giving:
> > + * of bmap blocks) giving (t2):
> > * the agf for the ags in which the blocks live: 3 * sector
> > size
> > * the agfl for the ags in which the blocks live: 3 * sector
> > size
> > * the superblock for the free block count: sector size
> > * the allocation btrees: 3 exts * 2 trees * (2 * max depth -
> > 1) * block size
> > + * If parent pointers are enabled (t3), then each transaction in
> > the chain
> > + * must be capable of setting or removing the extended
> > attribute
> > + * containing the parent information. It must also be able to
> > handle
> > + * the three xattr intent items that track the progress of the
> > parent
> > + * pointer update.
> > */
> > STATIC uint
> > xfs_calc_rename_reservation(
> > struct xfs_mount *mp)
> > {
> > - return XFS_DQUOT_LOGRES(mp) +
> > - max((xfs_calc_inode_res(mp, 4) +
> > - xfs_calc_buf_res(2 * XFS_DIROP_LOG_COUNT(mp),
> > - XFS_FSB_TO_B(mp, 1))),
> > - (xfs_calc_buf_res(7, mp->m_sb.sb_sectsize) +
> > - xfs_calc_buf_res(xfs_allocfree_block_count(mp,
> > 3),
> > - XFS_FSB_TO_B(mp, 1))));
> > + unsigned int overhead = XFS_DQUOT_LOGRES(mp);
> > + struct xfs_trans_resv *resp = M_RES(mp);
> > + unsigned int t1, t2, t3 = 0;
> > +
> > + t1 = xfs_calc_inode_res(mp, 5) +
> > + xfs_calc_buf_res(2 * XFS_DIROP_LOG_COUNT(mp),
> > + XFS_FSB_TO_B(mp, 1));
> > +
> > + t2 = xfs_calc_buf_res(7, mp->m_sb.sb_sectsize) +
> > + xfs_calc_buf_res(xfs_allocfree_block_count(mp, 3),
> > + XFS_FSB_TO_B(mp, 1));
> > +
> > + if (xfs_has_parent(mp)) {
> > + t3 = max(resp->tr_attrsetm.tr_logres,
> > + resp->tr_attrrm.tr_logres);
>
> Ooh I like this refactoring of xfs_calc_rename_reservation. :)
>
> I guess we now tr_attr{setm,rm} before computing the rename
> reservation
> so this is ok.
>
> > + overhead += 3 * (sizeof(struct
> > xfs_attri_log_item));
>
> Should the size of the name, newname, and value buffers be added into
> overhead? They take up log space too.
That would make sense, but we cant really calculate that ahead of time
with out just assuming the max size which is up to 64k for the value.
Maybe those sizes should be added on after we know what they are?
>
> > + }
> > +
> > + return overhead + max3(t1, t2, t3);
> > }
> >
> > /*
> > @@ -909,24 +927,59 @@ xfs_calc_sb_reservation(
> > return xfs_calc_buf_res(1, mp->m_sb.sb_sectsize);
> > }
> >
> > -void
> > -xfs_trans_resv_calc(
> > - struct xfs_mount *mp,
> > - struct xfs_trans_resv *resp)
> > +/*
> > + * Calculate extra space needed for parent pointer attributes
> > + */
> > +STATIC void
> > +xfs_calc_parent_ptr_reservations(
> > + struct xfs_mount *mp)
> > {
> > - int logcount_adj = 0;
> > + struct xfs_trans_resv *resp = M_RES(mp);
> >
> > - /*
> > - * 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,
> > false);
> > - resp->tr_write.tr_logcount = XFS_WRITE_LOG_COUNT;
> > - resp->tr_write.tr_logflags |= XFS_TRANS_PERM_LOG_RES;
> > + if (!xfs_has_parent(mp))
> > + return;
> >
> > - resp->tr_itruncate.tr_logres =
> > xfs_calc_itruncate_reservation(mp, false);
> > - resp->tr_itruncate.tr_logcount = XFS_ITRUNCATE_LOG_COUNT;
> > - resp->tr_itruncate.tr_logflags |= XFS_TRANS_PERM_LOG_RES;
> > + resp->tr_rename.tr_logres += max(resp-
> > >tr_attrsetm.tr_logres,
> > + resp-
> > >tr_attrrm.tr_logres);
> > + resp->tr_rename.tr_logcount += max(resp-
> > >tr_attrsetm.tr_logcount,
> > + resp-
> > >tr_attrrm.tr_logcount);
>
> Doesn't xfs_calc_rename_reservation add this to tr_rename already?
Oh, I think we can remove the tr_rename.tr_logres update. But not the
logcount update right?
xfs_calc_rename_reservation just calculates what tr_rename.tr_logres
should be, but it doesnt make any additions. It's that's used over in
xfs_calc_namespace_reservations, but we still need to update
tr_rename.tr_logcount which is separate field from tr_rename.tr_logres.
>
> > +
> > + resp->tr_create.tr_logres += resp->tr_attrsetm.tr_logres;
> > + resp->tr_create.tr_logcount += resp-
> > >tr_attrsetm.tr_logcount;
> > +
> > + resp->tr_mkdir.tr_logres += resp->tr_attrsetm.tr_logres;
> > + resp->tr_mkdir.tr_logcount += resp-
> > >tr_attrsetm.tr_logcount;
> > +
> > + resp->tr_link.tr_logres += resp->tr_attrsetm.tr_logres;
> > + resp->tr_link.tr_logcount += resp->tr_attrsetm.tr_logcount;
> > +
> > + resp->tr_symlink.tr_logres += resp->tr_attrsetm.tr_logres;
> > + resp->tr_symlink.tr_logcount += resp-
> > >tr_attrsetm.tr_logcount;
> > +
> > + resp->tr_remove.tr_logres += resp->tr_attrrm.tr_logres;
> > + resp->tr_remove.tr_logcount += resp->tr_attrrm.tr_logcount;
>
> Shouldn't each of these += additions be made to
> xfs_calc_{icreate,mkdir,link,symlink,remove}_reservation,
> respectively?
>
I suppose we could redo it that way? But then not all of the "+="
would disappear if they worked like xfs_calc_rename_reservation. Just
the *.tr_logres. Do we really want separate wrappers for just these
oneliners though?
Allison
> --D
>
> > +}
> > +
> > +/*
> > + * Namespace reservations.
> > + *
> > + * These get tricky when parent pointers are enabled as we have
> > attribute
> > + * modifications occurring from within these transactions. Rather
> > than confuse
> > + * each of these reservation calculations with the conditional
> > attribute
> > + * reservations, add them here in a clear and concise manner. This
> > assumes that
> > + * the attribute reservations have already been calculated.
> > + *
> > + * Note that we only include the static attribute reservation
> > here; the runtime
> > + * reservation will have to be modified by the size of the
> > attributes being
> > + * added/removed/modified. See the comments on the attribute
> > reservation
> > + * calculations for more details.
> > + */
> > +STATIC void
> > +xfs_calc_namespace_reservations(
> > + struct xfs_mount *mp,
> > + struct xfs_trans_resv *resp)
> > +{
> > + ASSERT(resp->tr_attrsetm.tr_logres > 0);
> >
> > resp->tr_rename.tr_logres =
> > xfs_calc_rename_reservation(mp);
> > resp->tr_rename.tr_logcount = XFS_RENAME_LOG_COUNT;
> > @@ -948,15 +1001,37 @@ xfs_trans_resv_calc(
> > resp->tr_create.tr_logcount = XFS_CREATE_LOG_COUNT;
> > resp->tr_create.tr_logflags |= XFS_TRANS_PERM_LOG_RES;
> >
> > + resp->tr_mkdir.tr_logres = xfs_calc_mkdir_reservation(mp);
> > + resp->tr_mkdir.tr_logcount = XFS_MKDIR_LOG_COUNT;
> > + resp->tr_mkdir.tr_logflags |= XFS_TRANS_PERM_LOG_RES;
> > +
> > + xfs_calc_parent_ptr_reservations(mp);
> > +}
> > +
> > +void
> > +xfs_trans_resv_calc(
> > + struct xfs_mount *mp,
> > + struct xfs_trans_resv *resp)
> > +{
> > + int logcount_adj = 0;
> > +
> > + /*
> > + * 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,
> > false);
> > + 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, false);
> > + resp->tr_itruncate.tr_logcount = XFS_ITRUNCATE_LOG_COUNT;
> > + resp->tr_itruncate.tr_logflags |= XFS_TRANS_PERM_LOG_RES;
> > +
> > resp->tr_create_tmpfile.tr_logres =
> > xfs_calc_create_tmpfile_reservation(mp);
> > resp->tr_create_tmpfile.tr_logcount =
> > XFS_CREATE_TMPFILE_LOG_COUNT;
> > resp->tr_create_tmpfile.tr_logflags |=
> > XFS_TRANS_PERM_LOG_RES;
> >
> > - resp->tr_mkdir.tr_logres = xfs_calc_mkdir_reservation(mp);
> > - resp->tr_mkdir.tr_logcount = XFS_MKDIR_LOG_COUNT;
> > - resp->tr_mkdir.tr_logflags |= XFS_TRANS_PERM_LOG_RES;
> > -
> > resp->tr_ifree.tr_logres = xfs_calc_ifree_reservation(mp);
> > resp->tr_ifree.tr_logcount = XFS_INACTIVE_LOG_COUNT;
> > resp->tr_ifree.tr_logflags |= XFS_TRANS_PERM_LOG_RES;
> > @@ -986,6 +1061,8 @@ xfs_trans_resv_calc(
> > resp->tr_qm_dqalloc.tr_logcount = XFS_WRITE_LOG_COUNT;
> > resp->tr_qm_dqalloc.tr_logflags |= XFS_TRANS_PERM_LOG_RES;
> >
> > + xfs_calc_namespace_reservations(mp, resp);
> > +
> > /*
> > * The following transactions are logged in logical format
> > with
> > * a default log count.
> > --
> > 2.25.1
> >
