| 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 | expand |
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); >
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.
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
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.
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
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 > > >
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
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 > > >
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.
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);
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.
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.
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);
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(+)