mbox series

[xfstests,0/2] update test_dummy_encryption testing in ext4/053

Message ID 20220501051928.540278-1-ebiggers@kernel.org (mailing list archive)
Headers show
Series update test_dummy_encryption testing in ext4/053 | expand

Message

Eric Biggers May 1, 2022, 5:19 a.m. UTC
This series updates the testing of the test_dummy_encryption mount
option in ext4/053.

The first patch will be needed for the test to pass if the kernel patch
"ext4: only allow test_dummy_encryption when supported"
(https://lore.kernel.org/r/20220501050857.538984-2-ebiggers@kernel.org)
is applied.

The second patch starts testing a case that previously wasn't tested.
It reproduces a bug that was introduced in the v5.17 kernel and will
be fixed by the kernel patch
"ext4: fix up test_dummy_encryption handling for new mount API"
(https://lore.kernel.org/r/20220501050857.538984-6-ebiggers@kernel.org).

This applies on top of my recent patch
"ext4/053: fix the rejected mount option testing"
(https://lore.kernel.org/r/20220430192130.131842-1-ebiggers@kernel.org).

Eric Biggers (2):
  ext4/053: update the test_dummy_encryption tests
  ext4/053: test changing test_dummy_encryption on remount

 tests/ext4/053 | 38 ++++++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 14 deletions(-)

Comments

Zorro Lang May 18, 2022, 2:19 p.m. UTC | #1
On Sat, Apr 30, 2022 at 10:19:26PM -0700, Eric Biggers wrote:
> This series updates the testing of the test_dummy_encryption mount
> option in ext4/053.
> 
> The first patch will be needed for the test to pass if the kernel patch
> "ext4: only allow test_dummy_encryption when supported"
> (https://lore.kernel.org/r/20220501050857.538984-2-ebiggers@kernel.org)
> is applied.
> 
> The second patch starts testing a case that previously wasn't tested.
> It reproduces a bug that was introduced in the v5.17 kernel and will
> be fixed by the kernel patch
> "ext4: fix up test_dummy_encryption handling for new mount API"
> (https://lore.kernel.org/r/20220501050857.538984-6-ebiggers@kernel.org).
> 
> This applies on top of my recent patch
> "ext4/053: fix the rejected mount option testing"
> (https://lore.kernel.org/r/20220430192130.131842-1-ebiggers@kernel.org).

Hi Eric,

Your "ext4/053: fix the rejected mount option testing" has been merged. As the
two kernel patches haven't been merged by upstream linux, I'd like to merge
this patchset after the kernel patches be merged. (feel free to ping me, if
I forget this:)

And I saw some discussion under this patchset, and no any RVB, so I'm wondering
if you are still working/changing on it?

Thanks,
Zorro

> 
> Eric Biggers (2):
>   ext4/053: update the test_dummy_encryption tests
>   ext4/053: test changing test_dummy_encryption on remount
> 
>  tests/ext4/053 | 38 ++++++++++++++++++++++++--------------
>  1 file changed, 24 insertions(+), 14 deletions(-)
> 
> -- 
> 2.36.0
>
Eric Biggers May 18, 2022, 5:37 p.m. UTC | #2
On Wed, May 18, 2022 at 10:19:11PM +0800, Zorro Lang wrote:
> On Sat, Apr 30, 2022 at 10:19:26PM -0700, Eric Biggers wrote:
> > This series updates the testing of the test_dummy_encryption mount
> > option in ext4/053.
> > 
> > The first patch will be needed for the test to pass if the kernel patch
> > "ext4: only allow test_dummy_encryption when supported"
> > (https://lore.kernel.org/r/20220501050857.538984-2-ebiggers@kernel.org)
> > is applied.
> > 
> > The second patch starts testing a case that previously wasn't tested.
> > It reproduces a bug that was introduced in the v5.17 kernel and will
> > be fixed by the kernel patch
> > "ext4: fix up test_dummy_encryption handling for new mount API"
> > (https://lore.kernel.org/r/20220501050857.538984-6-ebiggers@kernel.org).
> > 
> > This applies on top of my recent patch
> > "ext4/053: fix the rejected mount option testing"
> > (https://lore.kernel.org/r/20220430192130.131842-1-ebiggers@kernel.org).
> 
> Hi Eric,
> 
> Your "ext4/053: fix the rejected mount option testing" has been merged. As the
> two kernel patches haven't been merged by upstream linux, I'd like to merge
> this patchset after the kernel patches be merged. (feel free to ping me, if
> I forget this:)

Yes, I'm waiting for them to be applied.

> 
> And I saw some discussion under this patchset, and no any RVB, so I'm wondering
> if you are still working/changing on it?
> 

I might add a check for kernel version >= 5.19 in patch 1.  Otherwise I'm not
planning any more changes.

- Eric
Zorro Lang May 18, 2022, 6:16 p.m. UTC | #3
On Wed, May 18, 2022 at 10:37:47AM -0700, Eric Biggers wrote:
> On Wed, May 18, 2022 at 10:19:11PM +0800, Zorro Lang wrote:
> > On Sat, Apr 30, 2022 at 10:19:26PM -0700, Eric Biggers wrote:
> > > This series updates the testing of the test_dummy_encryption mount
> > > option in ext4/053.
> > > 
> > > The first patch will be needed for the test to pass if the kernel patch
> > > "ext4: only allow test_dummy_encryption when supported"
> > > (https://lore.kernel.org/r/20220501050857.538984-2-ebiggers@kernel.org)
> > > is applied.
> > > 
> > > The second patch starts testing a case that previously wasn't tested.
> > > It reproduces a bug that was introduced in the v5.17 kernel and will
> > > be fixed by the kernel patch
> > > "ext4: fix up test_dummy_encryption handling for new mount API"
> > > (https://lore.kernel.org/r/20220501050857.538984-6-ebiggers@kernel.org).
> > > 
> > > This applies on top of my recent patch
> > > "ext4/053: fix the rejected mount option testing"
> > > (https://lore.kernel.org/r/20220430192130.131842-1-ebiggers@kernel.org).
> > 
> > Hi Eric,
> > 
> > Your "ext4/053: fix the rejected mount option testing" has been merged. As the
> > two kernel patches haven't been merged by upstream linux, I'd like to merge
> > this patchset after the kernel patches be merged. (feel free to ping me, if
> > I forget this:)
> 
> Yes, I'm waiting for them to be applied.

Thanks, I'll review this patches after your kernel patches be merged. Please
remind me, if I don't notice that in time.

> 
> > 
> > And I saw some discussion under this patchset, and no any RVB, so I'm wondering
> > if you are still working/changing on it?
> > 
> 
> I might add a check for kernel version >= 5.19 in patch 1.  Otherwise I'm not
> planning any more changes.

Actually I don't think the kernel version check (in fstests) is a good method. Better
to check a behavior/feature directly likes those "_require_*" functions.

Why ext4/053 need >=5.12 or even >=5.19, what features restrict that? If some
features testing might break the garden image (.out file), we can refer to
_link_out_file(). Or even split this case to several small cases, make ext4/053
only test old stable behaviors. Then use other cases to test new features,
and use _require_$feature_you_test for them (avoid the kernel version
restriction).

Thanks,
Zorro

> 
> - Eric
>
Eric Biggers May 18, 2022, 10:01 p.m. UTC | #4
Zorro, can you fix your email configuration?  Your emails have a
Mail-Followup-To header that excludes you, so replying doesn't work correctly;
I had to manually fix the recipients list.  If you're using mutt, you need to
add 'set followup_to = no' to your muttrc.

On Thu, May 19, 2022 at 02:16:07AM +0800, Zorro Lang wrote:
> > > 
> > > And I saw some discussion under this patchset, and no any RVB, so I'm wondering
> > > if you are still working/changing on it?
> > > 
> > 
> > I might add a check for kernel version >= 5.19 in patch 1.  Otherwise I'm not
> > planning any more changes.
> 
> Actually I don't think the kernel version check (in fstests) is a good method. Better
> to check a behavior/feature directly likes those "_require_*" functions.
> 
> Why ext4/053 need >=5.12 or even >=5.19, what features restrict that? If some
> features testing might break the garden image (.out file), we can refer to
> _link_out_file(). Or even split this case to several small cases, make ext4/053
> only test old stable behaviors. Then use other cases to test new features,
> and use _require_$feature_you_test for them (avoid the kernel version
> restriction).

This has been discussed earlier in this thread as well as on the patch that
added ext4/053 originally.  ext4/053 has been gated on version >= 5.12 since the
beginning.  Kernel version checks are certainly bad in general, but ext4/053 is
a very nit-picky test intended to detect if anything changed, where a change
does not necessarily mean a bug.  So maybe the kernel version check makes sense
there.  Lukas, any thoughts about the issues you encountered when running
ext4/053 on older kernels?

If you don't want a >= 5.19 version check for the test_dummy_encryption test
case as well, then I'd rather treat the kernel patch
"ext4: only allow test_dummy_encryption when supported" as a bug fix and
backport it to the LTS kernels.  The patch is fixing the mount option to work
the way it should have worked originally.  Either that or we just remove the
test_dummy_encryption test case as Ted suggested.

- Eric
Zorro Lang May 19, 2022, 4:47 a.m. UTC | #5
On Wed, May 18, 2022 at 03:01:08PM -0700, Eric Biggers wrote:
> Zorro, can you fix your email configuration?  Your emails have a
> Mail-Followup-To header that excludes you, so replying doesn't work correctly;
> I had to manually fix the recipients list.  If you're using mutt, you need to
> add 'set followup_to = no' to your muttrc.

Oh, I didn't notice that, I use neomutt, it might enable the followup_to by
default. OK, I've set followup_to = no, and restart my neomutt. Hope it helps:)

> 
> On Thu, May 19, 2022 at 02:16:07AM +0800, Zorro Lang wrote:
> > > > 
> > > > And I saw some discussion under this patchset, and no any RVB, so I'm wondering
> > > > if you are still working/changing on it?
> > > > 
> > > 
> > > I might add a check for kernel version >= 5.19 in patch 1.  Otherwise I'm not
> > > planning any more changes.
> > 
> > Actually I don't think the kernel version check (in fstests) is a good method. Better
> > to check a behavior/feature directly likes those "_require_*" functions.
> > 
> > Why ext4/053 need >=5.12 or even >=5.19, what features restrict that? If some
> > features testing might break the garden image (.out file), we can refer to
> > _link_out_file(). Or even split this case to several small cases, make ext4/053
> > only test old stable behaviors. Then use other cases to test new features,
> > and use _require_$feature_you_test for them (avoid the kernel version
> > restriction).
> 
> This has been discussed earlier in this thread as well as on the patch that
> added ext4/053 originally.  ext4/053 has been gated on version >= 5.12 since the
> beginning.  Kernel version checks are certainly bad in general, but ext4/053 is
> a very nit-picky test intended to detect if anything changed, where a change
> does not necessarily mean a bug.  So maybe the kernel version check makes sense

Even on old RHEL-8 system (with a variant of kernel 3.10), the ext4/053 fails
as [1]. Most of mount options test passed, only a few options (inlinecrypt,
test_dummy_encryption, prefetch_block_bitmaps, dioread_lock) might not be
supported.

I think it's not necessary to mix all old and new ext4 mount options test into
one single test cause. If it's too complicated, we can move some functions into
common/ext4 (or others you like), split ext4/053 to several cases. Let ext4/053
test stable enough mount option (supported from an old enough kernel). Then let
other newer mount options in different single cases.

For example, make those CONFIG_FS_ENCRYPTION* tests into a seperated case,
and add something likes require_(fs_encryption?), and src/feature.c can be
used too. Then about dioread_lock and prefetch_block_bitmaps things, we can
deal with them specially, or split them out from ext4/053. I even don't mind
if you test ext2 and ext3/4 in separate case.

That's my personal opinion, I can try to help to do that after merging this
patchset, if ext4 forks agree and would like to give me some supports
(review and Q&A). Anyway, as it's an ext4 specific testing, I respect the
opinion from ext4 list particularly.

[1]
+SHOULD FAIL remounting ext2 "commit=7" (remount unexpectedly succeeded) FAILED
+mounting ext2 "test_dummy_encryption=v1" (failed mount) FAILED
+mounting ext2 "test_dummy_encryption=v2" (failed mount) FAILED
+mounting ext2 "test_dummy_encryption=v3" (failed mount) FAILED
+mounting ext2 "inlinecrypt" (failed mount) FAILED
+mounting ext2 "prefetch_block_bitmaps" (failed mount) FAILED
+mounting ext2 "no_prefetch_block_bitmaps" (failed mount) FAILED
+mounting ext3 "test_dummy_encryption=v1" (failed mount) FAILED
+mounting ext3 "test_dummy_encryption=v2" (failed mount) FAILED
+mounting ext3 "test_dummy_encryption=v3" (failed mount) FAILED
+mounting ext3 "inlinecrypt" (failed mount) FAILED
+mounting ext3 "prefetch_block_bitmaps" (failed mount) FAILED
+mounting ext3 "no_prefetch_block_bitmaps" (failed mount) FAILED
+mounting ext4 "nodioread_nolock" (failed mount) FAILED
+mounting ext4 "dioread_lock" checking "nodioread_nolock" (not found) FAILED
+mounting ext4 "test_dummy_encryption=v1" (failed mount) FAILED
+mounting ext4 "test_dummy_encryption=v2" (failed mount) FAILED
+mounting ext4 "test_dummy_encryption=v3" (failed mount) FAILED
+mounting ext4 "inlinecrypt" (failed mount) FAILED
+mounting ext4 "prefetch_block_bitmaps" (failed mount) FAILED
+mounting ext4 "no_prefetch_block_bitmaps" (failed mount) FAILED

> there.  Lukas, any thoughts about the issues you encountered when running
> ext4/053 on older kernels?
> 
> If you don't want a >= 5.19 version check for the test_dummy_encryption test
> case as well, then I'd rather treat the kernel patch
> "ext4: only allow test_dummy_encryption when supported" as a bug fix and
> backport it to the LTS kernels.  The patch is fixing the mount option to work
> the way it should have worked originally.  Either that or we just remove the
> test_dummy_encryption test case as Ted suggested.

Oh, I'd not like to push anyone to do more jobs:) And there're many Linux
distributions with their downstream kernel, not only LTS kernel project.
So I don't mean to make fstests' cases support the oldest existing kernel
version, just hope some common cases try not *only* work for the latest
one, if they have the chance :)

Thanks,
Zorro

> 
> - Eric
>
Lukas Czerner May 19, 2022, 8:10 a.m. UTC | #6
On Wed, May 18, 2022 at 03:01:08PM -0700, Eric Biggers wrote:
> Zorro, can you fix your email configuration?  Your emails have a
> Mail-Followup-To header that excludes you, so replying doesn't work correctly;
> I had to manually fix the recipients list.  If you're using mutt, you need to
> add 'set followup_to = no' to your muttrc.
> 
> On Thu, May 19, 2022 at 02:16:07AM +0800, Zorro Lang wrote:
> > > > 
> > > > And I saw some discussion under this patchset, and no any RVB, so I'm wondering
> > > > if you are still working/changing on it?
> > > > 
> > > 
> > > I might add a check for kernel version >= 5.19 in patch 1.  Otherwise I'm not
> > > planning any more changes.
> > 
> > Actually I don't think the kernel version check (in fstests) is a good method. Better
> > to check a behavior/feature directly likes those "_require_*" functions.
> > 
> > Why ext4/053 need >=5.12 or even >=5.19, what features restrict that? If some
> > features testing might break the garden image (.out file), we can refer to
> > _link_out_file(). Or even split this case to several small cases, make ext4/053
> > only test old stable behaviors. Then use other cases to test new features,
> > and use _require_$feature_you_test for them (avoid the kernel version
> > restriction).
> 
> This has been discussed earlier in this thread as well as on the patch that
> added ext4/053 originally.  ext4/053 has been gated on version >= 5.12 since the
> beginning.  Kernel version checks are certainly bad in general, but ext4/053 is
> a very nit-picky test intended to detect if anything changed, where a change
> does not necessarily mean a bug.  So maybe the kernel version check makes sense
> there.  Lukas, any thoughts about the issues you encountered when running
> ext4/053 on older kernels?

No I haven't encountered any problems, it works fine. I think kernel
version gating in this case it's adequate technical solution for the
problem we have. We want this test to be very nitpicky so that we really
do notice user facing mount behavior change on one hand, while we still
want to have some flexibility.

> 
> If you don't want a >= 5.19 version check for the test_dummy_encryption test
> case as well, then I'd rather treat the kernel patch
> "ext4: only allow test_dummy_encryption when supported" as a bug fix and
> backport it to the LTS kernels.  The patch is fixing the mount option to work
> the way it should have worked originally.  Either that or we just remove the
> test_dummy_encryption test case as Ted suggested.

Both is fine with me, but I would have a preference to treat it as a bug
fix and let the test fail on older kernels without the patch.

-Lukas

> 
> - Eric
>
Lukas Czerner May 19, 2022, 8:33 a.m. UTC | #7
On Thu, May 19, 2022 at 12:47:01PM +0800, Zorro Lang wrote:
> On Wed, May 18, 2022 at 03:01:08PM -0700, Eric Biggers wrote:
> > Zorro, can you fix your email configuration?  Your emails have a
> > Mail-Followup-To header that excludes you, so replying doesn't work correctly;
> > I had to manually fix the recipients list.  If you're using mutt, you need to
> > add 'set followup_to = no' to your muttrc.
> 
> Oh, I didn't notice that, I use neomutt, it might enable the followup_to by
> default. OK, I've set followup_to = no, and restart my neomutt. Hope it helps:)
> 
> > 
> > On Thu, May 19, 2022 at 02:16:07AM +0800, Zorro Lang wrote:
> > > > > 
> > > > > And I saw some discussion under this patchset, and no any RVB, so I'm wondering
> > > > > if you are still working/changing on it?
> > > > > 
> > > > 
> > > > I might add a check for kernel version >= 5.19 in patch 1.  Otherwise I'm not
> > > > planning any more changes.
> > > 
> > > Actually I don't think the kernel version check (in fstests) is a good method. Better
> > > to check a behavior/feature directly likes those "_require_*" functions.
> > > 
> > > Why ext4/053 need >=5.12 or even >=5.19, what features restrict that? If some
> > > features testing might break the garden image (.out file), we can refer to
> > > _link_out_file(). Or even split this case to several small cases, make ext4/053
> > > only test old stable behaviors. Then use other cases to test new features,
> > > and use _require_$feature_you_test for them (avoid the kernel version
> > > restriction).
> > 
> > This has been discussed earlier in this thread as well as on the patch that
> > added ext4/053 originally.  ext4/053 has been gated on version >= 5.12 since the
> > beginning.  Kernel version checks are certainly bad in general, but ext4/053 is
> > a very nit-picky test intended to detect if anything changed, where a change
> > does not necessarily mean a bug.  So maybe the kernel version check makes sense
> 
> Even on old RHEL-8 system (with a variant of kernel 3.10), the ext4/053 fails
> as [1]. Most of mount options test passed, only a few options (inlinecrypt,
> test_dummy_encryption, prefetch_block_bitmaps, dioread_lock) might not be
> supported.

No it does not. On RHEL-8 system the test will not run because of kernel
version test. It will be skipped.

> 
> I think it's not necessary to mix all old and new ext4 mount options test into
> one single test cause. If it's too complicated, we can move some functions into
> common/ext4 (or others you like), split ext4/053 to several cases. Let ext4/053
> test stable enough mount option (supported from an old enough kernel). Then let
> other newer mount options in different single cases.
> 
> For example, make those CONFIG_FS_ENCRYPTION* tests into a seperated case,
> and add something likes require_(fs_encryption?), and src/feature.c can be
> used too. Then about dioread_lock and prefetch_block_bitmaps things, we can
> deal with them specially, or split them out from ext4/053. I even don't mind
> if you test ext2 and ext3/4 in separate case.

Sure, but why to split it? It all should be stable enough, it's user
facing interface, that's the whole point of the test. I certainly see
the benefit of having the test for all ext4 mount option in one test -
it's faster and it's easier to see what's there. I would be agains
splitting it.

As it is now there is only one kernel_gte() check to avoid testing the
entire history. With any new mount option as a separate test we would
still need kernel_gte test to avoid failing on kernels that don't have
the mount option. At least until kernel gains ability to list supported
mount options it's the only test we have.

On the other hand I do see some value in making a new test for a new
mount option. But I don't have a strong opinion about that.

As for the original topic of the discussion, as I said in previous
reply, maybe the right solution here is to treat the change as a bug fix
which is arguably is and let it fail on old behavior.

Thanks!
-Lukas

> 
> That's my personal opinion, I can try to help to do that after merging this
> patchset, if ext4 forks agree and would like to give me some supports
> (review and Q&A). Anyway, as it's an ext4 specific testing, I respect the
> opinion from ext4 list particularly.
> 
> [1]
> +SHOULD FAIL remounting ext2 "commit=7" (remount unexpectedly succeeded) FAILED
> +mounting ext2 "test_dummy_encryption=v1" (failed mount) FAILED
> +mounting ext2 "test_dummy_encryption=v2" (failed mount) FAILED
> +mounting ext2 "test_dummy_encryption=v3" (failed mount) FAILED
> +mounting ext2 "inlinecrypt" (failed mount) FAILED
> +mounting ext2 "prefetch_block_bitmaps" (failed mount) FAILED
> +mounting ext2 "no_prefetch_block_bitmaps" (failed mount) FAILED
> +mounting ext3 "test_dummy_encryption=v1" (failed mount) FAILED
> +mounting ext3 "test_dummy_encryption=v2" (failed mount) FAILED
> +mounting ext3 "test_dummy_encryption=v3" (failed mount) FAILED
> +mounting ext3 "inlinecrypt" (failed mount) FAILED
> +mounting ext3 "prefetch_block_bitmaps" (failed mount) FAILED
> +mounting ext3 "no_prefetch_block_bitmaps" (failed mount) FAILED
> +mounting ext4 "nodioread_nolock" (failed mount) FAILED
> +mounting ext4 "dioread_lock" checking "nodioread_nolock" (not found) FAILED
> +mounting ext4 "test_dummy_encryption=v1" (failed mount) FAILED
> +mounting ext4 "test_dummy_encryption=v2" (failed mount) FAILED
> +mounting ext4 "test_dummy_encryption=v3" (failed mount) FAILED
> +mounting ext4 "inlinecrypt" (failed mount) FAILED
> +mounting ext4 "prefetch_block_bitmaps" (failed mount) FAILED
> +mounting ext4 "no_prefetch_block_bitmaps" (failed mount) FAILED
> 
> > there.  Lukas, any thoughts about the issues you encountered when running
> > ext4/053 on older kernels?
> > 
> > If you don't want a >= 5.19 version check for the test_dummy_encryption test
> > case as well, then I'd rather treat the kernel patch
> > "ext4: only allow test_dummy_encryption when supported" as a bug fix and
> > backport it to the LTS kernels.  The patch is fixing the mount option to work
> > the way it should have worked originally.  Either that or we just remove the
> > test_dummy_encryption test case as Ted suggested.
> 
> Oh, I'd not like to push anyone to do more jobs:) And there're many Linux
> distributions with their downstream kernel, not only LTS kernel project.
> So I don't mean to make fstests' cases support the oldest existing kernel
> version, just hope some common cases try not *only* work for the latest
> one, if they have the chance :)
> 
> Thanks,
> Zorro
> 
> > 
> > - Eric
> > 
>
Zorro Lang May 19, 2022, 10:40 a.m. UTC | #8
On Thu, May 19, 2022 at 10:33:22AM +0200, Lukas Czerner wrote:
> On Thu, May 19, 2022 at 12:47:01PM +0800, Zorro Lang wrote:
> > On Wed, May 18, 2022 at 03:01:08PM -0700, Eric Biggers wrote:
> > > Zorro, can you fix your email configuration?  Your emails have a
> > > Mail-Followup-To header that excludes you, so replying doesn't work correctly;
> > > I had to manually fix the recipients list.  If you're using mutt, you need to
> > > add 'set followup_to = no' to your muttrc.
> > 
> > Oh, I didn't notice that, I use neomutt, it might enable the followup_to by
> > default. OK, I've set followup_to = no, and restart my neomutt. Hope it helps:)
> > 
> > > 
> > > On Thu, May 19, 2022 at 02:16:07AM +0800, Zorro Lang wrote:
> > > > > > 
> > > > > > And I saw some discussion under this patchset, and no any RVB, so I'm wondering
> > > > > > if you are still working/changing on it?
> > > > > > 
> > > > > 
> > > > > I might add a check for kernel version >= 5.19 in patch 1.  Otherwise I'm not
> > > > > planning any more changes.
> > > > 
> > > > Actually I don't think the kernel version check (in fstests) is a good method. Better
> > > > to check a behavior/feature directly likes those "_require_*" functions.
> > > > 
> > > > Why ext4/053 need >=5.12 or even >=5.19, what features restrict that? If some
> > > > features testing might break the garden image (.out file), we can refer to
> > > > _link_out_file(). Or even split this case to several small cases, make ext4/053
> > > > only test old stable behaviors. Then use other cases to test new features,
> > > > and use _require_$feature_you_test for them (avoid the kernel version
> > > > restriction).
> > > 
> > > This has been discussed earlier in this thread as well as on the patch that
> > > added ext4/053 originally.  ext4/053 has been gated on version >= 5.12 since the
> > > beginning.  Kernel version checks are certainly bad in general, but ext4/053 is
> > > a very nit-picky test intended to detect if anything changed, where a change
> > > does not necessarily mean a bug.  So maybe the kernel version check makes sense
> > 
> > Even on old RHEL-8 system (with a variant of kernel 3.10), the ext4/053 fails
> > as [1]. Most of mount options test passed, only a few options (inlinecrypt,
> > test_dummy_encryption, prefetch_block_bitmaps, dioread_lock) might not be
> > supported.
> 
> No it does not. On RHEL-8 system the test will not run because of kernel
> version test. It will be skipped.

Yes, it will be skipped, I just ran it by removing that "kernel_gte 5.12" line :)

> 
> > 
> > I think it's not necessary to mix all old and new ext4 mount options test into
> > one single test cause. If it's too complicated, we can move some functions into
> > common/ext4 (or others you like), split ext4/053 to several cases. Let ext4/053
> > test stable enough mount option (supported from an old enough kernel). Then let
> > other newer mount options in different single cases.
> > 
> > For example, make those CONFIG_FS_ENCRYPTION* tests into a seperated case,
> > and add something likes require_(fs_encryption?), and src/feature.c can be
> > used too. Then about dioread_lock and prefetch_block_bitmaps things, we can
> > deal with them specially, or split them out from ext4/053. I even don't mind
> > if you test ext2 and ext3/4 in separate case.
> 
> Sure, but why to split it? It all should be stable enough, it's user
> facing interface, that's the whole point of the test. I certainly see
> the benefit of having the test for all ext4 mount option in one test -
> it's faster and it's easier to see what's there. I would be agains
> splitting it.

OK, although you can have a 'group name' to help to run all ext4 mount options
regression test, but as I said: "as it's an ext4 specific testing, I respect
the opinion from ext4 list particularly", so I won't touch this case, if you
against :)

> 
> As it is now there is only one kernel_gte() check to avoid testing the
> entire history. With any new mount option as a separate test we would
> still need kernel_gte test to avoid failing on kernels that don't have
> the mount option. At least until kernel gains ability to list supported
> mount options it's the only test we have.
> 
> On the other hand I do see some value in making a new test for a new
> mount option. But I don't have a strong opinion about that.
> 
> As for the original topic of the discussion, as I said in previous
> reply, maybe the right solution here is to treat the change as a bug fix
> which is arguably is and let it fail on old behavior.
> 
> Thanks!
> -Lukas
> 
> > 
> > That's my personal opinion, I can try to help to do that after merging this
> > patchset, if ext4 forks agree and would like to give me some supports
> > (review and Q&A). Anyway, as it's an ext4 specific testing, I respect the
> > opinion from ext4 list particularly.
> > 
> > [1]
> > +SHOULD FAIL remounting ext2 "commit=7" (remount unexpectedly succeeded) FAILED
> > +mounting ext2 "test_dummy_encryption=v1" (failed mount) FAILED
> > +mounting ext2 "test_dummy_encryption=v2" (failed mount) FAILED
> > +mounting ext2 "test_dummy_encryption=v3" (failed mount) FAILED
> > +mounting ext2 "inlinecrypt" (failed mount) FAILED
> > +mounting ext2 "prefetch_block_bitmaps" (failed mount) FAILED
> > +mounting ext2 "no_prefetch_block_bitmaps" (failed mount) FAILED
> > +mounting ext3 "test_dummy_encryption=v1" (failed mount) FAILED
> > +mounting ext3 "test_dummy_encryption=v2" (failed mount) FAILED
> > +mounting ext3 "test_dummy_encryption=v3" (failed mount) FAILED
> > +mounting ext3 "inlinecrypt" (failed mount) FAILED
> > +mounting ext3 "prefetch_block_bitmaps" (failed mount) FAILED
> > +mounting ext3 "no_prefetch_block_bitmaps" (failed mount) FAILED
> > +mounting ext4 "nodioread_nolock" (failed mount) FAILED
> > +mounting ext4 "dioread_lock" checking "nodioread_nolock" (not found) FAILED
> > +mounting ext4 "test_dummy_encryption=v1" (failed mount) FAILED
> > +mounting ext4 "test_dummy_encryption=v2" (failed mount) FAILED
> > +mounting ext4 "test_dummy_encryption=v3" (failed mount) FAILED
> > +mounting ext4 "inlinecrypt" (failed mount) FAILED
> > +mounting ext4 "prefetch_block_bitmaps" (failed mount) FAILED
> > +mounting ext4 "no_prefetch_block_bitmaps" (failed mount) FAILED
> > 
> > > there.  Lukas, any thoughts about the issues you encountered when running
> > > ext4/053 on older kernels?
> > > 
> > > If you don't want a >= 5.19 version check for the test_dummy_encryption test
> > > case as well, then I'd rather treat the kernel patch
> > > "ext4: only allow test_dummy_encryption when supported" as a bug fix and
> > > backport it to the LTS kernels.  The patch is fixing the mount option to work
> > > the way it should have worked originally.  Either that or we just remove the
> > > test_dummy_encryption test case as Ted suggested.
> > 
> > Oh, I'd not like to push anyone to do more jobs:) And there're many Linux
> > distributions with their downstream kernel, not only LTS kernel project.
> > So I don't mean to make fstests' cases support the oldest existing kernel
> > version, just hope some common cases try not *only* work for the latest
> > one, if they have the chance :)
> > 
> > Thanks,
> > Zorro
> > 
> > > 
> > > - Eric
> > > 
> > 
>
Lukas Czerner May 19, 2022, 10:58 a.m. UTC | #9
On Sat, Apr 30, 2022 at 10:19:26PM -0700, Eric Biggers wrote:
> This series updates the testing of the test_dummy_encryption mount
> option in ext4/053.
> 
> The first patch will be needed for the test to pass if the kernel patch
> "ext4: only allow test_dummy_encryption when supported"
> (https://lore.kernel.org/r/20220501050857.538984-2-ebiggers@kernel.org)
> is applied.
> 
> The second patch starts testing a case that previously wasn't tested.
> It reproduces a bug that was introduced in the v5.17 kernel and will
> be fixed by the kernel patch
> "ext4: fix up test_dummy_encryption handling for new mount API"
> (https://lore.kernel.org/r/20220501050857.538984-6-ebiggers@kernel.org).
> 
> This applies on top of my recent patch
> "ext4/053: fix the rejected mount option testing"
> (https://lore.kernel.org/r/20220430192130.131842-1-ebiggers@kernel.org).
> 
> Eric Biggers (2):
>   ext4/053: update the test_dummy_encryption tests
>   ext4/053: test changing test_dummy_encryption on remount
> 
>  tests/ext4/053 | 38 ++++++++++++++++++++++++--------------
>  1 file changed, 24 insertions(+), 14 deletions(-)
> 
> -- 
> 2.36.0

The series looks good to me, you can add

Reviewed-by: Lukas Czerner <lczerner@redhat.com>