[RFC] unionmount metacopy tests

[RFC] unionmount metacopy tests

[Amir Goldstein]

Hi Vivek,

I was working on extending snapshot validation tests and got
this as a by-product:

https://github.com/amir73il/unionmount-testsuite/commits/metacopy

ca566c3 Check that data was not copied up with metacopy=on
140d99c Reset dentry copy_up state on upper layer rotate
960a5ce Check that files were copied up as expected
1bfcc7d Record meta copy_up vs. data copy_up
c3db453 Fix instantiation of hardlinked dentry
2104e51 Simplify initialization of __upper
1fc2eec Fix ./run --ov --verify --recycle

Would you be interested to review these changes,
so I would merge them to master?

Would you or someone else be interested in running those tests
regularly on pre release kernel?

If anyone is running unionmount-testsuite on regular basis
I would be happy to know which configurations are being tested,
because the test matrix grew considerably since I took over the project -
both Overlayfs config options and the testsuite config options.

Thanks,
Amir.

Re: [RFC] unionmount metacopy tests

[Vivek Goyal]

On Thu, Jul 04, 2019 at 06:11:25PM +0300, Amir Goldstein wrote:
> Hi Vivek,
> 
> I was working on extending snapshot validation tests and got
> this as a by-product:
> 
> https://github.com/amir73il/unionmount-testsuite/commits/metacopy
> 
> ca566c3 Check that data was not copied up with metacopy=on
> 140d99c Reset dentry copy_up state on upper layer rotate
> 960a5ce Check that files were copied up as expected
> 1bfcc7d Record meta copy_up vs. data copy_up
> c3db453 Fix instantiation of hardlinked dentry
> 2104e51 Simplify initialization of __upper
> 1fc2eec Fix ./run --ov --verify --recycle
> 
> Would you be interested to review these changes,
> so I would merge them to master?

Hi Amir,

Glad to see more tests for metacopy feature. I will have a look at
these.

> 
> Would you or someone else be interested in running those tests
> regularly on pre release kernel?

I generally don't run tests regularly on latest kernel. Whenever I 
am fixing something, I run tests to make sure I have not broken
anything.

So I can't say I will run the tests regularly, but once in a while
I should be able to run it.


> 
> If anyone is running unionmount-testsuite on regular basis
> I would be happy to know which configurations are being tested,
> because the test matrix grew considerably since I took over the project -
> both Overlayfs config options and the testsuite config options.

For me, I think I am most interested in configuration used by
container runtimes (docker/podman). Docker seems to turn off
redirects as of now. podman is turning on metacopy (hence redirect)
by default now to see how do things go.

So for me (redirect=on/off and metacopy=on/off) are important
configurations as of now. Havind said that, I think I should talk
to container folks and encourage them to use "index" and "xino"
as well to be more posix like fs.

I think container folks still have not modified their code to
be able to generate an image layer properly if redirect is
enabled. Last time Miklos had some good ideas. I will poke them
again. It will be nice if they can use redirect (instead of
disabling it) and be able to generate image layer efficiently. 

Thanks
Vivek

[RFC] Passing extra mount options to unionmount tests

[Amir Goldstein]

> >
> > If anyone is running unionmount-testsuite on regular basis
> > I would be happy to know which configurations are being tested,
> > because the test matrix grew considerably since I took over the project -
> > both Overlayfs config options and the testsuite config options.
>
> For me, I think I am most interested in configuration used by
> container runtimes (docker/podman). Docker seems to turn off
> redirects as of now. podman is turning on metacopy (hence redirect)
> by default now to see how do things go.
>
> So for me (redirect=on/off and metacopy=on/off) are important
> configurations as of now. Having said that, I think I should talk
> to container folks and encourage them to use "index" and "xino"
> as well to be more posix like fs.
>

Hi Vivek,

I remember you asked me about configuring extra mount options
for unionmount but couldn't find that conversation, so replying to this
related old discussion with my thoughts on the subject.

Now that unionmount supports the environment variables:
UNIONMOUNT_{BASEDIR,LOWERDIR,MNTPOINT}

And now that xfstests has helpers to convert xfstests env vars to
UNIONMOUNT_* env vars, one might ask: why won't we support
UNIONMOUNT_OPTIONS=$OVERLAY_MOUNT_OPTIONS

So when you asked me a question along those lines, my answer was that
unionmount performs different validations depending on the test options,
so for example, the test option ./run --meta adds the mount option
"metacopy=on", but it also performs different validation tests, such as
upper file st_blocks == 0 after metadata change.

Right, so I gave a reason for why supporting extra mount options is not
straight forward, but that doesn't mean that it is not possible.
unionmount test could very well parse the extra mount options passed
in env var and translate them to test config options.  As a matter of fact,
unionmount already parses the following overlay module parameters
and translates the following values to test config options:

1) redirect_dir does not exist => --xdev (expect EXDEV on dir rename)
2) redirect_dir exists and no explicit --xdev => add redirect_dir=on
3) index=N and --verify => add index=on and check st_ino validations
4) metacopy=Y => check --meta validations (e.g. upper st_blocks)
5) xino_auto=Y => add xino=on and check --xino validations (e.g. uniform st_dev)

So apart from blindly adding the extra mount options to mount command,
will also need to translate:

6) redirect_dir=off => --xdev
   (redirect_dir=on conflicts with --xdev)
7) index=off => overrides index=on added by --verify
   (st_ino validations should still pass on tests without multi layers)
8) metacopy=on => --meta
   (metacopy=off conflicts with --meta)
9) xino=auto/on => --xino
   (xino=off conflicts with --xino)

At the moment, I have a patch to xfstests [1] that implements rule 8 in the
xfstests _unionmount_testsuite_run helper, but I came to realize that would
be wrong and that the correct way would be to implement conversion rules
6-9 in unionmount itself and then blindly assign in xfstest helper:
UNIONMOUNT_OPTIONS=$OVL_BASE_MOUNT_OPTIONS

Does anyone spot any obvious flaws in this plan before I make those changes?

Thanks,
Amir.

[1] https://github.com/amir73il/xfstests/commits/unionmount

Re: [RFC] Passing extra mount options to unionmount tests

[Vivek Goyal]

On Fri, Jul 31, 2020 at 03:35:40PM +0300, Amir Goldstein wrote:
> > >
> > > If anyone is running unionmount-testsuite on regular basis
> > > I would be happy to know which configurations are being tested,
> > > because the test matrix grew considerably since I took over the project -
> > > both Overlayfs config options and the testsuite config options.
> >
> > For me, I think I am most interested in configuration used by
> > container runtimes (docker/podman). Docker seems to turn off
> > redirects as of now. podman is turning on metacopy (hence redirect)
> > by default now to see how do things go.
> >
> > So for me (redirect=on/off and metacopy=on/off) are important
> > configurations as of now. Having said that, I think I should talk
> > to container folks and encourage them to use "index" and "xino"
> > as well to be more posix like fs.
> >
> 
> Hi Vivek,
> 
> I remember you asked me about configuring extra mount options
> for unionmount but couldn't find that conversation, so replying to this
> related old discussion with my thoughts on the subject.
> 
> Now that unionmount supports the environment variables:
> UNIONMOUNT_{BASEDIR,LOWERDIR,MNTPOINT}
> 
> And now that xfstests has helpers to convert xfstests env vars to
> UNIONMOUNT_* env vars, one might ask: why won't we support
> UNIONMOUNT_OPTIONS=$OVERLAY_MOUNT_OPTIONS
> 
> So when you asked me a question along those lines, my answer was that
> unionmount performs different validations depending on the test options,
> so for example, the test option ./run --meta adds the mount option
> "metacopy=on", but it also performs different validation tests, such as
> upper file st_blocks == 0 after metadata change.
> 
> Right, so I gave a reason for why supporting extra mount options is not
> straight forward, but that doesn't mean that it is not possible.
> unionmount test could very well parse the extra mount options passed
> in env var and translate them to test config options.

Hi Amir,

I am not able to understand this point. Why an extra mount option
needs to be translated into a "test config" option. If I pass
"metacopy=on", that does not mean that I also want to run tests
which verify st_blocks == 0 on upper. It just means that whatever
tests I am running, are run with metacopy=on. All I want to make
sure that tests I am running are not broken if run with metacopy=on.

Thanks
Vivek

> As a matter of fact,
> unionmount already parses the following overlay module parameters
> and translates the following values to test config options:
> 
> 1) redirect_dir does not exist => --xdev (expect EXDEV on dir rename)
> 2) redirect_dir exists and no explicit --xdev => add redirect_dir=on
> 3) index=N and --verify => add index=on and check st_ino validations
> 4) metacopy=Y => check --meta validations (e.g. upper st_blocks)
> 5) xino_auto=Y => add xino=on and check --xino validations (e.g. uniform st_dev)
> 
> So apart from blindly adding the extra mount options to mount command,
> will also need to translate:
> 
> 6) redirect_dir=off => --xdev
>    (redirect_dir=on conflicts with --xdev)
> 7) index=off => overrides index=on added by --verify
>    (st_ino validations should still pass on tests without multi layers)
> 8) metacopy=on => --meta
>    (metacopy=off conflicts with --meta)
> 9) xino=auto/on => --xino
>    (xino=off conflicts with --xino)
> 
> At the moment, I have a patch to xfstests [1] that implements rule 8 in the
> xfstests _unionmount_testsuite_run helper, but I came to realize that would
> be wrong and that the correct way would be to implement conversion rules
> 6-9 in unionmount itself and then blindly assign in xfstest helper:
> UNIONMOUNT_OPTIONS=$OVL_BASE_MOUNT_OPTIONS
> 
> Does anyone spot any obvious flaws in this plan before I make those changes?
> 
> Thanks,
> Amir.
> 
> [1] https://github.com/amir73il/xfstests/commits/unionmount
> 

Re: [RFC] Passing extra mount options to unionmount tests

[Amir Goldstein]

On Fri, Jul 31, 2020 at 4:13 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Fri, Jul 31, 2020 at 03:35:40PM +0300, Amir Goldstein wrote:
> > > >
> > > > If anyone is running unionmount-testsuite on regular basis
> > > > I would be happy to know which configurations are being tested,
> > > > because the test matrix grew considerably since I took over the project -
> > > > both Overlayfs config options and the testsuite config options.
> > >
> > > For me, I think I am most interested in configuration used by
> > > container runtimes (docker/podman). Docker seems to turn off
> > > redirects as of now. podman is turning on metacopy (hence redirect)
> > > by default now to see how do things go.
> > >
> > > So for me (redirect=on/off and metacopy=on/off) are important
> > > configurations as of now. Having said that, I think I should talk
> > > to container folks and encourage them to use "index" and "xino"
> > > as well to be more posix like fs.
> > >
> >
> > Hi Vivek,
> >
> > I remember you asked me about configuring extra mount options
> > for unionmount but couldn't find that conversation, so replying to this
> > related old discussion with my thoughts on the subject.
> >
> > Now that unionmount supports the environment variables:
> > UNIONMOUNT_{BASEDIR,LOWERDIR,MNTPOINT}
> >
> > And now that xfstests has helpers to convert xfstests env vars to
> > UNIONMOUNT_* env vars, one might ask: why won't we support
> > UNIONMOUNT_OPTIONS=$OVERLAY_MOUNT_OPTIONS
> >
> > So when you asked me a question along those lines, my answer was that
> > unionmount performs different validations depending on the test options,
> > so for example, the test option ./run --meta adds the mount option
> > "metacopy=on", but it also performs different validation tests, such as
> > upper file st_blocks == 0 after metadata change.
> >
> > Right, so I gave a reason for why supporting extra mount options is not
> > straight forward, but that doesn't mean that it is not possible.
> > unionmount test could very well parse the extra mount options passed
> > in env var and translate them to test config options.
>
> Hi Amir,
>
> I am not able to understand this point. Why an extra mount option
> needs to be translated into a "test config" option. If I pass
> "metacopy=on", that does not mean that I also want to run tests
> which verify st_blocks == 0 on upper. It just means that whatever
> tests I am running, are run with metacopy=on. All I want to make
> sure that tests I am running are not broken if run with metacopy=on.
>

I guess the confusion is what defines a "test".

A specific xfstests test script (e.g. overlay/060) defines:
- requirements (run or skip)
- setup and operations to execute
- expectations

So when you pass extra mount options it might:
- cause test not to meet requirements and be skipped
- affect the setup/operations
- usually it does not change the expectations

But you can also define some global options like
USE_KMEMLEAK, TEST_XFS_REPAIR_REBUILD, TEST_XFS_SCRUB
which affect the validations that are run after each test
in *addition* to the specific expectations encoded in the test script.

A specific unionmount test (e.g. tests/rename-pop-dir.py) defines only:
- operations to execute
- expected return value

But the test engine performs extra validations after *each* operation
in addition to verifying the expected return value.

So when I write "verify st_blocks == 0 on upper", this is not an expectation
that is explicitly written in any specific test.
The test engine records the state of objects before and after each filesystem
operation is executed to know for each object if it is expected to be lower,
metacopy,upper and then it runs some validations after each operation
(like upper st_blocks) to verify that the actual state of the object matches the
expected state.

The test run option --xdev only is similar to the xfstests requirement
and it skips a few dir rename tests and changes expected return
value in others.

But the test run options --verify, --meta, --xino, --verify, are global test
options which determine what sort of validations are performed after
each and every operation in all the operations of all of the test cases.

Those checks are implemented in context.py functions:
check_dev_ino(), check_copy_up(), check_layer()

Is my explanation more clear now?
It is clear why unionmount needs to parse mount options in order
to set the expectations from validators?

If that is clear, please review my proposal to see if I missed anything.

Thanks,
Amir.

Re: [RFC] Passing extra mount options to unionmount tests

[Vivek Goyal]

On Fri, Jul 31, 2020 at 05:09:16PM +0300, Amir Goldstein wrote:
> On Fri, Jul 31, 2020 at 4:13 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > On Fri, Jul 31, 2020 at 03:35:40PM +0300, Amir Goldstein wrote:
> > > > >
> > > > > If anyone is running unionmount-testsuite on regular basis
> > > > > I would be happy to know which configurations are being tested,
> > > > > because the test matrix grew considerably since I took over the project -
> > > > > both Overlayfs config options and the testsuite config options.
> > > >
> > > > For me, I think I am most interested in configuration used by
> > > > container runtimes (docker/podman). Docker seems to turn off
> > > > redirects as of now. podman is turning on metacopy (hence redirect)
> > > > by default now to see how do things go.
> > > >
> > > > So for me (redirect=on/off and metacopy=on/off) are important
> > > > configurations as of now. Having said that, I think I should talk
> > > > to container folks and encourage them to use "index" and "xino"
> > > > as well to be more posix like fs.
> > > >
> > >
> > > Hi Vivek,
> > >
> > > I remember you asked me about configuring extra mount options
> > > for unionmount but couldn't find that conversation, so replying to this
> > > related old discussion with my thoughts on the subject.
> > >
> > > Now that unionmount supports the environment variables:
> > > UNIONMOUNT_{BASEDIR,LOWERDIR,MNTPOINT}
> > >
> > > And now that xfstests has helpers to convert xfstests env vars to
> > > UNIONMOUNT_* env vars, one might ask: why won't we support
> > > UNIONMOUNT_OPTIONS=$OVERLAY_MOUNT_OPTIONS
> > >
> > > So when you asked me a question along those lines, my answer was that
> > > unionmount performs different validations depending on the test options,
> > > so for example, the test option ./run --meta adds the mount option
> > > "metacopy=on", but it also performs different validation tests, such as
> > > upper file st_blocks == 0 after metadata change.
> > >
> > > Right, so I gave a reason for why supporting extra mount options is not
> > > straight forward, but that doesn't mean that it is not possible.
> > > unionmount test could very well parse the extra mount options passed
> > > in env var and translate them to test config options.
> >
> > Hi Amir,
> >
> > I am not able to understand this point. Why an extra mount option
> > needs to be translated into a "test config" option. If I pass
> > "metacopy=on", that does not mean that I also want to run tests
> > which verify st_blocks == 0 on upper. It just means that whatever
> > tests I am running, are run with metacopy=on. All I want to make
> > sure that tests I am running are not broken if run with metacopy=on.
> >
> 
> I guess the confusion is what defines a "test".
> 
> A specific xfstests test script (e.g. overlay/060) defines:
> - requirements (run or skip)
> - setup and operations to execute
> - expectations
> 
> So when you pass extra mount options it might:
> - cause test not to meet requirements and be skipped
> - affect the setup/operations
> - usually it does not change the expectations
> 
> But you can also define some global options like
> USE_KMEMLEAK, TEST_XFS_REPAIR_REBUILD, TEST_XFS_SCRUB
> which affect the validations that are run after each test
> in *addition* to the specific expectations encoded in the test script.
> 
> A specific unionmount test (e.g. tests/rename-pop-dir.py) defines only:
> - operations to execute
> - expected return value
> 
> But the test engine performs extra validations after *each* operation
> in addition to verifying the expected return value.
> 
> So when I write "verify st_blocks == 0 on upper", this is not an expectation
> that is explicitly written in any specific test.
> The test engine records the state of objects before and after each filesystem
> operation is executed to know for each object if it is expected to be lower,
> metacopy,upper and then it runs some validations after each operation
> (like upper st_blocks) to verify that the actual state of the object matches the
> expected state.
> 
> The test run option --xdev only is similar to the xfstests requirement
> and it skips a few dir rename tests and changes expected return
> value in others.
> 
> But the test run options --verify, --meta, --xino, --verify, are global test
> options which determine what sort of validations are performed after
> each and every operation in all the operations of all of the test cases.
> 
> Those checks are implemented in context.py functions:
> check_dev_ino(), check_copy_up(), check_layer()
> 
> Is my explanation more clear now?

Hi Amir,

Thanks for the explanation. I understand your perspective little
better now.

But why do we have to enable these st_blocks checks after the test if
user asked to be mounted with "metacopy=on". 

IOW, if user wants to run these additional st_blocks checks, that
should be drive by explicit option "--meta" to _unionmount_testsuite_run.

Enabling "--meta" implicitly probably does not hurt now given what
"--meta" is doing but some other option "--foo" might be doing much
more (including running additional tests) and mapping that becomes
very tricky and kind of unexpected.

So how about we keep it simple. That is we don't try to map mount
options to associated arguments to unionmount testsuite. We probably
just need to detect conflicts and then skip test? For example, if
I run "_unionmount_testsuite_run --ov --meta" and also specifcy
"metacopy=off", then its a conflict and probably need to either
skip this test or error out and point towards the conflict etc.

If I pass mount option "metacopy=on" I don't expect unionmount
testsuite to suddenly start verifying that on upper layer st_blocks==0.
That I will expect from a specific test or if I call
"_unionmount_testsuite_run --ov --meta". This is not necessarily
similar to generally xfs options you mentioned for additional
verification, because unionmount testsuite options are generic
and one can implement anything behind these options (and are not
limited to additional verification only).

I don't have too detailed understanding of design of both testsuites.
So all my thoughts are from a pure user perspective. 

Thanks
Vivek

> It is clear why unionmount needs to parse mount options in order
> to set the expectations from validators?
> 
> If that is clear, please review my proposal to see if I missed anything.
> 
> Thanks,
> Amir.
> 

Re: [RFC] Passing extra mount options to unionmount tests

[Amir Goldstein]

On Fri, Jul 31, 2020 at 9:22 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Fri, Jul 31, 2020 at 05:09:16PM +0300, Amir Goldstein wrote:
> > On Fri, Jul 31, 2020 at 4:13 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> > >
> > > On Fri, Jul 31, 2020 at 03:35:40PM +0300, Amir Goldstein wrote:
> > > > > >
> > > > > > If anyone is running unionmount-testsuite on regular basis
> > > > > > I would be happy to know which configurations are being tested,
> > > > > > because the test matrix grew considerably since I took over the project -
> > > > > > both Overlayfs config options and the testsuite config options.
> > > > >
> > > > > For me, I think I am most interested in configuration used by
> > > > > container runtimes (docker/podman). Docker seems to turn off
> > > > > redirects as of now. podman is turning on metacopy (hence redirect)
> > > > > by default now to see how do things go.
> > > > >
> > > > > So for me (redirect=on/off and metacopy=on/off) are important
> > > > > configurations as of now. Having said that, I think I should talk
> > > > > to container folks and encourage them to use "index" and "xino"
> > > > > as well to be more posix like fs.
> > > > >
> > > >
> > > > Hi Vivek,
> > > >
> > > > I remember you asked me about configuring extra mount options
> > > > for unionmount but couldn't find that conversation, so replying to this
> > > > related old discussion with my thoughts on the subject.
> > > >
> > > > Now that unionmount supports the environment variables:
> > > > UNIONMOUNT_{BASEDIR,LOWERDIR,MNTPOINT}
> > > >
> > > > And now that xfstests has helpers to convert xfstests env vars to
> > > > UNIONMOUNT_* env vars, one might ask: why won't we support
> > > > UNIONMOUNT_OPTIONS=$OVERLAY_MOUNT_OPTIONS
> > > >
> > > > So when you asked me a question along those lines, my answer was that
> > > > unionmount performs different validations depending on the test options,
> > > > so for example, the test option ./run --meta adds the mount option
> > > > "metacopy=on", but it also performs different validation tests, such as
> > > > upper file st_blocks == 0 after metadata change.
> > > >
> > > > Right, so I gave a reason for why supporting extra mount options is not
> > > > straight forward, but that doesn't mean that it is not possible.
> > > > unionmount test could very well parse the extra mount options passed
> > > > in env var and translate them to test config options.
> > >
> > > Hi Amir,
> > >
> > > I am not able to understand this point. Why an extra mount option
> > > needs to be translated into a "test config" option. If I pass
> > > "metacopy=on", that does not mean that I also want to run tests
> > > which verify st_blocks == 0 on upper. It just means that whatever
> > > tests I am running, are run with metacopy=on. All I want to make
> > > sure that tests I am running are not broken if run with metacopy=on.
> > >
> >
> > I guess the confusion is what defines a "test".
> >
> > A specific xfstests test script (e.g. overlay/060) defines:
> > - requirements (run or skip)
> > - setup and operations to execute
> > - expectations
> >
> > So when you pass extra mount options it might:
> > - cause test not to meet requirements and be skipped
> > - affect the setup/operations
> > - usually it does not change the expectations
> >
> > But you can also define some global options like
> > USE_KMEMLEAK, TEST_XFS_REPAIR_REBUILD, TEST_XFS_SCRUB
> > which affect the validations that are run after each test
> > in *addition* to the specific expectations encoded in the test script.
> >
> > A specific unionmount test (e.g. tests/rename-pop-dir.py) defines only:
> > - operations to execute
> > - expected return value
> >
> > But the test engine performs extra validations after *each* operation
> > in addition to verifying the expected return value.
> >
> > So when I write "verify st_blocks == 0 on upper", this is not an expectation
> > that is explicitly written in any specific test.
> > The test engine records the state of objects before and after each filesystem
> > operation is executed to know for each object if it is expected to be lower,
> > metacopy,upper and then it runs some validations after each operation
> > (like upper st_blocks) to verify that the actual state of the object matches the
> > expected state.
> >
> > The test run option --xdev only is similar to the xfstests requirement
> > and it skips a few dir rename tests and changes expected return
> > value in others.
> >
> > But the test run options --verify, --meta, --xino, --verify, are global test
> > options which determine what sort of validations are performed after
> > each and every operation in all the operations of all of the test cases.
> >
> > Those checks are implemented in context.py functions:
> > check_dev_ino(), check_copy_up(), check_layer()
> >
> > Is my explanation more clear now?
>
> Hi Amir,
>
> Thanks for the explanation. I understand your perspective little
> better now.
>
> But why do we have to enable these st_blocks checks after the test if
> user asked to be mounted with "metacopy=on".
>
> IOW, if user wants to run these additional st_blocks checks, that
> should be drive by explicit option "--meta" to _unionmount_testsuite_run.
>
> Enabling "--meta" implicitly probably does not hurt now given what
> "--meta" is doing but some other option "--foo" might be doing much
> more (including running additional tests) and mapping that becomes
> very tricky and kind of unexpected.
>

Maybe I wasn't accurate in my explanation.

--verify is the only option that *adds* verifications.
I added it for backward compat because old kernels simply do not
pass those verifications (like const st_ino etc).
All the tests I added to xfstests use the --verify option because
want to test the upstream kernel and we want maximum test coverage.

--meta and --xino OTOH do not *add* verifications, they change the
validations that --verify does in subtle ways.

You will have to dive into the check_ functions for the details, they are
not pretty, but for example without --meta st_blocks is checked to be
positive after copy up of not empty file and with --meta it is checked to
be zero (which is actually wrong because xattr takes 1 block in xfs,
but let's not mix reality with theory for now).

Same goes for checks of st_dev and st_ino, they are always checked
for but expectations are different depending on xino and index.

IOW, if we do not parse the mount options to understand them and
just blindly set them, the verifications will likely fail.

Thanks,
Amir.

Re: [RFC] Passing extra mount options to unionmount tests

[Amir Goldstein]

[-- Attachment #1: Type: text/plain, Size: 3805 bytes --]

On Fri, Jul 31, 2020 at 3:35 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> > >
> > > If anyone is running unionmount-testsuite on regular basis
> > > I would be happy to know which configurations are being tested,
> > > because the test matrix grew considerably since I took over the project -
> > > both Overlayfs config options and the testsuite config options.
> >
> > For me, I think I am most interested in configuration used by
> > container runtimes (docker/podman). Docker seems to turn off
> > redirects as of now. podman is turning on metacopy (hence redirect)
> > by default now to see how do things go.
> >
> > So for me (redirect=on/off and metacopy=on/off) are important
> > configurations as of now. Having said that, I think I should talk
> > to container folks and encourage them to use "index" and "xino"
> > as well to be more posix like fs.
> >
>
> Hi Vivek,
>
> I remember you asked me about configuring extra mount options
> for unionmount but couldn't find that conversation, so replying to this
> related old discussion with my thoughts on the subject.
>
> Now that unionmount supports the environment variables:
> UNIONMOUNT_{BASEDIR,LOWERDIR,MNTPOINT}
>
> And now that xfstests has helpers to convert xfstests env vars to
> UNIONMOUNT_* env vars, one might ask: why won't we support
> UNIONMOUNT_OPTIONS=$OVERLAY_MOUNT_OPTIONS
>
> So when you asked me a question along those lines, my answer was that
> unionmount performs different validations depending on the test options,
> so for example, the test option ./run --meta adds the mount option
> "metacopy=on", but it also performs different validation tests, such as
> upper file st_blocks == 0 after metadata change.
>
> Right, so I gave a reason for why supporting extra mount options is not
> straight forward, but that doesn't mean that it is not possible.
> unionmount test could very well parse the extra mount options passed
> in env var and translate them to test config options.  As a matter of fact,
> unionmount already parses the following overlay module parameters
> and translates the following values to test config options:
>
> 1) redirect_dir does not exist => --xdev (expect EXDEV on dir rename)
> 2) redirect_dir exists and no explicit --xdev => add redirect_dir=on
> 3) index=N and --verify => add index=on and check st_ino validations
> 4) metacopy=Y => check --meta validations (e.g. upper st_blocks)
> 5) xino_auto=Y => add xino=on and check --xino validations (e.g. uniform st_dev)
>
> So apart from blindly adding the extra mount options to mount command,
> will also need to translate:
>
> 6) redirect_dir=off => --xdev
>    (redirect_dir=on conflicts with --xdev)
> 7) index=off => overrides index=on added by --verify
>    (st_ino validations should still pass on tests without multi layers)
> 8) metacopy=on => --meta
>    (metacopy=off conflicts with --meta)
> 9) xino=auto/on => --xino
>    (xino=off conflicts with --xino)
>
> At the moment, I have a patch to xfstests [1] that implements rule 8 in the
> xfstests _unionmount_testsuite_run helper, but I came to realize that would
> be wrong and that the correct way would be to implement conversion rules
> 6-9 in unionmount itself and then blindly assign in xfstest helper:
> UNIONMOUNT_OPTIONS=$OVL_BASE_MOUNT_OPTIONS
>

For whoever is interested, I implemented the above and pushed to:
https://github.com/amir73il/unionmount-testsuite/commits/mntopts

There are a few more corner cases I dealt with that are not mentioned above
(e.g. "redirect_dir=nofollow") as well as minor changes to "index" <=> --verify
dependency. For more details, see the commit message of attached patch.

There are a lot of combinations (mount options/module params/test options)
to test. I tested some manually, but might have missed something.

Thanks,
Amir.

[-- Attachment #2: unionmount-Add-support-for-user-defined-mount-options.patch.txt --]
[-- Type: text/plain, Size: 11058 bytes --]

From 4e7028221af426415e7de87f3e10607fbb8e6b21 Mon Sep 17 00:00:00 2001
From: Amir Goldstein <amir73il@gmail.com>
Date: Wed, 12 Aug 2020 22:12:25 +0300
Subject: [PATCH] Add support for user defined mount options

Until this change, the mount options used for an overalyfs mount where
determined by:
1) Support in the kernel by checking module params
2) Test run options: --xdev, --verify, --xino, --meta

Apart from determining the overlayfs mount options, the test run options
also impact the test logic.  For example, after an operation that changes
a lower file metadata, the test verifies that an upper file exists.
Without --meta, the test verifies that the upper file has data, while
with --meta, the test verifies that the upper file does not have data.

The following test options are implied from overlayfs module parameters:
1) xino_auto=Y => --xino
2) metacopy=Y => --metacopy

Add support for adding arbitrary overalyfs mount options with the
environment variable UNIONMOUNT_MNTOPTIONS.

The user provided mount options will be used for an overalyfs mount
and extra mount options could be added on top of them with the existing
test run options (e.g. --meta).

In order to adapt the test logic to the effective mount options, the
following mount options imply the respective test options:
1) "redirect_dir="<NOT "on"> => --xdev
2) "nfs_export=on" => "index=on" => --verify
3) "xino=on" => --xino
4) "metacopy=on" => --meta

When mount and test options are in conflict, the test options wins.
For example: "metacopy=off" and --meta result in metacopy enabled.

Features disabled by mount options override module parameter.
For example: "metacopy=off" and metacopy=Y result in metacopy disabled.

The conflict resolution of "index=off" and --verify is a special case.
The test option --verify activates test verifications that are executed
after every filesystem operation.  Some of those verification may fail
on multi layer tests with index feature disabled due to broken hardlinks.

Until this change, --verify would auto enable the index feature, so
verifications will not fail in multi layer tests.  When mount option
"index=off" is provided, --verify does not auto enable index feature.

To conform with behavior of test options --xino and --meta, the --verify
test option is now also implied when index feature is enabled by default
via module parameter and not only by mount option.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 mount_union.py   |  2 +-
 remount_union.py |  2 +-
 run              | 73 ++++++++++++++++++++++++++++++------------------
 settings.py      | 12 +++++---
 tool_box.py      | 18 ++++++++++++
 5 files changed, 74 insertions(+), 33 deletions(-)

diff --git a/mount_union.py b/mount_union.py
index aaaa242..8821695 100644
--- a/mount_union.py
+++ b/mount_union.py
@@ -41,7 +41,7 @@ def mount_union(ctx):
             os.mkdir(nested_upper)
             os.mkdir(nested_work)
 
-        mntopt = " -orw" + cfg.mntopts()
+        mntopt = " -o" + cfg.mntopts()
         if cfg.is_nested():
             nested_mntopt = mntopt
             if cfg.is_verify():
diff --git a/remount_union.py b/remount_union.py
index dc5525a..b2e8c3e 100644
--- a/remount_union.py
+++ b/remount_union.py
@@ -30,7 +30,7 @@ def remount_union(ctx, rotate_upper=False):
             workdir = layer_mntroot + "/w"
 
         mnt = union_mntroot
-        mntopt = " -orw" + cfg.mntopts()
+        mntopt = " -o" + cfg.mntopts()
         cmd = "mount -t " + cfg.fstype() + " " + cfg.fsname() + " " + mnt + mntopt + ",lowerdir=" + lowerlayers + ",upperdir=" + upperdir + ",workdir=" + workdir
         system(cmd)
         if cfg.is_verbose():
diff --git a/run b/run
index d841a45..4f35e51 100755
--- a/run
+++ b/run
@@ -103,35 +103,45 @@ if len(args) > 0 and args[0].startswith("--fuse="):
     cfg.set_fusefs(t)
     args = args[1:]
 
-if cfg.testing_none():
-    index = None
-    xino_auto = None
-    redirect_dir = None
-    metacopy = None
-elif cfg.is_fusefs():
-    # Don't check kernel module params for FUSE test and assume the following
-    # defaults, because user can disable redirect_dir with --xdev and user can
-    # enable index and xino with --verify
-    index = False
-    xino_auto = False
+xino = None
+index_def = None
+index_opt = None
+metacopy = None
+redirect_dir = None
+if cfg.is_fusefs():
+    # fuse-overlayfs only supports redirect_dir=off
+    # user can disable redirect_dir with --xdev
     redirect_dir = True
-    metacopy = False
 elif cfg.testing_overlayfs():
-    index = check_bool_modparam("index")
-    xino_auto = check_bool_modparam("xino_auto")
+    # Overlayfs feature "redirect_dir" is auto enabled with kernel version >= v4.10,
+    # unless explicitly disabled with --xdev or by mount option.  When redirect_dir
+    # is disabled, overlayfs tests skip rename tests that would result in EXDEV.
     redirect_dir = check_bool_modparam("redirect_dir")
-    # Overlayfs feature "redirect_dir" can be enabled with kernel version >= v4.10.
-    # Otherwise, overlayfs tests should skip rename tests that would result in EXDEV.
-    if redirect_dir is False:
+    redirect_dir_off = "redirect_dir" in cfg.mntopts() and not "redirect_dir=on" in cfg.mntopts()
+    if redirect_dir is False and not redirect_dir_off:
         cfg.add_mntopt("redirect_dir=on")
         redirect_dir = True
-    # xino can be enabled with kernel version >= v4.17
-    if xino_auto is True:
-        cfg.add_mntopt("xino=on")
+    # Overlayfs feature "index" can be enabled with --verify or by mount option on kernel version >= v4.13.
+    # When index is disabled some verifications in multi layer tests may fail due to broken hardlinks.
+    # When index is enabled we set_verify(), because all verifications are expected to pass.
+    index_def = check_bool_modparam("index")
+    if not index_def is None:
+        index_opt = check_bool_mntopt("index", cfg.mntopts(), None, onopt2="nfs_export=on")
+    if index_def or index_opt:
+        cfg.set_verify()
+    # Overlayfs feature "xino" can be enabled with --xino or by mount option on kernel version >= v4.17.
+    # When xino is enabled we set_xino(), so st_ino/st_dev verifications will work correctly.
+    xino_def = check_bool_modparam("xino_auto")
+    if not xino_def is None:
+        xino = check_bool_mntopt("xino", cfg.mntopts(), xino_def, onopt2="xino=auto")
+    if xino:
         cfg.set_xino()
-    # metacopy can be enabled with kernel version >= v4.19
-    metacopy = check_bool_modparam("metacopy")
-    if metacopy is True:
+    # Overlayfs feature "metacopy" can be enabled with --meta and by mount option on kernel version >= v4.19.
+    # When metacopy is enabled we set_metacopy(), so copy up verifications will work correctly.
+    metacopy_def = check_bool_modparam("metacopy")
+    if not metacopy_def is None:
+        metacopy = check_bool_mntopt("metacopy", cfg.mntopts(), metacopy_def, offopt2="nfs_export=on")
+    if metacopy:
         cfg.set_metacopy()
 
 maxfs = cfg.maxfs()
@@ -174,11 +184,11 @@ while len(args) > 0 and args[0].startswith("-"):
         termslash = "1"
     elif args[0] == "--xdev":
         # Disable "redirect_dir" and skip dir rename tests
-        if redirect_dir is True:
+        if redirect_dir:
             cfg.add_mntopt("redirect_dir=off")
             redirect_dir = False
     elif args[0] == "--xino":
-        if xino_auto is None:
+        if xino is None:
             print("xino not supported - ignoring --xino")
         else:
             cfg.add_mntopt("xino=on")
@@ -191,7 +201,8 @@ while len(args) > 0 and args[0].startswith("-"):
             cfg.set_metacopy()
     elif args[0] == "--verify":
         cfg.set_verify()
-        if index is False:
+        # auto enable index for --verify unless explicitly disabled
+        if index_def is False and not index_opt is False:
             cfg.add_mntopt("index=on")
     else:
         show_format("Invalid flag " + args[0])
@@ -296,7 +307,15 @@ for test in tests:
                 test_how += " --maxfs=" + str(maxfs)
             elif maxfs < 0:
                 test_how += " --samefs"
-        msg = cfg.progname() + " " + test_how + " --ts=" + termslash + " " + test
+            if not redirect_dir:
+                test_how += " --xdev"
+            if cfg.is_xino():
+                test_how += " --xino"
+            if cfg.is_metacopy():
+                test_how += " --meta"
+            if cfg.is_verify():
+                test_how += " --verify"
+        msg = cfg.progname() + " " + test_how + " " + test
         print("***");
         print("***", msg);
         print("***");
diff --git a/settings.py b/settings.py
index ffae527..b79e1f7 100644
--- a/settings.py
+++ b/settings.py
@@ -30,13 +30,18 @@ class config:
         self.__base_mntroot = os.getenv('UNIONMOUNT_BASEDIR')
         self.__lower_mntroot = os.getenv('UNIONMOUNT_LOWERDIR')
         self.__union_mntroot = os.getenv('UNIONMOUNT_MNTPOINT')
+        self.__mntopts = os.getenv('UNIONMOUNT_MNTOPTIONS')
         print("Environment variables:")
         if self.__base_mntroot:
-            print("UNIONMOUNT_BASEDIR=" + self.__base_mntroot)
+            print("UNIONMOUNT_BASEDIR='" + self.__base_mntroot + "'")
         if self.__lower_mntroot:
-            print("UNIONMOUNT_LOWERDIR=" + self.__lower_mntroot)
+            print("UNIONMOUNT_LOWERDIR='" + self.__lower_mntroot + "'")
         if self.__union_mntroot:
-            print("UNIONMOUNT_MNTPOINT=" + self.__union_mntroot)
+            print("UNIONMOUNT_MNTPOINT='" + self.__union_mntroot + "'")
+        if self.__mntopts:
+            print("UNIONMOUNT_MNTOPTIONS='" + self.__mntopts + "'")
+        else: # Use arbitrary non empty string to simplify add_mntopt()
+            self.__mntopts = "rw"
         print()
         if self.__base_mntroot and not self.__lower_mntroot:
             # Empty UNIONMOUNT_LOWERDIR with non-empty UNIONMOUNT_BASEDIR imply --samefs
@@ -50,7 +55,6 @@ class config:
         self.__metacopy = False
         self.__nested = False
         self.__xino = False
-        self.__mntopts = ""
         self.__fusefs = False
         self.__fstype = "overlay"
         self.__fsname = "overlay"
diff --git a/tool_box.py b/tool_box.py
index 03506fe..bf1f578 100644
--- a/tool_box.py
+++ b/tool_box.py
@@ -64,3 +64,21 @@ def check_bool_modparam(param):
     except FileNotFoundError:
         return None
     return value.startswith("Y")
+
+#
+# Check if overlay feature is enabled by mount option
+#
+# Return 'default' if mount option was not provided
+#
+def check_bool_mntopt(feature, mntopts, default, onopt2=None, offopt2=None):
+    onopt = feature + "=on"
+    offopt = feature + "=off"
+    on = onopt in mntopts or (onopt2 and onopt2 in mntopts)
+    off = offopt in mntopts or (offopt2 and offopt2 in mntopts)
+    if on and off:
+        raise RuntimeError("Conflicting mount options w.r.t feature '" + feature + "': " + mntopts)
+    if on:
+        return True
+    if off:
+        return False
+    return default;
-- 
2.17.1