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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxx
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
