Received-SPF: pass (google.com: domain of linux-ext4+bounces-1067-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Fri, 2 Feb 2024 14:06:15 -0500
From: Eric Whitney <enwlinux@gmail.com>
To: Zorro Lang <zlang@redhat.com>
Cc: Eric Whitney <enwlinux@gmail.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: <Zb09Jy8KyxoBAnEN@debian-BULLSEYE-live-builder-AMD64>
References: <20240124195306.1177737-1-enwlinux@gmail.com>
 <20240202131529.jn5z64qfm5r5ibte@dell-per750-06-vm-08.rhts.eng.pek2.redhat.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240202131529.jn5z64qfm5r5ibte@dell-per750-06-vm-08.rhts.eng.pek2.redhat.com>

* 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?

Eric

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