On Mon, Sep 26, 2022 at 09:49:10PM +0000, Allison Henderson wrote:
> 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;
> }
<nod>
--D
> 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
> > >
>
