From: Su Yanjun <suyj.fnst@cn.fujitsu.com> To: Eryu Guan <guaneryu@gmail.com> Cc: fstests@vger.kernel.org Subject: Re: [PATCH] fsx: add more check for copy_file_range Date: Fri, 27 Sep 2019 09:38:27 +0800 [thread overview] Message-ID: <5307926c-1fdb-5283-5053-b6f4f7d23a41@cn.fujitsu.com> (raw) In-Reply-To: <20190925130809.GT2622@desktop> 在 2019/9/25 21:08, Eryu Guan 写道: > On Wed, Sep 25, 2019 at 03:16:55PM +0800, Su Yanjun wrote: >> On some linux distros(RHEL7, centos 7) copy_file_range uses >> general implementation (splice interface). splice interace >> uses pipe_to_file. pipe_to_file only work for different page. >> The userspace cant's be aware of such error because copy_file_range >> returns ok too. > This looks like a kernel bug to me. Yes, it's RHEL kernel 3.10's bug. The new kernel is ok. >> So for such case when copy_file_range return we read back data >> then check it. > Yeah, I think we should make sure the data copied is correct. > >> Signed-off-by: Su Yanjun <suyj.fnst@cn.fujitsu.com> >> --- >> ltp/fsx.c | 32 ++++++++++++++++++++++++++++++++ >> 1 file changed, 32 insertions(+) >> >> diff --git a/ltp/fsx.c b/ltp/fsx.c >> index 06d08e4..0439430 100644 >> --- a/ltp/fsx.c >> +++ b/ltp/fsx.c >> @@ -1602,6 +1602,7 @@ do_copy_range(unsigned offset, unsigned length, unsigned dest) >> size_t olen; >> ssize_t nr; >> int tries = 0; >> + int ret = 0; >> >> if (length == 0) { >> if (!quiet && testcalls > simulatedopcount) >> @@ -1665,6 +1666,37 @@ do_copy_range(unsigned offset, unsigned length, unsigned dest) >> memset(good_buf + file_size, '\0', dest - file_size); >> if (dest + length > file_size) >> file_size = dest + length; >> + /* >> + * Although copy_file_range returns ok >> + * for some linux distros that use general implementation >> + * (splice interface) such as RHEL 7, centos 7 that use >> + * pipe_to_file will cause test fail. >> + * >> + * Here we add a little more check here for copy_file_range >> + * We read copied data then check it. If check fail here >> + * then report it. >> + */ > Above comments seem like not necessary, the content check is inspired by > Rthis HEL7 issue, but not the main purpose. > >> + ret = lseek(fd, (off_t)dest, SEEK_SET); >> + if (ret == (off_t)-1) { >> + prterr("doread: lseek"); > ^^^^^^^ not doread > >> + report_failure(140); > Use a different failure number than that in doread, i.e. don't copy the > code from doread. > >> + } >> + ret = fsxread(fd, temp_buf, length, dest); >> + if (ret != length) { >> + if (ret == -1) >> + prterr("doread: read"); >> + else >> + prt("short read: 0x%x bytes instead of 0x%x\n", >> + ret, length); > Same here, not doread, and weired indention above. > >> + report_failure(141); >> + } >> + if (memcmp(good_buf+dest, temp_buf, length) != 0) { >> + prt("copy range: 0x%x to 0x%x at 0x%x\n", offset, >> + offset + length, dest); >> + prterr("do_copy_range:"); >> + report_failure(161); > I think we could take use of check_buffers(). > > And similar check could be added to do_clone_range as well, I think. Ok, i'll send patch v2 later. Thanks > > Thanks, > Eryu > >> + >> + } >> } >> >> #else >> -- >> 2.7.4 >> >> >> >
WARNING: multiple messages have this Message-ID (diff)
From: Su Yanjun <suyj.fnst@cn.fujitsu.com> To: Eryu Guan <guaneryu@gmail.com> Cc: <fstests@vger.kernel.org> Subject: Re: [PATCH] fsx: add more check for copy_file_range Date: Fri, 27 Sep 2019 09:38:27 +0800 [thread overview] Message-ID: <5307926c-1fdb-5283-5053-b6f4f7d23a41@cn.fujitsu.com> (raw) Message-ID: <20190927013827.hlhNRHtsdsU2hSyVRGKuFUnn5Uw1ir_PunRxgZw6mg0@z> (raw) In-Reply-To: <20190925130809.GT2622@desktop> [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain; charset="UTF-8"; format="flowed", Size: 2905 bytes --] ÔÚ 2019/9/25 21:08, Eryu Guan дµÀ: > On Wed, Sep 25, 2019 at 03:16:55PM +0800, Su Yanjun wrote: >> On some linux distros(RHEL7, centos 7) copy_file_range uses >> general implementation (splice interface). splice interace >> uses pipe_to_file. pipe_to_file only work for different page. >> The userspace cant's be aware of such error because copy_file_range >> returns ok too. > This looks like a kernel bug to me. Yes, it's RHEL kernel 3.10's bug. The new kernel is ok. >> So for such case when copy_file_range return we read back data >> then check it. > Yeah, I think we should make sure the data copied is correct. > >> Signed-off-by: Su Yanjun <suyj.fnst@cn.fujitsu.com> >> --- >> ltp/fsx.c | 32 ++++++++++++++++++++++++++++++++ >> 1 file changed, 32 insertions(+) >> >> diff --git a/ltp/fsx.c b/ltp/fsx.c >> index 06d08e4..0439430 100644 >> --- a/ltp/fsx.c >> +++ b/ltp/fsx.c >> @@ -1602,6 +1602,7 @@ do_copy_range(unsigned offset, unsigned length, unsigned dest) >> size_t olen; >> ssize_t nr; >> int tries = 0; >> + int ret = 0; >> >> if (length == 0) { >> if (!quiet && testcalls > simulatedopcount) >> @@ -1665,6 +1666,37 @@ do_copy_range(unsigned offset, unsigned length, unsigned dest) >> memset(good_buf + file_size, '\0', dest - file_size); >> if (dest + length > file_size) >> file_size = dest + length; >> + /* >> + * Although copy_file_range returns ok >> + * for some linux distros that use general implementation >> + * (splice interface) such as RHEL 7, centos 7 that use >> + * pipe_to_file will cause test fail. >> + * >> + * Here we add a little more check here for copy_file_range >> + * We read copied data then check it. If check fail here >> + * then report it. >> + */ > Above comments seem like not necessary, the content check is inspired by > Rthis HEL7 issue, but not the main purpose. > >> + ret = lseek(fd, (off_t)dest, SEEK_SET); >> + if (ret == (off_t)-1) { >> + prterr("doread: lseek"); > ^^^^^^^ not doread > >> + report_failure(140); > Use a different failure number than that in doread, i.e. don't copy the > code from doread. > >> + } >> + ret = fsxread(fd, temp_buf, length, dest); >> + if (ret != length) { >> + if (ret == -1) >> + prterr("doread: read"); >> + else >> + prt("short read: 0x%x bytes instead of 0x%x\n", >> + ret, length); > Same here, not doread, and weired indention above. > >> + report_failure(141); >> + } >> + if (memcmp(good_buf+dest, temp_buf, length) != 0) { >> + prt("copy range: 0x%x to 0x%x at 0x%x\n", offset, >> + offset + length, dest); >> + prterr("do_copy_range:"); >> + report_failure(161); > I think we could take use of check_buffers(). > > And similar check could be added to do_clone_range as well, I think. Ok, i'll send patch v2 later. Thanks > > Thanks, > Eryu > >> + >> + } >> } >> >> #else >> -- >> 2.7.4 >> >> >> >
next prev parent reply other threads:[~2019-09-27 1:38 UTC|newest] Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-09-25 7:16 [PATCH] fsx: add more check for copy_file_range Su Yanjun 2019-09-25 13:08 ` Eryu Guan 2019-09-27 1:38 ` Su Yanjun [this message] 2019-09-27 1:38 ` Su Yanjun
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=5307926c-1fdb-5283-5053-b6f4f7d23a41@cn.fujitsu.com \ --to=suyj.fnst@cn.fujitsu.com \ --cc=fstests@vger.kernel.org \ --cc=guaneryu@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).