[PATCH] generic/4[13,62]: restore TEST mount options

[PATCH] generic/4[13,62]: restore TEST mount options

[Omer Zilberberg]

These tests locally change the TEST_FS_MOUNT_OPTS/MOUNT_OPTIONS
environment variables, and run _test_cycle_mount. As a result, following
tests using the TEST mount point may start with different mount options,
depending on run order.

This patch restores the TEST mount point's options.

Signed-off-by: Omer Zilberberg <Omer.Zilberberg@netapp.com>
---
 tests/generic/413 | 7 +++++++
 tests/generic/462 | 5 +++++
 2 files changed, 12 insertions(+)

diff --git a/tests/generic/413 b/tests/generic/413
index a1cc51443..94633a4d4 100755
--- a/tests/generic/413
+++ b/tests/generic/413
@@ -136,6 +136,8 @@ fi
 _scratch_mkfs "$mkfs_opts" > /dev/null 2>&1
 
 # mount SCRATCH_DEV with dax option, TEST_DEV not
+mount_options_orig=$MOUNT_OPTIONS
+test_fs_mount_opts_orig=$TEST_FS_MOUNT_OPTS
 export MOUNT_OPTIONS=""
 export TEST_FS_MOUNT_OPTS=""
 _test_cycle_mount
@@ -147,6 +149,11 @@ tsize=$((128 * 1024 * 1024))
 
 do_tests
 
+# restore TEST_FS to original mount options
+export MOUNT_OPTIONS=$mount_options_orig
+export TEST_FS_MOUNT_OPTS=$test_fs_mount_opts_orig
+_test_cycle_mount
+
 # success, all done
 echo "Silence is golden"
 status=0
diff --git a/tests/generic/462 b/tests/generic/462
index a5d6c4f85..176035ddc 100755
--- a/tests/generic/462
+++ b/tests/generic/462
@@ -64,6 +64,7 @@ _scratch_mkfs >>$seqres.full 2>&1
 _scratch_mount "-o dax"
 
 # remount TEST_DEV wo/ dax
+test_fs_mount_opts_orig=$TEST_FS_MOUNT_OPTS
 export TEST_FS_MOUNT_OPTS=""
 _test_cycle_mount
 
@@ -90,6 +91,10 @@ md5_2="$(_md5_checksum $SCRATCH_MNT/readonlyfile)"
 
 [ "$md5_1" != "$md5_2" ] && echo "read only file changed"
 
+# restore TEST_FS to original mount options
+export TEST_FS_MOUNT_OPTS=$test_fs_mount_opts_orig
+_test_cycle_mount
+
 # success, all done
 status=0
 exit
-- 
2.13.6

Re: [PATCH] generic/4[13,62]: restore TEST mount options

[Dave Chinner]

On Mon, Oct 30, 2017 at 10:08:31AM +0200, Omer Zilberberg wrote:
> These tests locally change the TEST_FS_MOUNT_OPTS/MOUNT_OPTIONS
> environment variables, and run _test_cycle_mount. As a result, following
> tests using the TEST mount point may start with different mount options,
> depending on run order.

I don't think that's the case. The change of the environment
variable should only affect the current test process and it's
children. When the test exits, we go back to the environment of the
check process, where the TEST_FS_MOUNT_OPTS environment variable is
still correctly set, and all future tests inherit from that. i.e.:

$ export FOO=foo
$ echo $FOO
foo
$ bash
$ echo $FOO
foo
$ export FOO=bar
$ echo $FOO
bar
$ exit
$ echo $FOO
foo
$

And after each test, check runs _check_filesystems(), which cycles
the test mount, so for each new test process that is run they should
already start in the correct state...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] generic/4[13,62]: restore TEST mount options

[Eryu Guan]

On Tue, Oct 31, 2017 at 07:36:58AM +1100, Dave Chinner wrote:
> On Mon, Oct 30, 2017 at 10:08:31AM +0200, Omer Zilberberg wrote:
> > These tests locally change the TEST_FS_MOUNT_OPTS/MOUNT_OPTIONS
> > environment variables, and run _test_cycle_mount. As a result, following
> > tests using the TEST mount point may start with different mount options,
> > depending on run order.
> 
> I don't think that's the case. The change of the environment
> variable should only affect the current test process and it's
> children. When the test exits, we go back to the environment of the
> check process, where the TEST_FS_MOUNT_OPTS environment variable is
> still correctly set, and all future tests inherit from that. i.e.:
> 
> $ export FOO=foo
> $ echo $FOO
> foo
> $ bash
> $ echo $FOO
> foo
> $ export FOO=bar
> $ echo $FOO
> bar
> $ exit
> $ echo $FOO
> foo
> $
> 
> And after each test, check runs _check_filesystems(), which cycles
> the test mount, so for each new test process that is run they should
> already start in the correct state...

I agreed, the changing of variables in a sub-shell won't affect the
parent's copy, and check will restore the mounts with the untouched
options.

But the problem is that _check_test_fs() will cycle mount TEST_DEV with
MOUNT_OPTIONS not TEST_FS_MOUNT_OPTS, so if you have different mount
options set for TEST_DEV and SCRATCH_DEV, you'll see mount options
changed for TEST_DEV. e.g.

MOUNT_OPTIONS="-o dax" TEST_FS_MOUNT_OPTS="" ./check generic/413 generic/445
generic/445 mount TEST_DEV with "-o dax" too

MOUNT_OPTIONS="" TEST_FS_MOUNT_OPTS="-o dax" ./check generic/413 generic/445
generic/445 mount TEST_DEV without "-o dax"

MOUNT_OPTIONS="-o dax" TEST_FS_MOUNT_OPTS="-o dax" ./check generic/413 generic/445
both tests and both devices mount with "-o dax"

That's been discussed in this thread:
https://patchwork.kernel.org/patch/9742039/

Omer, can you please confirm if you're hitting this issue?

I think fixing _check_<fs>_filesystem() is the correct way. And I guess
we can refactor out a common function and call it in
_check_[xfs|btrfs|generic]_filesystem, pass the correct mount options
based on what device we're working on.

Thanks,
Eryu

Re: [PATCH] generic/4[13,62]: restore TEST mount options

[Omer Zilberberg]

On 10/31/2017 06:37 AM, Eryu Guan wrote:
> On Tue, Oct 31, 2017 at 07:36:58AM +1100, Dave Chinner wrote:
>> On Mon, Oct 30, 2017 at 10:08:31AM +0200, Omer Zilberberg wrote:
>>> These tests locally change the TEST_FS_MOUNT_OPTS/MOUNT_OPTIONS
>>> environment variables, and run _test_cycle_mount. As a result, following
>>> tests using the TEST mount point may start with different mount options,
>>> depending on run order.
>> I don't think that's the case. The change of the environment
>> variable should only affect the current test process and it's
>> children. When the test exits, we go back to the environment of the
>> check process, where the TEST_FS_MOUNT_OPTS environment variable is
>> still correctly set, and all future tests inherit from that. i.e.:
>>
>> $ export FOO=foo
>> $ echo $FOO
>> foo
>> $ bash
>> $ echo $FOO
>> foo
>> $ export FOO=bar
>> $ echo $FOO
>> bar
>> $ exit
>> $ echo $FOO
>> foo
>> $
>>
>> And after each test, check runs _check_filesystems(), which cycles
>> the test mount, so for each new test process that is run they should
>> already start in the correct state...
> I agreed, the changing of variables in a sub-shell won't affect the
> parent's copy, and check will restore the mounts with the untouched
> options.
>
> But the problem is that _check_test_fs() will cycle mount TEST_DEV with
> MOUNT_OPTIONS not TEST_FS_MOUNT_OPTS, so if you have different mount
> options set for TEST_DEV and SCRATCH_DEV, you'll see mount options
> changed for TEST_DEV. e.g.
>
> MOUNT_OPTIONS="-o dax" TEST_FS_MOUNT_OPTS="" ./check generic/413 generic/445
> generic/445 mount TEST_DEV with "-o dax" too
>
> MOUNT_OPTIONS="" TEST_FS_MOUNT_OPTS="-o dax" ./check generic/413 generic/445
> generic/445 mount TEST_DEV without "-o dax"
>
> MOUNT_OPTIONS="-o dax" TEST_FS_MOUNT_OPTS="-o dax" ./check generic/413 generic/445
> both tests and both devices mount with "-o dax"
>
> That's been discussed in this thread:
> https://patchwork.kernel.org/patch/9742039/
>
> Omer, can you please confirm if you're hitting this issue?
I'm not 100% that's the case, so I better describe my settings more clearly:
I have a debug mount option on my system to recover the FS from a backup.
When that flag is set, umount writes everything to the backup.
Mount restores from it, overwriting everything.

As long as generic/413 is not involved, everything works well.
All _test_cycle_mount() calls first back everything up on umount,
then restore upon mount. So I get the same FS contents.

But, consider generic/118 running after generic/413:
- generic/413 finishes with a mount point with no mount options
- generic/118 begins with restored TEST_FS_MOUNT_OPTS, as you've pointed out.
- some writes are performed to the FS
- next _test_cycle_mount:
  calls umount w/o backing up (debug flag previously unset by generic/413).
  calls mount WITH the debug flag, and recovers from an empty backup,
  deleting the earlier writes.
- subsequent md5sum fails on "No such file or directory", as FS is now empty.

> I think fixing _check_<fs>_filesystem() is the correct way. And I guess
> we can refactor out a common function and call it in
> _check_[xfs|btrfs|generic]_filesystem, pass the correct mount options
> based on what device we're working on.
If indeed we're talking about the same problem,
please let me know if you'd like me to prepare a different patch.
>
> Thanks,
> Eryu
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] generic/4[13,62]: restore TEST mount options

[Eryu Guan]

On Tue, Oct 31, 2017 at 12:25:51PM +0200, Omer Zilberberg wrote:
> 
> 
> On 10/31/2017 06:37 AM, Eryu Guan wrote:
> > On Tue, Oct 31, 2017 at 07:36:58AM +1100, Dave Chinner wrote:
> >> On Mon, Oct 30, 2017 at 10:08:31AM +0200, Omer Zilberberg wrote:
> >>> These tests locally change the TEST_FS_MOUNT_OPTS/MOUNT_OPTIONS
> >>> environment variables, and run _test_cycle_mount. As a result, following
> >>> tests using the TEST mount point may start with different mount options,
> >>> depending on run order.
> >> I don't think that's the case. The change of the environment
> >> variable should only affect the current test process and it's
> >> children. When the test exits, we go back to the environment of the
> >> check process, where the TEST_FS_MOUNT_OPTS environment variable is
> >> still correctly set, and all future tests inherit from that. i.e.:
> >>
> >> $ export FOO=foo
> >> $ echo $FOO
> >> foo
> >> $ bash
> >> $ echo $FOO
> >> foo
> >> $ export FOO=bar
> >> $ echo $FOO
> >> bar
> >> $ exit
> >> $ echo $FOO
> >> foo
> >> $
> >>
> >> And after each test, check runs _check_filesystems(), which cycles
> >> the test mount, so for each new test process that is run they should
> >> already start in the correct state...
> > I agreed, the changing of variables in a sub-shell won't affect the
> > parent's copy, and check will restore the mounts with the untouched
> > options.
> >
> > But the problem is that _check_test_fs() will cycle mount TEST_DEV with
> > MOUNT_OPTIONS not TEST_FS_MOUNT_OPTS, so if you have different mount
> > options set for TEST_DEV and SCRATCH_DEV, you'll see mount options
> > changed for TEST_DEV. e.g.
> >
> > MOUNT_OPTIONS="-o dax" TEST_FS_MOUNT_OPTS="" ./check generic/413 generic/445
> > generic/445 mount TEST_DEV with "-o dax" too
> >
> > MOUNT_OPTIONS="" TEST_FS_MOUNT_OPTS="-o dax" ./check generic/413 generic/445
> > generic/445 mount TEST_DEV without "-o dax"
> >
> > MOUNT_OPTIONS="-o dax" TEST_FS_MOUNT_OPTS="-o dax" ./check generic/413 generic/445
> > both tests and both devices mount with "-o dax"
> >
> > That's been discussed in this thread:
> > https://patchwork.kernel.org/patch/9742039/
> >
> > Omer, can you please confirm if you're hitting this issue?
> I'm not 100% that's the case, so I better describe my settings more clearly:
> I have a debug mount option on my system to recover the FS from a backup.
> When that flag is set, umount writes everything to the backup.
> Mount restores from it, overwriting everything.

If you're testing with setting your debug mount option to both
TEST_FS_MOUNT_OPTS and MOUNT_OPTIONS, and you still see the failure you
were seeing, then that's a different problem.

> 
> As long as generic/413 is not involved, everything works well.
> All _test_cycle_mount() calls first back everything up on umount,
> then restore upon mount. So I get the same FS contents.
> 
> But, consider generic/118 running after generic/413:
> - generic/413 finishes with a mount point with no mount options
> - generic/118 begins with restored TEST_FS_MOUNT_OPTS, as you've pointed out.
> - some writes are performed to the FS
> - next _test_cycle_mount:
>   calls umount w/o backing up (debug flag previously unset by generic/413).

Does this clear the backup too? If so, I suspect TEST_DEV got cleared on
first mount with the debug option in generic/118, because the backup has
been cleared in the _test_cycle_mount call in generic/413.

>   calls mount WITH the debug flag, and recovers from an empty backup,
>   deleting the earlier writes.
> - subsequent md5sum fails on "No such file or directory", as FS is now empty.
> 
> > I think fixing _check_<fs>_filesystem() is the correct way. And I guess
> > we can refactor out a common function and call it in
> > _check_[xfs|btrfs|generic]_filesystem, pass the correct mount options
> > based on what device we're working on.
> If indeed we're talking about the same problem,
> please let me know if you'd like me to prepare a different patch.

Sure, really appreciated if you can prepare a different patch, even if
it's not the same problem :)

Thanks,
Eryu

Re: [PATCH] generic/4[13,62]: restore TEST mount options

[Dave Chinner]

On Tue, Oct 31, 2017 at 12:37:58PM +0800, Eryu Guan wrote:
> On Tue, Oct 31, 2017 at 07:36:58AM +1100, Dave Chinner wrote:
> > On Mon, Oct 30, 2017 at 10:08:31AM +0200, Omer Zilberberg wrote:
> > > These tests locally change the TEST_FS_MOUNT_OPTS/MOUNT_OPTIONS
> > > environment variables, and run _test_cycle_mount. As a result, following
> > > tests using the TEST mount point may start with different mount options,
> > > depending on run order.
> > 
> > I don't think that's the case. The change of the environment
> > variable should only affect the current test process and it's
> > children. When the test exits, we go back to the environment of the
> > check process, where the TEST_FS_MOUNT_OPTS environment variable is
> > still correctly set, and all future tests inherit from that. i.e.:
> > 
> > $ export FOO=foo
> > $ echo $FOO
> > foo
> > $ bash
> > $ echo $FOO
> > foo
> > $ export FOO=bar
> > $ echo $FOO
> > bar
> > $ exit
> > $ echo $FOO
> > foo
> > $
> > 
> > And after each test, check runs _check_filesystems(), which cycles
> > the test mount, so for each new test process that is run they should
> > already start in the correct state...
> 
> I agreed, the changing of variables in a sub-shell won't affect the
> parent's copy, and check will restore the mounts with the untouched
> options.
> 
> But the problem is that _check_test_fs() will cycle mount TEST_DEV with
> MOUNT_OPTIONS not TEST_FS_MOUNT_OPTS, so if you have different mount
> options set for TEST_DEV and SCRATCH_DEV, you'll see mount options
> changed for TEST_DEV. e.g.

That's a bug in _check_test_fs() that needs fixing.

> MOUNT_OPTIONS="-o dax" TEST_FS_MOUNT_OPTS="" ./check generic/413 generic/445
> generic/445 mount TEST_DEV with "-o dax" too
> 
> MOUNT_OPTIONS="" TEST_FS_MOUNT_OPTS="-o dax" ./check generic/413 generic/445
> generic/445 mount TEST_DEV without "-o dax"
> 
> MOUNT_OPTIONS="-o dax" TEST_FS_MOUNT_OPTS="-o dax" ./check generic/413 generic/445
> both tests and both devices mount with "-o dax"
> 
> That's been discussed in this thread:
> https://patchwork.kernel.org/patch/9742039/

Ok, so it definitely needs fixing - using MOUNT_OPTIONS with
the test device is just simply wrong.

> I think fixing _check_<fs>_filesystem() is the correct way. And I guess
> we can refactor out a common function and call it in
> _check_[xfs|btrfs|generic]_filesystem, pass the correct mount options
> based on what device we're working on.

check_xfs_filesystem already takes a mount option parameter. The
problem is that it's considered "extra" and appended to
MOUNT_OPTIONS. Should be simple to fix...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] generic/4[13,62]: restore TEST mount options

[Omer Zilberberg]

On 10/31/2017 01:34 PM, Eryu Guan wrote:
> On Tue, Oct 31, 2017 at 12:25:51PM +0200, Omer Zilberberg wrote:
>>
>> On 10/31/2017 06:37 AM, Eryu Guan wrote:
>>> On Tue, Oct 31, 2017 at 07:36:58AM +1100, Dave Chinner wrote:
>>>> On Mon, Oct 30, 2017 at 10:08:31AM +0200, Omer Zilberberg wrote:
>>>>> These tests locally change the TEST_FS_MOUNT_OPTS/MOUNT_OPTIONS
>>>>> environment variables, and run _test_cycle_mount. As a result, following
>>>>> tests using the TEST mount point may start with different mount options,
>>>>> depending on run order.
>>>> I don't think that's the case. The change of the environment
>>>> variable should only affect the current test process and it's
>>>> children. When the test exits, we go back to the environment of the
>>>> check process, where the TEST_FS_MOUNT_OPTS environment variable is
>>>> still correctly set, and all future tests inherit from that. i.e.:
>>>>
>>>> $ export FOO=foo
>>>> $ echo $FOO
>>>> foo
>>>> $ bash
>>>> $ echo $FOO
>>>> foo
>>>> $ export FOO=bar
>>>> $ echo $FOO
>>>> bar
>>>> $ exit
>>>> $ echo $FOO
>>>> foo
>>>> $
>>>>
>>>> And after each test, check runs _check_filesystems(), which cycles
>>>> the test mount, so for each new test process that is run they should
>>>> already start in the correct state...
>>> I agreed, the changing of variables in a sub-shell won't affect the
>>> parent's copy, and check will restore the mounts with the untouched
>>> options.
>>>
>>> But the problem is that _check_test_fs() will cycle mount TEST_DEV with
>>> MOUNT_OPTIONS not TEST_FS_MOUNT_OPTS, so if you have different mount
>>> options set for TEST_DEV and SCRATCH_DEV, you'll see mount options
>>> changed for TEST_DEV. e.g.
>>>
>>> MOUNT_OPTIONS="-o dax" TEST_FS_MOUNT_OPTS="" ./check generic/413 generic/445
>>> generic/445 mount TEST_DEV with "-o dax" too
>>>
>>> MOUNT_OPTIONS="" TEST_FS_MOUNT_OPTS="-o dax" ./check generic/413 generic/445
>>> generic/445 mount TEST_DEV without "-o dax"
>>>
>>> MOUNT_OPTIONS="-o dax" TEST_FS_MOUNT_OPTS="-o dax" ./check generic/413 generic/445
>>> both tests and both devices mount with "-o dax"
>>>
>>> That's been discussed in this thread:
>>> https://patchwork.kernel.org/patch/9742039/
>>>
>>> Omer, can you please confirm if you're hitting this issue?
>> I'm not 100% that's the case, so I better describe my settings more clearly:
>> I have a debug mount option on my system to recover the FS from a backup.
>> When that flag is set, umount writes everything to the backup.
>> Mount restores from it, overwriting everything.
> If you're testing with setting your debug mount option to both
> TEST_FS_MOUNT_OPTS and MOUNT_OPTIONS, and you still see the failure you
> were seeing, then that's a different problem.
Yeah, that's what I'm doing, setting both with that flag.
>
>> As long as generic/413 is not involved, everything works well.
>> All _test_cycle_mount() calls first back everything up on umount,
>> then restore upon mount. So I get the same FS contents.
>>
>> But, consider generic/118 running after generic/413:
>> - generic/413 finishes with a mount point with no mount options
>> - generic/118 begins with restored TEST_FS_MOUNT_OPTS, as you've pointed out.
>> - some writes are performed to the FS
>> - next _test_cycle_mount:
>>   calls umount w/o backing up (debug flag previously unset by generic/413).
> Does this clear the backup too? If so, I suspect TEST_DEV got cleared on
> first mount with the debug option in generic/118, because the backup has
> been cleared in the _test_cycle_mount call in generic/413.
Yeah the backup is cleared, which is normal behavior when the debug flag is off.
And exactly, it's generic/413 clearing the flag from the mount point,
that's caused this.
>
>>   calls mount WITH the debug flag, and recovers from an empty backup,
>>   deleting the earlier writes.
>> - subsequent md5sum fails on "No such file or directory", as FS is now empty.
>>
>>> I think fixing _check_<fs>_filesystem() is the correct way. And I guess
>>> we can refactor out a common function and call it in
>>> _check_[xfs|btrfs|generic]_filesystem, pass the correct mount options
>>> based on what device we're working on.
>> If indeed we're talking about the same problem,
>> please let me know if you'd like me to prepare a different patch.
> Sure, really appreciated if you can prepare a different patch, even if
> it's not the same problem :)
Ok.
But are we in agreement that there are 2 different issues here?
If so, please let me know what you think of this patch,
which does resolve that issue I had originally (at least locally for me).

And I'll explore the issue with check_test_fs and the different mount options,
based on what you've both written here and the thread you've pointed to.
I'll send another patch to address that later.
>
> Thanks,
> Eryu

Re: [PATCH] generic/4[13,62]: restore TEST mount options

[Eryu Guan]

On Wed, Nov 01, 2017 at 02:06:27PM +0200, Omer Zilberberg wrote:
> 
> 
> On 10/31/2017 01:34 PM, Eryu Guan wrote:
> > On Tue, Oct 31, 2017 at 12:25:51PM +0200, Omer Zilberberg wrote:
> >>
> >> On 10/31/2017 06:37 AM, Eryu Guan wrote:
> >>> On Tue, Oct 31, 2017 at 07:36:58AM +1100, Dave Chinner wrote:
> >>>> On Mon, Oct 30, 2017 at 10:08:31AM +0200, Omer Zilberberg wrote:
> >>>>> These tests locally change the TEST_FS_MOUNT_OPTS/MOUNT_OPTIONS
> >>>>> environment variables, and run _test_cycle_mount. As a result, following
> >>>>> tests using the TEST mount point may start with different mount options,
> >>>>> depending on run order.
> >>>> I don't think that's the case. The change of the environment
> >>>> variable should only affect the current test process and it's
> >>>> children. When the test exits, we go back to the environment of the
> >>>> check process, where the TEST_FS_MOUNT_OPTS environment variable is
> >>>> still correctly set, and all future tests inherit from that. i.e.:
> >>>>
> >>>> $ export FOO=foo
> >>>> $ echo $FOO
> >>>> foo
> >>>> $ bash
> >>>> $ echo $FOO
> >>>> foo
> >>>> $ export FOO=bar
> >>>> $ echo $FOO
> >>>> bar
> >>>> $ exit
> >>>> $ echo $FOO
> >>>> foo
> >>>> $
> >>>>
> >>>> And after each test, check runs _check_filesystems(), which cycles
> >>>> the test mount, so for each new test process that is run they should
> >>>> already start in the correct state...
> >>> I agreed, the changing of variables in a sub-shell won't affect the
> >>> parent's copy, and check will restore the mounts with the untouched
> >>> options.
> >>>
> >>> But the problem is that _check_test_fs() will cycle mount TEST_DEV with
> >>> MOUNT_OPTIONS not TEST_FS_MOUNT_OPTS, so if you have different mount
> >>> options set for TEST_DEV and SCRATCH_DEV, you'll see mount options
> >>> changed for TEST_DEV. e.g.
> >>>
> >>> MOUNT_OPTIONS="-o dax" TEST_FS_MOUNT_OPTS="" ./check generic/413 generic/445
> >>> generic/445 mount TEST_DEV with "-o dax" too
> >>>
> >>> MOUNT_OPTIONS="" TEST_FS_MOUNT_OPTS="-o dax" ./check generic/413 generic/445
> >>> generic/445 mount TEST_DEV without "-o dax"
> >>>
> >>> MOUNT_OPTIONS="-o dax" TEST_FS_MOUNT_OPTS="-o dax" ./check generic/413 generic/445
> >>> both tests and both devices mount with "-o dax"
> >>>
> >>> That's been discussed in this thread:
> >>> https://patchwork.kernel.org/patch/9742039/
> >>>
> >>> Omer, can you please confirm if you're hitting this issue?
> >> I'm not 100% that's the case, so I better describe my settings more clearly:
> >> I have a debug mount option on my system to recover the FS from a backup.
> >> When that flag is set, umount writes everything to the backup.
> >> Mount restores from it, overwriting everything.
> > If you're testing with setting your debug mount option to both
> > TEST_FS_MOUNT_OPTS and MOUNT_OPTIONS, and you still see the failure you
> > were seeing, then that's a different problem.
> Yeah, that's what I'm doing, setting both with that flag.
> >
> >> As long as generic/413 is not involved, everything works well.
> >> All _test_cycle_mount() calls first back everything up on umount,
> >> then restore upon mount. So I get the same FS contents.
> >>
> >> But, consider generic/118 running after generic/413:
> >> - generic/413 finishes with a mount point with no mount options
> >> - generic/118 begins with restored TEST_FS_MOUNT_OPTS, as you've pointed out.
> >> - some writes are performed to the FS
> >> - next _test_cycle_mount:
> >>   calls umount w/o backing up (debug flag previously unset by generic/413).
> > Does this clear the backup too? If so, I suspect TEST_DEV got cleared on
> > first mount with the debug option in generic/118, because the backup has
> > been cleared in the _test_cycle_mount call in generic/413.
> Yeah the backup is cleared, which is normal behavior when the debug flag is off.
> And exactly, it's generic/413 clearing the flag from the mount point,
> that's caused this.

IMHO, in this case fstests doesn't do anything wrong, all the mount
options are restored when running the next test, it's just not
compatible with your debug option and workflow.

> >
> >>   calls mount WITH the debug flag, and recovers from an empty backup,
> >>   deleting the earlier writes.
> >> - subsequent md5sum fails on "No such file or directory", as FS is now empty.
> >>
> >>> I think fixing _check_<fs>_filesystem() is the correct way. And I guess
> >>> we can refactor out a common function and call it in
> >>> _check_[xfs|btrfs|generic]_filesystem, pass the correct mount options
> >>> based on what device we're working on.
> >> If indeed we're talking about the same problem,
> >> please let me know if you'd like me to prepare a different patch.
> > Sure, really appreciated if you can prepare a different patch, even if
> > it's not the same problem :)
> Ok.
> But are we in agreement that there are 2 different issues here?

Yes, the problem you hit is different with the issue I described above.

> If so, please let me know what you think of this patch,
> which does resolve that issue I had originally (at least locally for me).

I prefer not merging your patch, sorry. Because in this specific case,
from fstests' point of view, it's all doing well and everything works as
expected.

> 
> And I'll explore the issue with check_test_fs and the different mount options,
> based on what you've both written here and the thread you've pointed to.
> I'll send another patch to address that later.

Thanks a lot!

Eryu

Re: [PATCH] generic/4[13,62]: restore TEST mount options

[Omer Zilberberg]

On 11/01/2017 02:52 PM, Eryu Guan wrote:
> On Wed, Nov 01, 2017 at 02:06:27PM +0200, Omer Zilberberg wrote:
>>
>> On 10/31/2017 01:34 PM, Eryu Guan wrote:
>>> On Tue, Oct 31, 2017 at 12:25:51PM +0200, Omer Zilberberg wrote:
>>>> On 10/31/2017 06:37 AM, Eryu Guan wrote:
>>>>> On Tue, Oct 31, 2017 at 07:36:58AM +1100, Dave Chinner wrote:
>>>>>> On Mon, Oct 30, 2017 at 10:08:31AM +0200, Omer Zilberberg wrote:
>>>>>>> These tests locally change the TEST_FS_MOUNT_OPTS/MOUNT_OPTIONS
>>>>>>> environment variables, and run _test_cycle_mount. As a result, following
>>>>>>> tests using the TEST mount point may start with different mount options,
>>>>>>> depending on run order.
>>>>>> I don't think that's the case. The change of the environment
>>>>>> variable should only affect the current test process and it's
>>>>>> children. When the test exits, we go back to the environment of the
>>>>>> check process, where the TEST_FS_MOUNT_OPTS environment variable is
>>>>>> still correctly set, and all future tests inherit from that. i.e.:
>>>>>>
>>>>>> $ export FOO=foo
>>>>>> $ echo $FOO
>>>>>> foo
>>>>>> $ bash
>>>>>> $ echo $FOO
>>>>>> foo
>>>>>> $ export FOO=bar
>>>>>> $ echo $FOO
>>>>>> bar
>>>>>> $ exit
>>>>>> $ echo $FOO
>>>>>> foo
>>>>>> $
>>>>>>
>>>>>> And after each test, check runs _check_filesystems(), which cycles
>>>>>> the test mount, so for each new test process that is run they should
>>>>>> already start in the correct state...
>>>>> I agreed, the changing of variables in a sub-shell won't affect the
>>>>> parent's copy, and check will restore the mounts with the untouched
>>>>> options.
>>>>>
>>>>> But the problem is that _check_test_fs() will cycle mount TEST_DEV with
>>>>> MOUNT_OPTIONS not TEST_FS_MOUNT_OPTS, so if you have different mount
>>>>> options set for TEST_DEV and SCRATCH_DEV, you'll see mount options
>>>>> changed for TEST_DEV. e.g.
>>>>>
>>>>> MOUNT_OPTIONS="-o dax" TEST_FS_MOUNT_OPTS="" ./check generic/413 generic/445
>>>>> generic/445 mount TEST_DEV with "-o dax" too
>>>>>
>>>>> MOUNT_OPTIONS="" TEST_FS_MOUNT_OPTS="-o dax" ./check generic/413 generic/445
>>>>> generic/445 mount TEST_DEV without "-o dax"
>>>>>
>>>>> MOUNT_OPTIONS="-o dax" TEST_FS_MOUNT_OPTS="-o dax" ./check generic/413 generic/445
>>>>> both tests and both devices mount with "-o dax"
>>>>>
>>>>> That's been discussed in this thread:
>>>>> https://patchwork.kernel.org/patch/9742039/
>>>>>
>>>>> Omer, can you please confirm if you're hitting this issue?
>>>> I'm not 100% that's the case, so I better describe my settings more clearly:
>>>> I have a debug mount option on my system to recover the FS from a backup.
>>>> When that flag is set, umount writes everything to the backup.
>>>> Mount restores from it, overwriting everything.
>>> If you're testing with setting your debug mount option to both
>>> TEST_FS_MOUNT_OPTS and MOUNT_OPTIONS, and you still see the failure you
>>> were seeing, then that's a different problem.
>> Yeah, that's what I'm doing, setting both with that flag.
>>>> As long as generic/413 is not involved, everything works well.
>>>> All _test_cycle_mount() calls first back everything up on umount,
>>>> then restore upon mount. So I get the same FS contents.
>>>>
>>>> But, consider generic/118 running after generic/413:
>>>> - generic/413 finishes with a mount point with no mount options
>>>> - generic/118 begins with restored TEST_FS_MOUNT_OPTS, as you've pointed out.
>>>> - some writes are performed to the FS
>>>> - next _test_cycle_mount:
>>>>   calls umount w/o backing up (debug flag previously unset by generic/413).
>>> Does this clear the backup too? If so, I suspect TEST_DEV got cleared on
>>> first mount with the debug option in generic/118, because the backup has
>>> been cleared in the _test_cycle_mount call in generic/413.
>> Yeah the backup is cleared, which is normal behavior when the debug flag is off.
>> And exactly, it's generic/413 clearing the flag from the mount point,
>> that's caused this.
> IMHO, in this case fstests doesn't do anything wrong, all the mount
> options are restored when running the next test, it's just not
> compatible with your debug option and workflow.
But the mount options are not restored when running the next test...
Only the first _test_cycle_mount restores them.
In case I haven't made myself clear, here's a final demonstration.
If you still think this is valid, I'll back down :)

I've edited generic/118 locally, and after _require_test_reflink, I've added:
mount | grep $TEST_DIR
Just to see the mount options the TEST FS has.

I've also put aside my debug option, and used -o strictatime instead.
If I mount w/o special options, I get the "relatime" mount option as default.
If I mount with -o strictatime, I do not see "relatime". Now back to fstests:

If the order of run is generic/118 followed by generic/413,
Then generic/118 does NOT print "relatime" in its options, because strictatime
is active.
However, if the order of run is reversed, then "relatime" IS printed, because
the TEST FS' was left with default mount options by generic/413.

So, generic/118's start conditions depend on its predecessor.
Is that valid for TEST fs mount options?

>
>>>>   calls mount WITH the debug flag, and recovers from an empty backup,
>>>>   deleting the earlier writes.
>>>> - subsequent md5sum fails on "No such file or directory", as FS is now empty.
>>>>
>>>>> I think fixing _check_<fs>_filesystem() is the correct way. And I guess
>>>>> we can refactor out a common function and call it in
>>>>> _check_[xfs|btrfs|generic]_filesystem, pass the correct mount options
>>>>> based on what device we're working on.
>>>> If indeed we're talking about the same problem,
>>>> please let me know if you'd like me to prepare a different patch.
>>> Sure, really appreciated if you can prepare a different patch, even if
>>> it's not the same problem :)
>> Ok.
>> But are we in agreement that there are 2 different issues here?
> Yes, the problem you hit is different with the issue I described above.
>
>> If so, please let me know what you think of this patch,
>> which does resolve that issue I had originally (at least locally for me).
> I prefer not merging your patch, sorry. Because in this specific case,
> from fstests' point of view, it's all doing well and everything works as
> expected.
>
>> And I'll explore the issue with check_test_fs and the different mount options,
>> based on what you've both written here and the thread you've pointed to.
>> I'll send another patch to address that later.
> Thanks a lot!
>
> Eryu
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] generic/4[13,62]: restore TEST mount options

[Eryu Guan]

On Wed, Nov 01, 2017 at 05:03:21PM +0200, Omer Zilberberg wrote:
> 
> 
> On 11/01/2017 02:52 PM, Eryu Guan wrote:
> > On Wed, Nov 01, 2017 at 02:06:27PM +0200, Omer Zilberberg wrote:
> >>
> >> On 10/31/2017 01:34 PM, Eryu Guan wrote:
> >>> On Tue, Oct 31, 2017 at 12:25:51PM +0200, Omer Zilberberg wrote:
> >>>> On 10/31/2017 06:37 AM, Eryu Guan wrote:
> >>>>> On Tue, Oct 31, 2017 at 07:36:58AM +1100, Dave Chinner wrote:
> >>>>>> On Mon, Oct 30, 2017 at 10:08:31AM +0200, Omer Zilberberg wrote:
> >>>>>>> These tests locally change the TEST_FS_MOUNT_OPTS/MOUNT_OPTIONS
> >>>>>>> environment variables, and run _test_cycle_mount. As a result, following
> >>>>>>> tests using the TEST mount point may start with different mount options,
> >>>>>>> depending on run order.
> >>>>>> I don't think that's the case. The change of the environment
> >>>>>> variable should only affect the current test process and it's
> >>>>>> children. When the test exits, we go back to the environment of the
> >>>>>> check process, where the TEST_FS_MOUNT_OPTS environment variable is
> >>>>>> still correctly set, and all future tests inherit from that. i.e.:
> >>>>>>
> >>>>>> $ export FOO=foo
> >>>>>> $ echo $FOO
> >>>>>> foo
> >>>>>> $ bash
> >>>>>> $ echo $FOO
> >>>>>> foo
> >>>>>> $ export FOO=bar
> >>>>>> $ echo $FOO
> >>>>>> bar
> >>>>>> $ exit
> >>>>>> $ echo $FOO
> >>>>>> foo
> >>>>>> $
> >>>>>>
> >>>>>> And after each test, check runs _check_filesystems(), which cycles
> >>>>>> the test mount, so for each new test process that is run they should
> >>>>>> already start in the correct state...
> >>>>> I agreed, the changing of variables in a sub-shell won't affect the
> >>>>> parent's copy, and check will restore the mounts with the untouched
> >>>>> options.
> >>>>>
> >>>>> But the problem is that _check_test_fs() will cycle mount TEST_DEV with
> >>>>> MOUNT_OPTIONS not TEST_FS_MOUNT_OPTS, so if you have different mount
> >>>>> options set for TEST_DEV and SCRATCH_DEV, you'll see mount options
> >>>>> changed for TEST_DEV. e.g.
> >>>>>
> >>>>> MOUNT_OPTIONS="-o dax" TEST_FS_MOUNT_OPTS="" ./check generic/413 generic/445
> >>>>> generic/445 mount TEST_DEV with "-o dax" too
> >>>>>
> >>>>> MOUNT_OPTIONS="" TEST_FS_MOUNT_OPTS="-o dax" ./check generic/413 generic/445
> >>>>> generic/445 mount TEST_DEV without "-o dax"
> >>>>>
> >>>>> MOUNT_OPTIONS="-o dax" TEST_FS_MOUNT_OPTS="-o dax" ./check generic/413 generic/445
> >>>>> both tests and both devices mount with "-o dax"
> >>>>>
> >>>>> That's been discussed in this thread:
> >>>>> https://patchwork.kernel.org/patch/9742039/
> >>>>>
> >>>>> Omer, can you please confirm if you're hitting this issue?
> >>>> I'm not 100% that's the case, so I better describe my settings more clearly:
> >>>> I have a debug mount option on my system to recover the FS from a backup.
> >>>> When that flag is set, umount writes everything to the backup.
> >>>> Mount restores from it, overwriting everything.
> >>> If you're testing with setting your debug mount option to both
> >>> TEST_FS_MOUNT_OPTS and MOUNT_OPTIONS, and you still see the failure you
> >>> were seeing, then that's a different problem.
> >> Yeah, that's what I'm doing, setting both with that flag.
> >>>> As long as generic/413 is not involved, everything works well.
> >>>> All _test_cycle_mount() calls first back everything up on umount,
> >>>> then restore upon mount. So I get the same FS contents.
> >>>>
> >>>> But, consider generic/118 running after generic/413:
> >>>> - generic/413 finishes with a mount point with no mount options
> >>>> - generic/118 begins with restored TEST_FS_MOUNT_OPTS, as you've pointed out.
> >>>> - some writes are performed to the FS
> >>>> - next _test_cycle_mount:
> >>>>   calls umount w/o backing up (debug flag previously unset by generic/413).
> >>> Does this clear the backup too? If so, I suspect TEST_DEV got cleared on
> >>> first mount with the debug option in generic/118, because the backup has
> >>> been cleared in the _test_cycle_mount call in generic/413.
> >> Yeah the backup is cleared, which is normal behavior when the debug flag is off.
> >> And exactly, it's generic/413 clearing the flag from the mount point,
> >> that's caused this.
> > IMHO, in this case fstests doesn't do anything wrong, all the mount
> > options are restored when running the next test, it's just not
> > compatible with your debug option and workflow.
> But the mount options are not restored when running the next test...
> Only the first _test_cycle_mount restores them.
> In case I haven't made myself clear, here's a final demonstration.
> If you still think this is valid, I'll back down :)
> 
> I've edited generic/118 locally, and after _require_test_reflink, I've added:
> mount | grep $TEST_DIR
> Just to see the mount options the TEST FS has.
> 
> I've also put aside my debug option, and used -o strictatime instead.
> If I mount w/o special options, I get the "relatime" mount option as default.
> If I mount with -o strictatime, I do not see "relatime". Now back to fstests:

I assume you still set "-o strictatime" to both TEST_FS_MOUNT_OPTS and
MOUNT_OPTIONS, so all went well so far.

> 
> If the order of run is generic/118 followed by generic/413,
> Then generic/118 does NOT print "relatime" in its options, because strictatime
> is active.

Yeah, I saw the same result, which was also expected.

> However, if the order of run is reversed, then "relatime" IS printed, because
> the TEST FS' was left with default mount options by generic/413.

I saw different result here, generic/118 didn't print relatime for me.

[root@ibm-x3550m3-05 xfstests]# TEST_FS_MOUNT_OPTS="-o strictatime" MOUNT_OPTIONS="-o strictatime" ./check  generic/413 generic/118 
FSTYP         -- xfs (debug)
PLATFORM      -- Linux/x86_64 ibm-x3550m3-05 4.14.0-rc7
MKFS_OPTIONS  -- -f -bsize=4096 /dev/sdc2
MOUNT_OPTIONS -- -o strictatime /dev/sdc2 /mnt/testarea/scratch

generic/413 1s ... - output mismatch (see /root/xfstests/results//generic/413.out.bad)
    --- tests/generic/413.out   2017-10-11 18:15:46.720424655 +0800
    +++ /root/xfstests/results//generic/413.out.bad     2017-11-02 20:02:39.546307209 +0800
    @@ -1,2 +1,4 @@
     QA output created by 413
    +/dev/sdc1 on /mnt/testarea/test type xfs (rw,seclabel,attr2,inode64,noquota) # right after _require_test
    +/dev/sdc1 on /mnt/testarea/test type xfs (rw,relatime,attr2,inode64,noquota) # after _cycle_test_mount
     Silence is golden
    ...
    (Run 'diff -u tests/generic/413.out /root/xfstests/results//generic/413.out.bad'  to see the entire diff)
generic/118 0s ... - output mismatch (see /root/xfstests/results//generic/118.out.bad)
    --- tests/generic/118.out   2017-10-11 18:15:46.644423183 +0800
    +++ /root/xfstests/results//generic/118.out.bad     2017-11-02 20:02:41.080336481 +0800
    @@ -1,5 +1,7 @@
     QA output created by 118
    +/dev/sdc1 on /mnt/testarea/test type xfs (rw,seclabel,attr2,inode64,noquota) # right after _require_test_reflink
     Create the original files
    +/dev/sdc1 on /mnt/testarea/test type xfs (rw,attr2,inode64,noquota)	  # after _cycle_test_mount

Note that I hacked 'check' too to not sort the tests by seq number.

Did I miss anything?

Thanks,
Eryu

> 
> So, generic/118's start conditions depend on its predecessor.
> Is that valid for TEST fs mount options?
> 
> >
> >>>>   calls mount WITH the debug flag, and recovers from an empty backup,
> >>>>   deleting the earlier writes.
> >>>> - subsequent md5sum fails on "No such file or directory", as FS is now empty.
> >>>>
> >>>>> I think fixing _check_<fs>_filesystem() is the correct way. And I guess
> >>>>> we can refactor out a common function and call it in
> >>>>> _check_[xfs|btrfs|generic]_filesystem, pass the correct mount options
> >>>>> based on what device we're working on.
> >>>> If indeed we're talking about the same problem,
> >>>> please let me know if you'd like me to prepare a different patch.
> >>> Sure, really appreciated if you can prepare a different patch, even if
> >>> it's not the same problem :)
> >> Ok.
> >> But are we in agreement that there are 2 different issues here?
> > Yes, the problem you hit is different with the issue I described above.
> >
> >> If so, please let me know what you think of this patch,
> >> which does resolve that issue I had originally (at least locally for me).
> > I prefer not merging your patch, sorry. Because in this specific case,
> > from fstests' point of view, it's all doing well and everything works as
> > expected.
> >
> >> And I'll explore the issue with check_test_fs and the different mount options,
> >> based on what you've both written here and the thread you've pointed to.
> >> I'll send another patch to address that later.
> > Thanks a lot!
> >
> > Eryu
> > --
> > To unsubscribe from this list: send the line "unsubscribe fstests" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] generic/4[13,62]: restore TEST mount options

[Omer Zilberberg]

On 11/02/2017 02:13 PM, Eryu Guan wrote:
> On Wed, Nov 01, 2017 at 05:03:21PM +0200, Omer Zilberberg wrote:
>>
>> On 11/01/2017 02:52 PM, Eryu Guan wrote:
>>> On Wed, Nov 01, 2017 at 02:06:27PM +0200, Omer Zilberberg wrote:
>>>> On 10/31/2017 01:34 PM, Eryu Guan wrote:
>>>>> On Tue, Oct 31, 2017 at 12:25:51PM +0200, Omer Zilberberg wrote:
>>>>>> On 10/31/2017 06:37 AM, Eryu Guan wrote:
>>>>>>> On Tue, Oct 31, 2017 at 07:36:58AM +1100, Dave Chinner wrote:
>>>>>>>> On Mon, Oct 30, 2017 at 10:08:31AM +0200, Omer Zilberberg wrote:
>>>>>>>>> These tests locally change the TEST_FS_MOUNT_OPTS/MOUNT_OPTIONS
>>>>>>>>> environment variables, and run _test_cycle_mount. As a result, following
>>>>>>>>> tests using the TEST mount point may start with different mount options,
>>>>>>>>> depending on run order.
>>>>>>>> I don't think that's the case. The change of the environment
>>>>>>>> variable should only affect the current test process and it's
>>>>>>>> children. When the test exits, we go back to the environment of the
>>>>>>>> check process, where the TEST_FS_MOUNT_OPTS environment variable is
>>>>>>>> still correctly set, and all future tests inherit from that. i.e.:
>>>>>>>>
>>>>>>>> $ export FOO=foo
>>>>>>>> $ echo $FOO
>>>>>>>> foo
>>>>>>>> $ bash
>>>>>>>> $ echo $FOO
>>>>>>>> foo
>>>>>>>> $ export FOO=bar
>>>>>>>> $ echo $FOO
>>>>>>>> bar
>>>>>>>> $ exit
>>>>>>>> $ echo $FOO
>>>>>>>> foo
>>>>>>>> $
>>>>>>>>
>>>>>>>> And after each test, check runs _check_filesystems(), which cycles
>>>>>>>> the test mount, so for each new test process that is run they should
>>>>>>>> already start in the correct state...
>>>>>>> I agreed, the changing of variables in a sub-shell won't affect the
>>>>>>> parent's copy, and check will restore the mounts with the untouched
>>>>>>> options.
>>>>>>>
>>>>>>> But the problem is that _check_test_fs() will cycle mount TEST_DEV with
>>>>>>> MOUNT_OPTIONS not TEST_FS_MOUNT_OPTS, so if you have different mount
>>>>>>> options set for TEST_DEV and SCRATCH_DEV, you'll see mount options
>>>>>>> changed for TEST_DEV. e.g.
>>>>>>>
>>>>>>> MOUNT_OPTIONS="-o dax" TEST_FS_MOUNT_OPTS="" ./check generic/413 generic/445
>>>>>>> generic/445 mount TEST_DEV with "-o dax" too
>>>>>>>
>>>>>>> MOUNT_OPTIONS="" TEST_FS_MOUNT_OPTS="-o dax" ./check generic/413 generic/445
>>>>>>> generic/445 mount TEST_DEV without "-o dax"
>>>>>>>
>>>>>>> MOUNT_OPTIONS="-o dax" TEST_FS_MOUNT_OPTS="-o dax" ./check generic/413 generic/445
>>>>>>> both tests and both devices mount with "-o dax"
>>>>>>>
>>>>>>> That's been discussed in this thread:
>>>>>>> https://patchwork.kernel.org/patch/9742039/
>>>>>>>
>>>>>>> Omer, can you please confirm if you're hitting this issue?
>>>>>> I'm not 100% that's the case, so I better describe my settings more clearly:
>>>>>> I have a debug mount option on my system to recover the FS from a backup.
>>>>>> When that flag is set, umount writes everything to the backup.
>>>>>> Mount restores from it, overwriting everything.
>>>>> If you're testing with setting your debug mount option to both
>>>>> TEST_FS_MOUNT_OPTS and MOUNT_OPTIONS, and you still see the failure you
>>>>> were seeing, then that's a different problem.
>>>> Yeah, that's what I'm doing, setting both with that flag.
>>>>>> As long as generic/413 is not involved, everything works well.
>>>>>> All _test_cycle_mount() calls first back everything up on umount,
>>>>>> then restore upon mount. So I get the same FS contents.
>>>>>>
>>>>>> But, consider generic/118 running after generic/413:
>>>>>> - generic/413 finishes with a mount point with no mount options
>>>>>> - generic/118 begins with restored TEST_FS_MOUNT_OPTS, as you've pointed out.
>>>>>> - some writes are performed to the FS
>>>>>> - next _test_cycle_mount:
>>>>>>   calls umount w/o backing up (debug flag previously unset by generic/413).
>>>>> Does this clear the backup too? If so, I suspect TEST_DEV got cleared on
>>>>> first mount with the debug option in generic/118, because the backup has
>>>>> been cleared in the _test_cycle_mount call in generic/413.
>>>> Yeah the backup is cleared, which is normal behavior when the debug flag is off.
>>>> And exactly, it's generic/413 clearing the flag from the mount point,
>>>> that's caused this.
>>> IMHO, in this case fstests doesn't do anything wrong, all the mount
>>> options are restored when running the next test, it's just not
>>> compatible with your debug option and workflow.
>> But the mount options are not restored when running the next test...
>> Only the first _test_cycle_mount restores them.
>> In case I haven't made myself clear, here's a final demonstration.
>> If you still think this is valid, I'll back down :)
>>
>> I've edited generic/118 locally, and after _require_test_reflink, I've added:
>> mount | grep $TEST_DIR
>> Just to see the mount options the TEST FS has.
>>
>> I've also put aside my debug option, and used -o strictatime instead.
>> If I mount w/o special options, I get the "relatime" mount option as default.
>> If I mount with -o strictatime, I do not see "relatime". Now back to fstests:
> I assume you still set "-o strictatime" to both TEST_FS_MOUNT_OPTS and
> MOUNT_OPTIONS, so all went well so far.
>
>> If the order of run is generic/118 followed by generic/413,
>> Then generic/118 does NOT print "relatime" in its options, because strictatime
>> is active.
> Yeah, I saw the same result, which was also expected.
>
>> However, if the order of run is reversed, then "relatime" IS printed, because
>> the TEST FS' was left with default mount options by generic/413.
> I saw different result here, generic/118 didn't print relatime for me.
>
> [root@ibm-x3550m3-05 xfstests]# TEST_FS_MOUNT_OPTS="-o strictatime" MOUNT_OPTIONS="-o strictatime" ./check  generic/413 generic/118 
> FSTYP         -- xfs (debug)
> PLATFORM      -- Linux/x86_64 ibm-x3550m3-05 4.14.0-rc7
> MKFS_OPTIONS  -- -f -bsize=4096 /dev/sdc2
> MOUNT_OPTIONS -- -o strictatime /dev/sdc2 /mnt/testarea/scratch
>
> generic/413 1s ... - output mismatch (see /root/xfstests/results//generic/413.out.bad)
>     --- tests/generic/413.out   2017-10-11 18:15:46.720424655 +0800
>     +++ /root/xfstests/results//generic/413.out.bad     2017-11-02 20:02:39.546307209 +0800
>     @@ -1,2 +1,4 @@
>      QA output created by 413
>     +/dev/sdc1 on /mnt/testarea/test type xfs (rw,seclabel,attr2,inode64,noquota) # right after _require_test
>     +/dev/sdc1 on /mnt/testarea/test type xfs (rw,relatime,attr2,inode64,noquota) # after _cycle_test_mount
>      Silence is golden
>     ...
>     (Run 'diff -u tests/generic/413.out /root/xfstests/results//generic/413.out.bad'  to see the entire diff)
> generic/118 0s ... - output mismatch (see /root/xfstests/results//generic/118.out.bad)
>     --- tests/generic/118.out   2017-10-11 18:15:46.644423183 +0800
>     +++ /root/xfstests/results//generic/118.out.bad     2017-11-02 20:02:41.080336481 +0800
>     @@ -1,5 +1,7 @@
>      QA output created by 118
>     +/dev/sdc1 on /mnt/testarea/test type xfs (rw,seclabel,attr2,inode64,noquota) # right after _require_test_reflink
>      Create the original files
>     +/dev/sdc1 on /mnt/testarea/test type xfs (rw,attr2,inode64,noquota)	  # after _cycle_test_mount
>
> Note that I hacked 'check' too to not sort the tests by seq number.
>
> Did I miss anything?
It took me a while to figure this one out... You haven't missed anything.
The mechanism that revives the mount options is _check_generic_filesystem,
or _check_xfs_filesystem in this case, and it does this by unmounting and later
mounting again.

In my code path, _check_generic_filesystem is skipped, replaced by NOP,
because I didn't need fsck, etc., but I also lost the inherent cycle mount
that's included in that method.

Sorry about that.
>
> Thanks,
> Eryu
>
>> So, generic/118's start conditions depend on its predecessor.
>> Is that valid for TEST fs mount options?
>>
>>>>>>   calls mount WITH the debug flag, and recovers from an empty backup,
>>>>>>   deleting the earlier writes.
>>>>>> - subsequent md5sum fails on "No such file or directory", as FS is now empty.
>>>>>>
>>>>>>> I think fixing _check_<fs>_filesystem() is the correct way. And I guess
>>>>>>> we can refactor out a common function and call it in
>>>>>>> _check_[xfs|btrfs|generic]_filesystem, pass the correct mount options
>>>>>>> based on what device we're working on.
>>>>>> If indeed we're talking about the same problem,
>>>>>> please let me know if you'd like me to prepare a different patch.
>>>>> Sure, really appreciated if you can prepare a different patch, even if
>>>>> it's not the same problem :)
>>>> Ok.
>>>> But are we in agreement that there are 2 different issues here?
>>> Yes, the problem you hit is different with the issue I described above.
>>>
>>>> If so, please let me know what you think of this patch,
>>>> which does resolve that issue I had originally (at least locally for me).
>>> I prefer not merging your patch, sorry. Because in this specific case,
>>> from fstests' point of view, it's all doing well and everything works as
>>> expected.
>>>
>>>> And I'll explore the issue with check_test_fs and the different mount options,
>>>> based on what you've both written here and the thread you've pointed to.
>>>> I'll send another patch to address that later.
>>> Thanks a lot!
>>>
>>> Eryu
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe fstests" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html