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. :/
> * 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.
> + }
> +
> + 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?
> +
> + 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?
--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
>
