[PATCH] xfsprogs: copy_range don't truncate dstfile if same with srcfile

[PATCH] xfsprogs: copy_range don't truncate dstfile if same with srcfile

[Jianhong.Yin]

now if we do copy_range in same file without any extra option
will truncate the file, and not any document indicate this default
action. that's risky to users.

'''
$ LANG=C ll testfile
-rw-rw-r--. 1 yjh yjh 4054 Sep  4 14:22 testfile
$ ./xfs_io -c 'copy_range testfile' testfile
$ LANG=C ll testfile
-rw-rw-r--. 1 yjh yjh 4054 Sep  4 14:23 testfile
'''

Signed-off-by: Jianhong Yin <yin-jianhong@163.com>
---
 io/copy_file_range.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/io/copy_file_range.c b/io/copy_file_range.c
index b7b9fd88..487041c0 100644
--- a/io/copy_file_range.c
+++ b/io/copy_file_range.c
@@ -75,6 +75,19 @@ copy_dst_truncate(void)
 	return ret;
 }
 
+int is_same_file(int fd1, int fd2) {
+	struct stat stat1, stat2;
+	if (fstat(fd1, &stat1) < 0) {
+		perror("fstat");
+		return -1;
+	}
+	if (fstat(fd2, &stat2) < 0) {
+		perror("fstat");
+		return -1;
+	}
+	return (stat1.st_dev == stat2.st_dev) && (stat1.st_ino == stat2.st_ino);
+}
+
 static int
 copy_range_f(int argc, char **argv)
 {
@@ -147,10 +160,12 @@ copy_range_f(int argc, char **argv)
 		}
 		len = sz;
 
-		ret = copy_dst_truncate();
-		if (ret < 0) {
-			ret = 1;
-			goto out;
+		if (!is_same_file(fd, file->fd)) {
+			ret = copy_dst_truncate();
+			if (ret < 0) {
+				ret = 1;
+				goto out;
+			}
 		}
 	}
 
-- 
2.17.2

Re: [PATCH] xfsprogs: copy_range don't truncate dstfile if same with srcfile

[Darrick J. Wong]

On Wed, Sep 04, 2019 at 02:32:22PM +0800, Jianhong.Yin wrote:
> now if we do copy_range in same file without any extra option
> will truncate the file, and not any document indicate this default
> action. that's risky to users.
> 
> '''
> $ LANG=C ll testfile
> -rw-rw-r--. 1 yjh yjh 4054 Sep  4 14:22 testfile
> $ ./xfs_io -c 'copy_range testfile' testfile
> $ LANG=C ll testfile
> -rw-rw-r--. 1 yjh yjh 4054 Sep  4 14:23 testfile
> '''
> 
> Signed-off-by: Jianhong Yin <yin-jianhong@163.com>
> ---
>  io/copy_file_range.c | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/io/copy_file_range.c b/io/copy_file_range.c
> index b7b9fd88..487041c0 100644
> --- a/io/copy_file_range.c
> +++ b/io/copy_file_range.c
> @@ -75,6 +75,19 @@ copy_dst_truncate(void)
>  	return ret;
>  }
>  
> +int is_same_file(int fd1, int fd2) {
> +	struct stat stat1, stat2;
> +	if (fstat(fd1, &stat1) < 0) {
> +		perror("fstat");
> +		return -1;
> +	}
> +	if (fstat(fd2, &stat2) < 0) {
> +		perror("fstat");
> +		return -1;
> +	}
> +	return (stat1.st_dev == stat2.st_dev) && (stat1.st_ino == stat2.st_ino);
> +}
> +
>  static int
>  copy_range_f(int argc, char **argv)
>  {
> @@ -147,10 +160,12 @@ copy_range_f(int argc, char **argv)
>  		}
>  		len = sz;
>  
> -		ret = copy_dst_truncate();
> -		if (ret < 0) {
> -			ret = 1;
> -			goto out;
> +		if (!is_same_file(fd, file->fd)) {

Uggggh, why does xfs_io copy_range have this weird behavior?  It should
be a simple wrapper for copy_file_range (the syscall) and nothing else.

The code patch looks fine for solving this edge case, but we really
shouldn't have this "extra" functionality in a debugging tool that
should be athin wrapper around the syscall for xfstests purposes.

--D

> +			ret = copy_dst_truncate();
> +			if (ret < 0) {
> +				ret = 1;
> +				goto out;
> +			}
>  		}
>  	}
>  
> -- 
> 2.17.2
> 

Re: [PATCH] xfsprogs: copy_range don't truncate dstfile if same with srcfile

[Jianhong Yin]

----- 原始邮件 -----
> 发件人: "Darrick J. Wong" <darrick.wong@oracle.com>
> 收件人: "Jianhong.Yin" <yin-jianhong@163.com>
> 抄送: linux-xfs@vger.kernel.org, jiyin@redhat.com
> 发送时间: 星期四, 2019年 9 月 05日 上午 1:27:36
> 主题: Re: [PATCH] xfsprogs: copy_range don't truncate dstfile if same with srcfile
> 
> On Wed, Sep 04, 2019 at 02:32:22PM +0800, Jianhong.Yin wrote:
> > now if we do copy_range in same file without any extra option
> > will truncate the file, and not any document indicate this default
> > action. that's risky to users.
> > 
> > '''
> > $ LANG=C ll testfile
> > -rw-rw-r--. 1 yjh yjh 4054 Sep  4 14:22 testfile
> > $ ./xfs_io -c 'copy_range testfile' testfile
> > $ LANG=C ll testfile
> > -rw-rw-r--. 1 yjh yjh 4054 Sep  4 14:23 testfile
> > '''
> > 
> > Signed-off-by: Jianhong Yin <yin-jianhong@163.com>
> > ---
> >  io/copy_file_range.c | 23 +++++++++++++++++++----
> >  1 file changed, 19 insertions(+), 4 deletions(-)
> > 
> > diff --git a/io/copy_file_range.c b/io/copy_file_range.c
> > index b7b9fd88..487041c0 100644
> > --- a/io/copy_file_range.c
> > +++ b/io/copy_file_range.c
> > @@ -75,6 +75,19 @@ copy_dst_truncate(void)
> >  	return ret;
> >  }
> >  
> > +int is_same_file(int fd1, int fd2) {
> > +	struct stat stat1, stat2;
> > +	if (fstat(fd1, &stat1) < 0) {
> > +		perror("fstat");
> > +		return -1;
> > +	}
> > +	if (fstat(fd2, &stat2) < 0) {
> > +		perror("fstat");
> > +		return -1;
> > +	}
> > +	return (stat1.st_dev == stat2.st_dev) && (stat1.st_ino == stat2.st_ino);
> > +}
> > +
> >  static int
> >  copy_range_f(int argc, char **argv)
> >  {
> > @@ -147,10 +160,12 @@ copy_range_f(int argc, char **argv)
> >  		}
> >  		len = sz;
> >  
> > -		ret = copy_dst_truncate();
> > -		if (ret < 0) {
> > -			ret = 1;
> > -			goto out;
> > +		if (!is_same_file(fd, file->fd)) {
> 
> Uggggh, why does xfs_io copy_range have this weird behavior?  It should
> be a simple wrapper for copy_file_range (the syscall) and nothing else.
> 
> The code patch looks fine for solving this edge case, but we really
> shouldn't have this "extra" functionality in a debugging tool that
> should be athin wrapper around the syscall for xfstests purposes.

right, agree.

> 
> --D
> 
> > +			ret = copy_dst_truncate();
> > +			if (ret < 0) {
> > +				ret = 1;
> > +				goto out;
> > +			}
> >  		}
> >  	}
> >  
> > --
> > 2.17.2
> > 
> 

Re: [PATCH] xfsprogs: copy_range don't truncate dstfile if same with srcfile

[Zorro Lang]

On Wed, Sep 04, 2019 at 02:32:22PM +0800, Jianhong.Yin wrote:
> now if we do copy_range in same file without any extra option
> will truncate the file, and not any document indicate this default
> action. that's risky to users.
> 
> '''
> $ LANG=C ll testfile
> -rw-rw-r--. 1 yjh yjh 4054 Sep  4 14:22 testfile
> $ ./xfs_io -c 'copy_range testfile' testfile
> $ LANG=C ll testfile
> -rw-rw-r--. 1 yjh yjh 4054 Sep  4 14:23 testfile
> '''
> 
> Signed-off-by: Jianhong Yin <yin-jianhong@163.com>
> ---
>  io/copy_file_range.c | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/io/copy_file_range.c b/io/copy_file_range.c
> index b7b9fd88..487041c0 100644
> --- a/io/copy_file_range.c
> +++ b/io/copy_file_range.c
> @@ -75,6 +75,19 @@ copy_dst_truncate(void)
>  	return ret;
>  }
>  
> +int is_same_file(int fd1, int fd2) {
> +	struct stat stat1, stat2;
> +	if (fstat(fd1, &stat1) < 0) {
> +		perror("fstat");
> +		return -1;
> +	}
> +	if (fstat(fd2, &stat2) < 0) {
> +		perror("fstat");
> +		return -1;
> +	}
> +	return (stat1.st_dev == stat2.st_dev) && (stat1.st_ino == stat2.st_ino);
> +}
> +
>  static int
>  copy_range_f(int argc, char **argv)
>  {
> @@ -147,10 +160,12 @@ copy_range_f(int argc, char **argv)
>  		}
>  		len = sz;
>  
> -		ret = copy_dst_truncate();
> -		if (ret < 0) {
> -			ret = 1;
> -			goto out;
> +		if (!is_same_file(fd, file->fd)) {
> +			ret = copy_dst_truncate();
> +			if (ret < 0) {
> +				ret = 1;
> +				goto out;
> +			}

Finally we turn to talk about this part now:) The patch itself looks fine to me.

I just have one question to other xfsprogs developers, why the copy_range would
like to truncate(0) the target file by default (len=0 by default)? I think this's
not a same or not same files problem, this's why we truncate the target file?

Why not set the 'len' to the size of srcfile(copy_src_filesize) by default if
there's not a '-l N' specified? And never truncate the target file (if someone need
to truncate target file, do it by -c 'truncate N' or '-t').

Anyway, talk is cheap, show my demo code to explain what I mean:

diff --git a/io/copy_file_range.c b/io/copy_file_range.c
index b7b9fd88..51c3dc55 100644
--- a/io/copy_file_range.c
+++ b/io/copy_file_range.c
@@ -66,15 +66,6 @@ copy_src_filesize(int fd)
        return st.st_size;
 }
 
-static int
-copy_dst_truncate(void)
-{
-       int ret = ftruncate(file->fd, 0);
-       if (ret < 0)
-               perror("ftruncate");
-       return ret;
-}
-
 static int
 copy_range_f(int argc, char **argv)
 {
@@ -87,6 +78,7 @@ copy_range_f(int argc, char **argv)
        int src_path_arg = 1;
        int src_file_nr = 0;
        size_t fsblocksize, fssectsize;
+       int lflag=0;
 
        init_cvtnum(&fsblocksize, &fssectsize);
 
@@ -112,6 +104,7 @@ copy_range_f(int argc, char **argv)
                                printf(_("invalid length -- %s\n"), optarg);
                                return 0;
                        }
+                       lflag = 1;
                        break;
                case 'f':
                        src_file_nr = atoi(argv[1]);
@@ -137,23 +130,15 @@ copy_range_f(int argc, char **argv)
                fd = filetable[src_file_nr].fd;
        }
 
-       if (src == 0 && dst == 0 && len == 0) {
+       if (!lflag) {
                off64_t sz;
-
                sz = copy_src_filesize(fd);
-               if (sz < 0 || (unsigned long long)sz > SIZE_MAX) {
-                       ret = 1;
-                       goto out;
-               }
-               len = sz;
-
-               ret = copy_dst_truncate();
-               if (ret < 0) {
+               if (sz < 0 || (unsigned long long)sz > SIZE_MAX || sz < src) {
                        ret = 1;
                        goto out;
                }
+               len = sz - src;
        }
-
        ret = copy_file_range_cmd(fd, &src, &dst, len);
 out:
        close(fd);

Thanks,
Zorro

>  		}
>  	}
>  
> -- 
> 2.17.2
> 

Re: [PATCH] xfsprogs: copy_range don't truncate dstfile if same with srcfile

[Jianhong Yin]

I've send new patch do same work

----- 原始邮件 -----
> 发件人: "Zorro Lang" <zlang@redhat.com>
> 收件人: "Jianhong.Yin" <yin-jianhong@163.com>
> 抄送: linux-xfs@vger.kernel.org, jiyin@redhat.com
> 发送时间: 星期四, 2019年 9 月 05日 下午 1:02:35
> 主题: Re: [PATCH] xfsprogs: copy_range don't truncate dstfile if same with srcfile
> 
> On Wed, Sep 04, 2019 at 02:32:22PM +0800, Jianhong.Yin wrote:
> > now if we do copy_range in same file without any extra option
> > will truncate the file, and not any document indicate this default
> > action. that's risky to users.
> > 
> > '''
> > $ LANG=C ll testfile
> > -rw-rw-r--. 1 yjh yjh 4054 Sep  4 14:22 testfile
> > $ ./xfs_io -c 'copy_range testfile' testfile
> > $ LANG=C ll testfile
> > -rw-rw-r--. 1 yjh yjh 4054 Sep  4 14:23 testfile
> > '''
> > 
> > Signed-off-by: Jianhong Yin <yin-jianhong@163.com>
> > ---
> >  io/copy_file_range.c | 23 +++++++++++++++++++----
> >  1 file changed, 19 insertions(+), 4 deletions(-)
> > 
> > diff --git a/io/copy_file_range.c b/io/copy_file_range.c
> > index b7b9fd88..487041c0 100644
> > --- a/io/copy_file_range.c
> > +++ b/io/copy_file_range.c
> > @@ -75,6 +75,19 @@ copy_dst_truncate(void)
> >  	return ret;
> >  }
> >  
> > +int is_same_file(int fd1, int fd2) {
> > +	struct stat stat1, stat2;
> > +	if (fstat(fd1, &stat1) < 0) {
> > +		perror("fstat");
> > +		return -1;
> > +	}
> > +	if (fstat(fd2, &stat2) < 0) {
> > +		perror("fstat");
> > +		return -1;
> > +	}
> > +	return (stat1.st_dev == stat2.st_dev) && (stat1.st_ino == stat2.st_ino);
> > +}
> > +
> >  static int
> >  copy_range_f(int argc, char **argv)
> >  {
> > @@ -147,10 +160,12 @@ copy_range_f(int argc, char **argv)
> >  		}
> >  		len = sz;
> >  
> > -		ret = copy_dst_truncate();
> > -		if (ret < 0) {
> > -			ret = 1;
> > -			goto out;
> > +		if (!is_same_file(fd, file->fd)) {
> > +			ret = copy_dst_truncate();
> > +			if (ret < 0) {
> > +				ret = 1;
> > +				goto out;
> > +			}
> 
> Finally we turn to talk about this part now:) The patch itself looks fine to
> me.
> 
> I just have one question to other xfsprogs developers, why the copy_range
> would
> like to truncate(0) the target file by default (len=0 by default)? I think
> this's
> not a same or not same files problem, this's why we truncate the target file?
> 
> Why not set the 'len' to the size of srcfile(copy_src_filesize) by default if
> there's not a '-l N' specified? And never truncate the target file (if
> someone need
> to truncate target file, do it by -c 'truncate N' or '-t').
> 
> Anyway, talk is cheap, show my demo code to explain what I mean:
> 
> diff --git a/io/copy_file_range.c b/io/copy_file_range.c
> index b7b9fd88..51c3dc55 100644
> --- a/io/copy_file_range.c
> +++ b/io/copy_file_range.c
> @@ -66,15 +66,6 @@ copy_src_filesize(int fd)
>         return st.st_size;
>  }
>  
> -static int
> -copy_dst_truncate(void)
> -{
> -       int ret = ftruncate(file->fd, 0);
> -       if (ret < 0)
> -               perror("ftruncate");
> -       return ret;
> -}
> -
>  static int
>  copy_range_f(int argc, char **argv)
>  {
> @@ -87,6 +78,7 @@ copy_range_f(int argc, char **argv)
>         int src_path_arg = 1;
>         int src_file_nr = 0;
>         size_t fsblocksize, fssectsize;
> +       int lflag=0;
>  
>         init_cvtnum(&fsblocksize, &fssectsize);
>  
> @@ -112,6 +104,7 @@ copy_range_f(int argc, char **argv)
>                                 printf(_("invalid length -- %s\n"), optarg);
>                                 return 0;
>                         }
> +                       lflag = 1;
>                         break;
>                 case 'f':
>                         src_file_nr = atoi(argv[1]);
> @@ -137,23 +130,15 @@ copy_range_f(int argc, char **argv)
>                 fd = filetable[src_file_nr].fd;
>         }
>  
> -       if (src == 0 && dst == 0 && len == 0) {
> +       if (!lflag) {
>                 off64_t sz;
> -
>                 sz = copy_src_filesize(fd);
> -               if (sz < 0 || (unsigned long long)sz > SIZE_MAX) {
> -                       ret = 1;
> -                       goto out;
> -               }
> -               len = sz;
> -
> -               ret = copy_dst_truncate();
> -               if (ret < 0) {
> +               if (sz < 0 || (unsigned long long)sz > SIZE_MAX || sz < src)
> {
>                         ret = 1;
>                         goto out;
>                 }
> +               len = sz - src;
>         }
> -
>         ret = copy_file_range_cmd(fd, &src, &dst, len);
>  out:
>         close(fd);
> 
> Thanks,
> Zorro
> 
> >  		}
> >  	}
> >  
> > --
> > 2.17.2
> > 
>