[PATCH] generic/459: don't run on non-journaled ext4 file systems

[PATCH] generic/459: don't run on non-journaled ext4 file systems

[Eric Whitney]

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
+fi
+
 vgname=vg_$seq
 lvname=lv_$seq
 poolname=pool_$seq
-- 
2.30.2

Re: [PATCH] generic/459: don't run on non-journaled ext4 file systems

[Zorro Lang]

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?

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

Re: [PATCH] generic/459: don't run on non-journaled ext4 file systems

[Eric Whitney]

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

Re: [PATCH] generic/459: don't run on non-journaled ext4 file systems

[Darrick J. Wong]

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