| Message ID | 20240124195306.1177737-1-enwlinux@gmail.com |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | generic/459: don't run on non-journaled ext4 file systems | expand |
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 > >
* 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 > > > > >
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 > > > > > > > > >
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
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(+)