[LTP] [PATCH RFC] sync_file_range02: remove the toplimit of write back

[LTP] [PATCH RFC] sync_file_range02: remove the toplimit of write back

[Li Wang]

Test tries to sync a range of $BYTES bytes, and then makes sure that
between $BYTES and $BYTES+10% was written to disk. But sometimes, more
than $BYTES+10% hit the disk: "Synced 39843840, expected 33554432" so
it failed as below.

  tst_test.c:1179: INFO: Testing on ext4
  sync_file_range02.c:74: FAIL: Sync equals write: Synced 39843840,
                          expected 33554432
  sync_file_range02.c:74: FAIL: Sync inside of write: Synced 18612224,
                          expected 16777216

From FS dev's view:

" The test's assumptions are fundamentally false; it thinks it can look
at IO counters (tst_dev_bytes_written) for a disk before and after a
system call, and attribute all of the IO seen to the system call that
was made - this isn't necessarily correct. Other processes may generate
IO in the background.

ext4 defers a lot of IO on a freshly made filesystem to the kernel -
for example it will initialize the journal and inode tables after the
mount, and this will cause extra IO.

Creating ext4 filesystems with the options: "-E lazy_itable_init=0,
lazy_journal_init=0" might help.

Another option would be to raise the threshold. Essentially, the report
here is that the test is failing because the filesystem wrote "too much"
as a result of the sync. How much is "too much?" ..."

Let's remove the toplimit of write back, and think as long as we synced
at least the expected amount, the test passes. The +10% limit seems arbitrary.

Signed-off-by: Li Wang <liwang@redhat.com>
Cc: Sumit Garg <sumit.garg@linaro.org>
---
 testcases/kernel/syscalls/sync_file_range/sync_file_range02.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

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 eb08143c3..643be14fa 100644
--- a/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c
+++ b/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c
@@ -66,8 +66,7 @@ static void verify_sync_file_range(struct testcase *tc)
 
 	SAFE_CLOSE(fd);
 
-	if ((written >= tc->exp_sync_size) &&
-	    (written <= (tc->exp_sync_size + tc->exp_sync_size/10)))
+	if ((written >= tc->exp_sync_size))
 		tst_res(TPASS, "%s", tc->desc);
 	else
 		tst_res(TFAIL, "%s: Synced %li, expected %li", tc->desc,
-- 
2.20.1

[LTP] [PATCH RFC] sync_file_range02: remove the toplimit of write back

[Jan Stancek]

----- Original Message -----
> " The test's assumptions are fundamentally false; it thinks it can look
> at IO counters (tst_dev_bytes_written) for a disk before and after a
> system call, and attribute all of the IO seen to the system call that
> was made - this isn't necessarily correct. Other processes may generate
> IO in the background.

We create our own block device, so there shouldn't be other processes
writing to it.

> ext4 defers a lot of IO on a freshly made filesystem to the kernel -
> for example it will initialize the journal and inode tables after the
> mount

Journal was my guess as well.

> Let's remove the toplimit of write back, and think as long as we synced
> at least the expected amount, the test passes. The +10% limit seems
> arbitrary.

I think this is reasonable approach until we find better way
to measure what was synced.

Acked-by: Jan Stancek <jstancek@redhat.com>

[LTP] [PATCH RFC] sync_file_range02: remove the toplimit of write back

[Sumit Garg]

+ Cyril (who originally proposed upper bound check)

On Wed, 18 Dec 2019 at 14:55, Jan Stancek <jstancek@redhat.com> wrote:
>
>
> ----- Original Message -----
> > " The test's assumptions are fundamentally false; it thinks it can look
> > at IO counters (tst_dev_bytes_written) for a disk before and after a
> > system call, and attribute all of the IO seen to the system call that
> > was made - this isn't necessarily correct. Other processes may generate
> > IO in the background.
>
> We create our own block device, so there shouldn't be other processes
> writing to it.
>
> > ext4 defers a lot of IO on a freshly made filesystem to the kernel -
> > for example it will initialize the journal and inode tables after the
> > mount
>
> Journal was my guess as well.

To avoid similar scenarios, I suggested to add a "sync()" call just
prior to test here [1]. And I couldn't reproduce the failure in
1000-times run with 4.19 kernel.

Also, I think having a "sync()" call becomes more important in case we
remove the upper bound otherwise test might not provide reliable
results.

[1] http://lists.linux.it/pipermail/ltp/2019-October/014157.html

-Sumit

>
> > Let's remove the toplimit of write back, and think as long as we synced
> > at least the expected amount, the test passes. The +10% limit seems
> > arbitrary.
>
> I think this is reasonable approach until we find better way
> to measure what was synced.
>
> Acked-by: Jan Stancek <jstancek@redhat.com>
>
>
> --
> Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH RFC] sync_file_range02: remove the toplimit of write back

[Li Wang]

Hi Sumit,

On Wed, Dec 18, 2019 at 6:14 PM Sumit Garg <sumit.garg@linaro.org> wrote:

> + Cyril (who originally proposed upper bound check)
>
> On Wed, 18 Dec 2019 at 14:55, Jan Stancek <jstancek@redhat.com> wrote:
> >
> >
> > ----- Original Message -----
> > > " The test's assumptions are fundamentally false; it thinks it can look
> > > at IO counters (tst_dev_bytes_written) for a disk before and after a
> > > system call, and attribute all of the IO seen to the system call that
> > > was made - this isn't necessarily correct. Other processes may generate
> > > IO in the background.
> >
> > We create our own block device, so there shouldn't be other processes
> > writing to it.
> >
> > > ext4 defers a lot of IO on a freshly made filesystem to the kernel -
> > > for example it will initialize the journal and inode tables after the
> > > mount
> >
> > Journal was my guess as well.
>
> To avoid similar scenarios, I suggested to add a "sync()" call just
> prior to test here [1]. And I couldn't reproduce the failure in
> 1000-times run with 4.19 kernel.
>

Yes, that makes sense to me.


>
> Also, I think having a "sync()" call becomes more important in case we
> remove the upper bound otherwise test might not provide reliable
> results.
>
> [1] http://lists.linux.it/pipermail/ltp/2019-October/014157.html
>
> -Sumit
>
> >
> > > Let's remove the toplimit of write back, and think as long as we synced
> > > at least the expected amount, the test passes. The +10% limit seems
> > > arbitrary.
> >
> > I think this is reasonable approach until we find better way
> > to measure what was synced.
> >
> > Acked-by: Jan Stancek <jstancek@redhat.com>
> >
> >
> > --
> > Mailing list info: https://lists.linux.it/listinfo/ltp
>
>

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

[LTP] [PATCH RFC] sync_file_range02: remove the toplimit of write back

[Sumit Garg]

Hi Li,

On Thu, 19 Dec 2019 at 12:40, Li Wang <liwang@redhat.com> wrote:
>
> Hi Sumit,
>
> On Wed, Dec 18, 2019 at 6:14 PM Sumit Garg <sumit.garg@linaro.org> wrote:
>>
>> + Cyril (who originally proposed upper bound check)
>>
>> On Wed, 18 Dec 2019 at 14:55, Jan Stancek <jstancek@redhat.com> wrote:
>> >
>> >
>> > ----- Original Message -----
>> > > " The test's assumptions are fundamentally false; it thinks it can look
>> > > at IO counters (tst_dev_bytes_written) for a disk before and after a
>> > > system call, and attribute all of the IO seen to the system call that
>> > > was made - this isn't necessarily correct. Other processes may generate
>> > > IO in the background.
>> >
>> > We create our own block device, so there shouldn't be other processes
>> > writing to it.
>> >
>> > > ext4 defers a lot of IO on a freshly made filesystem to the kernel -
>> > > for example it will initialize the journal and inode tables after the
>> > > mount
>> >
>> > Journal was my guess as well.
>>
>> To avoid similar scenarios, I suggested to add a "sync()" call just
>> prior to test here [1]. And I couldn't reproduce the failure in
>> 1000-times run with 4.19 kernel.
>
>
> Yes, that makes sense to me.
>

Would you mind to give it a try and check if you could reproduce the failure?

-Sumit

>>
>>
>> Also, I think having a "sync()" call becomes more important in case we
>> remove the upper bound otherwise test might not provide reliable
>> results.
>>
>> [1] http://lists.linux.it/pipermail/ltp/2019-October/014157.html
>>
>> -Sumit
>>
>> >
>> > > Let's remove the toplimit of write back, and think as long as we synced
>> > > at least the expected amount, the test passes. The +10% limit seems
>> > > arbitrary.
>> >
>> > I think this is reasonable approach until we find better way
>> > to measure what was synced.
>> >
>> > Acked-by: Jan Stancek <jstancek@redhat.com>
>> >
>> >
>> > --
>> > Mailing list info: https://lists.linux.it/listinfo/ltp
>>
>
>
> --
> Regards,
> Li Wang

[LTP] [PATCH RFC] sync_file_range02: remove the toplimit of write back

[Li Wang]

On Thu, Dec 19, 2019 at 4:52 PM Sumit Garg <sumit.garg@linaro.org> wrote:

> ...
> >> To avoid similar scenarios, I suggested to add a "sync()" call just
> >> prior to test here [1]. And I couldn't reproduce the failure in
> >> 1000-times run with 4.19 kernel.
> >
> >
> > Yes, that makes sense to me.
> >
>
> Would you mind to give it a try and check if you could reproduce the
> failure?
>

Actually, I haven't had one time to reproduce it. This failure was reported
by CI jobs and very low frequency occurred. The fix is based on a
discussion with FS developer analysis.

But I would have a try with this patch(+ sync()).

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

[LTP] [PATCH RFC] sync_file_range02: remove the toplimit of write back

[Li Wang]

Hi,

On Thu, Dec 19, 2019 at 5:15 PM Li Wang <liwang@redhat.com> wrote:

>
>
> On Thu, Dec 19, 2019 at 4:52 PM Sumit Garg <sumit.garg@linaro.org> wrote:
>
>> ...
>> >> To avoid similar scenarios, I suggested to add a "sync()" call just
>> >> prior to test here [1]. And I couldn't reproduce the failure in
>> >> 1000-times run with 4.19 kernel.
>> >
>> >
>> > Yes, that makes sense to me.
>> >
>>
>> Would you mind to give it a try and check if you could reproduce the
>> failure?
>>
>
> Actually, I haven't had one time to reproduce it. This failure was
> reported by CI jobs and very low frequency occurred. The fix is based on a
> discussion with FS developer analysis.
>
> But I would have a try with this patch(+ sync()).
>

I have tried this for many different systems, and it doesn't hit the
failure at least one time. So if no more comments, I would merge the patch
as below in the next step.

--- a/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c
+++ b/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c
@@ -48,6 +48,8 @@ static void verify_sync_file_range(struct testcase *tc)

        lseek(fd, tc->write_off, SEEK_SET);

+       sync();
+
        tst_dev_bytes_written(tst_device->dev);

        tst_fill_fd(fd, 0, TST_MB, tc->write_size_mb);
@@ -66,8 +68,7 @@ static void verify_sync_file_range(struct testcase *tc)

        SAFE_CLOSE(fd);

-       if ((written >= tc->exp_sync_size) &&
-           (written <= (tc->exp_sync_size + tc->exp_sync_size/10)))
+       if (written >= tc->exp_sync_size)
                tst_res(TPASS, "%s", tc->desc);
        else
                tst_res(TFAIL, "%s: Synced %li, expected %li", tc->desc,

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

[LTP] [PATCH RFC] sync_file_range02: remove the toplimit of write back

[Sumit Garg]

On Tue, 31 Dec 2019 at 10:55, Li Wang <liwang@redhat.com> wrote:
>
> Hi,
>
> On Thu, Dec 19, 2019 at 5:15 PM Li Wang <liwang@redhat.com> wrote:
>>
>>
>>
>> On Thu, Dec 19, 2019 at 4:52 PM Sumit Garg <sumit.garg@linaro.org> wrote:
>>>
>>> ...
>>> >> To avoid similar scenarios, I suggested to add a "sync()" call just
>>> >> prior to test here [1]. And I couldn't reproduce the failure in
>>> >> 1000-times run with 4.19 kernel.
>>> >
>>> >
>>> > Yes, that makes sense to me.
>>> >
>>>
>>> Would you mind to give it a try and check if you could reproduce the failure?
>>
>>
>> Actually, I haven't had one time to reproduce it. This failure was reported by CI jobs and very low frequency occurred. The fix is based on a discussion with FS developer analysis.
>>
>> But I would have a try with this patch(+ sync()).
>
>
> I have tried this for many different systems, and it doesn't hit the failure at least one time. So if no more comments, I would merge the patch as below in the next step.
>
> --- a/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c
> +++ b/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c
> @@ -48,6 +48,8 @@ static void verify_sync_file_range(struct testcase *tc)
>
>         lseek(fd, tc->write_off, SEEK_SET);
>
> +       sync();
> +
>         tst_dev_bytes_written(tst_device->dev);
>
>         tst_fill_fd(fd, 0, TST_MB, tc->write_size_mb);
> @@ -66,8 +68,7 @@ static void verify_sync_file_range(struct testcase *tc)
>
>         SAFE_CLOSE(fd);
>
> -       if ((written >= tc->exp_sync_size) &&
> -           (written <= (tc->exp_sync_size + tc->exp_sync_size/10)))
> +       if (written >= tc->exp_sync_size)
>                 tst_res(TPASS, "%s", tc->desc);
>         else
>                 tst_res(TFAIL, "%s: Synced %li, expected %li", tc->desc,
>

Reviewed-by: Sumit Garg <sumit.garg@linaro.org>

> --
> Regards,
> Li Wang

[LTP] [PATCH RFC] sync_file_range02: remove the toplimit of write back

[Yang Xu]

Hi Li

on 2019/12/31 13:25, Li Wang wrote:
> Hi,
>
> On Thu, Dec 19, 2019 at 5:15 PM Li Wang <liwang@redhat.com 
> <mailto:liwang@redhat.com>> wrote:
>
>
>
>     On Thu, Dec 19, 2019 at 4:52 PM Sumit Garg <sumit.garg@linaro.org
>     <mailto:sumit.garg@linaro.org>> wrote:
>
>         ...
>         >> To avoid similar scenarios, I suggested to add a "sync()"
>         call just
>         >> prior to test here [1]. And I couldn't reproduce the failure in
>         >> 1000-times run with 4.19 kernel.
>         >
>         >
>         > Yes, that makes sense to me.
>         >
>
>         Would you mind to give it a try and check if you could
>         reproduce the failure?
>
>
>     Actually, I haven't had one time to reproduce it. This failure was
>     reported by CI jobs and very low frequency occurred. The fix is
>     based on a discussion?with FS developer analysis.
>
>     But I would have a try with this patch(+ sync()).
>
>
> I have tried this for many different systems, and it doesn't hit the 
> failure at least one time. So if no more comments, I would merge the 
> patch as below in the next step.

I have a question.we must call sync()? I think syncfs is more accurate.

Best Regards
Yang Xu

>
> --- a/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c
> +++ b/testcases/kernel/syscalls/sync_file_range/sync_file_range02.c
> @@ -48,6 +48,8 @@ static void verify_sync_file_range(struct testcase *tc)
>
> ? ? ? ? lseek(fd, tc->write_off, SEEK_SET);
>
> + ? ? ? sync();
> +
> ? ? ? ? tst_dev_bytes_written(tst_device->dev);
>
> ? ? ? ? tst_fill_fd(fd, 0, TST_MB, tc->write_size_mb);
> @@ -66,8 +68,7 @@ static void verify_sync_file_range(struct testcase *tc)
>
> ? ? ? ? SAFE_CLOSE(fd);
>
> - ? ? ? if ((written >= tc->exp_sync_size) &&
> - ? ? ? ? ? (written <= (tc->exp_sync_size + tc->exp_sync_size/10)))
> + ? ? ? if (written >= tc->exp_sync_size)
> ? ? ? ? ? ? ? ? tst_res(TPASS, "%s", tc->desc);
> ? ? ? ? else
> ? ? ? ? ? ? ? ? tst_res(TFAIL, "%s: Synced %li, expected %li", tc->desc,
>
> -- 
> Regards,
> Li Wang
>


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20191231/eb79f954/attachment.htm>

[LTP] [PATCH RFC] sync_file_range02: remove the toplimit of write back

[Li Wang]

Hi Yang,

Yang Xu <xuyang2018.jy@cn.fujitsu.com> wrote:

> ...
>
>
> I have tried this for many different systems, and it doesn't hit the
> failure at least one time. So if no more comments, I would merge the patch
> as below in the next step.
>
> I have a question.we must call sync()? I think syncfs is more accurate.
>
>
Here we use sync() is to make fs metadata/cache being written back before
testing because there is no obtainable file descriptor 'fd' for the ext4
deferred IO (e.g. initialize the journal and inode tables).

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

[LTP] [PATCH RFC] sync_file_range02: remove the toplimit of write back

[Yang Xu]

Hi Li
> Hi Yang,
> 
> Yang Xu <xuyang2018.jy@cn.fujitsu.com 
> <mailto:xuyang2018.jy@cn.fujitsu.com>> wrote:
> 
>     ...
> 
>>
>>     I have tried this for many different systems, and it doesn't hit
>>     the failure at least one time. So if no more comments, I would
>>     merge the patch as below in the next step.
> 
>     I have a question.we must call sync()? I think syncfs is more accurate.
> 
> 
> Here we use sync() is to make fs metadata/cache being written back 
> before testing because there is no obtainable?file descriptor 'fd' for 
> the ext4 deferred?IO (e.g. initialize the journal and inode tables).
I see. For other test cases using tst_dev_bytes_written api such as 
fdatasync03.c,  we also need to call sync() to make this written value 
more accurate.  What do you think about it?

Best Regards
Yang Xu
> 
> -- 
> Regards,
> Li Wang

[LTP] [PATCH RFC] sync_file_range02: remove the toplimit of write back

[Li Wang]

On Tue, Dec 31, 2019 at 3:05 PM Yang Xu <xuyang2018.jy@cn.fujitsu.com>
wrote:

> ...
> >>     I have tried this for many different systems, and it doesn't hit
> >>     the failure at least one time. So if no more comments, I would
> >>     merge the patch as below in the next step.
> >
> >     I have a question.we must call sync()? I think syncfs is more
> accurate.
> >
> >
> > Here we use sync() is to make fs metadata/cache being written back
> > before testing because there is no obtainable file descriptor 'fd' for
> > the ext4 deferred IO (e.g. initialize the journal and inode tables).
>


> I see. For other test cases using tst_dev_bytes_written api such as
> fdatasync03.c,  we also need to call sync() to make this written value
> more accurate.  What do you think about it?
>

Good point. I think we should take care of them as we do for
sync_file_range02 too. It will more easily report fail in the case of a
situation that synced data is less than expected.

$ git grep tst_dev_bytes_written
kernel/syscalls/fdatasync/fdatasync03.c:
 tst_dev_bytes_written(tst_device->dev);
kernel/syscalls/fdatasync/fdatasync03.c:        written =
tst_dev_bytes_written(tst_device->dev);
kernel/syscalls/fsync/fsync04.c:
 tst_dev_bytes_written(tst_device->dev);
kernel/syscalls/fsync/fsync04.c:        written =
tst_dev_bytes_written(tst_device->dev);
kernel/syscalls/sync/sync03.c:  tst_dev_bytes_written(tst_device->dev);
kernel/syscalls/sync/sync03.c:  written =
tst_dev_bytes_written(tst_device->dev);
kernel/syscalls/sync_file_range/sync_file_range02.c:
 tst_dev_bytes_written(tst_device->dev);
kernel/syscalls/sync_file_range/sync_file_range02.c:    written =
tst_dev_bytes_written(tst_device->dev);
kernel/syscalls/syncfs/syncfs01.c:
 tst_dev_bytes_written(tst_device->dev);
kernel/syscalls/syncfs/syncfs01.c:      written =
tst_dev_bytes_written(tst_device->dev);

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

[LTP] [PATCH RFC] sync_file_range02: remove the toplimit of write back

[Sumit Garg]

On Tue, 31 Dec 2019 at 13:02, Li Wang <liwang@redhat.com> wrote:
>
>
>
> On Tue, Dec 31, 2019 at 3:05 PM Yang Xu <xuyang2018.jy@cn.fujitsu.com> wrote:
>>
>> ...
>> >>     I have tried this for many different systems, and it doesn't hit
>> >>     the failure at least one time. So if no more comments, I would
>> >>     merge the patch as below in the next step.
>> >
>> >     I have a question.we must call sync()? I think syncfs is more accurate.
>> >
>> >
>> > Here we use sync() is to make fs metadata/cache being written back
>> > before testing because there is no obtainable file descriptor 'fd' for
>> > the ext4 deferred IO (e.g. initialize the journal and inode tables).
>
>
>>
>> I see. For other test cases using tst_dev_bytes_written api such as
>> fdatasync03.c,  we also need to call sync() to make this written value
>> more accurate.  What do you think about it?
>
>
> Good point. I think we should take care of them as we do for sync_file_range02 too. It will more easily report fail in the case of a situation that synced data is less than expected.
>
> $ git grep tst_dev_bytes_written
> kernel/syscalls/fdatasync/fdatasync03.c:        tst_dev_bytes_written(tst_device->dev);
> kernel/syscalls/fdatasync/fdatasync03.c:        written = tst_dev_bytes_written(tst_device->dev);
> kernel/syscalls/fsync/fsync04.c:        tst_dev_bytes_written(tst_device->dev);
> kernel/syscalls/fsync/fsync04.c:        written = tst_dev_bytes_written(tst_device->dev);
> kernel/syscalls/sync/sync03.c:  tst_dev_bytes_written(tst_device->dev);
> kernel/syscalls/sync/sync03.c:  written = tst_dev_bytes_written(tst_device->dev);
> kernel/syscalls/sync_file_range/sync_file_range02.c:    tst_dev_bytes_written(tst_device->dev);
> kernel/syscalls/sync_file_range/sync_file_range02.c:    written = tst_dev_bytes_written(tst_device->dev);
> kernel/syscalls/syncfs/syncfs01.c:      tst_dev_bytes_written(tst_device->dev);
> kernel/syscalls/syncfs/syncfs01.c:      written = tst_dev_bytes_written(tst_device->dev);
>

Agree.

-Sumit

> --
> Regards,
> Li Wang

[LTP] [PATCH RFC] sync_file_range02: remove the toplimit of write back

[Li Wang]

On Tue, Dec 31, 2019 at 1:31 PM Sumit Garg <sumit.garg@linaro.org> wrote:

> ...
>
> Reviewed-by: Sumit Garg <sumit.garg@linaro.org>
>

Pushed.

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