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

[Eryu Guan <eguan@redhat.com>]

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