[RFC] generic: test negative timespecs are accurate

Commit Message

Alyssa Ross Sept. 7, 2024, 3:45 p.m. UTC

Link: https://github.com/koverstreet/bcachefs/issues/743
Signed-off-by: Alyssa Ross <[e-mail]>
---
This is an adapted version of generic/258, but it tests that the stored 
timestamp is accurate to the second, rather than just testing the 
timestamp remains negative.  I created a new test rather than just
making 258 more precise, because I understand that there may be
filesystems that don't store timestamps that accurately by design.
As an example, I've heard that FAT only has 2 second precision, so this
patch is an RFC because I'm not sure how I should write a _require
function (if at all) that restricts the test to filesystems that are
expected to be able to do this.

 tests/generic/363     | 26 ++++++++++++++++++++++++++
 tests/generic/363.out |  2 ++
 2 files changed, 28 insertions(+)
 create mode 100755 tests/generic/363
 create mode 100644 tests/generic/363.out

Comments

Zorro Lang Sept. 9, 2024, 8:01 a.m. UTC | #1

On Sat, Sep 07, 2024 at 05:45:28PM +0200, Alyssa Ross wrote:
> Link: https://github.com/koverstreet/bcachefs/issues/743

Great, a bcachefs regression test case :)

Can you add a bit more details in commit log, not only a link.

> Signed-off-by: Alyssa Ross <[e-mail]>
> ---
> This is an adapted version of generic/258, but it tests that the stored 
> timestamp is accurate to the second, rather than just testing the 
> timestamp remains negative.  I created a new test rather than just
> making 258 more precise, because I understand that there may be
> filesystems that don't store timestamps that accurately by design.
> As an example, I've heard that FAT only has 2 second precision, so this
> patch is an RFC because I'm not sure how I should write a _require
> function (if at all) that restricts the test to filesystems that are
> expected to be able to do this.
> 
>  tests/generic/363     | 26 ++++++++++++++++++++++++++
>  tests/generic/363.out |  2 ++

The g/363 has been taken, please rebase to latest for-next branch.
You can use `xfstests/tools/mvtest generic/363 generic/365` to change
the case number, before rebasing.

>  2 files changed, 28 insertions(+)
>  create mode 100755 tests/generic/363
>  create mode 100644 tests/generic/363.out
> 
> diff --git a/tests/generic/363 b/tests/generic/363
> new file mode 100755
> index 00000000..50459d01
> --- /dev/null
> +++ b/tests/generic/363
> @@ -0,0 +1,26 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2011 Red Hat, Inc.  All Rights Reserved.
> +# Copyright (c) 2024 Alyssa Ross.  All Rights Reserved.
> +#
> +# FS QA Test 363
> +#
> +# Test timestamps prior to epoch with nanosecond components are
> +# accurate to the second.
> +# bcachefs was slightly off.
> +#
> +. ./common/preamble
> +_begin_fstest auto quick bigtime
> +

As this's a bcachefs regression test:
https://lore.kernel.org/linux-bcachefs/[e-mail]/

So better to mark as:
if [ "$FSTYP" = "bcachefs" ];then
	_fixed_by_kernel_commit xxxxxxxxxxxx "bcachefs: Fix negative timespecs"
fi
(replace the xxxxxxx if it's merged on mainline linux)

Others looks good to me, with above changes, I'd like to

Reviewed-by: Zorro Lang <[e-mail]>

> +_require_test
> +_require_negative_timestamps
> +
> +TESTFILE=$TEST_DIR/timestamp-test.txt
> +
> +# Create a file with a timestamp prior to the epoch
> +touch -d "1969-07-20T03:55:59.750000000+0100" $TESTFILE
> +stat -c %X $TESTFILE
> +
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/generic/363.out b/tests/generic/363.out
> new file mode 100644
> index 00000000..052bacfd
> --- /dev/null
> +++ b/tests/generic/363.out
> @@ -0,0 +1,2 @@
> +QA output created by 363
> +-14245441
> -- 
> 2.45.2
> 
>

Alyssa Ross Sept. 10, 2024, 10:02 a.m. UTC | #2

Zorro Lang <[e-mail]> writes:

> On Sat, Sep 07, 2024 at 05:45:28PM +0200, Alyssa Ross wrote:
>> Link: https://github.com/koverstreet/bcachefs/issues/743
>
> Great, a bcachefs regression test case :)
>
> Can you add a bit more details in commit log, not only a link.
>
>> Signed-off-by: Alyssa Ross <[e-mail]>
>> ---
>> This is an adapted version of generic/258, but it tests that the stored 
>> timestamp is accurate to the second, rather than just testing the 
>> timestamp remains negative.  I created a new test rather than just
>> making 258 more precise, because I understand that there may be
>> filesystems that don't store timestamps that accurately by design.
>> As an example, I've heard that FAT only has 2 second precision, so this
>> patch is an RFC because I'm not sure how I should write a _require
>> function (if at all) that restricts the test to filesystems that are
>> expected to be able to do this.
>> 
>>  tests/generic/363     | 26 ++++++++++++++++++++++++++
>>  tests/generic/363.out |  2 ++
>
> The g/363 has been taken, please rebase to latest for-next branch.
> You can use `xfstests/tools/mvtest generic/363 generic/365` to change
> the case number, before rebasing.
>
>>  2 files changed, 28 insertions(+)
>>  create mode 100755 tests/generic/363
>>  create mode 100644 tests/generic/363.out
>> 
>> diff --git a/tests/generic/363 b/tests/generic/363
>> new file mode 100755
>> index 00000000..50459d01
>> --- /dev/null
>> +++ b/tests/generic/363
>> @@ -0,0 +1,26 @@
>> +#! /bin/bash
>> +# SPDX-License-Identifier: GPL-2.0
>> +# Copyright (c) 2011 Red Hat, Inc.  All Rights Reserved.
>> +# Copyright (c) 2024 Alyssa Ross.  All Rights Reserved.
>> +#
>> +# FS QA Test 363
>> +#
>> +# Test timestamps prior to epoch with nanosecond components are
>> +# accurate to the second.
>> +# bcachefs was slightly off.
>> +#
>> +. ./common/preamble
>> +_begin_fstest auto quick bigtime
>> +
>
> As this's a bcachefs regression test:
> https://lore.kernel.org/linux-bcachefs/[e-mail]/
>
> So better to mark as:
> if [ "$FSTYP" = "bcachefs" ];then
> 	_fixed_by_kernel_commit xxxxxxxxxxxx "bcachefs: Fix negative timespecs"
> fi
> (replace the xxxxxxx if it's merged on mainline linux)
>
> Others looks good to me, with above changes, I'd like to
>
> Reviewed-by: Zorro Lang <[e-mail]>

Thanks!  So you don't think the test needs to express any extra
requirement that filesystems support to-the-second precision?

Zorro Lang Sept. 10, 2024, 1:52 p.m. UTC | #3

On Tue, Sep 10, 2024 at 12:02:05PM +0200, Alyssa Ross wrote:
> Zorro Lang <[e-mail]> writes:
> 
> > On Sat, Sep 07, 2024 at 05:45:28PM +0200, Alyssa Ross wrote:
> >> Link: https://github.com/koverstreet/bcachefs/issues/743
> >
> > Great, a bcachefs regression test case :)
> >
> > Can you add a bit more details in commit log, not only a link.
> >
> >> Signed-off-by: Alyssa Ross <[e-mail]>
> >> ---
> >> This is an adapted version of generic/258, but it tests that the stored 
> >> timestamp is accurate to the second, rather than just testing the 
> >> timestamp remains negative.  I created a new test rather than just
> >> making 258 more precise, because I understand that there may be
> >> filesystems that don't store timestamps that accurately by design.
> >> As an example, I've heard that FAT only has 2 second precision, so this
> >> patch is an RFC because I'm not sure how I should write a _require
> >> function (if at all) that restricts the test to filesystems that are
> >> expected to be able to do this.
> >> 
> >>  tests/generic/363     | 26 ++++++++++++++++++++++++++
> >>  tests/generic/363.out |  2 ++
> >
> > The g/363 has been taken, please rebase to latest for-next branch.
> > You can use `xfstests/tools/mvtest generic/363 generic/365` to change
> > the case number, before rebasing.
> >
> >>  2 files changed, 28 insertions(+)
> >>  create mode 100755 tests/generic/363
> >>  create mode 100644 tests/generic/363.out
> >> 
> >> diff --git a/tests/generic/363 b/tests/generic/363
> >> new file mode 100755
> >> index 00000000..50459d01
> >> --- /dev/null
> >> +++ b/tests/generic/363
> >> @@ -0,0 +1,26 @@
> >> +#! /bin/bash
> >> +# SPDX-License-Identifier: GPL-2.0
> >> +# Copyright (c) 2011 Red Hat, Inc.  All Rights Reserved.
> >> +# Copyright (c) 2024 Alyssa Ross.  All Rights Reserved.
> >> +#
> >> +# FS QA Test 363
> >> +#
> >> +# Test timestamps prior to epoch with nanosecond components are
> >> +# accurate to the second.
> >> +# bcachefs was slightly off.
> >> +#
> >> +. ./common/preamble
> >> +_begin_fstest auto quick bigtime
> >> +
> >
> > As this's a bcachefs regression test:
> > https://lore.kernel.org/linux-bcachefs/[e-mail]/
> >
> > So better to mark as:
> > if [ "$FSTYP" = "bcachefs" ];then
> > 	_fixed_by_kernel_commit xxxxxxxxxxxx "bcachefs: Fix negative timespecs"
> > fi
> > (replace the xxxxxxx if it's merged on mainline linux)
> >
> > Others looks good to me, with above changes, I'd like to
> >
> > Reviewed-by: Zorro Lang <[e-mail]>
> 
> Thanks!  So you don't think the test needs to express any extra
> requirement that filesystems support to-the-second precision?

Oh, does this test needs 1 second precision? I thought you concerned some
filesystems cannot be the epoch you set. The _require_negative_timestamps
will skip the test from exfat and ceph. It doesn't help the FAT, maybe you
want to add vfat to _require_negative_timestamps.

I thought you might want to output the stored timestamp (accurate to the
second) to reproduce this bug, if not, why you hope to write this new one,
not use the g/258 to be the reproducer directly?

Thanks,
Zorro

Alyssa Ross Sept. 10, 2024, 3:04 p.m. UTC | #4

Zorro Lang <[e-mail]> writes:

> On Tue, Sep 10, 2024 at 12:02:05PM +0200, Alyssa Ross wrote:
>> Zorro Lang <[e-mail]> writes:
>> 
>> > On Sat, Sep 07, 2024 at 05:45:28PM +0200, Alyssa Ross wrote:
>> >> Link: https://github.com/koverstreet/bcachefs/issues/743
>> >
>> > Great, a bcachefs regression test case :)
>> >
>> > Can you add a bit more details in commit log, not only a link.
>> >
>> >> Signed-off-by: Alyssa Ross <[e-mail]>
>> >> ---
>> >> This is an adapted version of generic/258, but it tests that the stored 
>> >> timestamp is accurate to the second, rather than just testing the 
>> >> timestamp remains negative.  I created a new test rather than just
>> >> making 258 more precise, because I understand that there may be
>> >> filesystems that don't store timestamps that accurately by design.
>> >> As an example, I've heard that FAT only has 2 second precision, so this
>> >> patch is an RFC because I'm not sure how I should write a _require
>> >> function (if at all) that restricts the test to filesystems that are
>> >> expected to be able to do this.
>> >> 
>> >>  tests/generic/363     | 26 ++++++++++++++++++++++++++
>> >>  tests/generic/363.out |  2 ++
>> >
>> > The g/363 has been taken, please rebase to latest for-next branch.
>> > You can use `xfstests/tools/mvtest generic/363 generic/365` to change
>> > the case number, before rebasing.
>> >
>> >>  2 files changed, 28 insertions(+)
>> >>  create mode 100755 tests/generic/363
>> >>  create mode 100644 tests/generic/363.out
>> >> 
>> >> diff --git a/tests/generic/363 b/tests/generic/363
>> >> new file mode 100755
>> >> index 00000000..50459d01
>> >> --- /dev/null
>> >> +++ b/tests/generic/363
>> >> @@ -0,0 +1,26 @@
>> >> +#! /bin/bash
>> >> +# SPDX-License-Identifier: GPL-2.0
>> >> +# Copyright (c) 2011 Red Hat, Inc.  All Rights Reserved.
>> >> +# Copyright (c) 2024 Alyssa Ross.  All Rights Reserved.
>> >> +#
>> >> +# FS QA Test 363
>> >> +#
>> >> +# Test timestamps prior to epoch with nanosecond components are
>> >> +# accurate to the second.
>> >> +# bcachefs was slightly off.
>> >> +#
>> >> +. ./common/preamble
>> >> +_begin_fstest auto quick bigtime
>> >> +
>> >
>> > As this's a bcachefs regression test:
>> > https://lore.kernel.org/linux-bcachefs/[e-mail]/
>> >
>> > So better to mark as:
>> > if [ "$FSTYP" = "bcachefs" ];then
>> > 	_fixed_by_kernel_commit xxxxxxxxxxxx "bcachefs: Fix negative timespecs"
>> > fi
>> > (replace the xxxxxxx if it's merged on mainline linux)
>> >
>> > Others looks good to me, with above changes, I'd like to
>> >
>> > Reviewed-by: Zorro Lang <[e-mail]>
>> 
>> Thanks!  So you don't think the test needs to express any extra
>> requirement that filesystems support to-the-second precision?
>
> Oh, does this test needs 1 second precision? I thought you concerned some
> filesystems cannot be the epoch you set. The _require_negative_timestamps
> will skip the test from exfat and ceph. It doesn't help the FAT, maybe you
> want to add vfat to _require_negative_timestamps.
>
> I thought you might want to output the stored timestamp (accurate to the
> second) to reproduce this bug, if not, why you hope to write this new one,
> not use the g/258 to be the reproducer directly?

I'm sorry, I don't understand the question.  I explained why I created a
new test in my commentary — g/258 doesn't require 1 second precision,
and this test does.

Zorro Lang Sept. 14, 2024, 4:58 a.m. UTC | #5

On Tue, Sep 10, 2024 at 05:04:03PM +0200, Alyssa Ross wrote:
> Zorro Lang <[e-mail]> writes:
> 
> > On Tue, Sep 10, 2024 at 12:02:05PM +0200, Alyssa Ross wrote:
> >> Zorro Lang <[e-mail]> writes:
> >> 
> >> > On Sat, Sep 07, 2024 at 05:45:28PM +0200, Alyssa Ross wrote:
> >> >> Link: https://github.com/koverstreet/bcachefs/issues/743
> >> >
> >> > Great, a bcachefs regression test case :)
> >> >
> >> > Can you add a bit more details in commit log, not only a link.
> >> >
> >> >> Signed-off-by: Alyssa Ross <[e-mail]>
> >> >> ---
> >> >> This is an adapted version of generic/258, but it tests that the stored 
> >> >> timestamp is accurate to the second, rather than just testing the 
> >> >> timestamp remains negative.  I created a new test rather than just
> >> >> making 258 more precise, because I understand that there may be
> >> >> filesystems that don't store timestamps that accurately by design.
> >> >> As an example, I've heard that FAT only has 2 second precision, so this
> >> >> patch is an RFC because I'm not sure how I should write a _require
> >> >> function (if at all) that restricts the test to filesystems that are
> >> >> expected to be able to do this.
> >> >> 
> >> >>  tests/generic/363     | 26 ++++++++++++++++++++++++++
> >> >>  tests/generic/363.out |  2 ++
> >> >
> >> > The g/363 has been taken, please rebase to latest for-next branch.
> >> > You can use `xfstests/tools/mvtest generic/363 generic/365` to change
> >> > the case number, before rebasing.
> >> >
> >> >>  2 files changed, 28 insertions(+)
> >> >>  create mode 100755 tests/generic/363
> >> >>  create mode 100644 tests/generic/363.out
> >> >> 
> >> >> diff --git a/tests/generic/363 b/tests/generic/363
> >> >> new file mode 100755
> >> >> index 00000000..50459d01
> >> >> --- /dev/null
> >> >> +++ b/tests/generic/363
> >> >> @@ -0,0 +1,26 @@
> >> >> +#! /bin/bash
> >> >> +# SPDX-License-Identifier: GPL-2.0
> >> >> +# Copyright (c) 2011 Red Hat, Inc.  All Rights Reserved.
> >> >> +# Copyright (c) 2024 Alyssa Ross.  All Rights Reserved.
> >> >> +#
> >> >> +# FS QA Test 363
> >> >> +#
> >> >> +# Test timestamps prior to epoch with nanosecond components are
> >> >> +# accurate to the second.
> >> >> +# bcachefs was slightly off.
> >> >> +#
> >> >> +. ./common/preamble
> >> >> +_begin_fstest auto quick bigtime
> >> >> +
> >> >
> >> > As this's a bcachefs regression test:
> >> > https://lore.kernel.org/linux-bcachefs/[e-mail]/
> >> >
> >> > So better to mark as:
> >> > if [ "$FSTYP" = "bcachefs" ];then
> >> > 	_fixed_by_kernel_commit xxxxxxxxxxxx "bcachefs: Fix negative timespecs"
> >> > fi
> >> > (replace the xxxxxxx if it's merged on mainline linux)
> >> >
> >> > Others looks good to me, with above changes, I'd like to
> >> >
> >> > Reviewed-by: Zorro Lang <[e-mail]>
> >> 
> >> Thanks!  So you don't think the test needs to express any extra
> >> requirement that filesystems support to-the-second precision?
> >
> > Oh, does this test needs 1 second precision? I thought you concerned some
> > filesystems cannot be the epoch you set. The _require_negative_timestamps
> > will skip the test from exfat and ceph. It doesn't help the FAT, maybe you
> > want to add vfat to _require_negative_timestamps.
> >
> > I thought you might want to output the stored timestamp (accurate to the
> > second) to reproduce this bug, if not, why you hope to write this new one,
> > not use the g/258 to be the reproducer directly?
> 
> I'm sorry, I don't understand the question.  I explained why I created a
> new test in my commentary — g/258 doesn't require 1 second precision,
> and this test does.

Sorry, I mixed several questions together. I mean:

1) Can g/258 reproduce this bug. If not, that means you need the accurate
"-14245441" output, which might break the test on fs not support to-the-second
precision, right?

2) If the 1) is yes. Then I think this case doesn't need to care about
"fs not support to-the-second", as you say you don't have a good way to
"_require" that.

3) If you agree 2). I think most of filesystems (fstests supports) support
the to-the-second precision. And the _require_negative_timestamps() helper
can help to skip this test from some improper filesystems for this test.
If you concern vfat, you can add it beside the exfat in that helper.

4) If the 1) is right, the fs which doesn't support to-the-second precision
also can be added into the blacklist of "_supported_fs" now or later, e.g.
_supported_fs ^cifs.

Hope I explain clearly this time :)

Thanks,
Zorro

Patch

diff --git a/tests/generic/363 b/tests/generic/363
new file mode 100755
index 00000000..50459d01
--- /dev/null
+++ b/tests/generic/363
@@ -0,0 +1,26 @@ 
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2011 Red Hat, Inc.  All Rights Reserved.
+# Copyright (c) 2024 Alyssa Ross.  All Rights Reserved.
+#
+# FS QA Test 363
+#
+# Test timestamps prior to epoch with nanosecond components are
+# accurate to the second.
+# bcachefs was slightly off.
+#
+. ./common/preamble
+_begin_fstest auto quick bigtime
+
+_require_test
+_require_negative_timestamps
+
+TESTFILE=$TEST_DIR/timestamp-test.txt
+
+# Create a file with a timestamp prior to the epoch
+touch -d "1969-07-20T03:55:59.750000000+0100" $TESTFILE
+stat -c %X $TESTFILE
+
+# success, all done
+status=0
+exit
diff --git a/tests/generic/363.out b/tests/generic/363.out
new file mode 100644
index 00000000..052bacfd
--- /dev/null
+++ b/tests/generic/363.out
@@ -0,0 +1,2 @@ 
+QA output created by 363
+-14245441