On Thu, Aug 04, 2022 at 12:40:08PM -0700, Allison Henderson wrote: > This patch modifies xfs_link to add a parent pointer to the inode. > > [bfoster: rebase, use VFS inode fields, fix xfs_bmap_finish() usage] > [achender: rebased, changed __unint32_t to xfs_dir2_dataptr_t, > fixed null pointer bugs] > > Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx> > Signed-off-by: Allison Henderson <allison.henderson@xxxxxxxxxx> > --- > fs/xfs/xfs_inode.c | 43 ++++++++++++++++++++++++++++++++++--------- > 1 file changed, 34 insertions(+), 9 deletions(-) > > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c > index ef993c3a8963..6e5deb0d42c4 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, sip, target_name, &parent); Why does xfs_parent_init check xfs_has_parent if the callers already do that? > + if (error) > + goto std_return; > + } > + > resblks = XFS_LINK_SPACE_RES(mp, target_name->len); Same comment about increasing XFS_LINK_SPACE_RES to accomodate xattr expansion as I had for the last patch. > 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,26 @@ 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, tdp, parent, diroffset); A followup to the comments I made to the previous patch about parent->args.dp -- Since you're partially initializing the xfs_defer_parent structure before you even have the dir offset, why not delay initializing the parent and child pointers until the xfs_parent_defer_add step? int xfs_parent_init( struct xfs_mount *mp, struct xfs_parent_defer **parentp) { struct xfs_parent_defer *parent; int error; if (!xfs_has_parent(mp)) return 0; error = xfs_attr_grab_log_assist(mp); if (error) return error; parent = kzalloc(sizeof(*parent), GFP_KERNEL); if (!parent) return -ENOMEM; /* init parent da_args */ parent->args.geo = mp->m_attr_geo; parent->args.whichfork = XFS_ATTR_FORK; parent->args.attr_filter = XFS_ATTR_PARENT; parent->args.op_flags = XFS_DA_OP_OKNOENT | XFS_DA_OP_LOGGED; parent->args.name = (const uint8_t *)&parent->rec; parent->args.namelen = sizeof(struct xfs_parent_name_rec); *parentp = parent; return 0; } int xfs_parent_defer_add( struct xfs_trans *tp, struct xfs_parent_defer *parent, struct xfs_inode *dp, struct xfs_name *parent_name, xfs_dir2_dataptr_t parent_offset, struct xfs_inode *child) { struct xfs_da_args *args = &parent->args; xfs_init_parent_name_rec(&parent->rec, dp, diroffset); args->hashval = xfs_da_hashname(args->name, args->namelen); args->trans = tp; args->dp = child; if (parent_name) { args->name = parent_name->name; args->valuelen = parent_name->len; } return xfs_attr_defer_add(args); } And then the callsites become: /* * 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; } Aside from the API suggestions, the rest looks good to me. --D > + 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 +1330,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 >