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

[Omer Zilberberg <omerz@netapp.com>]

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