[PATCH 2/2] Add support to RENAME_EXCHANGE flag

[PATCH 2/2] Add support to RENAME_EXCHANGE flag

[Carlos Maiolino]

Adds a new function named xfs_cross_rename(), responsible to handle requests
from sys_renameat2() using RENAME_EXCHANGE flag.

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 fs/xfs/xfs_inode.c | 187 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/xfs/xfs_inode.h |   4 ++
 fs/xfs/xfs_iops.c  |   7 +-
 3 files changed, 197 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index fea3c92..a5bc88d 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -2920,6 +2920,193 @@ xfs_rename(
 	return error;
 }
 
+/* xfs_cross_rename()
+ *
+ * responsible to handle RENAME_EXCHANGE flag
+ * in renameat2() sytemcall
+ */
+int
+xfs_cross_rename(
+	xfs_inode_t	*src_dp,
+	struct xfs_name	*src_name,
+	xfs_inode_t	*src_ip,
+	xfs_inode_t	*target_dp,
+	struct xfs_name	*target_name,
+	xfs_inode_t	*target_ip)
+{
+	xfs_trans_t	*tp = NULL;
+	xfs_mount_t	*mp = src_dp->i_mount;
+	int		new_parent;		/* Crossing from different parents */
+	int		src_is_directory;
+	int		tgt_is_directory;
+	int		error;
+	xfs_bmap_free_t	free_list;
+	xfs_fsblock_t	first_block;
+	int		cancel_flags;
+	int		committed;
+	xfs_inode_t	*inodes[4];
+	int		spaceres;
+	int		num_inodes;
+
+	new_parent = (src_dp != target_dp);
+	src_is_directory = S_ISDIR(src_ip->i_d.di_mode);
+	tgt_is_directory = S_ISDIR(target_ip->i_d.di_mode);
+
+	xfs_sort_for_rename(src_dp, target_dp, src_ip, target_ip,
+				inodes, &num_inodes);
+
+	xfs_bmap_init(&free_list, &first_block);
+	tp = xfs_trans_alloc(mp, XFS_TRANS_RENAME);
+	cancel_flags = XFS_TRANS_RELEASE_LOG_RES;
+	spaceres = XFS_RENAME_SPACE_RES(mp, target_name->len);
+	error = xfs_trans_reserve(tp, &M_RES(mp)->tr_rename, spaceres, 0);
+
+	if (error == -ENOSPC) {
+		spaceres = 0;
+		error = xfs_trans_reserve(tp, &M_RES(mp)->tr_rename, 0, 0);
+	}
+	if (error) {
+		xfs_trans_cancel(tp, 0);
+		goto std_return;
+	}
+
+	/*
+	 * Attach the dquots to the inodes
+	 */
+	error = xfs_qm_vop_rename_dqattach(inodes);
+	if (error) {
+		xfs_trans_cancel(tp, cancel_flags);
+		goto std_return;
+	}
+
+	/*
+	 * Lock all participating inodes. In case of RENAME_EXCHANGE, target
+	 * must exist, so we'll be locking at least 3 inodes here.
+	 */
+	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.
+	 * target_ip will always exist, so, no need to check its existence.
+	 */
+	xfs_trans_ijoin(tp, src_dp, XFS_ILOCK_EXCL);
+	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_ip, XFS_ILOCK_EXCL);
+
+	/*
+	* If we are using project inheritance, we only allow RENAME_EXCHANGE
+	* into our tree when the project IDs are the same; else the tree quota
+	* mechanism would be circumvented.
+	*/
+	if (unlikely(((target_dp->i_d.di_flags & XFS_DIFLAG_PROJINHERIT) ||
+		     (src_dp->i_d.di_flags & XFS_DIFLAG_PROJINHERIT)) &&
+		     (xfs_get_projid(src_dp) != xfs_get_projid(target_dp)) )) {
+		error = -EXDEV;
+		goto error_return;
+	}
+
+	error = xfs_dir_replace(tp, src_dp, src_name,
+				target_ip->i_ino,
+				&first_block, &free_list, spaceres);
+	if (error)
+		goto abort_return;
+
+	/*
+	 * Update ".." entry to match the new parent
+	 */
+	if (new_parent && tgt_is_directory) {
+		error = xfs_dir_replace(tp, target_ip, &xfs_name_dotdot,
+					src_dp->i_ino, &first_block, &free_list, spaceres);
+		if (error)
+			goto abort_return;
+	}
+
+	xfs_trans_ichgtime(tp, src_dp, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
+
+	error = xfs_dir_replace(tp, target_dp, target_name,
+				src_ip->i_ino,
+				&first_block, &free_list, spaceres);
+	if (error)
+		goto abort_return;
+
+	/*
+	 * Update ".." entry to match the new parent
+	 */
+	if (new_parent && src_is_directory) {
+		error = xfs_dir_replace(tp, src_ip, &xfs_name_dotdot,
+					target_dp->i_ino, &first_block, &free_list, spaceres);
+		if (error)
+			goto abort_return;
+	}
+
+	/*
+	 * In case we are crossing different file types between different
+	 * parents, we must update parent's link count to match the ".."
+	 * entry of the new child (or the removal of it).
+	 */
+	if (new_parent) {
+		xfs_trans_ichgtime(tp, target_dp, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
+
+		if (src_is_directory && !tgt_is_directory) {
+			error = xfs_droplink(tp, src_dp);
+				if (error)
+					goto abort_return;
+			error = xfs_bumplink(tp, target_dp);
+				if (error)
+					goto abort_return;
+		}
+
+		if (tgt_is_directory && !src_is_directory) {
+			error = xfs_droplink(tp, target_dp);
+				if (error)
+					goto abort_return;
+			error = xfs_bumplink(tp, src_dp);
+				if (error)
+					goto abort_return;
+		}
+
+		/*
+		 * We don't need to log the source dir if
+		 * this is the same as the target.
+		 */
+		xfs_trans_log_inode(tp, src_dp, XFS_ILOG_CORE);
+	}
+
+	xfs_trans_log_inode(tp, target_dp, XFS_ILOG_CORE);
+	xfs_trans_log_inode(tp, src_ip, XFS_ILOG_CORE);
+	xfs_trans_log_inode(tp, target_ip, XFS_ILOG_CORE);
+
+	/*
+	 * If this is a synchronous mount, make sure the rename transaction goes
+	 * to disk before returning to the user.
+	 */
+	if (mp->m_flags & (XFS_MOUNT_WSYNC|XFS_MOUNT_DIRSYNC))
+		xfs_trans_set_sync(tp);
+
+	error = xfs_bmap_finish(&tp, &free_list, &committed);
+	if (error) {
+		xfs_bmap_cancel(&free_list);
+		xfs_trans_cancel(tp, (XFS_TRANS_RELEASE_LOG_RES | XFS_TRANS_ABORT));
+		goto std_return;
+	}
+
+	return xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES);
+
+abort_return:
+	cancel_flags |= XFS_TRANS_ABORT;
+error_return:
+	xfs_bmap_cancel(&free_list);
+	xfs_trans_cancel(tp, cancel_flags);
+std_return:
+	return error;
+
+}
+
 STATIC int
 xfs_iflush_cluster(
 	xfs_inode_t	*ip,
diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
index c10e3fa..16889d3 100644
--- a/fs/xfs/xfs_inode.h
+++ b/fs/xfs/xfs_inode.h
@@ -341,6 +341,10 @@ int		xfs_rename(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);
+int		xfs_cross_rename(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);
 
 void		xfs_ilock(xfs_inode_t *, uint);
 int		xfs_ilock_nowait(xfs_inode_t *, uint);
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index b2b92c7..bc164df 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -391,12 +391,17 @@ xfs_vn_rename2(
 	struct xfs_name	nname;
 
 	/* XFS does not support RENAME_EXCHANGE yet */
-	if (flags & ~RENAME_NOREPLACE)
+	if (flags & ~(RENAME_NOREPLACE | RENAME_EXCHANGE))
 		return -EINVAL;
 
 	xfs_dentry_to_name(&oname, odentry, 0);
 	xfs_dentry_to_name(&nname, ndentry, odentry->d_inode->i_mode);
 
+	if (flags & RENAME_EXCHANGE)
+		return xfs_cross_rename(XFS_I(odir), &oname, XFS_I(odentry->d_inode),
+					XFS_I(ndir), &nname,
+					new_inode ? XFS_I(new_inode) : NULL);
+
 	return xfs_rename(XFS_I(odir), &oname, XFS_I(odentry->d_inode),
 			  XFS_I(ndir), &nname,
 			  new_inode ? XFS_I(new_inode) : NULL);
-- 
2.1.0

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/2] Add support to RENAME_EXCHANGE flag

[Brian Foster]

On Wed, Oct 15, 2014 at 03:17:22PM -0300, Carlos Maiolino wrote:
> Adds a new function named xfs_cross_rename(), responsible to handle requests
> from sys_renameat2() using RENAME_EXCHANGE flag.
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---

Hi Carlos,

Some high-level comments from a first pass...

>  fs/xfs/xfs_inode.c | 187 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  fs/xfs/xfs_inode.h |   4 ++
>  fs/xfs/xfs_iops.c  |   7 +-
>  3 files changed, 197 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index fea3c92..a5bc88d 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -2920,6 +2920,193 @@ xfs_rename(
>  	return error;
>  }
>  
> +/* xfs_cross_rename()
> + *
> + * responsible to handle RENAME_EXCHANGE flag
> + * in renameat2() sytemcall
> + */
> +int
> +xfs_cross_rename(
> +	xfs_inode_t	*src_dp,
> +	struct xfs_name	*src_name,
> +	xfs_inode_t	*src_ip,
> +	xfs_inode_t	*target_dp,
> +	struct xfs_name	*target_name,
> +	xfs_inode_t	*target_ip)
> +{
> +	xfs_trans_t	*tp = NULL;
> +	xfs_mount_t	*mp = src_dp->i_mount;
> +	int		new_parent;		/* Crossing from different parents */
> +	int		src_is_directory;
> +	int		tgt_is_directory;
> +	int		error;
> +	xfs_bmap_free_t	free_list;
> +	xfs_fsblock_t	first_block;
> +	int		cancel_flags;
> +	int		committed;
> +	xfs_inode_t	*inodes[4];
> +	int		spaceres;
> +	int		num_inodes;
> +
> +	new_parent = (src_dp != target_dp);
> +	src_is_directory = S_ISDIR(src_ip->i_d.di_mode);
> +	tgt_is_directory = S_ISDIR(target_ip->i_d.di_mode);
> +
> +	xfs_sort_for_rename(src_dp, target_dp, src_ip, target_ip,
> +				inodes, &num_inodes);
> +
> +	xfs_bmap_init(&free_list, &first_block);
> +	tp = xfs_trans_alloc(mp, XFS_TRANS_RENAME);
> +	cancel_flags = XFS_TRANS_RELEASE_LOG_RES;
> +	spaceres = XFS_RENAME_SPACE_RES(mp, target_name->len);
> +	error = xfs_trans_reserve(tp, &M_RES(mp)->tr_rename, spaceres, 0);
> +

It seems to me that the existing block and log reservations would
"cover" the rename exchange case, but it might be worth defining new
reservations for the purpose of clarity and to prevent future problems.

XFS_RENAME_SPACE_RES() covers directory removal and insertion. Here we
are doing neither, which makes me wonder whether we need a block
reservation at all. It does appear that we have a sf dir case where the
inode number could cause a format conversion. Perhaps we need something
that calculates the blocks required for the insertion of the max of both
names (it seems like the conversion would only happen once, but we don't
know which way)? I haven't spent a ton of time in directory code, so I
could easily be missing something.

The tr_rename log reservation considers four inodes, two directory
modifications, a target inode unlink (the overwrite case), and alloc
btree mods for directory blocks being freed. IIUC, the exchange case
should only ever log four inodes and the possible dir format conversion
(e.g., no unlink, no dir block frees). We could define a new
tr_rename_xchg reservation that encodes that and documents it
appropriately in the comment.

It might be worth getting a second opinion from Dave or somebody before
going too far ahead on the logging work...

> +	if (error == -ENOSPC) {
> +		spaceres = 0;
> +		error = xfs_trans_reserve(tp, &M_RES(mp)->tr_rename, 0, 0);
> +	}
> +	if (error) {
> +		xfs_trans_cancel(tp, 0);
> +		goto std_return;
> +	}
> +
> +	/*
> +	 * Attach the dquots to the inodes
> +	 */
> +	error = xfs_qm_vop_rename_dqattach(inodes);
> +	if (error) {
> +		xfs_trans_cancel(tp, cancel_flags);
> +		goto std_return;
> +	}
> +
> +	/*
> +	 * Lock all participating inodes. In case of RENAME_EXCHANGE, target
> +	 * must exist, so we'll be locking at least 3 inodes here.
> +	 */
> +	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.
> +	 * target_ip will always exist, so, no need to check its existence.
> +	 */
> +	xfs_trans_ijoin(tp, src_dp, XFS_ILOCK_EXCL);
> +	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_ip, XFS_ILOCK_EXCL);
> +
> +	/*
> +	* If we are using project inheritance, we only allow RENAME_EXCHANGE
> +	* into our tree when the project IDs are the same; else the tree quota
> +	* mechanism would be circumvented.
> +	*/
> +	if (unlikely(((target_dp->i_d.di_flags & XFS_DIFLAG_PROJINHERIT) ||
> +		     (src_dp->i_d.di_flags & XFS_DIFLAG_PROJINHERIT)) &&
> +		     (xfs_get_projid(src_dp) != xfs_get_projid(target_dp)) )) {
> +		error = -EXDEV;
> +		goto error_return;
> +	}
> +

I think that having a separate helper for the rename exchange case is
generally the right thing. That said, I wonder if we're splitting things
at the right level because it looks like xfs_rename() could handle
everything we have in xfs_cross_rename() up to about this point.

I definitely don't think we should go too far and try to handle all of
this in one function, even if there is some duplication in the directory
name replacement and inode link management. The logic would probably end
up unnecessarily hairy and difficult to reason about.

> +	error = xfs_dir_replace(tp, src_dp, src_name,
> +				target_ip->i_ino,
> +				&first_block, &free_list, spaceres);
> +	if (error)
> +		goto abort_return;
> +
> +	/*
> +	 * Update ".." entry to match the new parent
> +	 */
> +	if (new_parent && tgt_is_directory) {
> +		error = xfs_dir_replace(tp, target_ip, &xfs_name_dotdot,
> +					src_dp->i_ino, &first_block, &free_list, spaceres);
> +		if (error)
> +			goto abort_return;
> +	}
> +
> +	xfs_trans_ichgtime(tp, src_dp, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
> +
> +	error = xfs_dir_replace(tp, target_dp, target_name,
> +				src_ip->i_ino,
> +				&first_block, &free_list, spaceres);
> +	if (error)
> +		goto abort_return;
> +
> +	/*
> +	 * Update ".." entry to match the new parent
> +	 */
> +	if (new_parent && src_is_directory) {
> +		error = xfs_dir_replace(tp, src_ip, &xfs_name_dotdot,
> +					target_dp->i_ino, &first_block, &free_list, spaceres);
> +		if (error)
> +			goto abort_return;
> +	}
> +
> +	/*
> +	 * In case we are crossing different file types between different
> +	 * parents, we must update parent's link count to match the ".."
> +	 * entry of the new child (or the removal of it).
> +	 */
> +	if (new_parent) {
> +		xfs_trans_ichgtime(tp, target_dp, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
> +
> +		if (src_is_directory && !tgt_is_directory) {
> +			error = xfs_droplink(tp, src_dp);
> +				if (error)
> +					goto abort_return;
> +			error = xfs_bumplink(tp, target_dp);
> +				if (error)
> +					goto abort_return;
> +		}
> +
> +		if (tgt_is_directory && !src_is_directory) {
> +			error = xfs_droplink(tp, target_dp);
> +				if (error)
> +					goto abort_return;
> +			error = xfs_bumplink(tp, src_dp);
> +				if (error)
> +					goto abort_return;
> +		}
> +
> +		/*
> +		 * We don't need to log the source dir if
> +		 * this is the same as the target.
> +		 */
> +		xfs_trans_log_inode(tp, src_dp, XFS_ILOG_CORE);
> +	}
> +
> +	xfs_trans_log_inode(tp, target_dp, XFS_ILOG_CORE);
> +	xfs_trans_log_inode(tp, src_ip, XFS_ILOG_CORE);
> +	xfs_trans_log_inode(tp, target_ip, XFS_ILOG_CORE);
> +

... and from here to the end also looks equivalent to xfs_rename().
Could we do something like pass the flags (or some new parameter) to
xfs_rename() and convert the meat of the directory update calls into a
couple internal helpers? For example:

xfs_rename(...)
{
	/* setup tp, lock inodes, etc. */

	if (rename_exchange)
		error = xfs_rename_exchange_int(...);
	else
		error = xfs_rename(...);
	if (error)
		...

	/* tp completion handling */

	return xfs_trans_commit(...);

abort_return:
...

	return error;
}

... and that could be done with another refactoring patch to prepare
xfs_rename(). Just a thought.

> +	/*
> +	 * If this is a synchronous mount, make sure the rename transaction goes
> +	 * to disk before returning to the user.
> +	 */
> +	if (mp->m_flags & (XFS_MOUNT_WSYNC|XFS_MOUNT_DIRSYNC))
> +		xfs_trans_set_sync(tp);
> +
> +	error = xfs_bmap_finish(&tp, &free_list, &committed);
> +	if (error) {
> +		xfs_bmap_cancel(&free_list);
> +		xfs_trans_cancel(tp, (XFS_TRANS_RELEASE_LOG_RES | XFS_TRANS_ABORT));
> +		goto std_return;
> +	}
> +
> +	return xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES);
> +
> +abort_return:
> +	cancel_flags |= XFS_TRANS_ABORT;
> +error_return:
> +	xfs_bmap_cancel(&free_list);
> +	xfs_trans_cancel(tp, cancel_flags);
> +std_return:
> +	return error;
> +
> +}
> +
>  STATIC int
>  xfs_iflush_cluster(
>  	xfs_inode_t	*ip,
> diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
> index c10e3fa..16889d3 100644
> --- a/fs/xfs/xfs_inode.h
> +++ b/fs/xfs/xfs_inode.h
> @@ -341,6 +341,10 @@ int		xfs_rename(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);
> +int		xfs_cross_rename(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);
>  
>  void		xfs_ilock(xfs_inode_t *, uint);
>  int		xfs_ilock_nowait(xfs_inode_t *, uint);
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index b2b92c7..bc164df 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -391,12 +391,17 @@ xfs_vn_rename2(
>  	struct xfs_name	nname;
>  
>  	/* XFS does not support RENAME_EXCHANGE yet */
> -	if (flags & ~RENAME_NOREPLACE)
> +	if (flags & ~(RENAME_NOREPLACE | RENAME_EXCHANGE))
>  		return -EINVAL;
>  
>  	xfs_dentry_to_name(&oname, odentry, 0);

This might need to be handled differently for the exchange case. As
below, the new dentry always gets the old mode. I suspect we don't care
about the mode of the original dentry in traditional rename since that
entry goes away. It looks like it would be set to 0 here in the exchange
case, however, rather than ndentry->d_inode->i_mode (which we can't
assume exists for the non-exchange case).

Brian

>  	xfs_dentry_to_name(&nname, ndentry, odentry->d_inode->i_mode);
>  
> +	if (flags & RENAME_EXCHANGE)
> +		return xfs_cross_rename(XFS_I(odir), &oname, XFS_I(odentry->d_inode),
> +					XFS_I(ndir), &nname,
> +					new_inode ? XFS_I(new_inode) : NULL);
> +
>  	return xfs_rename(XFS_I(odir), &oname, XFS_I(odentry->d_inode),
>  			  XFS_I(ndir), &nname,
>  			  new_inode ? XFS_I(new_inode) : NULL);
> -- 
> 2.1.0
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/2] Add support to RENAME_EXCHANGE flag

[Dave Chinner]

On Thu, Oct 16, 2014 at 05:05:37PM -0400, Brian Foster wrote:
> On Wed, Oct 15, 2014 at 03:17:22PM -0300, Carlos Maiolino wrote:
> > Adds a new function named xfs_cross_rename(), responsible to handle requests
> > from sys_renameat2() using RENAME_EXCHANGE flag.
> > 
> > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> > ---
> 
> Hi Carlos,
> 
> Some high-level comments from a first pass...
> 
> >  fs/xfs/xfs_inode.c | 187 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  fs/xfs/xfs_inode.h |   4 ++
> >  fs/xfs/xfs_iops.c  |   7 +-
> >  3 files changed, 197 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > index fea3c92..a5bc88d 100644
> > --- a/fs/xfs/xfs_inode.c
> > +++ b/fs/xfs/xfs_inode.c
> > @@ -2920,6 +2920,193 @@ xfs_rename(
> >  	return error;
> >  }
> >  
> > +/* xfs_cross_rename()
> > + *
> > + * responsible to handle RENAME_EXCHANGE flag
> > + * in renameat2() sytemcall
> > + */
> > +int
> > +xfs_cross_rename(
> > +	xfs_inode_t	*src_dp,
> > +	struct xfs_name	*src_name,
> > +	xfs_inode_t	*src_ip,
> > +	xfs_inode_t	*target_dp,
> > +	struct xfs_name	*target_name,
> > +	xfs_inode_t	*target_ip)
> > +{
> > +	xfs_trans_t	*tp = NULL;
> > +	xfs_mount_t	*mp = src_dp->i_mount;
> > +	int		new_parent;		/* Crossing from different parents */
> > +	int		src_is_directory;
> > +	int		tgt_is_directory;
> > +	int		error;
> > +	xfs_bmap_free_t	free_list;
> > +	xfs_fsblock_t	first_block;
> > +	int		cancel_flags;
> > +	int		committed;
> > +	xfs_inode_t	*inodes[4];
> > +	int		spaceres;
> > +	int		num_inodes;
> > +
> > +	new_parent = (src_dp != target_dp);
> > +	src_is_directory = S_ISDIR(src_ip->i_d.di_mode);
> > +	tgt_is_directory = S_ISDIR(target_ip->i_d.di_mode);
> > +
> > +	xfs_sort_for_rename(src_dp, target_dp, src_ip, target_ip,
> > +				inodes, &num_inodes);
> > +
> > +	xfs_bmap_init(&free_list, &first_block);
> > +	tp = xfs_trans_alloc(mp, XFS_TRANS_RENAME);
> > +	cancel_flags = XFS_TRANS_RELEASE_LOG_RES;
> > +	spaceres = XFS_RENAME_SPACE_RES(mp, target_name->len);
> > +	error = xfs_trans_reserve(tp, &M_RES(mp)->tr_rename, spaceres, 0);
> > +
> 
> It seems to me that the existing block and log reservations would
> "cover" the rename exchange case, but it might be worth defining new
> reservations for the purpose of clarity and to prevent future problems.
> 
> XFS_RENAME_SPACE_RES() covers directory removal and insertion. Here we
> are doing neither, which makes me wonder whether we need a block
> reservation at all. It does appear that we have a sf dir case where the
> inode number could cause a format conversion. Perhaps we need something
> that calculates the blocks required for the insertion of the max of both
> names (it seems like the conversion would only happen once, but we don't
> know which way)? I haven't spent a ton of time in directory code, so I
> could easily be missing something.

The shortform replace can result in shortform->block conversion,
therefore we need the reservation.

> The tr_rename log reservation considers four inodes, two directory
> modifications, a target inode unlink (the overwrite case), and alloc
> btree mods for directory blocks being freed. IIUC, the exchange case
> should only ever log four inodes and the possible dir format conversion
> (e.g., no unlink, no dir block frees). We could define a new
> tr_rename_xchg reservation that encodes that and documents it
> appropriately in the comment.

The rename log reservation is the worse case that a rename operation
requires - it is not specific to a particular rename instance. This
new reanme type fits within the existing definition, so we should
just use it unchanged.

What it comes down to is that there is no point in trying to define
reservations for every single possible type of operation we can do -
it's just too much maintenance overhead to verify that they are
correct after some incidental change. If we define the worst case,
then everything else is covered and we don't have to care about
whether we have the reservation for a specific case right, or indeed
whether we are using the correct reservation for a specific rename
transaction....


> > +	if (error == -ENOSPC) {
> > +		spaceres = 0;
> > +		error = xfs_trans_reserve(tp, &M_RES(mp)->tr_rename, 0, 0);
> > +	}
> > +	if (error) {
> > +		xfs_trans_cancel(tp, 0);
> > +		goto std_return;
> > +	}

This is not necessary. The spaceres == 0 case in the rename is for
adding new directory entries at ENOSPC and that is checked by
xfs_dir_canenter(). We are not calling that function (because we
aren't adding a name) and therefore we can't run without a full
space reservation.

Oh, and kill that "std_return" name.

	if (error) {
		cancel_flags = 0;
		goto out_trans_cancel;
	}

> > +
> > +	/*
> > +	 * Attach the dquots to the inodes
> > +	 */
> > +	error = xfs_qm_vop_rename_dqattach(inodes);
> > +	if (error) {
> > +		xfs_trans_cancel(tp, cancel_flags);
> > +		goto std_return;
> > +	}

	if (error)
		goto out_trans_cancel;

> > +
> > +	/*
> > +	 * Lock all participating inodes. In case of RENAME_EXCHANGE, target
> > +	 * must exist, so we'll be locking at least 3 inodes here.
> > +	 */
> > +	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.
> > +	 * target_ip will always exist, so, no need to check its existence.
> > +	 */
> > +	xfs_trans_ijoin(tp, src_dp, XFS_ILOCK_EXCL);
> > +	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_ip, XFS_ILOCK_EXCL);
> > +
> > +	/*
> > +	* If we are using project inheritance, we only allow RENAME_EXCHANGE
> > +	* into our tree when the project IDs are the same; else the tree quota
> > +	* mechanism would be circumvented.
> > +	*/
> > +	if (unlikely(((target_dp->i_d.di_flags & XFS_DIFLAG_PROJINHERIT) ||
> > +		     (src_dp->i_d.di_flags & XFS_DIFLAG_PROJINHERIT)) &&
> > +		     (xfs_get_projid(src_dp) != xfs_get_projid(target_dp)) )) {
> > +		error = -EXDEV;
> > +		goto error_return;
> > +	}
> > +
> 
> I think that having a separate helper for the rename exchange case is
> generally the right thing. That said, I wonder if we're splitting things
> at the right level because it looks like xfs_rename() could handle
> everything we have in xfs_cross_rename() up to about this point.

Right. I think that splitting out the internal part of xfs_rename
after all this common setup code is the best way to proceed.

> I definitely don't think we should go too far and try to handle all of
> this in one function, even if there is some duplication in the directory
> name replacement and inode link management. The logic would probably end
> up unnecessarily hairy and difficult to reason about.
> 
> > +	error = xfs_dir_replace(tp, src_dp, src_name,
> > +				target_ip->i_ino,
> > +				&first_block, &free_list, spaceres);
> > +	if (error)
> > +		goto abort_return;
> > +
> > +	/*
> > +	 * Update ".." entry to match the new parent
> > +	 */
> > +	if (new_parent && tgt_is_directory) {
> > +		error = xfs_dir_replace(tp, target_ip, &xfs_name_dotdot,
> > +					src_dp->i_ino, &first_block, &free_list, spaceres);
> > +		if (error)
> > +			goto abort_return;
> > +	}
> > +
> > +	xfs_trans_ichgtime(tp, src_dp, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
> > +
> > +	error = xfs_dir_replace(tp, target_dp, target_name,
> > +				src_ip->i_ino,
> > +				&first_block, &free_list, spaceres);
> > +	if (error)
> > +		goto abort_return;
> > +
> > +	/*
> > +	 * Update ".." entry to match the new parent
> > +	 */
> > +	if (new_parent && src_is_directory) {
> > +		error = xfs_dir_replace(tp, src_ip, &xfs_name_dotdot,
> > +					target_dp->i_ino, &first_block, &free_list, spaceres);
> > +		if (error)
> > +			goto abort_return;
> > +	}

So you do a bunch of work based on new_parent and
tgt/src_is_directory, and then:

> > +
> > +	/*
> > +	 * In case we are crossing different file types between different
> > +	 * parents, we must update parent's link count to match the ".."
> > +	 * entry of the new child (or the removal of it).
> > +	 */
> > +	if (new_parent) {
> > +		xfs_trans_ichgtime(tp, target_dp, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
> > +
> > +		if (src_is_directory && !tgt_is_directory) {
> > +			error = xfs_droplink(tp, src_dp);
> > +				if (error)
> > +					goto abort_return;

[whitespace is screwed up]

> > +			error = xfs_bumplink(tp, target_dp);
> > +				if (error)
> > +					goto abort_return;
> > +		}
> > +
> > +		if (tgt_is_directory && !src_is_directory) {
> > +			error = xfs_droplink(tp, target_dp);
> > +				if (error)
> > +					goto abort_return;
> > +			error = xfs_bumplink(tp, src_dp);
> > +				if (error)
> > +					goto abort_return;
> > +		}
> > +
> > +		/*
> > +		 * We don't need to log the source dir if
> > +		 * this is the same as the target.
> > +		 */
> > +		xfs_trans_log_inode(tp, src_dp, XFS_ILOG_CORE);
> > +	}

You do a bunch more work based on the same variables. THis should
reall ybe combined into a single set of logic to manipulate the
directory states.

	if (new_parent) {
		if (tgt_is_directory) {
			error = xfs_dir_replace(tp, target_ip, &xfs_name_dotdot,
						src_dp->i_ino, &first_block, &free_list, spaceres);
			if (error)
				goto out_abort;
			if (!src_is_directory) {
				error = xfs_droplink(tp, target_dp);
				if (error)
					goto out_abort;
				error = xfs_bumplink(tp, src_dp);
				if (error)
					goto out_abort;
			}
		}

		if (src_is_directory) {
			error = xfs_dir_replace(tp, src_ip, &xfs_name_dotdot,
						target_dp->i_ino, &first_block, &free_list, spaceres);
			if (error)
				goto out_abort;
		.....


> 
> > +	/*
> > +	 * If this is a synchronous mount, make sure the rename transaction goes
> > +	 * to disk before returning to the user.
> > +	 */
> > +	if (mp->m_flags & (XFS_MOUNT_WSYNC|XFS_MOUNT_DIRSYNC))
> > +		xfs_trans_set_sync(tp);
> > +
> > +	error = xfs_bmap_finish(&tp, &free_list, &committed);
> > +	if (error) {
> > +		xfs_bmap_cancel(&free_list);
> > +		xfs_trans_cancel(tp, (XFS_TRANS_RELEASE_LOG_RES | XFS_TRANS_ABORT));
> > +		goto std_return;
> > +	}
> > +	return xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES);

	error = xfs_bmap_finish(&tp, &free_list, &committed);
	if (!error)
		return xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES);

> > +abort_return:
> > +	cancel_flags |= XFS_TRANS_ABORT;
> > +error_return:
> > +	xfs_bmap_cancel(&free_list);
> > +	xfs_trans_cancel(tp, cancel_flags);
> > +std_return:
> > +	return error;

out_abort:
	cancel_flags |= XFS_TRANS_ABORT;
out_bmap_cancel:
	xfs_bmap_cancel(&free_list);
out_trans_cancel:
	xfs_trans_cancel(tp, cancel_flags);
	return error;

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/2] Add support to RENAME_EXCHANGE flag

[Carlos Maiolino]

Thanks for the review guys, I'm going to apply the suggestions and send a V2

On Mon, Oct 20, 2014 at 11:25:28AM +1100, Dave Chinner wrote:
> On Thu, Oct 16, 2014 at 05:05:37PM -0400, Brian Foster wrote:
> > On Wed, Oct 15, 2014 at 03:17:22PM -0300, Carlos Maiolino wrote:
> > > Adds a new function named xfs_cross_rename(), responsible to handle requests
> > > from sys_renameat2() using RENAME_EXCHANGE flag.
> > > 
> > > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> > > ---
> > 
> > Hi Carlos,
> > 
> > Some high-level comments from a first pass...
> > 
> > >  fs/xfs/xfs_inode.c | 187 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  fs/xfs/xfs_inode.h |   4 ++
> > >  fs/xfs/xfs_iops.c  |   7 +-
> > >  3 files changed, 197 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > > index fea3c92..a5bc88d 100644
> > > --- a/fs/xfs/xfs_inode.c
> > > +++ b/fs/xfs/xfs_inode.c
> > > @@ -2920,6 +2920,193 @@ xfs_rename(
> > >  	return error;
> > >  }
> > >  
> > > +/* xfs_cross_rename()
> > > + *
> > > + * responsible to handle RENAME_EXCHANGE flag
> > > + * in renameat2() sytemcall
> > > + */
> > > +int
> > > +xfs_cross_rename(
> > > +	xfs_inode_t	*src_dp,
> > > +	struct xfs_name	*src_name,
> > > +	xfs_inode_t	*src_ip,
> > > +	xfs_inode_t	*target_dp,
> > > +	struct xfs_name	*target_name,
> > > +	xfs_inode_t	*target_ip)
> > > +{
> > > +	xfs_trans_t	*tp = NULL;
> > > +	xfs_mount_t	*mp = src_dp->i_mount;
> > > +	int		new_parent;		/* Crossing from different parents */
> > > +	int		src_is_directory;
> > > +	int		tgt_is_directory;
> > > +	int		error;
> > > +	xfs_bmap_free_t	free_list;
> > > +	xfs_fsblock_t	first_block;
> > > +	int		cancel_flags;
> > > +	int		committed;
> > > +	xfs_inode_t	*inodes[4];
> > > +	int		spaceres;
> > > +	int		num_inodes;
> > > +
> > > +	new_parent = (src_dp != target_dp);
> > > +	src_is_directory = S_ISDIR(src_ip->i_d.di_mode);
> > > +	tgt_is_directory = S_ISDIR(target_ip->i_d.di_mode);
> > > +
> > > +	xfs_sort_for_rename(src_dp, target_dp, src_ip, target_ip,
> > > +				inodes, &num_inodes);
> > > +
> > > +	xfs_bmap_init(&free_list, &first_block);
> > > +	tp = xfs_trans_alloc(mp, XFS_TRANS_RENAME);
> > > +	cancel_flags = XFS_TRANS_RELEASE_LOG_RES;
> > > +	spaceres = XFS_RENAME_SPACE_RES(mp, target_name->len);
> > > +	error = xfs_trans_reserve(tp, &M_RES(mp)->tr_rename, spaceres, 0);
> > > +
> > 
> > It seems to me that the existing block and log reservations would
> > "cover" the rename exchange case, but it might be worth defining new
> > reservations for the purpose of clarity and to prevent future problems.
> > 
> > XFS_RENAME_SPACE_RES() covers directory removal and insertion. Here we
> > are doing neither, which makes me wonder whether we need a block
> > reservation at all. It does appear that we have a sf dir case where the
> > inode number could cause a format conversion. Perhaps we need something
> > that calculates the blocks required for the insertion of the max of both
> > names (it seems like the conversion would only happen once, but we don't
> > know which way)? I haven't spent a ton of time in directory code, so I
> > could easily be missing something.
> 
> The shortform replace can result in shortform->block conversion,
> therefore we need the reservation.
> 
> > The tr_rename log reservation considers four inodes, two directory
> > modifications, a target inode unlink (the overwrite case), and alloc
> > btree mods for directory blocks being freed. IIUC, the exchange case
> > should only ever log four inodes and the possible dir format conversion
> > (e.g., no unlink, no dir block frees). We could define a new
> > tr_rename_xchg reservation that encodes that and documents it
> > appropriately in the comment.
> 
> The rename log reservation is the worse case that a rename operation
> requires - it is not specific to a particular rename instance. This
> new reanme type fits within the existing definition, so we should
> just use it unchanged.
> 
> What it comes down to is that there is no point in trying to define
> reservations for every single possible type of operation we can do -
> it's just too much maintenance overhead to verify that they are
> correct after some incidental change. If we define the worst case,
> then everything else is covered and we don't have to care about
> whether we have the reservation for a specific case right, or indeed
> whether we are using the correct reservation for a specific rename
> transaction....
> 
> 
> > > +	if (error == -ENOSPC) {
> > > +		spaceres = 0;
> > > +		error = xfs_trans_reserve(tp, &M_RES(mp)->tr_rename, 0, 0);
> > > +	}
> > > +	if (error) {
> > > +		xfs_trans_cancel(tp, 0);
> > > +		goto std_return;
> > > +	}
> 
> This is not necessary. The spaceres == 0 case in the rename is for
> adding new directory entries at ENOSPC and that is checked by
> xfs_dir_canenter(). We are not calling that function (because we
> aren't adding a name) and therefore we can't run without a full
> space reservation.
> 
> Oh, and kill that "std_return" name.
> 
> 	if (error) {
> 		cancel_flags = 0;
> 		goto out_trans_cancel;
> 	}
> 
> > > +
> > > +	/*
> > > +	 * Attach the dquots to the inodes
> > > +	 */
> > > +	error = xfs_qm_vop_rename_dqattach(inodes);
> > > +	if (error) {
> > > +		xfs_trans_cancel(tp, cancel_flags);
> > > +		goto std_return;
> > > +	}
> 
> 	if (error)
> 		goto out_trans_cancel;
> 
> > > +
> > > +	/*
> > > +	 * Lock all participating inodes. In case of RENAME_EXCHANGE, target
> > > +	 * must exist, so we'll be locking at least 3 inodes here.
> > > +	 */
> > > +	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.
> > > +	 * target_ip will always exist, so, no need to check its existence.
> > > +	 */
> > > +	xfs_trans_ijoin(tp, src_dp, XFS_ILOCK_EXCL);
> > > +	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_ip, XFS_ILOCK_EXCL);
> > > +
> > > +	/*
> > > +	* If we are using project inheritance, we only allow RENAME_EXCHANGE
> > > +	* into our tree when the project IDs are the same; else the tree quota
> > > +	* mechanism would be circumvented.
> > > +	*/
> > > +	if (unlikely(((target_dp->i_d.di_flags & XFS_DIFLAG_PROJINHERIT) ||
> > > +		     (src_dp->i_d.di_flags & XFS_DIFLAG_PROJINHERIT)) &&
> > > +		     (xfs_get_projid(src_dp) != xfs_get_projid(target_dp)) )) {
> > > +		error = -EXDEV;
> > > +		goto error_return;
> > > +	}
> > > +
> > 
> > I think that having a separate helper for the rename exchange case is
> > generally the right thing. That said, I wonder if we're splitting things
> > at the right level because it looks like xfs_rename() could handle
> > everything we have in xfs_cross_rename() up to about this point.
> 
> Right. I think that splitting out the internal part of xfs_rename
> after all this common setup code is the best way to proceed.
> 
> > I definitely don't think we should go too far and try to handle all of
> > this in one function, even if there is some duplication in the directory
> > name replacement and inode link management. The logic would probably end
> > up unnecessarily hairy and difficult to reason about.
> > 
> > > +	error = xfs_dir_replace(tp, src_dp, src_name,
> > > +				target_ip->i_ino,
> > > +				&first_block, &free_list, spaceres);
> > > +	if (error)
> > > +		goto abort_return;
> > > +
> > > +	/*
> > > +	 * Update ".." entry to match the new parent
> > > +	 */
> > > +	if (new_parent && tgt_is_directory) {
> > > +		error = xfs_dir_replace(tp, target_ip, &xfs_name_dotdot,
> > > +					src_dp->i_ino, &first_block, &free_list, spaceres);
> > > +		if (error)
> > > +			goto abort_return;
> > > +	}
> > > +
> > > +	xfs_trans_ichgtime(tp, src_dp, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
> > > +
> > > +	error = xfs_dir_replace(tp, target_dp, target_name,
> > > +				src_ip->i_ino,
> > > +				&first_block, &free_list, spaceres);
> > > +	if (error)
> > > +		goto abort_return;
> > > +
> > > +	/*
> > > +	 * Update ".." entry to match the new parent
> > > +	 */
> > > +	if (new_parent && src_is_directory) {
> > > +		error = xfs_dir_replace(tp, src_ip, &xfs_name_dotdot,
> > > +					target_dp->i_ino, &first_block, &free_list, spaceres);
> > > +		if (error)
> > > +			goto abort_return;
> > > +	}
> 
> So you do a bunch of work based on new_parent and
> tgt/src_is_directory, and then:
> 
> > > +
> > > +	/*
> > > +	 * In case we are crossing different file types between different
> > > +	 * parents, we must update parent's link count to match the ".."
> > > +	 * entry of the new child (or the removal of it).
> > > +	 */
> > > +	if (new_parent) {
> > > +		xfs_trans_ichgtime(tp, target_dp, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
> > > +
> > > +		if (src_is_directory && !tgt_is_directory) {
> > > +			error = xfs_droplink(tp, src_dp);
> > > +				if (error)
> > > +					goto abort_return;
> 
> [whitespace is screwed up]
> 
> > > +			error = xfs_bumplink(tp, target_dp);
> > > +				if (error)
> > > +					goto abort_return;
> > > +		}
> > > +
> > > +		if (tgt_is_directory && !src_is_directory) {
> > > +			error = xfs_droplink(tp, target_dp);
> > > +				if (error)
> > > +					goto abort_return;
> > > +			error = xfs_bumplink(tp, src_dp);
> > > +				if (error)
> > > +					goto abort_return;
> > > +		}
> > > +
> > > +		/*
> > > +		 * We don't need to log the source dir if
> > > +		 * this is the same as the target.
> > > +		 */
> > > +		xfs_trans_log_inode(tp, src_dp, XFS_ILOG_CORE);
> > > +	}
> 
> You do a bunch more work based on the same variables. THis should
> reall ybe combined into a single set of logic to manipulate the
> directory states.
> 
> 	if (new_parent) {
> 		if (tgt_is_directory) {
> 			error = xfs_dir_replace(tp, target_ip, &xfs_name_dotdot,
> 						src_dp->i_ino, &first_block, &free_list, spaceres);
> 			if (error)
> 				goto out_abort;
> 			if (!src_is_directory) {
> 				error = xfs_droplink(tp, target_dp);
> 				if (error)
> 					goto out_abort;
> 				error = xfs_bumplink(tp, src_dp);
> 				if (error)
> 					goto out_abort;
> 			}
> 		}
> 
> 		if (src_is_directory) {
> 			error = xfs_dir_replace(tp, src_ip, &xfs_name_dotdot,
> 						target_dp->i_ino, &first_block, &free_list, spaceres);
> 			if (error)
> 				goto out_abort;
> 		.....
> 
> 
> > 
> > > +	/*
> > > +	 * If this is a synchronous mount, make sure the rename transaction goes
> > > +	 * to disk before returning to the user.
> > > +	 */
> > > +	if (mp->m_flags & (XFS_MOUNT_WSYNC|XFS_MOUNT_DIRSYNC))
> > > +		xfs_trans_set_sync(tp);
> > > +
> > > +	error = xfs_bmap_finish(&tp, &free_list, &committed);
> > > +	if (error) {
> > > +		xfs_bmap_cancel(&free_list);
> > > +		xfs_trans_cancel(tp, (XFS_TRANS_RELEASE_LOG_RES | XFS_TRANS_ABORT));
> > > +		goto std_return;
> > > +	}
> > > +	return xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES);
> 
> 	error = xfs_bmap_finish(&tp, &free_list, &committed);
> 	if (!error)
> 		return xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES);
> 
> > > +abort_return:
> > > +	cancel_flags |= XFS_TRANS_ABORT;
> > > +error_return:
> > > +	xfs_bmap_cancel(&free_list);
> > > +	xfs_trans_cancel(tp, cancel_flags);
> > > +std_return:
> > > +	return error;
> 
> out_abort:
> 	cancel_flags |= XFS_TRANS_ABORT;
> out_bmap_cancel:
> 	xfs_bmap_cancel(&free_list);
> out_trans_cancel:
> 	xfs_trans_cancel(tp, cancel_flags);
> 	return error;
> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

-- 
Carlos

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 0/2] Add support to RENAME_EXCHANGE flat to XFS V2

[Carlos Maiolino]

This patchset aims to implement RENAME_EXCHANGE support (from sys_renameat2) to
XFS.

For this to be achieved, XFS need to export a rename2() method, which I included        
in the first patch.

The second patch is the real implementation of the RENAME_EXCHANGE flags, which
most of the work I based on xfs_rename().

This patchset passed the xfstests 23, 24 and 25 (specifically for
RENAME_EXCHANGE), and I also tested the projectID inheritance problem, where
both paths must be under the same projectID to be able to change (I'm going to
implement this test into the xfstests too).

In this version of the patch, I tried to use xfs_rename for everything in
common between a usual rename and the RENAME_EXCHANGE, using xfs_cross_rename()
just for the needed stuff to accomplish RENAME_EXCHANGE requirements (as
suggested by Brian).

Also, applied suggestions from Dave, merging all the logic involving new_parent
variable

Carlos Maiolino (2):
  Make xfs_vn_rename compliant with renameat2() syscall
  Add support to RENAME_EXCHANGE flag

 fs/xfs/xfs_inode.c | 315 +++++++++++++++++++++++++++++++++++------------------
 fs/xfs/xfs_inode.h |   8 +-
 fs/xfs/xfs_iops.c  |  15 ++-
 3 files changed, 228 insertions(+), 110 deletions(-)

-- 
2.1.0

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 1/2] Make xfs_vn_rename compliant with renameat2() syscall

[Carlos Maiolino]

To be able to support RENAME_EXCHANGE flag from renameat2() system call, XFS
must have its inode_operations updated, exporting .rename2 method, instead of
.rename.

This patch just replaces the (now old) .rename method by .rename2, using the
same infra-structure, but checking rename flags.

calls to .rename2 using RENAME_EXCHANGE flag, although now handled inside XFS,
still returns -EINVAL.

RENAME_NOREPLACE is handled via VFS and we don't need to care about it inside
xfs_vn_rename.

Changelog:

	V2: Use xfs_vn_rename as-is, instead of rename it to xfs_vn_rename2

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 fs/xfs/xfs_iops.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 7212949..8519442 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -383,18 +383,23 @@ xfs_vn_rename(
 	struct inode	*odir,
 	struct dentry	*odentry,
 	struct inode	*ndir,
-	struct dentry	*ndentry)
+	struct dentry	*ndentry,
+	unsigned int	flags)
 {
 	struct inode	*new_inode = ndentry->d_inode;
 	struct xfs_name	oname;
 	struct xfs_name	nname;
 
+	/* XFS does not support RENAME_EXCHANGE yet */
+	if (flags & ~RENAME_NOREPLACE)
+		return -EINVAL;
+
 	xfs_dentry_to_name(&oname, odentry, 0);
 	xfs_dentry_to_name(&nname, ndentry, odentry->d_inode->i_mode);
 
 	return xfs_rename(XFS_I(odir), &oname, XFS_I(odentry->d_inode),
-			  XFS_I(ndir), &nname, new_inode ?
-						XFS_I(new_inode) : NULL);
+			  XFS_I(ndir), &nname,
+			  new_inode ? XFS_I(new_inode) : NULL);
 }
 
 /*
@@ -1117,7 +1122,7 @@ static const struct inode_operations xfs_dir_inode_operations = {
 	 */
 	.rmdir			= xfs_vn_unlink,
 	.mknod			= xfs_vn_mknod,
-	.rename			= xfs_vn_rename,
+	.rename2		= xfs_vn_rename,
 	.get_acl		= xfs_get_acl,
 	.set_acl		= xfs_set_acl,
 	.getattr		= xfs_vn_getattr,
@@ -1145,7 +1150,7 @@ static const struct inode_operations xfs_dir_ci_inode_operations = {
 	 */
 	.rmdir			= xfs_vn_unlink,
 	.mknod			= xfs_vn_mknod,
-	.rename			= xfs_vn_rename,
+	.rename2		= xfs_vn_rename,
 	.get_acl		= xfs_get_acl,
 	.set_acl		= xfs_set_acl,
 	.getattr		= xfs_vn_getattr,
-- 
2.1.0

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 2/2] Add support to RENAME_EXCHANGE flag

[Carlos Maiolino]

Adds a new function named xfs_cross_rename(), responsible to handle requests
from sys_renameat2() using RENAME_EXCHANGE flag.

Changelog:

	V2: refactor xfs_cross_rename() to not duplicate code from xfs_rename()

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 fs/xfs/xfs_inode.c | 315 +++++++++++++++++++++++++++++++++++------------------
 fs/xfs/xfs_inode.h |   8 +-
 fs/xfs/xfs_iops.c  |   4 +-
 3 files changed, 220 insertions(+), 107 deletions(-)

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index fea3c92..bf09bfc 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -2674,12 +2674,14 @@ xfs_rename(
 	xfs_inode_t	*src_ip,
 	xfs_inode_t	*target_dp,
 	struct xfs_name	*target_name,
-	xfs_inode_t	*target_ip)
+	xfs_inode_t	*target_ip,
+	unsigned int	flags)
 {
 	xfs_trans_t	*tp = NULL;
 	xfs_mount_t	*mp = src_dp->i_mount;
 	int		new_parent;		/* moving to a new dir */
 	int		src_is_directory;	/* src_name is a directory */
+	int		tgt_is_directory;
 	int		error;
 	xfs_bmap_free_t free_list;
 	xfs_fsblock_t   first_block;
@@ -2752,141 +2754,158 @@ xfs_rename(
 	}
 
 	/*
-	 * Set up the target.
-	 */
-	if (target_ip == NULL) {
+	 * Handle RENAME_EXCHANGE flags
+	*/
+	if (flags & RENAME_EXCHANGE) {
 		/*
-		 * If there's no space reservation, check the entry will
-		 * fit before actually inserting it.
+		 * target_ip will always exist if RENAME_EXCHANGE flag is set
 		 */
-		error = xfs_dir_canenter(tp, target_dp, target_name, spaceres);
+		tgt_is_directory = S_ISDIR(target_ip->i_d.di_mode);
+
+		error = xfs_cross_rename(src_dp, src_name, src_ip, target_dp, target_name, target_ip,
+					 new_parent, src_is_directory, tgt_is_directory,
+					 &free_list, &first_block, tp, spaceres);
 		if (error)
-			goto error_return;
+			goto abort_return;
+	} else {
 		/*
-		 * If target does not exist and the rename crosses
-		 * directories, adjust the target directory link count
-		 * to account for the ".." reference from the new entry.
+		 * Set up the target.
 		 */
-		error = xfs_dir_createname(tp, target_dp, target_name,
-						src_ip->i_ino, &first_block,
-						&free_list, spaceres);
-		if (error == -ENOSPC)
-			goto error_return;
-		if (error)
-			goto abort_return;
+		if (target_ip == NULL) {
+			/*
+			 * If there's no space reservation, check the entry will
+			 * fit before actually inserting it.
+			 */
+			error = xfs_dir_canenter(tp, target_dp, target_name, spaceres);
+			if (error)
+				goto error_return;
+			/*
+			 * If target does not exist and the rename crosses
+			 * directories, adjust the target directory link count
+			 * to account for the ".." reference from the new entry.
+			 */
+			error = xfs_dir_createname(tp, target_dp, target_name,
+							src_ip->i_ino, &first_block,
+							&free_list, spaceres);
+			if (error == -ENOSPC)
+				goto error_return;
+			if (error)
+				goto abort_return;
 
-		xfs_trans_ichgtime(tp, target_dp,
-					XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
+			xfs_trans_ichgtime(tp, target_dp,
+						XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
 
-		if (new_parent && src_is_directory) {
-			error = xfs_bumplink(tp, target_dp);
+			if (new_parent && src_is_directory) {
+				error = xfs_bumplink(tp, target_dp);
+				if (error)
+					goto abort_return;
+			}
+		} else { /* target_ip != NULL */
+			/*
+			 * If target exists and it's a directory, check that both
+			 * target and source are directories and that target can be
+			 * destroyed, or that neither is a directory.
+			 */
+			if (S_ISDIR(target_ip->i_d.di_mode)) {
+				/*
+				 * Make sure target dir is empty.
+				 */
+				if (!(xfs_dir_isempty(target_ip)) ||
+				    (target_ip->i_d.di_nlink > 2)) {
+					error = -EEXIST;
+					goto error_return;
+				}
+			}
+
+			/*
+			 * Link the source inode under the target name.
+			 * If the source inode is a directory and we are moving
+			 * it across directories, its ".." entry will be
+			 * inconsistent until we replace that down below.
+			 *
+			 * In case there is already an entry with the same
+			 * name at the destination directory, remove it first.
+			 */
+			error = xfs_dir_replace(tp, target_dp, target_name,
+						src_ip->i_ino,
+						&first_block, &free_list, spaceres);
 			if (error)
 				goto abort_return;
-		}
-	} else { /* target_ip != NULL */
+
+			xfs_trans_ichgtime(tp, target_dp,
+						XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
+
+			/*
+			 * Decrement the link count on the target since the target
+			 * dir no longer points to it.
+			 */
+			error = xfs_droplink(tp, target_ip);
+			if (error)
+				goto abort_return;
+
+			if (src_is_directory) {
+				/*
+				 * Drop the link from the old "." entry.
+				 */
+				error = xfs_droplink(tp, target_ip);
+				if (error)
+					goto abort_return;
+			}
+		} /* target_ip != NULL */
+
 		/*
-		 * If target exists and it's a directory, check that both
-		 * target and source are directories and that target can be
-		 * destroyed, or that neither is a directory.
+		 * Remove the source.
 		 */
-		if (S_ISDIR(target_ip->i_d.di_mode)) {
+		if (new_parent && src_is_directory) {
 			/*
-			 * Make sure target dir is empty.
+			 * Rewrite the ".." entry to point to the new
+			 * directory.
 			 */
-			if (!(xfs_dir_isempty(target_ip)) ||
-			    (target_ip->i_d.di_nlink > 2)) {
-				error = -EEXIST;
-				goto error_return;
-			}
+			error = xfs_dir_replace(tp, src_ip, &xfs_name_dotdot,
+						target_dp->i_ino,
+						&first_block, &free_list, spaceres);
+			ASSERT(error != -EEXIST);
+			if (error)
+				goto abort_return;
 		}
 
 		/*
-		 * Link the source inode under the target name.
-		 * If the source inode is a directory and we are moving
-		 * it across directories, its ".." entry will be
-		 * inconsistent until we replace that down below.
+		 * We always want to hit the ctime on the source inode.
 		 *
-		 * In case there is already an entry with the same
-		 * name at the destination directory, remove it first.
+		 * This isn't strictly required by the standards since the source
+		 * inode isn't really being changed, but old unix file systems did
+		 * it and some incremental backup programs won't work without it.
 		 */
-		error = xfs_dir_replace(tp, target_dp, target_name,
-					src_ip->i_ino,
-					&first_block, &free_list, spaceres);
-		if (error)
-			goto abort_return;
-
-		xfs_trans_ichgtime(tp, target_dp,
-					XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
+		xfs_trans_ichgtime(tp, src_ip, XFS_ICHGTIME_CHG);
+		xfs_trans_log_inode(tp, src_ip, XFS_ILOG_CORE);
 
 		/*
-		 * Decrement the link count on the target since the target
-		 * dir no longer points to it.
+		 * Adjust the link count on src_dp.  This is necessary when
+		 * renaming a directory, either within one parent when
+		 * the target existed, or across two parent directories.
 		 */
-		error = xfs_droplink(tp, target_ip);
-		if (error)
-			goto abort_return;
+		if (src_is_directory && (new_parent || target_ip != NULL)) {
 
-		if (src_is_directory) {
 			/*
-			 * Drop the link from the old "." entry.
+			 * Decrement link count on src_directory since the
+			 * entry that's moved no longer points to it.
 			 */
-			error = xfs_droplink(tp, target_ip);
+			error = xfs_droplink(tp, src_dp);
 			if (error)
 				goto abort_return;
 		}
-	} /* target_ip != NULL */
-
-	/*
-	 * Remove the source.
-	 */
-	if (new_parent && src_is_directory) {
-		/*
-		 * Rewrite the ".." entry to point to the new
-		 * directory.
-		 */
-		error = xfs_dir_replace(tp, src_ip, &xfs_name_dotdot,
-					target_dp->i_ino,
-					&first_block, &free_list, spaceres);
-		ASSERT(error != -EEXIST);
-		if (error)
-			goto abort_return;
-	}
 
-	/*
-	 * We always want to hit the ctime on the source inode.
-	 *
-	 * This isn't strictly required by the standards since the source
-	 * inode isn't really being changed, but old unix file systems did
-	 * it and some incremental backup programs won't work without it.
-	 */
-	xfs_trans_ichgtime(tp, src_ip, XFS_ICHGTIME_CHG);
-	xfs_trans_log_inode(tp, src_ip, XFS_ILOG_CORE);
-
-	/*
-	 * Adjust the link count on src_dp.  This is necessary when
-	 * renaming a directory, either within one parent when
-	 * the target existed, or across two parent directories.
-	 */
-	if (src_is_directory && (new_parent || target_ip != NULL)) {
-
-		/*
-		 * Decrement link count on src_directory since the
-		 * entry that's moved no longer points to it.
-		 */
-		error = xfs_droplink(tp, src_dp);
+		error = xfs_dir_removename(tp, src_dp, src_name, src_ip->i_ino,
+						&first_block, &free_list, spaceres);
 		if (error)
 			goto abort_return;
-	}
 
-	error = xfs_dir_removename(tp, src_dp, src_name, src_ip->i_ino,
-					&first_block, &free_list, spaceres);
-	if (error)
-		goto abort_return;
+		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);
 
-	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);
+	} /* RENAME_EXCHANGE */
 
 	/*
 	 * If this is a synchronous mount, make sure that the
@@ -2920,6 +2939,94 @@ xfs_rename(
 	return error;
 }
 
+/* xfs_cross_rename()
+ *
+ * responsible to handle RENAME_EXCHANGE flag
+ * in renameat2() sytemcall
+ */
+int
+xfs_cross_rename(
+	xfs_inode_t	*src_dp,
+	struct xfs_name	*src_name,
+	xfs_inode_t	*src_ip,
+	xfs_inode_t	*target_dp,
+	struct xfs_name	*target_name,
+	xfs_inode_t	*target_ip,
+	int		new_parent,
+	int		src_is_directory,
+	int		tgt_is_directory,
+	xfs_bmap_free_t	*free_list,
+	xfs_fsblock_t	*first_block,
+	xfs_trans_t	*tp,
+	int		spaceres)
+{
+	int		error = 0;
+
+	/* Replace source inode */
+	error = xfs_dir_replace(tp, src_dp, src_name,
+				target_ip->i_ino,
+				first_block, free_list, spaceres);
+	if (error)
+		goto out_abort;
+
+	/* Replace target inode */
+	error = xfs_dir_replace(tp, target_dp, target_name,
+				src_ip->i_ino,
+				first_block, free_list, spaceres);
+	if (error)
+		goto out_abort;
+
+	xfs_trans_ichgtime(tp, src_dp, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
+
+	/*
+	 * Update ".." entry to match the new parents
+	 *
+	 * In case we are crossing different file types between different
+	 * parents, we must update parent's link count to match the ".."
+	 * entry of the new child (or the removal of it).
+	 */
+	if (new_parent) {
+		xfs_trans_ichgtime(tp, target_dp, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
+
+		if (tgt_is_directory) {
+			error = xfs_dir_replace(tp, target_ip, &xfs_name_dotdot, src_dp->i_ino,
+						first_block, free_list, spaceres);
+			if (error)
+				goto out_abort;
+			if (!src_is_directory) {
+				error = xfs_droplink(tp, target_dp);
+				if (error)
+					goto out_abort;
+				error = xfs_bumplink(tp, src_dp);
+				if (error)
+					goto out_abort;
+			}
+		}
+
+		if (src_is_directory) {
+			error = xfs_dir_replace(tp, src_ip, &xfs_name_dotdot, target_dp->i_ino,
+						first_block, free_list, spaceres);
+			if (error)
+				goto out_abort;
+			if (!tgt_is_directory) {
+				error = xfs_droplink(tp, src_dp);
+				if (error)
+					goto out_abort;
+				error = xfs_bumplink(tp, target_dp);
+				if (error)
+					goto out_abort;
+			}
+		}
+		xfs_trans_log_inode(tp, src_dp, XFS_ILOG_CORE);
+	}
+	xfs_trans_log_inode(tp, target_dp, XFS_ILOG_CORE);
+	xfs_trans_log_inode(tp, src_ip, XFS_ILOG_CORE);
+	xfs_trans_log_inode(tp, target_ip, XFS_ILOG_CORE);
+
+out_abort:
+	return error;
+}
+
 STATIC int
 xfs_iflush_cluster(
 	xfs_inode_t	*ip,
diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
index c10e3fa..c34dd41 100644
--- a/fs/xfs/xfs_inode.h
+++ b/fs/xfs/xfs_inode.h
@@ -340,7 +340,13 @@ int		xfs_link(struct xfs_inode *tdp, struct xfs_inode *sip,
 int		xfs_rename(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);
+			   struct xfs_inode *target_ip, unsigned int flags);
+int		xfs_cross_rename(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,
+			   int new_parent, int src_is_directory, int tgt_is_directory,
+			   struct xfs_bmap_free *free_list, xfs_fsblock_t *first_block,
+			   struct xfs_trans *tp, int spaceres);
 
 void		xfs_ilock(xfs_inode_t *, uint);
 int		xfs_ilock_nowait(xfs_inode_t *, uint);
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 8519442..d5ba974 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -391,7 +391,7 @@ xfs_vn_rename(
 	struct xfs_name	nname;
 
 	/* XFS does not support RENAME_EXCHANGE yet */
-	if (flags & ~RENAME_NOREPLACE)
+	if (flags & ~(RENAME_NOREPLACE | RENAME_EXCHANGE))
 		return -EINVAL;
 
 	xfs_dentry_to_name(&oname, odentry, 0);
@@ -399,7 +399,7 @@ xfs_vn_rename(
 
 	return xfs_rename(XFS_I(odir), &oname, XFS_I(odentry->d_inode),
 			  XFS_I(ndir), &nname,
-			  new_inode ? XFS_I(new_inode) : NULL);
+			  new_inode ? XFS_I(new_inode) : NULL, flags);
 }
 
 /*
-- 
2.1.0

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/2] Add support to RENAME_EXCHANGE flag

[Eric Sandeen]

On 11/10/14 11:41 AM, Carlos Maiolino wrote:
> Adds a new function named xfs_cross_rename(), responsible to handle requests
> from sys_renameat2() using RENAME_EXCHANGE flag.
> 
> Changelog:
> 
> 	V2: refactor xfs_cross_rename() to not duplicate code from xfs_rename()

Which tree is this against?

> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>  fs/xfs/xfs_inode.c | 315 +++++++++++++++++++++++++++++++++++------------------
>  fs/xfs/xfs_inode.h |   8 +-
>  fs/xfs/xfs_iops.c  |   4 +-
>  3 files changed, 220 insertions(+), 107 deletions(-)
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index fea3c92..bf09bfc 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -2674,12 +2674,14 @@ xfs_rename(
>  	xfs_inode_t	*src_ip,
>  	xfs_inode_t	*target_dp,
>  	struct xfs_name	*target_name,
> -	xfs_inode_t	*target_ip)
> +	xfs_inode_t	*target_ip,
> +	unsigned int	flags)
>  {
>  	xfs_trans_t	*tp = NULL;
>  	xfs_mount_t	*mp = src_dp->i_mount;
>  	int		new_parent;		/* moving to a new dir */
>  	int		src_is_directory;	/* src_name is a directory */
> +	int		tgt_is_directory;
>  	int		error;
>  	xfs_bmap_free_t free_list;
>  	xfs_fsblock_t   first_block;
> @@ -2752,141 +2754,158 @@ xfs_rename(
>  	}
>  
>  	/*
> -	 * Set up the target.
> -	 */
> -	if (target_ip == NULL) {
> +	 * Handle RENAME_EXCHANGE flags
> +	*/
> +	if (flags & RENAME_EXCHANGE) {
>  		/*
> -		 * If there's no space reservation, check the entry will
> -		 * fit before actually inserting it.
> +		 * target_ip will always exist if RENAME_EXCHANGE flag is set
>  		 */
> -		error = xfs_dir_canenter(tp, target_dp, target_name, spaceres);
> +		tgt_is_directory = S_ISDIR(target_ip->i_d.di_mode);
> +
> +		error = xfs_cross_rename(src_dp, src_name, src_ip, target_dp, target_name, target_ip,
> +					 new_parent, src_is_directory, tgt_is_directory,
> +					 &free_list, &first_block, tp, spaceres);

Ok, just some style drive-by comments, first -

EEK ;)  13 args and ~100 chars wide ;)

You could at least move some of those back to local vars in the
cross_rename function so you don't have to pass them on the stack.
It's also customary to pass the transaction pointer as the first arg,
i.e.:

+		error = xfs_cross_rename(tp, src_dp, src_name, src_ip,
+					 target_dp, target_name, target_ip,
+					 &free_list, &first_block, spaceres);

and:

xfs_cross_rename()
{
	...
	new_parent = (src_dp != target_dp);
	src_is_directory = S_ISDIR(src_ip->i_d.di_mode);
	tgt_is_directory = S_ISDIR(target_ip->i_d.di_mode);
	...

}

There are a few other places where you go well beyond 80 chars, would rather
not do that if at all possible.

-Eric

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/2] Add support to RENAME_EXCHANGE flag

[Carlos Maiolino]

On Mon, Nov 10, 2014 at 12:13:02PM -0600, Eric Sandeen wrote:
> On 11/10/14 11:41 AM, Carlos Maiolino wrote:
> > Adds a new function named xfs_cross_rename(), responsible to handle requests
> > from sys_renameat2() using RENAME_EXCHANGE flag.
> > 
> > Changelog:
> > 
> > 	V2: refactor xfs_cross_rename() to not duplicate code from xfs_rename()
> 
> Which tree is this against?
> 


It's based on an 'old' version of linux.git, which I was using when I started to
work on this patch, I'll rebase it on top of the current tree.

> Ok, just some style drive-by comments, first -
> 
> EEK ;)  13 args and ~100 chars wide ;)
> 
> You could at least move some of those back to local vars in the
> cross_rename function so you don't have to pass them on the stack.
> It's also customary to pass the transaction pointer as the first arg,
> i.e.:
> 
> 
> There are a few other places where you go well beyond 80 chars, would rather
> not do that if at all possible.
> 
Yes, after reading it on the list, although it works, it looks a mess :( sorry
about it.

I'll wait for some other comments and re-work on it, so, tomorrow I can send a
new version of it.

Thanks for the comments Eric

-- 
Carlos

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 2/2] Add support to RENAME_EXCHANGE flag

[Carlos Maiolino]

Adds a new function named xfs_cross_rename(), responsible for handling requests
from sys_renameat2() using RENAME_EXCHANGE flag.

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 fs/xfs/xfs_inode.c | 134 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 fs/xfs/xfs_inode.h |   2 +-
 fs/xfs/xfs_iops.c  |  12 +++--
 3 files changed, 142 insertions(+), 6 deletions(-)

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 8ed049d..61c796a 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -2669,6 +2669,124 @@ xfs_sort_for_rename(
 }
 
 /*
+ * xfs_cross_rename()
+ *
+ * responsible for handling RENAME_EXCHANGE flag in renameat2() sytemcall
+ */
+STATIC int
+xfs_cross_rename(
+	struct xfs_trans	*tp,
+	struct xfs_inode	*dp1,
+	struct xfs_name		*name1,
+	struct xfs_inode	*ip1,
+	struct xfs_inode	*dp2,
+	struct xfs_name		*name2,
+	struct xfs_inode	*ip2,
+	struct xfs_bmap_free	*free_list,
+	xfs_fsblock_t		*first_block,
+	int			spaceres)
+{
+	int		error = 0;
+	int		ip1_flags = 0;
+	int		ip2_flags = 0;
+	int		dp2_flags = 0;
+
+	/* Swap inode number for dirent in first parent */
+	error = xfs_dir_replace(tp, dp1, name1,
+				ip2->i_ino,
+				first_block, free_list, spaceres);
+	if (error)
+		goto out;
+
+	/* Swap inode number for dirent in second parent */
+	error = xfs_dir_replace(tp, dp2, name2,
+				ip1->i_ino,
+				first_block, free_list, spaceres);
+	if (error)
+		goto out;
+
+	/*
+	 * If we're renaming one or more directories across different parents,
+	 * update the respective ".." entries (and link counts) to match the new
+	 * parents.
+	 */
+	if (dp1 != dp2) {
+		dp2_flags = XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG;
+
+		if (S_ISDIR(ip2->i_d.di_mode)) {
+			error = xfs_dir_replace(tp, ip2, &xfs_name_dotdot,
+						dp1->i_ino, first_block,
+						free_list, spaceres);
+			if (error)
+				goto out;
+
+			/* transfer ip2 ".." reference to dp1 */
+			if (!S_ISDIR(ip1->i_d.di_mode)) {
+				error = xfs_droplink(tp, dp2);
+				if (error)
+					goto out;
+				error = xfs_bumplink(tp, dp1);
+				if (error)
+					goto out;
+			}
+
+			/*
+			 * Although ip1 isn't changed here, userspace needs
+			 * to be warned about the change, so that applications
+			 * relying on it (like backup ones), will properly
+			 * notify the change
+			 */
+			ip1_flags |= XFS_ICHGTIME_CHG;
+			ip2_flags |= XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG;
+		}
+
+		if (S_ISDIR(ip1->i_d.di_mode)) {
+			error = xfs_dir_replace(tp, ip1, &xfs_name_dotdot,
+						dp2->i_ino, first_block,
+						free_list, spaceres);
+			if (error)
+				goto out;
+
+			/* transfer ip1 ".." reference to dp2 */
+			if (!S_ISDIR(ip2->i_d.di_mode)) {
+				error = xfs_droplink(tp, dp1);
+				if (error)
+					goto out;
+				error = xfs_bumplink(tp, dp2);
+				if (error)
+					goto out;
+			}
+
+			/*
+			 * Although ip2 isn't changed here, userspace needs
+			 * to be warned about the change, so that applications
+			 * relying on it (like backup ones), will properly
+			 * notify the change
+			 */
+			ip1_flags |= XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG;
+			ip2_flags |= XFS_ICHGTIME_CHG;
+		}
+	}
+
+	if (ip1_flags) {
+		xfs_trans_ichgtime(tp, ip1, ip1_flags);
+		xfs_trans_log_inode(tp, ip1, XFS_ILOG_CORE);
+	}
+	if (ip2_flags) {
+		xfs_trans_ichgtime(tp, ip2, ip2_flags);
+		xfs_trans_log_inode(tp, ip2, XFS_ILOG_CORE);
+	}
+	if (dp2_flags) {
+		xfs_trans_ichgtime(tp, dp2, dp2_flags);
+		xfs_trans_log_inode(tp, dp2, XFS_ILOG_CORE);
+	}
+	xfs_trans_ichgtime(tp, dp1, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
+	xfs_trans_log_inode(tp, dp1, XFS_ILOG_CORE);
+out:
+	return error;
+}
+
+/*
  * xfs_rename
  */
 int
@@ -2678,7 +2796,8 @@ xfs_rename(
 	xfs_inode_t	*src_ip,
 	xfs_inode_t	*target_dp,
 	struct xfs_name	*target_name,
-	xfs_inode_t	*target_ip)
+	xfs_inode_t	*target_ip,
+	unsigned int	flags)
 {
 	xfs_trans_t	*tp = NULL;
 	xfs_mount_t	*mp = src_dp->i_mount;
@@ -2756,6 +2875,18 @@ xfs_rename(
 	}
 
 	/*
+	 * Handle RENAME_EXCHANGE flags
+	 */
+	if (flags & RENAME_EXCHANGE) {
+		error = xfs_cross_rename(tp, src_dp, src_name, src_ip,
+					 target_dp, target_name, target_ip,
+					 &free_list, &first_block, spaceres);
+		if (error)
+			goto abort_return;
+		goto finish_rename;
+	}
+
+	/*
 	 * Set up the target.
 	 */
 	if (target_ip == NULL) {
@@ -2894,6 +3025,7 @@ xfs_rename(
 	if (new_parent)
 		xfs_trans_log_inode(tp, target_dp, XFS_ILOG_CORE);
 
+finish_rename:
 	/*
 	 * If this is a synchronous mount, make sure that the
 	 * rename transaction goes to disk before returning to
diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
index 9af2882..051d9f0 100644
--- a/fs/xfs/xfs_inode.h
+++ b/fs/xfs/xfs_inode.h
@@ -340,7 +340,7 @@ int		xfs_link(struct xfs_inode *tdp, struct xfs_inode *sip,
 int		xfs_rename(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);
+			   struct xfs_inode *target_ip, unsigned int flags);
 
 void		xfs_ilock(xfs_inode_t *, uint);
 int		xfs_ilock_nowait(xfs_inode_t *, uint);
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 0b8704c..3049eb3 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -387,19 +387,23 @@ xfs_vn_rename(
 	unsigned int	flags)
 {
 	struct inode	*new_inode = ndentry->d_inode;
+	int		omode = 0;
 	struct xfs_name	oname;
 	struct xfs_name	nname;
 
-	/* XFS does not support RENAME_EXCHANGE yet */
-	if (flags & ~RENAME_NOREPLACE)
+	if (flags & ~(RENAME_NOREPLACE | RENAME_EXCHANGE))
 		return -EINVAL;
 
-	xfs_dentry_to_name(&oname, odentry, 0);
+	/* if we are exchanging files, we need to set i_mode of both files */
+	if (flags & RENAME_EXCHANGE)
+		omode = ndentry->d_inode->i_mode;
+
+	xfs_dentry_to_name(&oname, odentry, omode);
 	xfs_dentry_to_name(&nname, ndentry, odentry->d_inode->i_mode);
 
 	return xfs_rename(XFS_I(odir), &oname, XFS_I(odentry->d_inode),
 			  XFS_I(ndir), &nname,
-			  new_inode ? XFS_I(new_inode) : NULL);
+			  new_inode ? XFS_I(new_inode) : NULL, flags);
 }
 
 /*
-- 
1.9.3

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs