On Tue, May 10, 2022 at 11:43:59PM +0800, Zorro Lang wrote: > On Tue, May 10, 2022 at 11:43:08AM +0200, Lukas Czerner wrote: > > On Mon, May 09, 2022 at 04:42:03PM -0700, Eric Biggers wrote: > > > On Sat, Apr 30, 2022 at 12:21:30PM -0700, Eric Biggers wrote: > > > > From: Eric Biggers <ebiggers@xxxxxxxxxx> > > > > > > > > 'not_mnt OPTIONS' seems to have been intended to test that the > > > > filesystem cannot be mounted at all with the given OPTIONS, meaning that > > > > the mount fails as opposed to the options being ignored. However, this > > > > doesn't actually work, as shown by the fact that the test case 'not_mnt > > > > test_dummy_encryption=v3' is passing in the !CONFIG_FS_ENCRYPTION case. > > > > Actually ext4 ignores this mount option when !CONFIG_FS_ENCRYPTION. > > > > (The ext4 behavior might be changed, but that is besides the point.) > > > > > > > > The problem is that the do_mnt() helper function is being misused in a > > > > context where a mount failure is expected, and it does some additional > > > > remount tests that don't make sense in that context. So if the mount > > > > unexpectedly succeeds, then one of these later tests can still "fail", > > > > causing the unexpected success to be shadowed by a later failure, which > > > > causes the overall test case to pass since it expects a failure. > > > > > > > > Fix this by reworking not_mnt() and not_remount_noumount() to use > > > > simple_mount() in cases where they are expecting a failure. Also fix > > > > up some of the naming and calling conventions to be less confusing. > > > > Finally, make sure to test that remounting fails too, not just mounting. > > > > > > > > Signed-off-by: Eric Biggers <ebiggers@xxxxxxxxxx> > > > > --- > > > > tests/ext4/053 | 148 ++++++++++++++++++++++++++----------------------- > > > > 1 file changed, 78 insertions(+), 70 deletions(-) > > > > > > Lukas, any thoughts on this patch? You're the author of this test. > > > > > > - Eric > > > > Haven't tested it myself but the change looks fine, thanks. > > Thanks for you help to review this patch. There's an new failure[1] after we > merged this patch: > "SHOULD FAIL remounting ext2 "commit=7" (remount unexpectedly succeeded) FAILED" > > As this test generally passed, so before I give "Oops" to others, I hope to > check with you that if this's an expected failure we need to fix in kernel > or in this case itself? > This appears to be a kernel bug, so to fix it I've sent the patch "ext4: reject the 'commit' option on ext2 filesystems" (https://lore.kernel.org/r/20220510183232.172615-1-ebiggers@xxxxxxxxxx). I didn't notice this earlier because it's not reproducible with CONFIG_EXT2_FS=y. But it is reproducible with CONFIG_EXT2_FS=n and CONFIG_EXT4_USE_FOR_EXT2=y. - Eric
