From: "Darrick J. Wong" <djwong@kernel.org>
To: allison.henderson@oracle.com
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH v3 13/26] xfs: extend transaction reservations for parent attributes
Date: Fri, 23 Sep 2022 13:17:55 -0700 [thread overview]
Message-ID: <Yy4Uc62AbxUAWDXg@magnolia> (raw)
In-Reply-To: <20220922054458.40826-14-allison.henderson@oracle.com>
On Wed, Sep 21, 2022 at 10:44:45PM -0700, allison.henderson@oracle.com wrote:
> From: Allison Henderson <allison.henderson@oracle.com>
>
> 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@redhat.com>
> Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
> ---
> 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
>
next prev parent reply other threads:[~2022-09-23 20:26 UTC|newest]
Thread overview: 74+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-22 5:44 [PATCH v3 00/26] Parent Pointers allison.henderson
2022-09-22 5:44 ` [PATCH v3 01/26] xfs: Add new name to attri/d allison.henderson
2022-09-23 18:53 ` Darrick J. Wong
2022-09-23 20:43 ` Allison Henderson
2022-09-22 5:44 ` [PATCH v3 02/26] xfs: Increase XFS_DEFER_OPS_NR_INODES to 5 allison.henderson
2022-09-23 19:02 ` Darrick J. Wong
2022-09-23 20:45 ` Allison Henderson
2022-09-22 5:44 ` [PATCH v3 03/26] xfs: Hold inode locks in xfs_ialloc allison.henderson
2022-09-22 5:44 ` [PATCH v3 04/26] xfs: Hold inode locks in xfs_trans_alloc_dir allison.henderson
2022-09-23 19:04 ` Darrick J. Wong
2022-09-23 20:44 ` Allison Henderson
2022-09-22 5:44 ` [PATCH v3 05/26] xfs: Hold inode locks in xfs_rename allison.henderson
2022-09-23 19:21 ` Darrick J. Wong
2022-09-23 20:44 ` Allison Henderson
2022-09-22 5:44 ` [PATCH v3 06/26] xfs: Expose init_xattrs in xfs_create_tmpfile allison.henderson
2022-09-23 19:25 ` Darrick J. Wong
2022-09-23 20:45 ` Allison Henderson
2022-09-23 21:18 ` Darrick J. Wong
2022-09-22 5:44 ` [PATCH v3 07/26] xfs: get directory offset when adding directory name allison.henderson
2022-09-22 5:44 ` [PATCH v3 08/26] xfs: get directory offset when removing " allison.henderson
2022-09-22 5:44 ` [PATCH v3 09/26] xfs: get directory offset when replacing a " allison.henderson
2022-09-22 5:44 ` [PATCH v3 10/26] xfs: add parent pointer support to attribute code allison.henderson
2022-09-22 5:44 ` [PATCH v3 11/26] xfs: define parent pointer xattr format allison.henderson
2022-09-22 5:44 ` [PATCH v3 12/26] xfs: Add xfs_verify_pptr allison.henderson
2022-09-22 5:44 ` [PATCH v3 13/26] xfs: extend transaction reservations for parent attributes allison.henderson
2022-09-23 20:17 ` Darrick J. Wong [this message]
2022-09-23 23:53 ` Allison Henderson
2022-09-26 23:53 ` Darrick J. Wong
2022-09-27 20:04 ` Allison Henderson
2022-09-27 20:44 ` Darrick J. Wong
2022-09-22 5:44 ` [PATCH v3 14/26] xfs: parent pointer attribute creation allison.henderson
2022-09-23 21:11 ` Darrick J. Wong
2022-09-26 21:48 ` Allison Henderson
2022-09-26 23:54 ` Darrick J. Wong
2022-09-27 20:10 ` Allison Henderson
2022-09-22 5:44 ` [PATCH v3 15/26] xfs: add parent attributes to link allison.henderson
2022-09-23 20:31 ` Darrick J. Wong
2022-09-26 21:49 ` Allison Henderson
2022-09-26 23:55 ` Darrick J. Wong
2022-09-22 5:44 ` [PATCH v3 16/26] xfs: add parent attributes to symlink allison.henderson
2022-09-23 21:16 ` Darrick J. Wong
2022-09-26 21:48 ` Allison Henderson
2022-09-22 5:44 ` [PATCH v3 17/26] xfs: remove parent pointers in unlink allison.henderson
2022-09-23 21:22 ` Darrick J. Wong
2022-09-26 21:49 ` Allison Henderson
2022-09-22 5:44 ` [PATCH v3 18/26] xfs: Add parent pointers to xfs_cross_rename allison.henderson
2022-09-23 21:52 ` Darrick J. Wong
2022-09-26 21:50 ` Allison Henderson
2022-09-22 5:44 ` [PATCH v3 19/26] xfs: Indent xfs_rename allison.henderson
2022-09-23 21:22 ` Darrick J. Wong
2022-09-26 21:49 ` Allison Henderson
2022-09-22 5:44 ` [PATCH v3 20/26] xfs: Add parent pointers to rename allison.henderson
2022-09-23 22:08 ` Darrick J. Wong
2022-09-26 21:50 ` Allison Henderson
2022-09-22 5:44 ` [PATCH v3 21/26] xfs: Add the parent pointer support to the superblock version 5 allison.henderson
2022-09-22 5:44 ` [PATCH v3 22/26] xfs: Add helper function xfs_attr_list_context_init allison.henderson
2022-09-22 5:44 ` [PATCH v3 23/26] xfs: Filter XFS_ATTR_PARENT for getfattr allison.henderson
2022-09-22 16:55 ` Allison Henderson
2022-09-23 21:45 ` Darrick J. Wong
2022-09-26 21:49 ` Allison Henderson
2022-09-27 18:32 ` Darrick J. Wong
2022-09-28 18:22 ` Allison Henderson
2022-09-28 1:13 ` [xfs] b73248c4ee: xfstests.xfs.269.fail kernel test robot
2022-09-22 5:44 ` [PATCH v3 24/26] xfs: Add parent pointer ioctl allison.henderson
2022-09-24 0:30 ` Darrick J. Wong
2022-09-26 21:50 ` Allison Henderson
2022-09-27 18:34 ` Darrick J. Wong
2022-09-22 5:44 ` [PATCH v3 25/26] xfs: fix unit conversion error in xfs_log_calc_max_attrsetm_res allison.henderson
2022-09-23 21:47 ` Darrick J. Wong
2022-09-26 21:50 ` Allison Henderson
2022-09-27 0:02 ` Darrick J. Wong
2022-09-22 5:44 ` [PATCH v3 26/26] xfs: drop compatibility minimum log size computations for reflink allison.henderson
2022-09-23 21:48 ` Darrick J. Wong
2022-09-26 21:50 ` Allison Henderson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Yy4Uc62AbxUAWDXg@magnolia \
--to=djwong@kernel.org \
--cc=allison.henderson@oracle.com \
--cc=linux-xfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).