| Message ID | 20190904063222.21253-1-yin-jianhong@163.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfsprogs: copy_range don't truncate dstfile if same with srcfile | expand |
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 >
----- 原始邮件 ----- > 发件人: "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 > > >
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 >
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 > > >
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; + } } }
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(-)