| Message ID | 20240328170620.334-1-luis.henriques@linux.dev (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ext4/01{2,9}: remove invalid filesystem option 'journal' | expand |
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? 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.) - Ted
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 > >
"Darrick J. Wong" <djwong@kernel.org> writes: > 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. :/ Thank you both for the feedback. I'll send out v2 shortly. Cheers,
diff --git a/tests/ext4/012 b/tests/ext4/012 index c49e8ef4483b..4cef19bf6be5 100755 --- a/tests/ext4/012 +++ b/tests/ext4/012 @@ -32,7 +32,7 @@ TESTDIR="${SCRATCH_MNT}/scratchdir" TESTFILE="${TESTDIR}/testfile" echo "+ create scratch fs" -_scratch_mkfs_ext4 -O journal > /dev/null 2>&1 +_scratch_mkfs_ext4 > /dev/null 2>&1 echo "+ mount fs image" _scratch_mount diff --git a/tests/ext4/019 b/tests/ext4/019 index ab5f50c66b6f..9db7f23bb608 100755 --- a/tests/ext4/019 +++ b/tests/ext4/019 @@ -32,7 +32,7 @@ TESTDIR="${SCRATCH_MNT}/scratchdir" TESTFILE="${TESTDIR}/testfile" echo "+ create scratch fs" -_scratch_mkfs_ext4 -O journal > /dev/null 2>&1 +_scratch_mkfs_ext4 > /dev/null 2>&1 echo "+ mount fs image" _scratch_mount
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> --- tests/ext4/012 | 2 +- tests/ext4/019 | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)