[COMMITTED] tst_device: do sync() before reading test block device stat file
diff mbox series

Message ID 20200102015236.7400-1-liwang@redhat.com
State Accepted
Delegated to: Petr Vorel
Headers show
Series
  • [COMMITTED] tst_device: do sync() before reading test block device stat file
Related show

Commit Message

Li Wang Jan. 2, 2020, 1:52 a.m. UTC
To avoid FS deferred IO metadata/cache interferes test result, so we
do sync simply before the tst_dev_bytes_written invocation.

Signed-off-by: Li Wang <liwang@redhat.com>
Cc: Sumit Garg <sumit.garg@linaro.org>
Cc: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
---
 testcases/kernel/syscalls/fdatasync/fdatasync03.c | 2 ++
 testcases/kernel/syscalls/fsync/fsync04.c         | 2 ++
 testcases/kernel/syscalls/sync/sync03.c           | 2 ++
 testcases/kernel/syscalls/syncfs/syncfs01.c       | 2 ++
 4 files changed, 8 insertions(+)

Comments

Yang Xu Jan. 2, 2020, 2:09 a.m. UTC | #1
Hi Li
> To avoid FS deferred IO metadata/cache interferes test result, so we
> do sync simply before the tst_dev_bytes_written invocation.
> 
Looks good, acked. Also, I think we can mention it in 
doc/test-writing-guidelines.txt when using this api.

Best Regards
Yang Xu
> Signed-off-by: Li Wang <liwang@redhat.com>
> Cc: Sumit Garg <sumit.garg@linaro.org>
> Cc: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
> ---
>   testcases/kernel/syscalls/fdatasync/fdatasync03.c | 2 ++
>   testcases/kernel/syscalls/fsync/fsync04.c         | 2 ++
>   testcases/kernel/syscalls/sync/sync03.c           | 2 ++
>   testcases/kernel/syscalls/syncfs/syncfs01.c       | 2 ++
>   4 files changed, 8 insertions(+)
> 
> diff --git a/testcases/kernel/syscalls/fdatasync/fdatasync03.c b/testcases/kernel/syscalls/fdatasync/fdatasync03.c
> index ee50e75c9..032ac4b58 100644
> --- a/testcases/kernel/syscalls/fdatasync/fdatasync03.c
> +++ b/testcases/kernel/syscalls/fdatasync/fdatasync03.c
> @@ -32,6 +32,8 @@ static void verify_fdatasync(void)
>   
>   	fd = SAFE_OPEN(FNAME, O_RDWR|O_CREAT, MODE);
>   
> +	sync();
> +
>   	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 c67fc5692..3c1f45e94 100644
> --- a/testcases/kernel/syscalls/fsync/fsync04.c
> +++ b/testcases/kernel/syscalls/fsync/fsync04.c
> @@ -32,6 +32,8 @@ static void verify_fsync(void)
>   
>   	fd = SAFE_OPEN(FNAME, O_RDWR|O_CREAT, MODE);
>   
> +	sync();
> +
>   	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 a6f72d2ed..085ccfdeb 100644
> --- a/testcases/kernel/syscalls/sync/sync03.c
> +++ b/testcases/kernel/syscalls/sync/sync03.c
> @@ -32,6 +32,8 @@ static void verify_sync(void)
>   
>   	fd = SAFE_OPEN(FNAME, O_RDWR|O_CREAT, MODE);
>   
> +	sync();
> +
>   	tst_dev_bytes_written(tst_device->dev);
>   
>   	tst_fill_fd(fd, 0, TST_MB, FILE_SIZE_MB);
> diff --git a/testcases/kernel/syscalls/syncfs/syncfs01.c b/testcases/kernel/syscalls/syncfs/syncfs01.c
> index 051a19ea6..3cf404450 100644
> --- a/testcases/kernel/syscalls/syncfs/syncfs01.c
> +++ b/testcases/kernel/syscalls/syncfs/syncfs01.c
> @@ -33,6 +33,8 @@ static void verify_syncfs(void)
>   
>   	fd = SAFE_OPEN(FNAME, O_RDWR|O_CREAT, MODE);
>   
> +	sync();
> +
>   	tst_dev_bytes_written(tst_device->dev);
>   
>   	tst_fill_fd(fd, 0, TST_MB, FILE_SIZE_MB);
>
Li Wang Jan. 2, 2020, 6:31 a.m. UTC | #2
On Thu, Jan 2, 2020 at 10:10 AM Yang Xu <xuyang2018.jy@cn.fujitsu.com>
wrote:

>
>
> Hi Li
> > To avoid FS deferred IO metadata/cache interferes test result, so we
> > do sync simply before the tst_dev_bytes_written invocation.
> >
> Looks good, acked. Also, I think we can mention it in
> doc/test-writing-guidelines.txt when using this api.
>

Ok, I will append one line as:
--- a/doc/test-writing-guidelines.txt
+++ b/doc/test-writing-guidelines.txt
@@ -1072,7 +1072,9 @@ 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 test result, we suggest doing
sync()
+before the tst_dev_bytes_written first invocation.
Yang Xu Jan. 2, 2020, 6:46 a.m. UTC | #3
on 2020/01/02 14:31, Li Wang wrote:
> 
> 
> On Thu, Jan 2, 2020 at 10:10 AM Yang Xu <xuyang2018.jy@cn.fujitsu.com 
> <mailto:xuyang2018.jy@cn.fujitsu.com>> wrote:
> 
> 
> 
>     Hi Li
>      > To avoid FS deferred IO metadata/cache interferes test result, so we
>      > do sync simply before the tst_dev_bytes_written invocation.
>      >
>     Looks good, acked. Also, I think we can mention it in
>     doc/test-writing-guidelines.txt when using this api.
> 
> 
> Ok, I will append one line as:
> --- a/doc/test-writing-guidelines.txt
> +++ b/doc/test-writing-guidelines.txt
> @@ -1072,7 +1072,9 @@ 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 test result, we suggest 
> doing sync()
> +before the tst_dev_bytes_written first invocation.
OK.

I also meet another problem when using this api on old kernel.

tst_device.c:395: CONF: Test device stat file: /sys/block/loop0/stat broken

/sys/block/loop0/stat is all 0 and case reports TCONF. on new kernel,
this value is normal. This is a block layer or loop device driver 
modifition several yeas ago?

ps:I know ltp used LOOP_SET_FD to make loop device simulate other 
filesystems. I am trying to find a generic way about this api.

Best Regards
Yang Xu
> 
> -- 
> Regards,
> Li Wang
Cyril Hrubis Jan. 2, 2020, 12:57 p.m. UTC | #4
Hi!
> To avoid FS deferred IO metadata/cache interferes test result, so we
> do sync simply before the tst_dev_bytes_written invocation.

Can we do fsync() on the fd instead of full sync()? That should be
slightly faster.
Li Wang Jan. 3, 2020, 7:24 a.m. UTC | #5
Hi Cyril,

On Thu, Jan 2, 2020 at 8:58 PM Cyril Hrubis <chrubis@suse.cz> wrote:

> Hi!
> > To avoid FS deferred IO metadata/cache interferes test result, so we
> > do sync simply before the tst_dev_bytes_written invocation.
>
> Can we do fsync() on the fd instead of full sync()? That should be
> slightly faster.
>

Probably you miss the previous discussed [1], we use sync() here because we
do want to make sure all FS metadata/cache being written back before the
testing since there is no obtainable file descriptor 'fd' for the ext4
deferred IO (e.g. initialize the journal and inode tables).

If I was wrong here, feel free to correct me.

[1] http://lists.linux.it/pipermail/ltp/2019-December/014792.html
Li Wang Jan. 3, 2020, 7:25 a.m. UTC | #6
On Thu, Jan 2, 2020 at 2:46 PM Yang Xu <xuyang2018.jy@cn.fujitsu.com> wrote:

>
>
> on 2020/01/02 14:31, Li Wang wrote:
> >
> >
> > On Thu, Jan 2, 2020 at 10:10 AM Yang Xu <xuyang2018.jy@cn.fujitsu.com
> > <mailto:xuyang2018.jy@cn.fujitsu.com>> wrote:
> >
> >
> >
> >     Hi Li
> >      > To avoid FS deferred IO metadata/cache interferes test result, so
> we
> >      > do sync simply before the tst_dev_bytes_written invocation.
> >      >
> >     Looks good, acked. Also, I think we can mention it in
> >     doc/test-writing-guidelines.txt when using this api.
> >
> >
> > Ok, I will append one line as:
> > --- a/doc/test-writing-guidelines.txt
> > +++ b/doc/test-writing-guidelines.txt
> > @@ -1072,7 +1072,9 @@ 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 test result, we suggest
> > doing sync()
> > +before the tst_dev_bytes_written first invocation.
> OK.
>
> I also meet another problem when using this api on old kernel.
>
> tst_device.c:395: CONF: Test device stat file: /sys/block/loop0/stat broken
>
> /sys/block/loop0/stat is all 0 and case reports TCONF. on new kernel,
> this value is normal. This is a block layer or loop device driver
> modifition several yeas ago?
>

It sounds like a kernel issue, can you tell which kernel version you did
test?

>
> ps:I know ltp used LOOP_SET_FD to make loop device simulate other
> filesystems. I am trying to find a generic way about this api.
>
> Best Regards
> Yang Xu
> >
> > --
> > Regards,
> > Li Wang
>
>
>
Yang Xu Jan. 3, 2020, 7:41 a.m. UTC | #7
on 2020/01/03 15:25, Li Wang wrote:
> 
> 
> On Thu, Jan 2, 2020 at 2:46 PM Yang Xu <xuyang2018.jy@cn.fujitsu.com 
> <mailto:xuyang2018.jy@cn.fujitsu.com>> wrote:
> 
> 
> 
>     on 2020/01/02 14:31, Li Wang wrote:
>      >
>      >
>      > On Thu, Jan 2, 2020 at 10:10 AM Yang Xu
>     <xuyang2018.jy@cn.fujitsu.com <mailto:xuyang2018.jy@cn.fujitsu.com>
>      > <mailto:xuyang2018.jy@cn.fujitsu.com
>     <mailto:xuyang2018.jy@cn.fujitsu.com>>> wrote:
>      >
>      >
>      >
>      >     Hi Li
>      >      > To avoid FS deferred IO metadata/cache interferes test
>     result, so we
>      >      > do sync simply before the tst_dev_bytes_written invocation.
>      >      >
>      >     Looks good, acked. Also, I think we can mention it in
>      >     doc/test-writing-guidelines.txt when using this api.
>      >
>      >
>      > Ok, I will append one line as:
>      > --- a/doc/test-writing-guidelines.txt
>      > +++ b/doc/test-writing-guidelines.txt
>      > @@ -1072,7 +1072,9 @@ 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 test result, we
>     suggest
>      > doing sync()
>      > +before the tst_dev_bytes_written first invocation.
>     OK.
> 
>     I also meet another problem when using this api on old kernel.
> 
>     tst_device.c:395: CONF: Test device stat file: /sys/block/loop0/stat
>     broken
> 
>     /sys/block/loop0/stat is all 0 and case reports TCONF. on new kernel,
>     this value is normal. This is a block layer or loop device driver
>     modifition several yeas ago?
> 
> 
> It sounds like a kernel issue, can you tell which kernel version you did 
> test?
> 
I test this on 3.10.0-1101.el7.x86_64. It also reports TCONF on old 
fedora such as fedora 20,21. I guess loop driver modification(I am 
verifying ...) causes it.
> 
>     ps:I know ltp used LOOP_SET_FD to make loop device simulate other
>     filesystems. I am trying to find a generic way about this api.
> 
>     Best Regards
>     Yang Xu
>      >
>      > --
>      > Regards,
>      > Li Wang
> 
> 
> 
> 
> -- 
> Regards,
> Li Wang
Yang Xu Jan. 3, 2020, 10:03 a.m. UTC | #8
Hi Li
> 
> 
> on 2020/01/03 15:25, Li Wang wrote:
>>
>>
>> On Thu, Jan 2, 2020 at 2:46 PM Yang Xu <xuyang2018.jy@cn.fujitsu.com 
>> <mailto:xuyang2018.jy@cn.fujitsu.com>> wrote:
>>
>>
>>
>>     on 2020/01/02 14:31, Li Wang wrote:
>>      >
>>      >
>>      > On Thu, Jan 2, 2020 at 10:10 AM Yang Xu
>>     <xuyang2018.jy@cn.fujitsu.com <mailto:xuyang2018.jy@cn.fujitsu.com>
>>      > <mailto:xuyang2018.jy@cn.fujitsu.com
>>     <mailto:xuyang2018.jy@cn.fujitsu.com>>> wrote:
>>      >
>>      >
>>      >
>>      >     Hi Li
>>      >      > To avoid FS deferred IO metadata/cache interferes test
>>     result, so we
>>      >      > do sync simply before the tst_dev_bytes_written invocation.
>>      >      >
>>      >     Looks good, acked. Also, I think we can mention it in
>>      >     doc/test-writing-guidelines.txt when using this api.
>>      >
>>      >
>>      > Ok, I will append one line as:
>>      > --- a/doc/test-writing-guidelines.txt
>>      > +++ b/doc/test-writing-guidelines.txt
>>      > @@ -1072,7 +1072,9 @@ 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 test result, we
>>     suggest
>>      > doing sync()
>>      > +before the tst_dev_bytes_written first invocation.
>>     OK.
>>
>>     I also meet another problem when using this api on old kernel.
>>
>>     tst_device.c:395: CONF: Test device stat file: /sys/block/loop0/stat
>>     broken
>>
>>     /sys/block/loop0/stat is all 0 and case reports TCONF. on new kernel,
>>     this value is normal. This is a block layer or loop device driver
>>     modifition several yeas ago?
>>
>>
>> It sounds like a kernel issue, can you tell which kernel version you 
>> did test?
>>
> I test this on 3.10.0-1101.el7.x86_64. It also reports TCONF on old 
> fedora such as fedora 20,21. I guess loop driver modification(I am 
> verifying ...) causes it.
Before kernel commit b5dd2f6047ca[1] (block: loop: improve performance 
via blk-mq), this /sys/block/loop0/stat value is all 0 for loop device 
because loop device uses its own make requestion function but not blk 
end request (so these value can not be counted). After onversion of loop 
to blk-mq, this value is normal. This patch was merged into upstream 
kernel since v4.0.  When using tst_dev_bytes_written api(using loop 
device as block device) on older kernel, this /sys/block/loop0/stat 
value is all 0 and we should mention user to export $LTP_DEV to test. 
Also, we can do it in other patch.

[1]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/block/loop.c?id=b5dd2f6047ca108001328aac0e8588edd15f1778

ps:I don't know block device(bio,request) or loop driver very much. If I 
was wrong, please feel free to correct me.

Best Regards
Yang Xu
>>
>>     ps:I know ltp used LOOP_SET_FD to make loop device simulate other
>>     filesystems. I am trying to find a generic way about this api.
>>
>>     Best Regards
>>     Yang Xu
>>      >
>>      > --
>>      > Regards,
>>      > Li Wang
>>
>>
>>
>>
>> -- 
>> Regards,
>> Li Wang
> 
> 
>
Cyril Hrubis Jan. 6, 2020, 10:05 a.m. UTC | #9
Hi!
> > > To avoid FS deferred IO metadata/cache interferes test result, so we
> > > do sync simply before the tst_dev_bytes_written invocation.
> >
> > Can we do fsync() on the fd instead of full sync()? That should be
> > slightly faster.
> >
> 
> Probably you miss the previous discussed [1], we use sync() here because we
> do want to make sure all FS metadata/cache being written back before the
> testing since there is no obtainable file descriptor 'fd' for the ext4
> deferred IO (e.g. initialize the journal and inode tables).

Ah, right, we measure I/O to the whole device, so we would have to sync
the device in question. Then syncfs() on the fd we got should work
right? And it should avoid syncing unrelated filesystems as well.
Cyril Hrubis Jan. 6, 2020, 10:35 a.m. UTC | #10
Hi!
> > > > To avoid FS deferred IO metadata/cache interferes test result, so we
> > > > do sync simply before the tst_dev_bytes_written invocation.
> > >
> > > Can we do fsync() on the fd instead of full sync()? That should be
> > > slightly faster.
> > >
> > 
> > Probably you miss the previous discussed [1], we use sync() here because we
> > do want to make sure all FS metadata/cache being written back before the
> > testing since there is no obtainable file descriptor 'fd' for the ext4
> > deferred IO (e.g. initialize the journal and inode tables).
> 
> Ah, right, we measure I/O to the whole device, so we would have to sync
> the device in question. Then syncfs() on the fd we got should work
> right? And it should avoid syncing unrelated filesystems as well.

It wouldn't probably harm to create an inline function in the test
library just for a sake of documentation, something as:

static inline void tst_dev_sync(int fd)
{
	syncfs(fd);
}


Then we end up calling:

	tst_dev_sync(fd);
	tst_dev_bytes_written(device);
Li Wang Jan. 7, 2020, 6:13 a.m. UTC | #11
Hi Cyril,

On Mon, Jan 6, 2020 at 6:06 PM Cyril Hrubis <chrubis@suse.cz> wrote:

> Hi!
> > > > To avoid FS deferred IO metadata/cache interferes test result, so we
> > > > do sync simply before the tst_dev_bytes_written invocation.
> > >
> > > Can we do fsync() on the fd instead of full sync()? That should be
> > > slightly faster.
> > >
> >
> > Probably you miss the previous discussed [1], we use sync() here because
> we
> > do want to make sure all FS metadata/cache being written back before the
> > testing since there is no obtainable file descriptor 'fd' for the ext4
> > deferred IO (e.g. initialize the journal and inode tables).
>
> Ah, right, we measure I/O to the whole device, so we would have to sync
> the device in question. Then syncfs() on the fd we got should work
> right? And it should avoid syncing unrelated filesystems as well.
>

Good point, I double read the fsync() and syncfs() manual page. It looks
like syncfs() is not the same as the previous function, it synchronizes the
file system containing file referred to by the open file descriptor fd, I
mixed them before, to be honest.
Li Wang Jan. 7, 2020, 6:16 a.m. UTC | #12
On Mon, Jan 6, 2020 at 6:35 PM Cyril Hrubis <chrubis@suse.cz> wrote:

> Hi!
> > > > > To avoid FS deferred IO metadata/cache interferes test result, so
> we
> > > > > do sync simply before the tst_dev_bytes_written invocation.
> > > >
> > > > Can we do fsync() on the fd instead of full sync()? That should be
> > > > slightly faster.
> > > >
> > >
> > > Probably you miss the previous discussed [1], we use sync() here
> because we
> > > do want to make sure all FS metadata/cache being written back before
> the
> > > testing since there is no obtainable file descriptor 'fd' for the ext4
> > > deferred IO (e.g. initialize the journal and inode tables).
> >
> > Ah, right, we measure I/O to the whole device, so we would have to sync
> > the device in question. Then syncfs() on the fd we got should work
> > right? And it should avoid syncing unrelated filesystems as well.
>
> It wouldn't probably harm to create an inline function in the test
> library just for a sake of documentation, something as:
>
> static inline void tst_dev_sync(int fd)
> {
>         syncfs(fd);
> }
>
>
> Then we end up calling:
>
>         tst_dev_sync(fd);
>         tst_dev_bytes_written(device);
>

Agree.

Patch
diff mbox series

diff --git a/testcases/kernel/syscalls/fdatasync/fdatasync03.c b/testcases/kernel/syscalls/fdatasync/fdatasync03.c
index ee50e75c9..032ac4b58 100644
--- a/testcases/kernel/syscalls/fdatasync/fdatasync03.c
+++ b/testcases/kernel/syscalls/fdatasync/fdatasync03.c
@@ -32,6 +32,8 @@  static void verify_fdatasync(void)
 
 	fd = SAFE_OPEN(FNAME, O_RDWR|O_CREAT, MODE);
 
+	sync();
+
 	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 c67fc5692..3c1f45e94 100644
--- a/testcases/kernel/syscalls/fsync/fsync04.c
+++ b/testcases/kernel/syscalls/fsync/fsync04.c
@@ -32,6 +32,8 @@  static void verify_fsync(void)
 
 	fd = SAFE_OPEN(FNAME, O_RDWR|O_CREAT, MODE);
 
+	sync();
+
 	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 a6f72d2ed..085ccfdeb 100644
--- a/testcases/kernel/syscalls/sync/sync03.c
+++ b/testcases/kernel/syscalls/sync/sync03.c
@@ -32,6 +32,8 @@  static void verify_sync(void)
 
 	fd = SAFE_OPEN(FNAME, O_RDWR|O_CREAT, MODE);
 
+	sync();
+
 	tst_dev_bytes_written(tst_device->dev);
 
 	tst_fill_fd(fd, 0, TST_MB, FILE_SIZE_MB);
diff --git a/testcases/kernel/syscalls/syncfs/syncfs01.c b/testcases/kernel/syscalls/syncfs/syncfs01.c
index 051a19ea6..3cf404450 100644
--- a/testcases/kernel/syscalls/syncfs/syncfs01.c
+++ b/testcases/kernel/syscalls/syncfs/syncfs01.c
@@ -33,6 +33,8 @@  static void verify_syncfs(void)
 
 	fd = SAFE_OPEN(FNAME, O_RDWR|O_CREAT, MODE);
 
+	sync();
+
 	tst_dev_bytes_written(tst_device->dev);
 
 	tst_fill_fd(fd, 0, TST_MB, FILE_SIZE_MB);