Received-SPF: pass (google.com: domain of linux-ext4+bounces-1112-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Mon, 5 Feb 2024 08:32:36 -0800
From: "Darrick J. Wong" <djwong@kernel.org>
To: Eric Whitney <enwlinux@gmail.com>
Cc: Zorro Lang <zlang@redhat.com>, fstests@vger.kernel.org,
	linux-ext4@vger.kernel.org, tytso@mit.edu
Subject: Re: [PATCH] generic/459: don't run on non-journaled ext4 file systems
Message-ID: <20240205163236.GH6188@frogsfrogsfrogs>
References: <20240124195306.1177737-1-enwlinux@gmail.com>
 <20240202131529.jn5z64qfm5r5ibte@dell-per750-06-vm-08.rhts.eng.pek2.redhat.com>
 <Zb09Jy8KyxoBAnEN@debian-BULLSEYE-live-builder-AMD64>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Zb09Jy8KyxoBAnEN@debian-BULLSEYE-live-builder-AMD64>

On Fri, Feb 02, 2024 at 02:06:15PM -0500, Eric Whitney wrote:
> * Zorro Lang <zlang@redhat.com>:
> > On Wed, Jan 24, 2024 at 02:53:06PM -0500, Eric Whitney wrote:
> > > generic/459 fails when run on an ext4 file system created without a
> > > journal or when its journal has not been loaded at mount time.
> > > 
> > > The test expects that a file system that it has been unable to freeze
> > > will be automatically remounted read only.  However, the default error
> > > handling policy for ext4 is to continue when possible after errors.
> > > 
> > > A workaround was added to the test in the past to force ext4 to
> > > perform a read only remount in order to meet the test's expectations.
> > > The touch command was used to create a new file after a freeze failure.
> > > This forces ext4 to start a new journal transaction, where it discovers
> > > the journal has previously aborted due to lack of space, and triggers
> > > special case error handling that results in a read only remount.
> > > 
> > > The workaround requires a journal.  Since ext4 is behaving as designed,
> > > prevent the test from running if there isn't one.
> > > 
> > > Signed-off-by: Eric Whitney <enwlinux@gmail.com>
> > > ---
> > >  tests/generic/459 | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/tests/generic/459 b/tests/generic/459
> > > index c3f0b2b0..63fbbc9b 100755
> > > --- a/tests/generic/459
> > > +++ b/tests/generic/459
> > > @@ -49,6 +49,11 @@ _require_command "$THIN_CHECK_PROG" thin_check
> > >  _require_freeze
> > >  _require_odirect
> > >  
> > > +# non-journaled ext4 won't remount read only after freeze failure
> > > +if [ "$FSTYP" == "ext4" ]; then
> > > +	_require_metadata_journaling
> > 
> > I'm wondering ... won't other fs need this, besides ext4?
> 
> Thanks for looking at this patch.
> 
> I'd been mainly concerned with ext4, but since you mention it, ext2 and ext3
> ought to be added as well, since the failure behavior is similar.
> 
> The check for metadata journaling is limited to just ext4 here because the
> commit history for this test appears to suggest the touch command workaround
> is specific to ext4.  Perhaps the test should unconditionally require metadata
> journaling for all file system types, but given that I don't work with many
> other file systems I was uncomfortable asserting that in this patch.  If
> freezing any file system requires a metadata journal, then it would make
> sense to do that.
> 
> Your thoughts?

ext2 never had journalling, so requiring metadata journalling
effectively _notruns this test.

I don't think there's any harm done if you shortcut
"if [[ $FSTYP == ext* ]];..." to stop this test on ext2 and ext3-nojournal
as well.

That said, the exact behavior of filesystems during freeze is undefined,
so I think the only option is whackamole _notrun-listing. :/

--D

> Eric
> 
> > 
> > > +fi
> > > +
> > >  vgname=vg_$seq
> > >  lvname=lv_$seq
> > >  poolname=pool_$seq
> > > -- 
> > > 2.30.2
> > > 
> > > 
> > 
>