[PATCH] ext4: Avoid remount errors with 'abort' mount option

[PATCH] ext4: Avoid remount errors with 'abort' mount option

[Jan Kara]

When we remount filesystem with 'abort' mount option while changing
other mount options as well (as is LTP test doing), we can return error
from the system call after commit d3476f3dad4a ("ext4: don't set
SB_RDONLY after filesystem errors") because the application of mount
option changes detects shutdown filesystem and refuses to do anything.
The behavior of application of other mount options in presence of
'abort' mount option is currently rather arbitary as some mount option
changes are handled before 'abort' and some after it.

Move aborting of the filesystem to the end of remount handling so all
requested changes are properly applied before the filesystem is shutdown
to have a reasonably consistent behavior.

Fixes: d3476f3dad4a ("ext4: don't set SB_RDONLY after filesystem errors")
Reported-by: Jan Stancek <jstancek@redhat.com>
Link: https://lore.kernel.org/all/Zvp6L+oFnfASaoHl@t14s
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/super.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 16a4ce704460..4645f1629673 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -6518,9 +6518,6 @@ static int __ext4_remount(struct fs_context *fc, struct super_block *sb)
 		goto restore_opts;
 	}
 
-	if (test_opt2(sb, ABORT))
-		ext4_abort(sb, ESHUTDOWN, "Abort forced by user");
-
 	sb->s_flags = (sb->s_flags & ~SB_POSIXACL) |
 		(test_opt(sb, POSIX_ACL) ? SB_POSIXACL : 0);
 
@@ -6689,6 +6686,14 @@ static int __ext4_remount(struct fs_context *fc, struct super_block *sb)
 	if (!ext4_has_feature_mmp(sb) || sb_rdonly(sb))
 		ext4_stop_mmpd(sbi);
 
+	/*
+	 * Handle aborting the filesystem as the last thing during remount to
+	 * avoid obsure errors during remount when some option changes fail to
+	 * apply due to shutdown filesystem.
+	 */
+	if (test_opt2(sb, ABORT))
+		ext4_abort(sb, ESHUTDOWN, "Abort forced by user");
+
 	return 0;
 
 restore_opts:
-- 
2.35.3

Re: [PATCH] ext4: Avoid remount errors with 'abort' mount option

[Jan Stancek]

On Sat, Oct 5, 2024 at 12:16 AM Jan Kara <jack@suse.cz> wrote:
>
> When we remount filesystem with 'abort' mount option while changing
> other mount options as well (as is LTP test doing), we can return error
> from the system call after commit d3476f3dad4a ("ext4: don't set
> SB_RDONLY after filesystem errors") because the application of mount
> option changes detects shutdown filesystem and refuses to do anything.
> The behavior of application of other mount options in presence of
> 'abort' mount option is currently rather arbitary as some mount option
> changes are handled before 'abort' and some after it.
>
> Move aborting of the filesystem to the end of remount handling so all
> requested changes are properly applied before the filesystem is shutdown
> to have a reasonably consistent behavior.
>
> Fixes: d3476f3dad4a ("ext4: don't set SB_RDONLY after filesystem errors")
> Reported-by: Jan Stancek <jstancek@redhat.com>
> Link: https://lore.kernel.org/all/Zvp6L+oFnfASaoHl@t14s
> Signed-off-by: Jan Kara <jack@suse.cz>

In case the mount option isn't getting deprecated right away:
Tested-by: Jan Stancek <jstancek@redhat.com>


> ---
>  fs/ext4/super.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 16a4ce704460..4645f1629673 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -6518,9 +6518,6 @@ static int __ext4_remount(struct fs_context *fc, struct super_block *sb)
>                 goto restore_opts;
>         }
>
> -       if (test_opt2(sb, ABORT))
> -               ext4_abort(sb, ESHUTDOWN, "Abort forced by user");
> -
>         sb->s_flags = (sb->s_flags & ~SB_POSIXACL) |
>                 (test_opt(sb, POSIX_ACL) ? SB_POSIXACL : 0);
>
> @@ -6689,6 +6686,14 @@ static int __ext4_remount(struct fs_context *fc, struct super_block *sb)
>         if (!ext4_has_feature_mmp(sb) || sb_rdonly(sb))
>                 ext4_stop_mmpd(sbi);
>
> +       /*
> +        * Handle aborting the filesystem as the last thing during remount to
> +        * avoid obsure errors during remount when some option changes fail to
> +        * apply due to shutdown filesystem.
> +        */
> +       if (test_opt2(sb, ABORT))
> +               ext4_abort(sb, ESHUTDOWN, "Abort forced by user");
> +
>         return 0;
>
>  restore_opts:
> --
> 2.35.3
>

Re: [PATCH] ext4: Avoid remount errors with 'abort' mount option

[Jan Kara]

On Mon 07-10-24 11:13:38, Jan Stancek wrote:
> On Sat, Oct 5, 2024 at 12:16 AM Jan Kara <jack@suse.cz> wrote:
> >
> > When we remount filesystem with 'abort' mount option while changing
> > other mount options as well (as is LTP test doing), we can return error
> > from the system call after commit d3476f3dad4a ("ext4: don't set
> > SB_RDONLY after filesystem errors") because the application of mount
> > option changes detects shutdown filesystem and refuses to do anything.
> > The behavior of application of other mount options in presence of
> > 'abort' mount option is currently rather arbitary as some mount option
> > changes are handled before 'abort' and some after it.
> >
> > Move aborting of the filesystem to the end of remount handling so all
> > requested changes are properly applied before the filesystem is shutdown
> > to have a reasonably consistent behavior.
> >
> > Fixes: d3476f3dad4a ("ext4: don't set SB_RDONLY after filesystem errors")
> > Reported-by: Jan Stancek <jstancek@redhat.com>
> > Link: https://lore.kernel.org/all/Zvp6L+oFnfASaoHl@t14s
> > Signed-off-by: Jan Kara <jack@suse.cz>
> 
> In case the mount option isn't getting deprecated right away:
> Tested-by: Jan Stancek <jstancek@redhat.com>

Well, even if we decided to deprecate it, it would mean that now we'd just
start warning about deprecation and it would get removed in an year or so.
So fixing this still makes sense. Thanks for testing!

								Honza
> > ---
> >  fs/ext4/super.c | 11 ++++++++---
> >  1 file changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> > index 16a4ce704460..4645f1629673 100644
> > --- a/fs/ext4/super.c
> > +++ b/fs/ext4/super.c
> > @@ -6518,9 +6518,6 @@ static int __ext4_remount(struct fs_context *fc, struct super_block *sb)
> >                 goto restore_opts;
> >         }
> >
> > -       if (test_opt2(sb, ABORT))
> > -               ext4_abort(sb, ESHUTDOWN, "Abort forced by user");
> > -
> >         sb->s_flags = (sb->s_flags & ~SB_POSIXACL) |
> >                 (test_opt(sb, POSIX_ACL) ? SB_POSIXACL : 0);
> >
> > @@ -6689,6 +6686,14 @@ static int __ext4_remount(struct fs_context *fc, struct super_block *sb)
> >         if (!ext4_has_feature_mmp(sb) || sb_rdonly(sb))
> >                 ext4_stop_mmpd(sbi);
> >
> > +       /*
> > +        * Handle aborting the filesystem as the last thing during remount to
> > +        * avoid obsure errors during remount when some option changes fail to
> > +        * apply due to shutdown filesystem.
> > +        */
> > +       if (test_opt2(sb, ABORT))
> > +               ext4_abort(sb, ESHUTDOWN, "Abort forced by user");
> > +
> >         return 0;
> >
> >  restore_opts:
> > --
> > 2.35.3
> >
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] ext4: Avoid remount errors with 'abort' mount option

[Theodore Ts'o]

On Sat, 05 Oct 2024 00:15:56 +0200, Jan Kara wrote:
> When we remount filesystem with 'abort' mount option while changing
> other mount options as well (as is LTP test doing), we can return error
> from the system call after commit d3476f3dad4a ("ext4: don't set
> SB_RDONLY after filesystem errors") because the application of mount
> option changes detects shutdown filesystem and refuses to do anything.
> The behavior of application of other mount options in presence of
> 'abort' mount option is currently rather arbitary as some mount option
> changes are handled before 'abort' and some after it.
> 
> [...]

Applied, thanks!

[1/1] ext4: Avoid remount errors with 'abort' mount option
      commit: 76486b104168ae59703190566e372badf433314b

Best regards,
-- 
Theodore Ts'o <tytso@mit.edu>