From: Allison Henderson <allison.henderson@oracle.com>
To: "djwong@kernel.org" <djwong@kernel.org>
Cc: "linux-xfs@vger.kernel.org" <linux-xfs@vger.kernel.org>
Subject: Re: [PATCH v3 13/26] xfs: extend transaction reservations for parent attributes
Date: Fri, 23 Sep 2022 23:53:22 +0000 [thread overview]
Message-ID: <56e66827346fcc5ba40805a34abffc95f16c740b.camel@oracle.com> (raw)
In-Reply-To: <Yy4Uc62AbxUAWDXg@magnolia>
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@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. :/
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
> >
next prev parent reply other threads:[~2022-09-23 23:53 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
2022-09-23 23:53 ` Allison Henderson [this message]
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=56e66827346fcc5ba40805a34abffc95f16c740b.camel@oracle.com \
--to=allison.henderson@oracle.com \
--cc=djwong@kernel.org \
--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).