From: "Darrick J. Wong" <djwong@kernel.org>
To: Allison Henderson <allison.henderson@oracle.com>
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: Mon, 26 Sep 2022 16:53:20 -0700 [thread overview]
Message-ID: <YzI7cNJC7QE5OBNd@magnolia> (raw)
In-Reply-To: <56e66827346fcc5ba40805a34abffc95f16c740b.camel@oracle.com>
On Fri, Sep 23, 2022 at 11:53:22PM +0000, Allison Henderson wrote:
> 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?
They have to be worst case values, but fortunately we know what the
worst case is:
name: sizeof(ondisk parent ptr structure) + iovec overhead
newname: same for rename, and zero everywhere else iiuc?
value: 255 + iovec_overhead
> >
> > > + }
> > > +
> > > + 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?
Right, though you could update XFS_RENAME_LOG_COUNT (or turn it into a
helper function).
> 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?
Yes, once those macros start acquiring logic, they ought to turn into
static inline helpers.
--D
> 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-26 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
2022-09-26 23:53 ` Darrick J. Wong [this message]
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=YzI7cNJC7QE5OBNd@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).