[LTP] [PATCH] tst_device: add new tst_dev_sync

[LTP] [PATCH] tst_device: add new tst_dev_sync

[Li Wang]

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(-)

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);
-- 
2.20.1

[LTP] [PATCH] tst_device: add new tst_dev_sync

[Cyril Hrubis]

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.


-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] tst_device: add new tst_dev_sync

[Li Wang]

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.

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20200108/8350ae6e/attachment.htm>

[LTP] [PATCH] tst_device: add new tst_dev_sync

[Petr Vorel]

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

[LTP] [PATCH] tst_device: add new tst_dev_sync

[Petr Vorel]

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

[LTP] [PATCH] tst_device: add new tst_dev_sync

[Cyril Hrubis]

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.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] tst_device: add new tst_dev_sync

[Li Wang]

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
>
>

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20200108/1a77dcb4/attachment.htm>

[LTP] [PATCH] tst_device: add new tst_dev_sync

[Cyril Hrubis]

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.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] tst_device: add new tst_dev_sync

[Petr Vorel]

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

[LTP] [PATCH] tst_device: add new tst_dev_sync

[Li Wang]

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 ...

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20200109/b045fbe0/attachment-0001.htm>

[LTP] [PATCH] tst_device: add new tst_dev_sync

[Yang Xu]

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
> 

[LTP] [PATCH] tst_device: add new tst_dev_sync

[Petr Vorel]

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

[LTP] [PATCH] tst_device: add new tst_dev_sync

[Petr Vorel]

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/

[LTP] [PATCH] tst_device: add new tst_dev_sync

[Cyril Hrubis]

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


-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] tst_device: add new tst_dev_sync

[Yang Xu]

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/
> 
> 

[LTP] [PATCH] tst_device: add new tst_dev_sync

[Petr Vorel]

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

[LTP] [PATCH] tst_device: add new tst_dev_sync

[Petr Vorel]

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