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

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

[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

Refer to specific patch descriptions for more information regarding new patch
versions

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

 fs/xfs/xfs_inode.c | 112 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 fs/xfs/xfs_inode.h |   2 +-
 fs/xfs/xfs_iops.c  |  24 +++++++++---
 3 files changed, 130 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

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

[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

	V8: - Replace src/target names for better variable names
	    - Log and timestamp updates done in different places
	    - Fix missing space in comments
	    - get rid of {src,tgt}_is_directory and new_parent variables

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

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 8ed049d..5c5ed99 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -2668,6 +2668,102 @@ 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	*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;
+
+	/* Replace source inode */
+	error = xfs_dir_replace(tp, dp1, name1,
+				ip2->i_ino,
+				first_block, free_list, spaceres);
+	if (error)
+		goto out;
+
+	/* Replace target inode */
+	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) {
+
+		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 target ".." 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;
+			}
+			xfs_trans_ichgtime(tp, ip1, XFS_ICHGTIME_CHG);
+			xfs_trans_ichgtime(tp, ip2,
+					   XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
+			xfs_trans_log_inode(tp, ip2, XFS_ILOG_CORE);
+		}
+
+		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 src ".." 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;
+			}
+			xfs_trans_ichgtime(tp, ip1,
+					   XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
+			xfs_trans_ichgtime(tp, ip2, XFS_ICHGTIME_CHG);
+			xfs_trans_log_inode(tp, ip1, XFS_ILOG_CORE);
+		}
+		xfs_trans_ichgtime(tp, dp2,
+				   XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
+		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
  */
@@ -2678,7 +2774,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 +2803,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 +2854,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 +3003,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 V8

[Carlos Maiolino]

Dave,

can you please take a look at the places where I logged inodes and did the
timestamp updates? Although the places where I added them makes sense to me, I'm
not sure if they are correct.

Thanks


On Tue, Nov 25, 2014 at 11:49:28AM -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
> 
> 	V8: - Replace src/target names for better variable names
> 	    - Log and timestamp updates done in different places
> 	    - Fix missing space in comments
> 	    - get rid of {src,tgt}_is_directory and new_parent variables
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>  fs/xfs/xfs_inode.c | 112 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  fs/xfs/xfs_inode.h |   2 +-
>  fs/xfs/xfs_iops.c  |  15 +++++--
>  3 files changed, 123 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 8ed049d..5c5ed99 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -2668,6 +2668,102 @@ 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	*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;
> +
> +	/* Replace source inode */
> +	error = xfs_dir_replace(tp, dp1, name1,
> +				ip2->i_ino,
> +				first_block, free_list, spaceres);
> +	if (error)
> +		goto out;
> +
> +	/* Replace target inode */
> +	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) {
> +
> +		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 target ".." 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;
> +			}
> +			xfs_trans_ichgtime(tp, ip1, XFS_ICHGTIME_CHG);
> +			xfs_trans_ichgtime(tp, ip2,
> +					   XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
> +			xfs_trans_log_inode(tp, ip2, XFS_ILOG_CORE);
> +		}
> +
> +		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 src ".." 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;
> +			}
> +			xfs_trans_ichgtime(tp, ip1,
> +					   XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
> +			xfs_trans_ichgtime(tp, ip2, XFS_ICHGTIME_CHG);
> +			xfs_trans_log_inode(tp, ip1, XFS_ILOG_CORE);
> +		}
> +		xfs_trans_ichgtime(tp, dp2,
> +				   XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
> +		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
>   */
> @@ -2678,7 +2774,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 +2803,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 +2854,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 +3003,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

-- 
Carlos

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

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

[Dave Chinner]

On Tue, Nov 25, 2014 at 11:49:28AM -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
> 
> 	V8: - Replace src/target names for better variable names
> 	    - Log and timestamp updates done in different places
> 	    - Fix missing space in comments
> 	    - get rid of {src,tgt}_is_directory and new_parent variables

FYI, Changelog should be in the cover patch 0, not in the commit
message for the individual patch.

> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>  fs/xfs/xfs_inode.c | 112 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  fs/xfs/xfs_inode.h |   2 +-
>  fs/xfs/xfs_iops.c  |  15 +++++--
>  3 files changed, 123 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 8ed049d..5c5ed99 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -2668,6 +2668,102 @@ xfs_sort_for_rename(
>  	}
>  }
>  
> +/* xfs_cross_rename()
> + *
> + * responsible to handle RENAME_EXCHANGE flag
> + * in renameat2() sytemcall

Comments can use all 80 columns. And the first line of the comment
is "/*" by itself.

> + */
> +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;
> +
> +	/* Replace source inode */

still got source/target in comments that don't make sense. "Swap
inode number for dirent in first parent" might be better...

> +	error = xfs_dir_replace(tp, dp1, name1,
> +				ip2->i_ino,
> +				first_block, free_list, spaceres);
> +	if (error)
> +		goto out;
> +
> +	/* Replace target inode */

	/* 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) {
> +
> +		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;

now ip2 is modified, so it ctime/mtime dirty.

> +
> +			/* transfer target ".." 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;
> +			}
> +			xfs_trans_ichgtime(tp, ip1, XFS_ICHGTIME_CHG);
> +			xfs_trans_ichgtime(tp, ip2,
> +					   XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
> +			xfs_trans_log_inode(tp, ip2, XFS_ILOG_CORE);

But now you're unconditionally changing ctime on ip1 without it
having been modified and you aren't logging the change. Why is the
ctime changing (comments, please!)?


> +		}
> +
> +		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;

here ip2 is modified

> +
> +			/* transfer src ".." 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;
> +			}
> +			xfs_trans_ichgtime(tp, ip1,
> +					   XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
> +			xfs_trans_ichgtime(tp, ip2, XFS_ICHGTIME_CHG);
> +			xfs_trans_log_inode(tp, ip1, XFS_ILOG_CORE);

and same again - changing ctime on ip2 without logging it.

> +		}
> +		xfs_trans_ichgtime(tp, dp2,
> +				   XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
> +		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);

perhaps:

	int	ip1_flags = 0;
	int	ip2_flags = 0;
	int	dp2_flags = 0;

	if (dp1 != dp2)
		dp2_flags = XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG;

		if (S_ISDIR(ip1)) {
			ip1_flags |= XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG;
			ip2_flags |= XFS_ICHGTIME_CHG; /* because ... */
			.....
		}

		if (S_ISDIR(ip2)) {
			ip1_flags |= XFS_ICHGTIME_CHG; /* because ... */
			ip2_flags |= XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG;
			.....
		}
	}

	if (ip1_flags) {
		xfs_trans_ichgtime(tp, ip1, ip1_flags);
		xfs_trans_log_inode(tp, ip1);
	}
	if (ip2_flags) {
		xfs_trans_ichgtime(tp, ip2, ip2_flags);
		xfs_trans_log_inode(tp, ip2);
	}
	if (dp2_flags) {
		xfs_trans_ichgtime(tp, dp2, ip2_flags);
		xfs_trans_log_inode(tp, dp2);
	}
	xfs_trans_ichgtime(tp, dp2, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
	xfs_trans_log_inode(tp, dp2);
> +
> +out:
> +	return error;
> +}
> +
>  /*
>   * xfs_rename
>   */
> @@ -2678,7 +2774,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 +2803,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) {

Stray new line.

>  		spaceres = 0;
>  		error = xfs_trans_reserve(tp, &M_RES(mp)->tr_rename, 0, 0);
> @@ -2756,6 +2854,17 @@ xfs_rename(
>  	}
>  
>  	/*
> +	 * Handle RENAME_EXCHANGE flags
> +	*/

Comment whitespace still needs fixing.

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

Put an empty line between the "}" and the start of the comment.

>  	if (target_ip == NULL) {
> @@ -2894,6 +3003,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
> +	 */

Comments can use all 80 characters of the line. Also, s/should/need
to/.

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 V8

[Carlos Maiolino]

Hi Dave,


>> +			error = xfs_dir_replace(tp, ip2, &xfs_name_dotdot,
>> +						dp1->i_ino, first_block,
>> +						free_list, spaceres);
>> +			if (error)
>> +				goto out;

>now ip2 is modified, so it ctime/mtime dirty.

>> +
>> +			/* transfer target ".." 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;
>> +			}
>> +			xfs_trans_ichgtime(tp, ip1, XFS_ICHGTIME_CHG);
>> +			xfs_trans_ichgtime(tp, ip2,
>> +					   XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
>> +			xfs_trans_log_inode(tp, ip2, XFS_ILOG_CORE);

>But now you're unconditionally changing ctime on ip1 without it
>having been modified and you aren't logging the change. Why is the
>ctime changing (comments, please!)?

Ok, so, I can see that I didn't understand what we should do with logging here,
for the moment, I thought we should bump the ip1 to notify userspace that
changes has actually taken place here, even though we are not changing it
anyway. I'm not sure if I should log it too (and add a xfs_trans_log_inode for
ip1). So, should we also log it here together with ip1, or bumping ctime of ip1
here is wrong?
In this case, a more correct way to write it would be to not bump ip1 here but
only in the next block, where we actually touches it?
And regarding dp2 and dp1, we drop/bump its link counts here, should I care
about logging them in this code block? My xfs logging knowledge is shallow by
now :-(

>> +		}
>> +
>> +		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;

>here ip2 is modified

>> +
>> +			/* transfer src ".." 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;
>> +			}
>> +			xfs_trans_ichgtime(tp, ip1,
>> +					   XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
>> +			xfs_trans_ichgtime(tp, ip2, XFS_ICHGTIME_CHG);
>> +			xfs_trans_log_inode(tp, ip1, XFS_ILOG_CORE);

>and same again - changing ctime on ip2 without logging it.

>> +		}
>> +		xfs_trans_ichgtime(tp, dp2,
>> +				   XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
>> +		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);

>perhaps:

>	int	ip1_flags = 0;
>	int	ip2_flags = 0;
>	int	dp2_flags = 0;
>
>	if (dp1 != dp2)
>		dp2_flags = XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG;
>
>		if (S_ISDIR(ip1)) {
>			ip1_flags |= XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG;
>			ip2_flags |= XFS_ICHGTIME_CHG; /* because ... */
>			.....
>		}
>
>		if (S_ISDIR(ip2)) {
>			ip1_flags |= XFS_ICHGTIME_CHG; /* because ... */
>			ip2_flags |= XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG;
>			.....
>		}
>	}
>
>	if (ip1_flags) {
>		xfs_trans_ichgtime(tp, ip1, ip1_flags);
>		xfs_trans_log_inode(tp, ip1);
>	}
>	if (ip2_flags) {
>		xfs_trans_ichgtime(tp, ip2, ip2_flags);
>		xfs_trans_log_inode(tp, ip2);
>	}
>	if (dp2_flags) {
>		xfs_trans_ichgtime(tp, dp2, ip2_flags);
>		xfs_trans_log_inode(tp, dp2);
>	}
>	xfs_trans_ichgtime(tp, dp2, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
>	xfs_trans_log_inode(tp, dp2);

I don't think that these extra variables will actually clear the code and make
it more readable IMHO, calling xfs_trans_ichgtime with the flags directly looks
more clear and readable to me.

Thanks for the review,
I have fixed all the another points you mentioned now, I think that handling log
correct on the above snippet is the last thing to do by now.

cheers.

-- 
Carlos

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

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

[Carlos Maiolino]

Sigh...

For some reason, this thread has been broken, sorry if it was my mistake.


On Tue, Dec 02, 2014 at 12:13:42PM -0200, Carlos Maiolino wrote:
> Hi Dave,
> 
> 
> >> +			error = xfs_dir_replace(tp, ip2, &xfs_name_dotdot,
> >> +						dp1->i_ino, first_block,
> >> +						free_list, spaceres);
> >> +			if (error)
> >> +				goto out;
> 
> >now ip2 is modified, so it ctime/mtime dirty.
> 
> >> +
> >> +			/* transfer target ".." 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;
> >> +			}
> >> +			xfs_trans_ichgtime(tp, ip1, XFS_ICHGTIME_CHG);
> >> +			xfs_trans_ichgtime(tp, ip2,
> >> +					   XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
> >> +			xfs_trans_log_inode(tp, ip2, XFS_ILOG_CORE);
> 
> >But now you're unconditionally changing ctime on ip1 without it
> >having been modified and you aren't logging the change. Why is the
> >ctime changing (comments, please!)?
> 
> Ok, so, I can see that I didn't understand what we should do with logging here,
> for the moment, I thought we should bump the ip1 to notify userspace that
> changes has actually taken place here, even though we are not changing it
> anyway. I'm not sure if I should log it too (and add a xfs_trans_log_inode for
> ip1). So, should we also log it here together with ip1, or bumping ctime of ip1
> here is wrong?
> In this case, a more correct way to write it would be to not bump ip1 here but
> only in the next block, where we actually touches it?
> And regarding dp2 and dp1, we drop/bump its link counts here, should I care
> about logging them in this code block? My xfs logging knowledge is shallow by
> now :-(
> 
> >> +		}
> >> +
> >> +		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;
> 
> >here ip2 is modified
> 
> >> +
> >> +			/* transfer src ".." 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;
> >> +			}
> >> +			xfs_trans_ichgtime(tp, ip1,
> >> +					   XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
> >> +			xfs_trans_ichgtime(tp, ip2, XFS_ICHGTIME_CHG);
> >> +			xfs_trans_log_inode(tp, ip1, XFS_ILOG_CORE);
> 
> >and same again - changing ctime on ip2 without logging it.
> 
> >> +		}
> >> +		xfs_trans_ichgtime(tp, dp2,
> >> +				   XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
> >> +		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);
> 
> >perhaps:
> 
> >	int	ip1_flags = 0;
> >	int	ip2_flags = 0;
> >	int	dp2_flags = 0;
> >
> >	if (dp1 != dp2)
> >		dp2_flags = XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG;
> >
> >		if (S_ISDIR(ip1)) {
> >			ip1_flags |= XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG;
> >			ip2_flags |= XFS_ICHGTIME_CHG; /* because ... */
> >			.....
> >		}
> >
> >		if (S_ISDIR(ip2)) {
> >			ip1_flags |= XFS_ICHGTIME_CHG; /* because ... */
> >			ip2_flags |= XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG;
> >			.....
> >		}
> >	}
> >
> >	if (ip1_flags) {
> >		xfs_trans_ichgtime(tp, ip1, ip1_flags);
> >		xfs_trans_log_inode(tp, ip1);
> >	}
> >	if (ip2_flags) {
> >		xfs_trans_ichgtime(tp, ip2, ip2_flags);
> >		xfs_trans_log_inode(tp, ip2);
> >	}
> >	if (dp2_flags) {
> >		xfs_trans_ichgtime(tp, dp2, ip2_flags);
> >		xfs_trans_log_inode(tp, dp2);
> >	}
> >	xfs_trans_ichgtime(tp, dp2, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
> >	xfs_trans_log_inode(tp, dp2);
> 
> I don't think that these extra variables will actually clear the code and make
> it more readable IMHO, calling xfs_trans_ichgtime with the flags directly looks
> more clear and readable to me.
> 
> Thanks for the review,
> I have fixed all the another points you mentioned now, I think that handling log
> correct on the above snippet is the last thing to do by now.
> 
> cheers.
> 
> -- 
> Carlos
> 
> _______________________________________________
> 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

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

[Dave Chinner]

On Tue, Dec 02, 2014 at 12:13:42PM -0200, Carlos Maiolino wrote:
> Hi Dave,
> 
> 
> >> +			error = xfs_dir_replace(tp, ip2, &xfs_name_dotdot,
> >> +						dp1->i_ino, first_block,
> >> +						free_list, spaceres);
> >> +			if (error)
> >> +				goto out;
> 
> >now ip2 is modified, so it ctime/mtime dirty.
> 
> >> +
> >> +			/* transfer target ".." 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;
> >> +			}
> >> +			xfs_trans_ichgtime(tp, ip1, XFS_ICHGTIME_CHG);
> >> +			xfs_trans_ichgtime(tp, ip2,
> >> +					   XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
> >> +			xfs_trans_log_inode(tp, ip2, XFS_ILOG_CORE);
> 
> >But now you're unconditionally changing ctime on ip1 without it
> >having been modified and you aren't logging the change. Why is the
> >ctime changing (comments, please!)?
> 
> Ok, so, I can see that I didn't understand what we should do with
> logging here, for the moment, I thought we should bump the ip1 to
> notify userspace that changes has actually taken place here, even
> though we are not changing it anyway.

Comments are for explaining why you did something. If you are using
the same reasoning as xfs_rename (i.e. the "We always want to hit
the ctime on the source inode" comment), then the comment should be
present here explaining the reason for the ctime change.


> I'm not sure if I should log it too (and add a xfs_trans_log_inode for
> ip1).

If the code modifies the inode, it also needs to log it.

> So, should we also log it here together with ip1, or bumping ctime of ip1
> here is wrong?
> In this case, a more correct way to write it would be to not bump ip1 here but
> only in the next block, where we actually touches it?
> And regarding dp2 and dp1, we drop/bump its link counts here, should I care
> about logging them in this code block? My xfs logging knowledge is shallow by
> now :-(

You only need to log them once in a transaction, hence my
suggestion of using variables to track the timstamp changes required
by the cross rename followed by function exit logic that takes
action on all those modifications.

....

> >	int	ip1_flags = 0;
> >	int	ip2_flags = 0;
> >	int	dp2_flags = 0;
> >
> >	if (dp1 != dp2)
> >		dp2_flags = XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG;
> >
> >		if (S_ISDIR(ip1)) {
> >			ip1_flags |= XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG;
> >			ip2_flags |= XFS_ICHGTIME_CHG; /* because ... */
> >			.....
> >		}
> >
> >		if (S_ISDIR(ip2)) {
> >			ip1_flags |= XFS_ICHGTIME_CHG; /* because ... */
> >			ip2_flags |= XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG;
> >			.....
> >		}
> >	}
> >
> >	if (ip1_flags) {
> >		xfs_trans_ichgtime(tp, ip1, ip1_flags);
> >		xfs_trans_log_inode(tp, ip1);
> >	}
> >	if (ip2_flags) {
> >		xfs_trans_ichgtime(tp, ip2, ip2_flags);
> >		xfs_trans_log_inode(tp, ip2);
> >	}
> >	if (dp2_flags) {
> >		xfs_trans_ichgtime(tp, dp2, ip2_flags);
> >		xfs_trans_log_inode(tp, dp2);
> >	}
> >	xfs_trans_ichgtime(tp, dp2, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
> >	xfs_trans_log_inode(tp, dp2);
> 
> I don't think that these extra variables will actually clear the code and make
> it more readable IMHO, calling xfs_trans_ichgtime with the flags directly looks
> more clear and readable to me.

I find it much more informative that a random smattering of repeated
calls to xfs_trans_ichgtime/xfs_trans_log_inode throughout the
function. There's a clear separation of intent (i.e. what needs to
be changed in what branch) from action (the actual timestamp
changes and logging) and so is going to be much easier to understand
in a couple of years time. It's also more efficient code because we
only update timestamps once for each inode.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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