Re: [PATCH v1 13/17] xfs: Add parent pointers to rename

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

On Sat, Jun 11, 2022 at 02:41:56AM -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 | 137 +++++++++++++++++++++++++++++++++------------
>  1 file changed, 101 insertions(+), 36 deletions(-)
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 160f57df6d58..4566613c6a71 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -3153,7 +3153,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);
> @@ -3200,26 +3200,52 @@ 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 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 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_parent_name_rec	new_rec;
> +	struct xfs_parent_name_rec	old_rec;
> +	xfs_dir2_dataptr_t		new_diroffset;
> +	xfs_dir2_dataptr_t		old_diroffset;
> +	struct xfs_da_args		new_args = {
> +		.dp		= src_ip,
> +		.geo		= mp->m_attr_geo,
> +		.whichfork	= XFS_ATTR_FORK,
> +		.attr_filter	= XFS_ATTR_PARENT,
> +		.op_flags	= XFS_DA_OP_OKNOENT,
> +		.name		= (const uint8_t *)&new_rec,
> +		.namelen	= sizeof(new_rec),
> +		.value		= (void *)target_name->name,
> +		.valuelen	= target_name->len,
> +	};
> +	struct xfs_da_args		old_args = {
> +		.dp		= src_ip,
> +		.geo		= mp->m_attr_geo,
> +		.whichfork	= XFS_ATTR_FORK,
> +		.attr_filter	= XFS_ATTR_PARENT,
> +		.op_flags	= XFS_DA_OP_OKNOENT,
> +		.name		= (const uint8_t *)&old_rec,
> +		.namelen	= sizeof(old_rec),
> +		.value		= NULL,
> +		.valuelen	= 0,
> +	};
>  
>  	trace_xfs_rename(src_dp, target_dp, src_name, target_name);
>  
> @@ -3242,6 +3268,11 @@ xfs_rename(
>  
>  	xfs_sort_for_rename(src_dp, target_dp, src_ip, target_ip, wip,
>  				inodes, &num_inodes);
> +	if (xfs_has_larp(mp)) {
> +		error = xfs_attr_grab_log_assist(mp);
> +		if (error)
> +			goto out_release_wip;
> +	}
>  
>  retry:
>  	nospace_error = 0;
> @@ -3254,7 +3285,7 @@ xfs_rename(
>  				&tp);
>  	}
>  	if (error)
> -		goto out_release_wip;
> +		goto drop_incompat;
>  
>  	/*
>  	 * Attach the dquots to the inodes
> @@ -3276,14 +3307,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
> @@ -3293,15 +3324,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
> @@ -3415,7 +3447,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;
>  
> @@ -3436,7 +3468,7 @@ 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;
>  
> @@ -3470,7 +3502,7 @@ xfs_rename(
>  		 * directory.
>  		 */
>  		error = xfs_dir_replace(tp, src_ip, &xfs_name_dotdot,
> -					target_dp->i_ino, spaceres, NULL);
> +					target_dp->i_ino, spaceres, &new_diroffset);
>  		ASSERT(error != -EEXIST);
>  		if (error)
>  			goto out_trans_cancel;
> @@ -3509,26 +3541,59 @@ 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 (xfs_sb_version_hasparent(&mp->m_sb)) {
> +		new_args.trans	= tp;
> +		xfs_init_parent_name_rec(&new_rec, target_dp, new_diroffset);
> +		new_args.hashval = xfs_da_hashname(new_args.name,
> +						   new_args.namelen);
> +		error =  xfs_attr_defer_add(&new_args);
> +		if (error)
> +			goto out_trans_cancel;
> +
> +		old_args.trans	= tp;
> +		xfs_init_parent_name_rec(&old_rec, src_dp, old_diroffset);
> +		old_args.hashval = xfs_da_hashname(old_args.name,
> +						   old_args.namelen);
> +		error = xfs_attr_defer_remove(&old_args);
> +		if (error)
> +			goto out_trans_cancel;

Isn't this missing a xfs_attr_defer_remove call for target_ip if the
rename causes target_ip to be unlinked from target_dp?

If you had pptr helper functions, it would be easier to validate that
the code sequence is correct if we could see something like:

	if (want to add something) {
		xfs_dir_createname(..., &newoffset);
		xfs_dir_createpptr(..., newoffset);
	} else if (want to remove something) {
		xfs_dir_removename(..., &oldoffset);
		xfs_dir_removepptr(..., oldoffset);
	} else if (replacing something) {
		xfs_dir_replace(..., &oldoffset, &newoffset);
		xfs_dir_replacepptr(..., oldoffset, newoffset);
	}

The point being that the function call that updates the directory should
be right before the function call that schedules the corresponding pptr
xattr update.

--D

> +	}
> +
>  	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);
> +
>  	return error;
>  
>  out_trans_cancel:
>  	xfs_trans_cancel(tp);
> +drop_incompat:
> +	if (xfs_has_larp(mp))
> +		xlog_drop_incompat_feat(mp->m_log);
>  out_release_wip:
>  	if (wip)
>  		xfs_irele(wip);
> -- 
> 2.25.1
>