Re: [PATCH RESEND v2 13/18] xfs: add parent attributes to link

["Darrick J. Wong" <djwong@kernel.org>]

On Tue, Aug 09, 2022 at 08:09:15PM -0700, Alli wrote:
> On Tue, 2022-08-09 at 11:43 -0700, Darrick J. Wong wrote:
> > 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@redhat.com>
> > > Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
> > > ---
> > >  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?
> It was part of the solution outlined in the last review.  It is
> redundant, but not an inappropriate sanity check for that function
> either. I can remove it from the helper if it bothers folks. 
> 
> 
> > 
> > > +		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.
> So we do use XFS_LINK_SPACE_RES here, but didnt we update the tr_link
> below in patch 11 to accommodate for the extra space?  Maybe I'm not
> understanding why we would need both?

D'oh, I apparently forgot to respond to this. :/

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.

(Will copy this to my reply for the patch resend.)

--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,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;
> > 	}
> Sure, I can scoot that part down to the defer_add helper. Thanks for
> the reviews!
> 
> Allison
> > 
> > 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
> > > 
>