Date:   Sun, 26 Dec 2021 21:01:48 +0100
From:   Lukas Czerner <lczerner@redhat.com>
To:     Sedat Dilek <sedat.dilek@gmail.com>
Cc:     Heiner Kallweit <hkallweit1@gmail.com>, linux-ext4@vger.kernel.org,
        linux-fsdevel@vger.kernel.org
Subject: Re: Problem with data=ordered ext4 mount option in linux-next
Message-ID: <20211226200148.bvtvfufzaaj5tay3@work>
References: <b5817114-8122-cf0e-ca8e-b5d1c9f43bc2@gmail.com>
 <20211217152456.l7b2mbefdkk64fkj@work>
 <b1fa8d59-02e8-16b7-7218-a3f6ac66e3fa@gmail.com>
 <df69973d-47c5-fbd6-f83d-4d7d8a261c4c@gmail.com>
 <d05a95a9-0bbd-3495-2b81-18673909edd4@gmail.com>
 <20211220111231.ncdfcynvoiidl7is@work>
 <CA+icZUXBBgaeF3NhoVZ7YSg9F66XJSPsbfCgSR5RB6x5-s55gA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CA+icZUXBBgaeF3NhoVZ7YSg9F66XJSPsbfCgSR5RB6x5-s55gA@mail.gmail.com>
Precedence: bulk

On Sat, Dec 25, 2021 at 09:11:14PM +0100, Sedat Dilek wrote:
> On Mon, Dec 20, 2021 at 12:53 PM Lukas Czerner <lczerner@redhat.com> wrote:
> >
> > On Fri, Dec 17, 2021 at 07:26:30PM +0100, Heiner Kallweit wrote:
> > > On 17.12.2021 18:02, Heiner Kallweit wrote:
> > > > On 17.12.2021 16:34, Heiner Kallweit wrote:
> > > >> On 17.12.2021 16:24, Lukas Czerner wrote:
> > > >>> On Fri, Dec 17, 2021 at 04:11:32PM +0100, Heiner Kallweit wrote:
> > > >>>> On linux-next systemd-remount-fs complains about an invalid mount option
> > > >>>> here, resulting in a r/o root fs. After playing with the mount options
> > > >>>> it turned out that data=ordered causes the problem. linux-next from Dec
> > > >>>> 1st was ok, so it seems to be related to the new mount API patches.
> > > >>>>
> > > >>>> At a first glance I saw no obvious problem, the following looks good.
> > > >>>> Maybe you have an idea where to look ..
> > > >>>>
> > > >>>> static const struct constant_table ext4_param_data[] = {
> > > >>>>  {"journal",     EXT4_MOUNT_JOURNAL_DATA},
> > > >>>>  {"ordered",     EXT4_MOUNT_ORDERED_DATA},
> > > >>>>  {"writeback",   EXT4_MOUNT_WRITEBACK_DATA},
> > > >>>>  {}
> > > >>>> };
> > > >>>>
> > > >>>>  fsparam_enum    ("data",                Opt_data, ext4_param_data),
> > > >>>>
> > > >>>
> > > >>> Thank you for the report!
> > > >>>
> > > >>> The ext4 mount has been reworked to use the new mount api and the work
> > > >>> has been applied to linux-next couple of days ago so I definitelly
> > > >>> assume there is a bug in there that I've missed. I will be looking into
> > > >>> it.
> > > >>>
> > > >>> Can you be a little bit more specific about how to reproduce the problem
> > > >>> as well as the error it generates in the logs ? Any other mount options
> > > >>> used simultaneously, non-default file system features, or mount options
> > > >>> stored within the superblock ?
> > > >>>
> > > >>> Can you reproduce it outside of the systemd unit, say a script ?
> > > >>>
> > > >> Yes:
> > > >>
> > > >> [root@zotac ~]# mount -o remount,data=ordered /
> > > >> mount: /: mount point not mounted or bad option.
> > > >> [root@zotac ~]# mount -o remount,discard /
> > > >> [root@zotac ~]#
> > > >>
> > > >> "systemctl status systemd-remount-fs" shows the same error.
> > > >>
> > > >> Following options I had in my fstab (ext4 fs):
> > > >> rw,relatime,data=ordered,discard
> > > >>
> > > >> No non-default system features.
> > > >>
> > > >>> Thanks!
> > > >>> -Lukas
> > > >>>
> > > >> Heiner
> > > >
> > > > Sorry, should have looked at dmesg earlier. There I see:
> > > > EXT4-fs: Cannot change data mode on remount
> > > > Message seems to be triggered from ext4_check_opt_consistency().
> > > > Not sure why this error doesn't occur with old mount API.
> > > > And actually I don't change the data mode.
> > >
> > > Based on the old API code: Maybe we need something like this?
> > > At least it works for me.
> > >
> > > diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> > > index b72d989b7..9ec7e526c 100644
> > > --- a/fs/ext4/super.c
> > > +++ b/fs/ext4/super.c
> > > @@ -2821,7 +2821,9 @@ static int ext4_check_opt_consistency(struct fs_context *fc,
> > >                                  "Remounting file system with no journal "
> > >                                  "so ignoring journalled data option");
> > >                         ctx_clear_mount_opt(ctx, EXT4_MOUNT_DATA_FLAGS);
> > > -               } else if (ctx->mask_s_mount_opt & EXT4_MOUNT_DATA_FLAGS) {
> > > +               } else if (ctx->mask_s_mount_opt & EXT4_MOUNT_DATA_FLAGS &&
> > > +                          (ctx->vals_s_mount_opt & EXT4_MOUNT_DATA_FLAGS) !=
> > > +                          (sbi->s_mount_opt & EXT4_MOUNT_DATA_FLAGS)) {
> >
> > Hi,
> >
> > indeed that's where the problem is. It's not enogh to check whether
> > we have a data= mount options set, we also have to check whether it's
> > the same as it already is set on the file system during remount. In
> > which case we just ignore it, rather then error out.
> >
> > Thanks for tracking it down. I think the condition can be simplified a
> > bit. I also have to update the xfstest test to check for plain remount
> > without changing anything to catch errors like these. I'll send patch
> > soon.
> >
> 
> Is "ext4: don't fail remount if journalling mode didn't change" the
> fix for the issue reported by Heiner?

Yes, it is.

-Lukas

> 
> - Sedat -
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git/commit/?h=dev&id=4c2467287779f744cdd70c8ec70903034d6584f0
> 
> > Thanks!
> > -Lukas
> >
> > >                         ext4_msg(NULL, KERN_ERR, "Cannot change data mode "
> > >                                  "on remount");
> > >                         return -EINVAL;
> > >
> >
>