| Message ID | 20200107071324.29492-1-liwang@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | tst_device: add new tst_dev_sync | expand |
Hi! > This function reads test block device stat file (/sys/block/<device>/stat) and > -returns the bytes written since the last invocation of this function. > +returns the bytes written since the last invocation of this function. To avoid > +FS deferred IO metadata/cache interferes the result, we suggest doing "syncfs" | interfecence | > +before the tst_dev_bytes_written first invocation. And an inline function named > +tst_dev_sync is created for that intention. Other than this it looks good to me, thanks for doing this, acked.
On Tue, Jan 7, 2020 at 6:11 PM Cyril Hrubis <chrubis@suse.cz> wrote: > Hi! > > This function reads test block device stat file > (/sys/block/<device>/stat) and > > -returns the bytes written since the last invocation of this function. > > +returns the bytes written since the last invocation of this function. > To avoid > > +FS deferred IO metadata/cache interferes the result, we suggest doing > "syncfs" > | interfecence | > Thanks. > > > +before the tst_dev_bytes_written first invocation. And an inline > function named > > +tst_dev_sync is created for that intention. > > Other than this it looks good to me, thanks for doing this, acked. > Pushed.
Hi Li, Cyril, > > > +before the tst_dev_bytes_written first invocation. And an inline > > function named > > > +tst_dev_sync is created for that intention. > > Other than this it looks good to me, thanks for doing this, acked. > Pushed. This broke build with -Werror=implicit-function-declaration [1]: ../include/tst_device.h:78:2: error: implicit declaration of function 'syncfs'; did you mean 'sync'? [-Werror=implicit-function-declaration] 2770 syncfs(fd); 2771 ^~~~~~ 2772 sync The problem is that syncfs() is guarded with __USE_GNU (in glibc, in musl directly _GNU_SOURCE), so this requires to use _GNU_SOURCE (before including first header). Because it's in tst_device.h, we effectively need to build with -D_GNU_SOURCE. Is that what we want? Or should we always use tst_syscall(__NR_syncfs, fd) (without conditional check #ifndef HAVE_SYNCFS) ? Kind regards, Petr [1] https://travis-ci.org/linux-test-project/ltp/jobs/634178861
Hi Li, Cyril, > This broke build with -Werror=implicit-function-declaration [1]: > ../include/tst_device.h:78:2: error: implicit declaration of function 'syncfs'; did you mean 'sync'? [-Werror=implicit-function-declaration] > 2770 syncfs(fd); > 2771 ^~~~~~ > 2772 sync > The problem is that syncfs() is guarded with __USE_GNU (in glibc, in musl > directly _GNU_SOURCE), so this requires to use _GNU_SOURCE (before including > first header). > Because it's in tst_device.h, we effectively need to build with -D_GNU_SOURCE. > Is that what we want? Or should we always use tst_syscall(__NR_syncfs, fd) > (without conditional check #ifndef HAVE_SYNCFS) ? Or maybe use tst_syscall(__NR_syncfs, fd) in include/tst_device.h and keep include/lapi/syncfs.h for tests (which would need to define _GNU_SOURCE?). Kind regards, Petr > [1] https://travis-ci.org/linux-test-project/ltp/jobs/634178861
Hi! > This broke build with -Werror=implicit-function-declaration [1]: > > ../include/tst_device.h:78:2: error: implicit declaration of function 'syncfs'; did you mean 'sync'? [-Werror=implicit-function-declaration] > 2770 syncfs(fd); > 2771 ^~~~~~ > 2772 sync > > The problem is that syncfs() is guarded with __USE_GNU (in glibc, in musl > directly _GNU_SOURCE), so this requires to use _GNU_SOURCE (before including > first header). > Because it's in tst_device.h, we effectively need to build with -D_GNU_SOURCE. > Is that what we want? Or should we always use tst_syscall(__NR_syncfs, fd) > (without conditional check #ifndef HAVE_SYNCFS) ? I guess calling raw syscall in the tst_device.h would be easiest fix.
On Wed, Jan 8, 2020 at 7:35 PM Cyril Hrubis <chrubis@suse.cz> wrote: > Hi! > > This broke build with -Werror=implicit-function-declaration [1]: > > > > ../include/tst_device.h:78:2: error: implicit declaration of function > 'syncfs'; did you mean 'sync'? [-Werror=implicit-function-declaration] > > 2770 syncfs(fd); > > 2771 ^~~~~~ > > 2772 sync > Thanks for highlight this. > > > > The problem is that syncfs() is guarded with __USE_GNU (in glibc, in musl > > directly _GNU_SOURCE), so this requires to use _GNU_SOURCE (before > including > > first header). > > Because it's in tst_device.h, we effectively need to build with > -D_GNU_SOURCE. > > Is that what we want? Or should we always use tst_syscall(__NR_syncfs, > fd) > > (without conditional check #ifndef HAVE_SYNCFS) ? > > I guess calling raw syscall in the tst_device.h would be easiest fix. > Yes, we can have a try. Btw I just pushed a simple fix to include unistd.h, it seems not works. > -- > Cyril Hrubis > chrubis@suse.cz > >
Hi
> Btw I just pushed a simple fix to include unistd.h, it seems not works.
You would need to define _GNU_SOURCE at the top of each test that uses
it otherwise the definition is not exposed.
Hi Li, Cyril, > Hi > > Btw I just pushed a simple fix to include unistd.h, it seems not works. > You would need to define _GNU_SOURCE at the top of each test that uses > it otherwise the definition is not exposed. Yep :(. I join to Cyril's vote for raw syscall. BTW please test it with travis or locally build.sh script (which adds -Werror=implicit-function-declaration). I guess we should work on travis CI integration so we don't have to push it to travis manually [1]. Kind regards, Petr [1] https://github.com/linux-test-project/ltp/issues/599
On Wed, Jan 8, 2020 at 7:45 PM Petr Vorel <pvorel@suse.cz> wrote: > Hi Li, Cyril, > > > Hi > > > Btw I just pushed a simple fix to include unistd.h, it seems not works. > > > You would need to define _GNU_SOURCE at the top of each test that uses > > it otherwise the definition is not exposed. > Yep :(. I join to Cyril's vote for raw syscall. > BTW please test it with travis or locally build.sh script (which adds > -Werror=implicit-function-declaration). > I'd go with syscall(__NR_syncfs, fd) in the tst_device.h and define _GNU_SOURCE in relevant test cases. The reason why not use tst_syscall() is that involves a new compile error of tst_brk, and it can not get rid of errors only via include tst_test.h file. > > I guess we should work on travis CI integration so we don't have to push > it to > travis manually [1]. > That's a good proposal. I feel so sorry for pushing without a compiling in Travis this time, because I thought the code is good enough but ...
Hi Petr > Hi Li, Cyril, > >> Hi >>> Btw I just pushed a simple fix to include unistd.h, it seems not works. > >> You would need to define _GNU_SOURCE at the top of each test that uses >> it otherwise the definition is not exposed. > Yep :(. I join to Cyril's vote for raw syscall. > BTW please test it with travis or locally build.sh script (which adds > -Werror=implicit-function-declaration). > > I guess we should work on travis CI integration so we don't have to push it to > travis manually [1]. Do we have a tool or a function that when this pushing patch complie succeeded, then merge successfully or reject auto because of fail. Like gerrit. Or, pushing patch compiler on travis CI first, then merged into ltp master. If not, it seems that we can only try on our own ltp fork(Usually, I do it when I write a new case or cleanup) Best Regards Yang Xu > > Kind regards, > Petr > > [1] https://github.com/linux-test-project/ltp/issues/599 >
Hi Li, > > > You would need to define _GNU_SOURCE at the top of each test that uses > > > it otherwise the definition is not exposed. > > Yep :(. I join to Cyril's vote for raw syscall. > > BTW please test it with travis or locally build.sh script (which adds > > -Werror=implicit-function-declaration). > I'd go with syscall(__NR_syncfs, fd) in the tst_device.h and > define _GNU_SOURCE in relevant test cases. > The reason why not use tst_syscall() is that involves a new compile error > of tst_brk, and it can not get rid of errors only via include tst_test.h > file. +1. (I already tested it and replied to that patch). I wonder if this is a pattern (try to avoid _GNU_SOURCE code in the library, if needed use syscall() instead). I'm asking because I still plan to write v2 for library API writing guidelines docs patch. BTW you didn't update wiki after merging eca0fa3c3. I did it :) (we need some maintainer docs as well, we keep with Cyril to update wiki and (sometimes) maintain patchwork for other maintainers). BTW there is git repository for each git on github (our [2]), I maintain it with shell script. > > I guess we should work on travis CI integration so we don't have to push > > it to > > travis manually [1]. > That's a good proposal. I feel so sorry for pushing without a compiling in > Travis this time, because I thought the code is good enough but ... No problem :). Kind regards, Petr [1] https://patchwork.ozlabs.org/patch/1166786/ [2] https://github.com/linux-test-project/ltp.wiki.git
Hi Xu, > > I guess we should work on travis CI integration so we don't have to push it to > > travis manually [1]. > Do we have a tool or a function that > when this pushing patch complie succeeded, then merge successfully or reject > auto because of fail. Like gerrit. Well, we have patchwork and his REST API [2] (IMHO lightweight than gerrit, but needs to be setup). More info is at #599 [2]. > Or, pushing patch compiler on travis CI first, then merged into ltp master. > If not, it seems that we can only try on our own ltp fork(Usually, I do it > when I write a new case or cleanup) That's what I want to automate. It will probably needs some special github account, but we have it for travis anyway. Kind regards, Petr [1] https://github.com/linux-test-project/ltp/issues/599 [2] https://patchwork.readthedocs.io/en/latest/api/rest/
Hi! > The reason why not use tst_syscall() is that involves a new compile error > of tst_brk, and it can not get rid of errors only via include tst_test.h > file. The problem is that the header is used both for old and new test API, so until we convert the rest of the oldlib testcases we can't use anything from the new library there. We solve that in other places by ifdefs, such as: #ifdef TST_TEST_H__ tst_syscall(...); #else ltp_syscall(...); #endif
Hi Petr > Hi Xu, > >>> I guess we should work on travis CI integration so we don't have to push it to >>> travis manually [1]. >> Do we have a tool or a function that >> when this pushing patch complie succeeded, then merge successfully or reject >> auto because of fail. Like gerrit. > Well, we have patchwork and his REST API [2] (IMHO lightweight than gerrit, but > needs to be setup). More info is at #599 [2]. > >> Or, pushing patch compiler on travis CI first, then merged into ltp master. > >> If not, it seems that we can only try on our own ltp fork(Usually, I do it >> when I write a new case or cleanup) > That's what I want to automate. It will probably needs some special github account, > but we have it for travis anyway. Thanks for your info. If this can realize, it will be great. ps: When I have free time, I will read them in detail. Best Regards Yang Xu > > Kind regards, > Petr > > [1] https://github.com/linux-test-project/ltp/issues/599 > [2] https://patchwork.readthedocs.io/en/latest/api/rest/ > >
Hi Xu, > > > If not, it seems that we can only try on our own ltp fork(Usually, I do it > > > when I write a new case or cleanup) > > That's what I want to automate. It will probably needs some special github account, > > but we have it for travis anyway. > Thanks for your info. If this can realize, it will be great. > ps: When I have free time, I will read them in detail. Thanks! IMHO server will be needed to run these scripts. Kind regards, Petr
Hi, > > The reason why not use tst_syscall() is that involves a new compile error > > of tst_brk, and it can not get rid of errors only via include tst_test.h > > file. > The problem is that the header is used both for old and new test API, so > until we convert the rest of the oldlib testcases we can't use anything > from the new library there. Good point. > We solve that in other places by ifdefs, such as: > #ifdef TST_TEST_H__ > tst_syscall(...); > #else > ltp_syscall(...); > #endif or something like (less #ifdef/#else branches): #ifdef TST_TEST_H__ # define TST_SYSCALL(...) tst_syscall(##__VA_ARGS__) #else # define TST_SYSCALL(...) ltp_syscall(##__VA_ARGS__) #endif Kind regards, Petr
diff --git a/doc/test-writing-guidelines.txt b/doc/test-writing-guidelines.txt index 88f771823..dfe28ef3f 100644 --- a/doc/test-writing-guidelines.txt +++ b/doc/test-writing-guidelines.txt @@ -1072,7 +1072,10 @@ unsigned long tst_dev_bytes_written(const char *dev); ------------------------------------------------------------------------------- This function reads test block device stat file (/sys/block/<device>/stat) and -returns the bytes written since the last invocation of this function. +returns the bytes written since the last invocation of this function. To avoid +FS deferred IO metadata/cache interferes the result, we suggest doing "syncfs" +before the tst_dev_bytes_written first invocation. And an inline function named +tst_dev_sync is created for that intention. 2.2.16 Formatting a device with a filesystem ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ diff --git a/include/tst_device.h b/include/tst_device.h index 56835e712..992d1383d 100644 --- a/include/tst_device.h +++ b/include/tst_device.h @@ -68,6 +68,16 @@ int tst_attach_device(const char *dev_path, const char *file_path); */ int tst_detach_device(const char *dev_path); +/* + * To avoid FS deferred IO metadata/cache interferes test result, so we do + * syncfs simply before the tst_dev_bytes_written invocation. For easy to + * use, we create this inline function tst_dev_sync. + */ +static inline void tst_dev_sync(int fd) +{ + syncfs(fd); +} + /* * Reads test block device stat file and returns the bytes written since the * last call of this function. diff --git a/testcases/kernel/syscalls/fdatasync/fdatasync03.c b/testcases/kernel/syscalls/fdatasync/fdatasync03.c index 032ac4b58..263175b85 100644 --- a/testcases/kernel/syscalls/fdatasync/fdatasync03.c +++ b/testcases/kernel/syscalls/fdatasync/fdatasync03.c @@ -32,8 +32,7 @@ static void verify_fdatasync(void) fd = SAFE_OPEN(FNAME, O_RDWR|O_CREAT, MODE); - sync(); - + tst_dev_sync(fd); tst_dev_bytes_written(tst_device->dev); tst_fill_fd(fd, 0, TST_MB, FILE_SIZE_MB); diff --git a/testcases/kernel/syscalls/fsync/fsync04.c b/testcases/kernel/syscalls/fsync/fsync04.c index 3c1f45e94..1e4b8754c 100644 --- a/testcases/kernel/syscalls/fsync/fsync04.c +++ b/testcases/kernel/syscalls/fsync/fsync04.c @@ -32,8 +32,7 @@ static void verify_fsync(void) fd = SAFE_OPEN(FNAME, O_RDWR|O_CREAT, MODE); - sync(); - + tst_dev_sync(fd); tst_dev_bytes_written(tst_device->dev); tst_fill_fd(fd, 0, TST_MB, FILE_SIZE_MB); diff --git a/testcases/kernel/syscalls/sync/sync03.c b/testcases/kernel/syscalls/sync/sync03.c index 085ccfdeb..c5c02f877 100644 --- a/testcases/kernel/syscalls/sync/sync03.c +++ b/testcases/kernel/syscalls/sync/sync03.c @@ -32,8 +32,7 @@ static void verify_sync(void) fd = SAFE_OPEN(FNAME, O_RDWR|O_CREAT, MODE); - sync(); - + tst_dev_sync(fd); tst_dev_bytes_written(tst_device->dev); tst_fill_fd(fd, 0, TST_MB, FILE_SIZE_MB); diff --git a/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c b/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c index 1a6d84c49..64d069e93 100644 --- a/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c +++ b/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c @@ -48,8 +48,7 @@ static void verify_sync_file_range(struct testcase *tc) lseek(fd, tc->write_off, SEEK_SET); - sync(); - + tst_dev_sync(fd); tst_dev_bytes_written(tst_device->dev); tst_fill_fd(fd, 0, TST_MB, tc->write_size_mb); diff --git a/testcases/kernel/syscalls/syncfs/syncfs01.c b/testcases/kernel/syscalls/syncfs/syncfs01.c index 3cf404450..333726eaa 100644 --- a/testcases/kernel/syscalls/syncfs/syncfs01.c +++ b/testcases/kernel/syscalls/syncfs/syncfs01.c @@ -33,8 +33,7 @@ static void verify_syncfs(void) fd = SAFE_OPEN(FNAME, O_RDWR|O_CREAT, MODE); - sync(); - + tst_dev_sync(fd); tst_dev_bytes_written(tst_device->dev); tst_fill_fd(fd, 0, TST_MB, FILE_SIZE_MB);
To follow up commit: 0e2ec72de49ab48c151a3c30a1e6ea0cdaea321c tst_device: do sync() before reading test block device stat file Signed-off-by: Li Wang <liwang@redhat.com> Cc: Cyril Hrubis <chrubis@suse.cz> Cc: Sumit Garg <sumit.garg@linaro.org> Cc: Yang Xu <xuyang2018.jy@cn.fujitsu.com> --- doc/test-writing-guidelines.txt | 5 ++++- include/tst_device.h | 10 ++++++++++ testcases/kernel/syscalls/fdatasync/fdatasync03.c | 3 +-- testcases/kernel/syscalls/fsync/fsync04.c | 3 +-- testcases/kernel/syscalls/sync/sync03.c | 3 +-- .../syscalls/sync_file_range/sync_file_range02.c | 3 +-- testcases/kernel/syscalls/syncfs/syncfs01.c | 3 +-- 7 files changed, 19 insertions(+), 11 deletions(-)