Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 11 May 2022 14:25:04 +0800
From:   Zorro Lang <zlang@redhat.com>
To:     Eric Biggers <ebiggers@kernel.org>
Cc:     Lukas Czerner <lczerner@redhat.com>, fstests@vger.kernel.org,
        linux-ext4@vger.kernel.org
Subject: Re: [xfstests PATCH] ext4/053: fix the rejected mount option testing
Message-ID: <20220511062504.c4ed7rhdyqfe54y6@zlang-mailbox>
Mail-Followup-To: Eric Biggers <ebiggers@kernel.org>,
        Lukas Czerner <lczerner@redhat.com>, fstests@vger.kernel.org,
        linux-ext4@vger.kernel.org
References: <20220430192130.131842-1-ebiggers@kernel.org>
 <Ynmmy+bWp0Q1/747@sol.localdomain>
 <20220510094308.mhzvcgq5wrat5qao@fedora>
 <20220510154359.xfhmumcmb4o37qdy@zlang-mailbox>
 <Ynqwotv9lQvt3TV3@sol.localdomain>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Ynqwotv9lQvt3TV3@sol.localdomain>
Precedence: bulk

On Tue, May 10, 2022 at 11:36:18AM -0700, Eric Biggers wrote:
> 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@google.com>
> > > > > 
> > > > > '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@google.com>
> > > > > ---
> > > > >  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@kernel.org).

Thanks, great to know that.

> 
> 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.

Yes, I tested with this config. I'll remind this failure in next fstests
announcement, Thanks a lot!

Thanks,
Zorro

> 
> - Eric
>