[PATCH 0/2] Add support to RENAME_EXCHANGE flag to XFS V7

[PATCH 0/2] Add support to RENAME_EXCHANGE flag to XFS V7

[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, this new version contains a fix to ensure both files will have their
i_mode updated, so that d_type object (in superblock V5) will contain the proper
information after the exchange happens

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

 fs/xfs/xfs_inode.c | 113 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 fs/xfs/xfs_inode.h |   2 +-
 fs/xfs/xfs_iops.c  |  24 +++++++++---
 3 files changed, 131 insertions(+), 8 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 ec6dcdc..0b8704c 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);
 }
 
 /*
@@ -1147,7 +1152,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,
@@ -1175,7 +1180,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

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

[Brian Foster]

On Fri, Nov 14, 2014 at 04:32:19PM -0200, Carlos Maiolino wrote:
> 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>
> ---

Reviewed-by: Brian Foster <bfoster@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 ec6dcdc..0b8704c 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);
>  }
>  
>  /*
> @@ -1147,7 +1152,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,
> @@ -1175,7 +1180,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

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

[PATCH 2/2] Add support to RENAME_EXCHANGE flag V7

[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()

	V3: - fix indentation to avoid 80 column crossing, decrease the amount of
	      arguments passed to xfs_cross_rename()
	    - Rebase patches over the latest linux code

	v4: - use a label/goto statement instead of an if conditional after
	      xfs_cross_rename() return, to finish the rename operation
	    - Make xfs_cross_rename() static
	    - Fix some comments

	V5: - Keep all the code under 80 columns

	V6: - Ensure i_mode of both files are updated during exchange

	V7: - Use struct names instead of typedefs in the xfs_cross_rename()
	      definition

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

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 8ed049d..3a77254 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -2668,6 +2668,103 @@ xfs_sort_for_rename(
 	}
 }
 
+/* xfs_cross_rename()
+ *
+ * responsible to handle RENAME_EXCHANGE flag
+ * in renameat2() sytemcall
+ */
+STATIC int
+xfs_cross_rename(
+	struct xfs_trans	*tp,
+	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_bmap_free	*free_list,
+	xfs_fsblock_t		*first_block,
+	int			spaceres)
+{
+	int		error = 0;
+	int		new_parent;
+	int		src_is_directory;
+	int		tgt_is_directory;
+
+	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);
+
+	/* 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;
+
+	/* 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;
+
+	xfs_trans_ichgtime(tp, src_dp, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
+
+	/*
+	* If we're renaming one or more directories across different parents,
+	* update the respective ".." entries (and link counts) to match the new
+	* parents.
+	*/
+	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;
+
+			/* transfer target ".." reference to src_dp */
+			if (!src_is_directory) {
+				error = xfs_droplink(tp, target_dp);
+				if (error)
+					goto out;
+				error = xfs_bumplink(tp, src_dp);
+				if (error)
+					goto out;
+			}
+		}
+
+		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;
+
+			/* transfer src ".." reference to target_dp */
+			if (!tgt_is_directory) {
+				error = xfs_droplink(tp, src_dp);
+				if (error)
+					goto out;
+				error = xfs_bumplink(tp, target_dp);
+				if (error)
+					goto out;
+			}
+		}
+		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:
+	return error;
+}
+
 /*
  * xfs_rename
  */
@@ -2678,7 +2775,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;
@@ -2706,6 +2804,7 @@ xfs_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);
@@ -2756,6 +2855,17 @@ 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 +3004,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..4e5d8ce 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -387,19 +387,26 @@ 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 should 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);
 }
 
 /*
-- 
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 V7

[Eric Sandeen]

On 11/14/14 12:32 PM, Carlos Maiolino wrote:
> Adds a new function named xfs_cross_rename(), responsible to handle requests
> from sys_renameat2() using RENAME_EXCHANGE flag.

I really hate to do this, but, here:

> +	/*
> +	* If we're renaming one or more directories across different parents,
> +	* update the respective ".." entries (and link counts) to match the new
> +	* parents.
> +	*/

and here:
 
>  	/*
> +	 * Handle RENAME_EXCHANGE flags
> +	*/

there are whitespace issues w/ the comments (need a " " before the "*")

who knows, maybe Dave can fix them on commit ;)

-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 V7

[Brian Foster]

On Fri, Nov 14, 2014 at 04:32:20PM -0200, 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()
> 
> 	V3: - fix indentation to avoid 80 column crossing, decrease the amount of
> 	      arguments passed to xfs_cross_rename()
> 	    - Rebase patches over the latest linux code
> 
> 	v4: - use a label/goto statement instead of an if conditional after
> 	      xfs_cross_rename() return, to finish the rename operation
> 	    - Make xfs_cross_rename() static
> 	    - Fix some comments
> 
> 	V5: - Keep all the code under 80 columns
> 
> 	V6: - Ensure i_mode of both files are updated during exchange
> 
> 	V7: - Use struct names instead of typedefs in the xfs_cross_rename()
> 	      definition
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---

Aside from the comment alignment things that Eric pointed out, this one
looks Ok to me:

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/xfs_inode.c | 113 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  fs/xfs/xfs_inode.h |   2 +-
>  fs/xfs/xfs_iops.c  |  15 +++++--
>  3 files changed, 124 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 8ed049d..3a77254 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -2668,6 +2668,103 @@ xfs_sort_for_rename(
>  	}
>  }
>  
> +/* xfs_cross_rename()
> + *
> + * responsible to handle RENAME_EXCHANGE flag
> + * in renameat2() sytemcall
> + */
> +STATIC int
> +xfs_cross_rename(
> +	struct xfs_trans	*tp,
> +	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_bmap_free	*free_list,
> +	xfs_fsblock_t		*first_block,
> +	int			spaceres)
> +{
> +	int		error = 0;
> +	int		new_parent;
> +	int		src_is_directory;
> +	int		tgt_is_directory;
> +
> +	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);
> +
> +	/* 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;
> +
> +	/* 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;
> +
> +	xfs_trans_ichgtime(tp, src_dp, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
> +
> +	/*
> +	* If we're renaming one or more directories across different parents,
> +	* update the respective ".." entries (and link counts) to match the new
> +	* parents.
> +	*/
> +	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;
> +
> +			/* transfer target ".." reference to src_dp */
> +			if (!src_is_directory) {
> +				error = xfs_droplink(tp, target_dp);
> +				if (error)
> +					goto out;
> +				error = xfs_bumplink(tp, src_dp);
> +				if (error)
> +					goto out;
> +			}
> +		}
> +
> +		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;
> +
> +			/* transfer src ".." reference to target_dp */
> +			if (!tgt_is_directory) {
> +				error = xfs_droplink(tp, src_dp);
> +				if (error)
> +					goto out;
> +				error = xfs_bumplink(tp, target_dp);
> +				if (error)
> +					goto out;
> +			}
> +		}
> +		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:
> +	return error;
> +}
> +
>  /*
>   * xfs_rename
>   */
> @@ -2678,7 +2775,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;
> @@ -2706,6 +2804,7 @@ xfs_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);
> @@ -2756,6 +2855,17 @@ 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 +3004,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..4e5d8ce 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -387,19 +387,26 @@ 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 should 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);
>  }
>  
>  /*
> -- 
> 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 V7

[Dave Chinner]

On Fri, Nov 14, 2014 at 04:32:20PM -0200, 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()
> 
> 	V3: - fix indentation to avoid 80 column crossing, decrease the amount of
> 	      arguments passed to xfs_cross_rename()
> 	    - Rebase patches over the latest linux code
> 
> 	v4: - use a label/goto statement instead of an if conditional after
> 	      xfs_cross_rename() return, to finish the rename operation
> 	    - Make xfs_cross_rename() static
> 	    - Fix some comments
> 
> 	V5: - Keep all the code under 80 columns
> 
> 	V6: - Ensure i_mode of both files are updated during exchange
> 
> 	V7: - Use struct names instead of typedefs in the xfs_cross_rename()
> 	      definition
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>  fs/xfs/xfs_inode.c | 113 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  fs/xfs/xfs_inode.h |   2 +-
>  fs/xfs/xfs_iops.c  |  15 +++++--
>  3 files changed, 124 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 8ed049d..3a77254 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -2668,6 +2668,103 @@ xfs_sort_for_rename(
>  	}
>  }
>  
> +/* xfs_cross_rename()
> + *
> + * responsible to handle RENAME_EXCHANGE flag
> + * in renameat2() sytemcall
> + */
> +STATIC int
> +xfs_cross_rename(
> +	struct xfs_trans	*tp,
> +	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_bmap_free	*free_list,
> +	xfs_fsblock_t		*first_block,
> +	int			spaceres)
> +{
> +	int		error = 0;
> +	int		new_parent;
> +	int		src_is_directory;
> +	int		tgt_is_directory;
> +
> +	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);

As I read the code below, I read src_is_directory and wonder "is
that the src_dp or the src_ip it refers to?"

None of these local variables are really needed - they don't
actually clarify or simplify the code. Can you just use the
S_ISDIR() macros directly, and kill the new_parent variable, too?

Also, I find the src/target names
to be very hard to follow as this is an exchange and so there isn't
any source/target difference in the logic. I'd actually prefer
something like "dp1, ip1, name1, dp2, ip2, name2" because it implies
(correctly) that the inodes/name parameters could be swapped and the
result would be the same. 

> +
> +	/* 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;
> +
> +	/* 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;
> +
> +	xfs_trans_ichgtime(tp, src_dp, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);

Timestamp updates are usually done at the same time we log the
inode cores, so it's obvious that we are logging the inodes we are
modifying, otherwise mistakes are made.....

> +
> +	/*
> +	* If we're renaming one or more directories across different parents,
> +	* update the respective ".." entries (and link counts) to match the new
> +	* parents.
> +	*/
> +	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);

Here is the only place we modify target_ip, hence the core should be
logged here and not unconditionally a the end of the function.

However, we should also bump the ctime of this inode (and the
src_ip, too), because as per the comment in the rename code proper:

        /*
         * 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.
         */

So while we strictly aren't touching either inode, we really should
be unconditionally bumping the ctimes on them so that userspace can
see that some change to the inode has actually taken place.


> +			if (error)
> +				goto out;
> +
> +			/* transfer target ".." reference to src_dp */
> +			if (!src_is_directory) {
> +				error = xfs_droplink(tp, target_dp);
> +				if (error)
> +					goto out;
> +				error = xfs_bumplink(tp, src_dp);
> +				if (error)
> +					goto out;
> +			}
> +		}
> +
> +		if (src_is_directory) {
> +			error = xfs_dir_replace(tp, src_ip, &xfs_name_dotdot,
> +						target_dp->i_ino, first_block,
> +						free_list, spaceres);

Same here.

> +			if (error)
> +				goto out;
> +
> +			/* transfer src ".." reference to target_dp */
> +			if (!tgt_is_directory) {
> +				error = xfs_droplink(tp, src_dp);
> +				if (error)
> +					goto out;
> +				error = xfs_bumplink(tp, target_dp);
> +				if (error)
> +					goto out;
> +			}
> +		}
> +		xfs_trans_log_inode(tp, src_dp, XFS_ILOG_CORE);

That's wrong (see my comment above about pairing timestamp/inode
logging together) as it should be the target_dp that is logged here
to match the timestamp update done in this branch.

> +	}
> +	xfs_trans_log_inode(tp, target_dp, XFS_ILOG_CORE);

And this should be src_dp.

> +	xfs_trans_log_inode(tp, src_ip, XFS_ILOG_CORE);
> +	xfs_trans_log_inode(tp, target_ip, XFS_ILOG_CORE);

And those aren't modified unless we update the dotdot entries and
update timestamps, and hence need to be moved to where we modify
those.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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