Re: [PATCH] ext4/01{2,9}: remove invalid filesystem option 'journal'

From: "Darrick J. Wong" <djwong@kernel.org>
To: Theodore Ts'o <tytso@mit.edu>
Cc: "Luis Henriques (SUSE)" <luis.henriques@linux.dev>,
	fstests@vger.kernel.org
Subject: Re: [PATCH] ext4/01{2,9}: remove invalid filesystem option 'journal'
Date: Fri, 29 Mar 2024 16:08:59 -0700	[thread overview]
Message-ID: <20240329230859.GF6379@frogsfrogsfrogs> (raw)
In-Reply-To: <20240329225020.GB1189142@mit.edu>

On Fri, Mar 29, 2024 at 06:50:20PM -0400, Theodore Ts'o wrote:
> On Thu, Mar 28, 2024 at 05:06:20PM +0000, Luis Henriques (SUSE) wrote:
> > Creating an ext4 filesystem using '-O journal' will fail with:
> > 
> >     Invalid filesystem option set: journal
> > 
> > I didn't do any archaeological investigation to check if this option ever
> > existed, but the two tests using it will fail to create the scratch
> > filesystems.
> > 
> > Signed-off-by: Luis Henriques (SUSE) <luis.henriques@linux.dev>
> 
> The feature name has never been journal, but rather has_journal.  The
> reason why no once noticed is because the file system was created by
> the _require_attrs before the attempted _scratch_mkfs_ext4.  So when
> _scratch_mkfs_ext4 failed, it was a no-op that didn't actually do
> anything, and there was still a file system the default configuration
> for the test scenario.
> 
> What puzzles me is why there was an attempt to enable the journal
> feature in the first place.  As near as I can tell, the tests don't
> change what gets tested whether or not the journal is enabled.
> Darrick; you had added these tests were you were working on ext4's
> metadata checksum feature; do you remember your thinking at the time?

"Qwklgjwlqaetwqjetlweqqlgqgqtrrt", most likely.

> In any case, either better fix is to replace:
> 
> _scratch_mkfs_ext4 -O journal > /dev/null 2>&1
> 
> with:
> 
> _scratch_mkfs_ext4 -O has_journal >> $seqres.full 2>&1
> 
> Or:
> 
> _scratch_mkfs -O has_journal >> $seqres.full 2>&1
> 
> My preference would be latter, since I'm regularly testing with and
> without the journal, and I'd much rather run the test with whatever
> configuration I'm currently testing (e.g., ext4/4k, ext4/1k,
> ext4/nojournal, ext4/ext3conv, ext4/bigalloc, etc.)

I'm ok with either, though _scratch_mkfs -O has_journal is more
consistent with the way XFS fuzz tests do things.

Sorry about that. :/

--D

> 						- Ted
> 
> 