On Fri, 2022-09-23 at 13:31 -0700, Darrick J. Wong wrote:
> On Wed, Sep 21, 2022 at 10:44:47PM -0700,
> allison.henderson@xxxxxxxxxx wrote:
> > From: Allison Henderson <allison.henderson@xxxxxxxxxx>
> >
> > This patch modifies xfs_link to add a parent pointer to the inode.
> >
> > Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> > Signed-off-by: Allison Henderson <allison.henderson@xxxxxxxxxx>
> > ---
> > fs/xfs/xfs_inode.c | 44 +++++++++++++++++++++++++++++++++++-------
> > --
> > 1 file changed, 35 insertions(+), 9 deletions(-)
> >
> > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > index 181d6417412e..af3f5edb7319 100644
> > --- a/fs/xfs/xfs_inode.c
> > +++ b/fs/xfs/xfs_inode.c
> > @@ -1228,14 +1228,16 @@ xfs_create_tmpfile(
> >
> > int
> > xfs_link(
> > - xfs_inode_t *tdp,
> > - xfs_inode_t *sip,
> > + struct xfs_inode *tdp,
> > + struct xfs_inode *sip,
> > struct xfs_name *target_name)
> > {
> > - xfs_mount_t *mp = tdp->i_mount;
> > - xfs_trans_t *tp;
> > + struct xfs_mount *mp = tdp->i_mount;
> > + struct xfs_trans *tp;
> > int error, nospace_error = 0;
> > int resblks;
> > + xfs_dir2_dataptr_t diroffset;
> > + struct xfs_parent_defer *parent = NULL;
> >
> > trace_xfs_link(tdp, target_name);
> >
> > @@ -1252,11 +1254,17 @@ xfs_link(
> > if (error)
> > goto std_return;
> >
> > + if (xfs_has_parent(mp)) {
> > + error = xfs_parent_init(mp, &parent);
> > + if (error)
> > + goto std_return;
> > + }
> > +
> > resblks = XFS_LINK_SPACE_RES(mp, target_name->len);
>
> Forwarding on from the v2 series --
>
> This patch ought to be modifying XFS_LINK_SPACE_RES so that each
> link()
> update reserves enough space to handle an expansion in the tdp
> directory
> (as it does now) *and* an expansion in the xattr structure of the sip
> child file. This is how we avoid dipping into the free space reserve
> pool midway through a transaction, and avoid shutdowns when space is
> tight.
>
> tr_res == space we reserve in the *log* to record updates.
>
> XFS_LINK_SPACE_RES == block we reserve from the filesystem free space
> to
> handle expansions of metadata structures.
>
> At this point in this version of the patchset, you've increased the
> log
> space reservations in anticipation of logging more information per
> transaction. However, you've not increased the free space
> reservations
> to handle potential node splitting in the ondisk xattr btree.
>
> (The rest of the patchset looks ok.)
Ok, looking at the later reviews, I'll add a similar helper function
here to use in place of XFS_LINK_SPACE_RES:
unsigned int
xfs_link_space_res(
struct xfs_mount *mp,
unsigned int namelen)
{
unsigned int ret;
ret = XFS_DIRENTER_SPACE_RES(mp, namelen);
if (xfs_has_parent(mp))
ret += xfs_pptr_calc_space_res(mp, namelen);
return ret;
}
Thanks!
Allison
>
> --D
>
> > error = xfs_trans_alloc_dir(tdp, &M_RES(mp)->tr_link, sip,
> > &resblks,
> > &tp, &nospace_error);
> > if (error)
> > - goto std_return;
> > + goto drop_incompat;
> >
> > /*
> > * If we are using project inheritance, we only allow hard
> > link
> > @@ -1289,14 +1297,27 @@ xfs_link(
> > }
> >
> > error = xfs_dir_createname(tp, tdp, target_name, sip-
> > >i_ino,
> > - resblks, NULL);
> > + resblks, &diroffset);
> > if (error)
> > - goto error_return;
> > + goto out_defer_cancel;
> > xfs_trans_ichgtime(tp, tdp, XFS_ICHGTIME_MOD |
> > XFS_ICHGTIME_CHG);
> > xfs_trans_log_inode(tp, tdp, XFS_ILOG_CORE);
> >
> > xfs_bumplink(tp, sip);
> >
> > + /*
> > + * If we have parent pointers, we now need to add the
> > parent record to
> > + * the attribute fork of the inode. If this is the initial
> > parent
> > + * attribute, we need to create it correctly, otherwise we
> > can just add
> > + * the parent to the inode.
> > + */
> > + if (parent) {
> > + error = xfs_parent_defer_add(tp, parent, tdp,
> > target_name,
> > + diroffset, sip);
> > + if (error)
> > + goto out_defer_cancel;
> > + }
> > +
> > /*
> > * If this is a synchronous mount, make sure that the
> > * link transaction goes to disk before returning to
> > @@ -1310,11 +1331,16 @@ xfs_link(
> > xfs_iunlock(sip, XFS_ILOCK_EXCL);
> > return error;
> >
> > - error_return:
> > +out_defer_cancel:
> > + xfs_defer_cancel(tp);
> > +error_return:
> > xfs_trans_cancel(tp);
> > xfs_iunlock(tdp, XFS_ILOCK_EXCL);
> > xfs_iunlock(sip, XFS_ILOCK_EXCL);
> > - std_return:
> > +drop_incompat:
> > + if (parent)
> > + xfs_parent_cancel(mp, parent);
> > +std_return:
> > if (error == -ENOSPC && nospace_error)
> > error = nospace_error;
> > return error;
> > --
> > 2.25.1
> >
