Re: [PATCH v4 05/27] xfs: Hold inode locks in xfs_rename

From: Catherine Hoang <catherine.hoang@oracle.com>
To: Allison Henderson <allison.henderson@oracle.com>
Cc: "linux-xfs@vger.kernel.org" <linux-xfs@vger.kernel.org>
Subject: Re: [PATCH v4 05/27] xfs: Hold inode locks in xfs_rename
Date: Fri, 28 Oct 2022 01:59:08 +0000	[thread overview]
Message-ID: <DBA96878-A1C1-4602-839D-F8A0A7BB3A4F@oracle.com> (raw)
In-Reply-To: <20221021222936.934426-6-allison.henderson@oracle.com>

> On Oct 21, 2022, at 3:29 PM, allison.henderson@oracle.com wrote:
> 
> From: Allison Henderson <allison.henderson@oracle.com>
> 
> Modify xfs_rename to hold all inode locks across a rename operation
> We will need this later when we add parent pointers
> 
> Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>

Looks good
Reviewed-by: Catherine Hoang <catherine.hoang@oracle.com>
> ---
> fs/xfs/xfs_inode.c | 42 +++++++++++++++++++++++++++++-------------
> 1 file changed, 29 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 9a3174a8f895..44b68fa53a72 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -2539,6 +2539,21 @@ xfs_remove(
> 	return error;
> }
> 
> +static inline void
> +xfs_iunlock_after_rename(
> +	struct xfs_inode	**i_tab,
> +	int			num_inodes)
> +{
> +	int			i;
> +
> +	for (i = num_inodes - 1; i >= 0; i--) {
> +		/* Skip duplicate inodes if src and target dps are the same */
> +		if (!i_tab[i] || (i > 0 && i_tab[i] == i_tab[i - 1]))
> +			continue;
> +		xfs_iunlock(i_tab[i], XFS_ILOCK_EXCL);
> +	}
> +}
> +
> /*
>  * Enter all inodes for a rename transaction into a sorted array.
>  */
> @@ -2837,18 +2852,16 @@ xfs_rename(
> 	xfs_lock_inodes(inodes, num_inodes, XFS_ILOCK_EXCL);
> 
> 	/*
> -	 * Join all the inodes to the transaction. From this point on,
> -	 * we can rely on either trans_commit or trans_cancel to unlock
> -	 * them.
> +	 * Join all the inodes to the transaction.
> 	 */
> -	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
> @@ -2862,10 +2875,12 @@ xfs_rename(
> 	}
> 
> 	/* 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_unlock;
> +	}
> 
> 	/*
> 	 * Try to reserve quota to handle an expansion of the target directory.
> @@ -3090,12 +3105,13 @@ xfs_rename(
> 		xfs_trans_log_inode(tp, target_dp, XFS_ILOG_CORE);
> 
> 	error = xfs_finish_rename(tp);
> -	if (wip)
> -		xfs_irele(wip);
> -	return error;
> +
> +	goto out_unlock;
> 
> out_trans_cancel:
> 	xfs_trans_cancel(tp);
> +out_unlock:
> +	xfs_iunlock_after_rename(inodes, num_inodes);
> out_release_wip:
> 	if (wip)
> 		xfs_irele(wip);
> -- 
> 2.25.1
>