| Message ID | 20210906060020.3219023-1-liwang@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] copy_file_range03: comparing timestamp in tst_timespec_diff | expand |
Hi Li Looks good to me. Acked-by: Yang Xu <xuyang2018.jy@fujitsu.com> Best Regards Yang Xu > The st_mtime field is defined as st_mtim.tv_sec for backward > compatibility in struct stat, which might not precise enough > for timestamp comparing. > > Here switch to timespec diff (with compare nanosecond as well) to > get rid of this kind of rare faliure: > > 7 tst_test.c:1345: TINFO: Timeout per run is 0h 05m 00s > 8 copy_file_range.h:36: TINFO: Testing libc copy_file_range() > 9 copy_file_range03.c:48: TPASS: copy_file_range sucessfully updated the timestamp > 10 tst_test.c:1345: TINFO: Timeout per run is 0h 05m 00s > 11 copy_file_range.h:39: TINFO: Testing __NR_copy_file_range syscall > 12 copy_file_range03.c:46: TFAIL: copy_file_range did not update timestamp. > > Also, raise the sleep time to 1.5 sec to make test more robust. > > Signed-off-by: Li Wang<liwang@redhat.com> > --- > .../copy_file_range/copy_file_range03.c | 17 ++++++++++------- > 1 file changed, 10 insertions(+), 7 deletions(-) > > diff --git a/testcases/kernel/syscalls/copy_file_range/copy_file_range03.c b/testcases/kernel/syscalls/copy_file_range/copy_file_range03.c > index 253eb57ad..5950c80c1 100644 > --- a/testcases/kernel/syscalls/copy_file_range/copy_file_range03.c > +++ b/testcases/kernel/syscalls/copy_file_range/copy_file_range03.c > @@ -12,26 +12,27 @@ > #define _GNU_SOURCE > > #include "tst_test.h" > +#include "tst_timer.h" > #include "copy_file_range.h" > > static int fd_src; > static int fd_dest; > > -unsigned long get_timestamp(int fd) > +struct timespec get_timestamp(int fd) > { > struct stat filestat; > > fstat(fd,&filestat); > - return filestat.st_mtime; > + return filestat.st_mtim; > } > > static void verify_copy_file_range_timestamp(void) > { > loff_t offset; > - unsigned long timestamp, updated_timestamp; > + struct timespec timestamp1, timestamp2, diff; > > - timestamp = get_timestamp(fd_dest); > - usleep(1000000); > + timestamp1 = get_timestamp(fd_dest); > + usleep(1500000); > > offset = 0; > TEST(sys_copy_file_range(fd_src,&offset, > @@ -40,9 +41,11 @@ static void verify_copy_file_range_timestamp(void) > tst_brk(TBROK | TTERRNO, > "copy_file_range unexpectedly failed"); > > - updated_timestamp = get_timestamp(fd_dest); > + timestamp2 = get_timestamp(fd_dest); > > - if (timestamp == updated_timestamp) > + diff = tst_timespec_diff(timestamp1, timestamp2); > + > + if (!diff.tv_sec&& !diff.tv_nsec) > tst_brk(TFAIL, "copy_file_range did not update timestamp."); > > tst_res(TPASS, "copy_file_range sucessfully updated the timestamp");
Hi! > - updated_timestamp = get_timestamp(fd_dest); > + timestamp2 = get_timestamp(fd_dest); > > - if (timestamp == updated_timestamp) > + diff = tst_timespec_diff(timestamp1, timestamp2); > + > + if (!diff.tv_sec && !diff.tv_nsec) > tst_brk(TFAIL, "copy_file_range did not update timestamp."); So as we changed the code to sleep for 1.5 sec I guess that we can expect the difference to be at least 1 second because: - the minimal granularity is 1s in which case we will get exactly 1s in the diff - if the granularity is greater, we will get a bit more than 1s So I would go for something as: long long diff_us = tst_timespec_diff_us(timestamp2, timestamp1); if (diff_us >= 1000000 && diff_us <= 2000000) tst_res(TPASS, "..."); Which expects that the difference between timestamps is in a sane range not that it just have been changed. > tst_res(TPASS, "copy_file_range sucessfully updated the timestamp"); > -- > 2.31.1 > > > -- > Mailing list info: https://lists.linux.it/listinfo/ltp
On Mon, Sep 6, 2021 at 4:59 PM Cyril Hrubis <chrubis@suse.cz> wrote: > Hi! > > - updated_timestamp = get_timestamp(fd_dest); > > + timestamp2 = get_timestamp(fd_dest); > > > > - if (timestamp == updated_timestamp) > > + diff = tst_timespec_diff(timestamp1, timestamp2); > > + > > + if (!diff.tv_sec && !diff.tv_nsec) > > tst_brk(TFAIL, "copy_file_range did not update > timestamp."); > > So as we changed the code to sleep for 1.5 sec I guess that we can > expect the difference to be at least 1 second because: > > - the minimal granularity is 1s in which case we will get exactly 1s in > the diff > > - if the granularity is greater, we will get a bit more than 1s > > So I would go for something as: > > long long diff_us = tst_timespec_diff_us(timestamp2, timestamp1); > > if (diff_us >= 1000000 && diff_us <= 2000000) > tst_res(TPASS, "..."); > > Which expects that the difference between timestamps is in a sane range > not that it just have been changed. > Theoretically, this is correct, but I'm fearing that the process might cost more time on performing with different system loads. This means 'diff_us <= 2000000' is an unreliable condition to many virtual machines. That largely depends on the system scheduler at that moment as well. > > > tst_res(TPASS, "copy_file_range sucessfully updated the > timestamp"); > > -- > > 2.31.1 > > > > > > -- > > Mailing list info: https://lists.linux.it/listinfo/ltp > > -- > Cyril Hrubis > chrubis@suse.cz > >
Hi! > Theoretically, this is correct, but I'm fearing that the process might > cost more time on performing with different system loads. > > This means 'diff_us <= 2000000' is an unreliable condition to > many virtual machines. That largely depends on the system > scheduler at that moment as well. Right, I guess that we can relax the upper bound, it can be even a minute and it would still be a good sanity check.
On Mon, Sep 6, 2021 at 6:37 PM Cyril Hrubis <chrubis@suse.cz> wrote: > Hi! > > Theoretically, this is correct, but I'm fearing that the process might > > cost more time on performing with different system loads. > > > > This means 'diff_us <= 2000000' is an unreliable condition to > > many virtual machines. That largely depends on the system > > scheduler at that moment as well. > > Right, I guess that we can relax the upper bound, it can be even a > minute and it would still be a good sanity check. > Yes, I'd relax it to 30 sec to see how it performs in real scenarios. Patch v3 is on the way.
diff --git a/testcases/kernel/syscalls/copy_file_range/copy_file_range03.c b/testcases/kernel/syscalls/copy_file_range/copy_file_range03.c index 253eb57ad..5950c80c1 100644 --- a/testcases/kernel/syscalls/copy_file_range/copy_file_range03.c +++ b/testcases/kernel/syscalls/copy_file_range/copy_file_range03.c @@ -12,26 +12,27 @@ #define _GNU_SOURCE #include "tst_test.h" +#include "tst_timer.h" #include "copy_file_range.h" static int fd_src; static int fd_dest; -unsigned long get_timestamp(int fd) +struct timespec get_timestamp(int fd) { struct stat filestat; fstat(fd, &filestat); - return filestat.st_mtime; + return filestat.st_mtim; } static void verify_copy_file_range_timestamp(void) { loff_t offset; - unsigned long timestamp, updated_timestamp; + struct timespec timestamp1, timestamp2, diff; - timestamp = get_timestamp(fd_dest); - usleep(1000000); + timestamp1 = get_timestamp(fd_dest); + usleep(1500000); offset = 0; TEST(sys_copy_file_range(fd_src, &offset, @@ -40,9 +41,11 @@ static void verify_copy_file_range_timestamp(void) tst_brk(TBROK | TTERRNO, "copy_file_range unexpectedly failed"); - updated_timestamp = get_timestamp(fd_dest); + timestamp2 = get_timestamp(fd_dest); - if (timestamp == updated_timestamp) + diff = tst_timespec_diff(timestamp1, timestamp2); + + if (!diff.tv_sec && !diff.tv_nsec) tst_brk(TFAIL, "copy_file_range did not update timestamp."); tst_res(TPASS, "copy_file_range sucessfully updated the timestamp");
The st_mtime field is defined as st_mtim.tv_sec for backward compatibility in struct stat, which might not precise enough for timestamp comparing. Here switch to timespec diff (with compare nanosecond as well) to get rid of this kind of rare faliure: 7 tst_test.c:1345: TINFO: Timeout per run is 0h 05m 00s 8 copy_file_range.h:36: TINFO: Testing libc copy_file_range() 9 copy_file_range03.c:48: TPASS: copy_file_range sucessfully updated the timestamp 10 tst_test.c:1345: TINFO: Timeout per run is 0h 05m 00s 11 copy_file_range.h:39: TINFO: Testing __NR_copy_file_range syscall 12 copy_file_range03.c:46: TFAIL: copy_file_range did not update timestamp. Also, raise the sleep time to 1.5 sec to make test more robust. Signed-off-by: Li Wang <liwang@redhat.com> --- .../copy_file_range/copy_file_range03.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)