Re: [PATCH v3 15/26] xfs: add parent attributes to link

[Allison Henderson <allison.henderson@oracle.com>]

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@oracle.com wrote:
> > From: Allison Henderson <allison.henderson@oracle.com>
> > 
> > This patch modifies xfs_link to add a parent pointer to the inode.
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
> > ---
> >  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
> >