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

Omer Zilberberg
2017-10-31T10:26:09Z

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