Re: [PATCH 18/21] xfs: Add parent pointers to rename

[Allison Henderson <allison.henderson@xxxxxxxxxx>]

On 05/07/2018 02:52 PM, Darrick J. Wong wrote:
> On Sun, May 06, 2018 at 10:24:51AM -0700, Allison Henderson wrote:
> > This patch removes the old parent pointer attribute during the
> > rename operation, and re-adds the updated parent pointer
> >
> > Signed-off-by: Allison Henderson <allison.henderson@xxxxxxxxxx>
> > ---
> >   fs/xfs/xfs_inode.c | 68 +++++++++++++++++++++++++++++++++++++++++-------------
> >   1 file changed, 52 insertions(+), 16 deletions(-)
> >
> > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > index b18b20c..7fd1479 100644
> > --- a/fs/xfs/xfs_inode.c
> > +++ b/fs/xfs/xfs_inode.c
> > @@ -3004,6 +3004,8 @@ xfs_rename(
> >   	bool			src_is_directory = S_ISDIR(VFS_I(src_ip)->i_mode);
> >   	int			spaceres;
> >   	int			error;
> > +	xfs_dir2_dataptr_t	new_diroffset;
> > +	xfs_dir2_dataptr_t	old_diroffset;
> >   
> >   	trace_xfs_rename(src_dp, target_dp, src_name, target_name);
> >   
> > @@ -3058,14 +3060,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
> > @@ -3075,17 +3077,18 @@ xfs_rename(
> >   	if (unlikely((target_dp->i_d.di_flags & XFS_DIFLAG_PROJINHERIT) &&
> >   		     (xfs_get_projid(target_dp) != xfs_get_projid(src_ip)))) {
> >   		error = -EXDEV;
> > -		goto out_trans_cancel;
> > +		goto out_unlock;
> >   	}
> >   
> >   	xfs_defer_init(&dfops, &first_block);
> >   
> >   	/* 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,
> >   					&dfops, &first_block, spaceres);
> > -
> > +		goto out;
> > +	}
> >   	/*
> >   	 * Set up the target.
> >   	 */
> > @@ -3097,7 +3100,7 @@ xfs_rename(
> >   		if (!spaceres) {
> >   			error = xfs_dir_canenter(tp, target_dp, target_name);
> >   			if (error)
> > -				goto out_trans_cancel;
> > +				goto out_unlock;
> >   		}
> >   		/*
> >   		 * If target does not exist and the rename crosses
> > @@ -3106,7 +3109,7 @@ xfs_rename(
> >   		 */
> >   		error = xfs_dir_createname(tp, target_dp, target_name,
> >   					   src_ip->i_ino, &first_block, &dfops,
> > -					   spaceres, NULL);
> > +					   spaceres, &new_diroffset);
> >   		if (error)
> >   			goto out_bmap_cancel;
> >   
> > @@ -3131,7 +3134,7 @@ xfs_rename(
> >   			if (!(xfs_dir_isempty(target_ip)) ||
> >   			    (VFS_I(target_ip)->i_nlink > 2)) {
> >   				error = -EEXIST;
> > -				goto out_trans_cancel;
> > +				goto out_unlock;
> >   			}
> >   		}
> >   
> > @@ -3146,7 +3149,7 @@ xfs_rename(
> >   		 */
> >   		error = xfs_dir_replace(tp, target_dp, target_name,
> >   					src_ip->i_ino, &first_block, &dfops,
> > -					spaceres, NULL);
> > +					spaceres, &new_diroffset);
> >   		if (error)
> >   			goto out_bmap_cancel;
> >   
> > @@ -3181,7 +3184,7 @@ xfs_rename(
> >   		 */
> >   		error = xfs_dir_replace(tp, src_ip, &xfs_name_dotdot,
> >   					target_dp->i_ino, &first_block, &dfops,
> > -					spaceres, NULL);
> > +					spaceres, &new_diroffset);
> >   		ASSERT(error != -EEXIST);
> >   		if (error)
> >   			goto out_bmap_cancel;
> > @@ -3220,11 +3223,12 @@ xfs_rename(
> >   	 */
> >   	if (wip) {
> >   		error = xfs_dir_replace(tp, src_dp, src_name, wip->i_ino,
> > -					&first_block, &dfops, spaceres, NULL);
> > +					&first_block, &dfops, spaceres,
> > +					&old_diroffset);
> >   	} else
> >   		error = xfs_dir_removename(tp, src_dp, src_name, src_ip->i_ino,
> >   					   &first_block, &dfops, spaceres,
> > -					   NULL);
> > +					   &old_diroffset);
> >   	if (error)
> >   		goto out_bmap_cancel;
> >   
> > @@ -3254,6 +3258,18 @@ xfs_rename(
> >   		VFS_I(wip)->i_state &= ~I_LINKABLE;
> >   	}
> >   
> > +	if (xfs_sb_version_hasparent(&mp->m_sb)) {
> > +		error = xfs_parent_add_deferred(target_dp, src_ip, target_name,
> > +				       new_diroffset, &dfops);
>
> Only two indents needed for the second line:
>
> 		error = xfs_parent_add_deferred(target_dp, src_ip, target_name,
> 				new_diroffset, &dfops);
> 		if (error)
> 			goto out_bmap_cancel;
>
> > +		if (error)
> > +			goto out_bmap_cancel;
> > +
> > +		error = xfs_parent_remove_deferred(src_dp, src_ip,
> > +						   old_diroffset, &dfops);
> > +		if (error)
> > +			goto out_bmap_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)
> > @@ -3262,10 +3278,30 @@ xfs_rename(
> >   	error = xfs_finish_rename(tp, &dfops);
> >   	if (wip)
> >   		IRELE(wip);
> > +out:
> > +	if (wip)
> > +		xfs_iunlock(wip, XFS_ILOCK_EXCL);
>
> IRELE = iput = release inode, which means that you have to unlock the
> wip inode before you can release it.
>
> --D
Ok, I'll move that down and fix indents.  Thx!

Allison


> > +	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_bmap_cancel:
> >   	xfs_defer_cancel(&dfops);
> > +out_unlock:
> > +	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);
> > +
> >   out_trans_cancel:
> >   	xfs_trans_cancel(tp);
> >   out_release_wip:
> > --
> > 2.7.4
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html