Re: [PATCH v3 05/26] xfs: Hold inode locks in xfs_rename

From: "Darrick J. Wong" <djwong@kernel.org>
To: allison.henderson@oracle.com
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH v3 05/26] xfs: Hold inode locks in xfs_rename
Date: Fri, 23 Sep 2022 12:21:33 -0700	[thread overview]
Message-ID: <Yy4HPUJR7Wathz+A@magnolia> (raw)
In-Reply-To: <20220922054458.40826-6-allison.henderson@oracle.com>

On Wed, Sep 21, 2022 at 10:44:37PM -0700, 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>
> ---
>  fs/xfs/xfs_inode.c | 35 ++++++++++++++++++++++-------------
>  1 file changed, 22 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 9a3174a8f895..4bfa4a1579f0 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -2837,18 +2837,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 +2860,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 +3090,21 @@ 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:
> +	/* Unlock inodes in reverse order */
> +	for (i = num_inodes - 1; i >= 0; i--) {
> +		if (inodes[i])
> +			xfs_iunlock(inodes[i], XFS_ILOCK_EXCL);
> +
> +		/* Skip duplicate inodes if src and target dps are the same */
> +		if (i && (inodes[i] == inodes[i - 1]))
> +			i--;
> +	}

Could you hoist this to a static inline xfs_iunlock_after_rename
function that is adjacent to xfs_sort_for_rename, please?  It's easier
to verify that it does the right thing w.r.t. multiple array references
pointing to the same incore inode when the two array management
functions are right next to each other.

static inline void
xfs_iunlock_after_rename(
	struct xfs_inode	**i_tab,
	int			num_inodes)
{
	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);
	}
}

With that cleaned up,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

>  out_release_wip:
>  	if (wip)
>  		xfs_irele(wip);
> -- 
> 2.25.1
> 