Re: [PATCH RESEND v2 15/18] xfs: Add parent pointers to rename

[Alli <allison.henderson@oracle.com>]

On Tue, 2022-08-09 at 11:49 -0700, Darrick J. Wong wrote:
> On Thu, Aug 04, 2022 at 12:40:10PM -0700, Allison Henderson wrote:
> > This patch removes the old parent pointer attribute during the
> > rename
> > operation, and re-adds the updated parent pointer.  In the case of
> > xfs_cross_rename, we modify the routine not to roll the transaction
> > just
> > yet.  We will do this after the parent pointer is added in the
> > calling
> > xfs_rename function.
> > 
> > Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
> > ---
> >  fs/xfs/xfs_inode.c | 128 +++++++++++++++++++++++++++++++++------
> > ------
> >  1 file changed, 94 insertions(+), 34 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > index 69bb67f2a252..8a81b78b6dd7 100644
> > --- a/fs/xfs/xfs_inode.c
> > +++ b/fs/xfs/xfs_inode.c
> > @@ -2776,7 +2776,7 @@ xfs_cross_rename(
> >  	}
> >  	xfs_trans_ichgtime(tp, dp1, XFS_ICHGTIME_MOD |
> > XFS_ICHGTIME_CHG);
> >  	xfs_trans_log_inode(tp, dp1, XFS_ILOG_CORE);
> > -	return xfs_finish_rename(tp);
> > +	return 0;
> >  
> >  out_trans_abort:
> >  	xfs_trans_cancel(tp);
> > @@ -2834,26 +2834,31 @@ xfs_rename_alloc_whiteout(
> >   */
> >  int
> >  xfs_rename(
> > -	struct user_namespace	*mnt_userns,
> > -	struct xfs_inode	*src_dp,
> > -	struct xfs_name		*src_name,
> > -	struct xfs_inode	*src_ip,
> > -	struct xfs_inode	*target_dp,
> > -	struct xfs_name		*target_name,
> > -	struct xfs_inode	*target_ip,
> > -	unsigned int		flags)
> > +	struct user_namespace		*mnt_userns,
> > +	struct xfs_inode		*src_dp,
> > +	struct xfs_name			*src_name,
> > +	struct xfs_inode		*src_ip,
> > +	struct xfs_inode		*target_dp,
> > +	struct xfs_name			*target_name,
> > +	struct xfs_inode		*target_ip,
> > +	unsigned int			flags)
> >  {
> > -	struct xfs_mount	*mp = src_dp->i_mount;
> > -	struct xfs_trans	*tp;
> > -	struct xfs_inode	*wip = NULL;		/* whiteout inode
> > */
> > -	struct xfs_inode	*inodes[__XFS_SORT_INODES];
> > -	int			i;
> > -	int			num_inodes = __XFS_SORT_INODES;
> > -	bool			new_parent = (src_dp != target_dp);
> > -	bool			src_is_directory =
> > S_ISDIR(VFS_I(src_ip)->i_mode);
> > -	int			spaceres;
> > -	bool			retried = false;
> > -	int			error, nospace_error = 0;
> > +	struct xfs_mount		*mp = src_dp->i_mount;
> > +	struct xfs_trans		*tp;
> > +	struct xfs_inode		*wip = NULL;		/* whiteout
> > inode */
> > +	struct xfs_inode		*inodes[__XFS_SORT_INODES];
> > +	int				i;
> > +	int				num_inodes = __XFS_SORT_INODES;
> > +	bool				new_parent = (src_dp !=
> > target_dp);
> > +	bool				src_is_directory =
> > S_ISDIR(VFS_I(src_ip)->i_mode);
> > +	int				spaceres;
> > +	bool				retried = false;
> > +	int				error, nospace_error = 0;
> > +	xfs_dir2_dataptr_t		new_diroffset;
> > +	xfs_dir2_dataptr_t		old_diroffset;
> > +	struct xfs_parent_defer		*old_parent_ptr = NULL;
> > +	struct xfs_parent_defer		*new_parent_ptr = NULL;
> > +	struct xfs_parent_defer		*target_parent_ptr = NULL;
> >  
> >  	trace_xfs_rename(src_dp, target_dp, src_name, target_name);
> >  
> > @@ -2877,6 +2882,15 @@ xfs_rename(
> >  
> >  	xfs_sort_for_rename(src_dp, target_dp, src_ip, target_ip, wip,
> >  				inodes, &num_inodes);
> > +	if (xfs_has_parent(mp)) {
> > +		error = xfs_parent_init(mp, src_ip, NULL,
> > &old_parent_ptr);
> > +		if (error)
> > +			goto out_release_wip;
> > +		error = xfs_parent_init(mp, src_ip, target_name,
> > +					&new_parent_ptr);
> > +		if (error)
> > +			goto out_release_wip;
> > +	}
> >  
> >  retry:
> >  	nospace_error = 0;
> > @@ -2889,7 +2903,7 @@ xfs_rename(
> >  				&tp);
> >  	}
> >  	if (error)
> > -		goto out_release_wip;
> > +		goto drop_incompat;
> >  
> >  	/*
> >  	 * Attach the dquots to the inodes
> > @@ -2911,14 +2925,14 @@ xfs_rename(
> >  	 * we can rely on either trans_commit or trans_cancel to unlock
> >  	 * them.
> >  	 */
> > -	xfs_trans_ijoin(tp, src_dp, XFS_ILOCK_EXCL);
> > +	xfs_trans_ijoin(tp, src_dp, 0);
> >  	if (new_parent)
> > -		xfs_trans_ijoin(tp, target_dp, XFS_ILOCK_EXCL);
> > -	xfs_trans_ijoin(tp, src_ip, XFS_ILOCK_EXCL);
> > +		xfs_trans_ijoin(tp, target_dp, 0);
> > +	xfs_trans_ijoin(tp, src_ip, 0);
> >  	if (target_ip)
> > -		xfs_trans_ijoin(tp, target_ip, XFS_ILOCK_EXCL);
> > +		xfs_trans_ijoin(tp, target_ip, 0);
> >  	if (wip)
> > -		xfs_trans_ijoin(tp, wip, XFS_ILOCK_EXCL);
> > +		xfs_trans_ijoin(tp, wip, 0);
> >  
> >  	/*
> >  	 * If we are using project inheritance, we only allow renames
> > @@ -2928,15 +2942,16 @@ xfs_rename(
> >  	if (unlikely((target_dp->i_diflags & XFS_DIFLAG_PROJINHERIT) &&
> >  		     target_dp->i_projid != src_ip->i_projid)) {
> >  		error = -EXDEV;
> > -		goto out_trans_cancel;
> > +		goto out_unlock;
> >  	}
> >  
> >  	/* RENAME_EXCHANGE is unique from here on. */
> > -	if (flags & RENAME_EXCHANGE)
> > -		return xfs_cross_rename(tp, src_dp, src_name, src_ip,
> > +	if (flags & RENAME_EXCHANGE) {
> > +		error = xfs_cross_rename(tp, src_dp, src_name, src_ip,
> >  					target_dp, target_name,
> > target_ip,
> >  					spaceres);
> > -
> > +		goto out_pptr;
> > +	}
> >  	/*
> >  	 * Try to reserve quota to handle an expansion of the target
> > directory.
> >  	 * We'll allow the rename to continue in reservationless mode
> > if we hit
> > @@ -3052,7 +3067,7 @@ xfs_rename(
> >  		 * to account for the ".." reference from the new
> > entry.
> >  		 */
> >  		error = xfs_dir_createname(tp, target_dp, target_name,
> > -					   src_ip->i_ino, spaceres,
> > NULL);
> > +					   src_ip->i_ino, spaceres,
> > &new_diroffset);
> >  		if (error)
> >  			goto out_trans_cancel;
> >  
> > @@ -3073,10 +3088,14 @@ xfs_rename(
> >  		 * name at the destination directory, remove it first.
> >  		 */
> >  		error = xfs_dir_replace(tp, target_dp, target_name,
> > -					src_ip->i_ino, spaceres, NULL);
> > +					src_ip->i_ino, spaceres,
> > &new_diroffset);
> >  		if (error)
> >  			goto out_trans_cancel;
> >  
> > +		if (xfs_has_parent(mp))
> > +			error = xfs_parent_init(mp, target_ip, NULL,
> > +						&target_parent_ptr);
> > +
> >  		xfs_trans_ichgtime(tp, target_dp,
> >  					XFS_ICHGTIME_MOD |
> > XFS_ICHGTIME_CHG);
> >  
> > @@ -3146,26 +3165,67 @@ xfs_rename(
> >  	 */
> >  	if (wip)
> >  		error = xfs_dir_replace(tp, src_dp, src_name, wip-
> > >i_ino,
> > -					spaceres, NULL);
> > +					spaceres, &old_diroffset);
> >  	else
> >  		error = xfs_dir_removename(tp, src_dp, src_name,
> > src_ip->i_ino,
> > -					   spaceres, NULL);
> > +					   spaceres, &old_diroffset);
> >  
> >  	if (error)
> >  		goto out_trans_cancel;
> >  
> > +out_pptr:
> > +	if (new_parent_ptr) {
> > +		error = xfs_parent_defer_add(tp, target_dp,
> > new_parent_ptr,
> > +					     new_diroffset);
> > +		if (error)
> > +			goto out_trans_cancel;
> > +	}
> > +
> > +	if (old_parent_ptr) {
> > +		error = xfs_parent_defer_remove(tp, src_dp,
> > old_parent_ptr,
> > +						old_diroffset);
> > +		if (error)
> > +			goto out_trans_cancel;
> > +	}
> > +
> > +	if (target_parent_ptr) {
> > +		error = xfs_parent_defer_remove(tp, target_dp,
> > +						target_parent_ptr,
> > +						new_diroffset);
> > +		if (error)
> > +			goto out_trans_cancel;
> > +	}
> > +
> >  	xfs_trans_ichgtime(tp, src_dp, XFS_ICHGTIME_MOD |
> > XFS_ICHGTIME_CHG);
> >  	xfs_trans_log_inode(tp, src_dp, XFS_ILOG_CORE);
> >  	if (new_parent)
> >  		xfs_trans_log_inode(tp, target_dp, XFS_ILOG_CORE);
> >  
> >  	error = xfs_finish_rename(tp);
> > +
> > +out_unlock:
> >  	if (wip)
> >  		xfs_irele(wip);
> > +	if (wip)
> > +		xfs_iunlock(wip, XFS_ILOCK_EXCL);
> > +	if (target_ip)
> > +		xfs_iunlock(target_ip, XFS_ILOCK_EXCL);
> > +	xfs_iunlock(src_ip, XFS_ILOCK_EXCL);
> > +	if (new_parent)
> > +		xfs_iunlock(target_dp, XFS_ILOCK_EXCL);
> > +	xfs_iunlock(src_dp, XFS_ILOCK_EXCL);
> 
> Sorry to be fussy, but could you separate the ILOCK unlocking changes
> (and maybe the variable indentation part too) into a separate prep
> patch, please?
Sure, that should be fine.

> 
> Also, who frees the xfs_parent_defer objects?
> 
its the xfs_parent_cancel() calls below

> --D
> 
> > +
> >  	return error;
> >  
> >  out_trans_cancel:
> >  	xfs_trans_cancel(tp);
> > +drop_incompat:
> > +	if (new_parent_ptr)
> > +		xfs_parent_cancel(mp, new_parent_ptr);
> > +	if (old_parent_ptr)
> > +		xfs_parent_cancel(mp, old_parent_ptr);
> > +	if (target_parent_ptr)
> > +		xfs_parent_cancel(mp, target_parent_ptr);
> >  out_release_wip:
> >  	if (wip)
> >  		xfs_irele(wip);
> > -- 
> > 2.25.1
> >