Re: [PATCH V1 7/9] fs/ext4: Make DAX mount option a tri-state

From: Ira Weiny
Date: Thu May 14 2020 - 10:27:40 EST


On Thu, May 14, 2020 at 01:25:53PM +0200, Jan Kara wrote:
> On Wed 13-05-20 23:53:13, ira.weiny@xxxxxxxxx wrote:
> > From: Ira Weiny <ira.weiny@xxxxxxxxx>
> >
> > We add 'always', 'never', and 'inode' (default). '-o dax' continue to
> > operate the same.
> >
> > Specifically we introduce a 2nd DAX mount flag EXT4_MOUNT2_DAX_NEVER and set
> > it and EXT4_MOUNT_DAX_ALWAYS appropriately.
> >
> > We also force EXT4_MOUNT2_DAX_NEVER if !CONFIG_FS_DAX.
> >
> > https://lore.kernel.org/lkml/20200405061945.GA94792@xxxxxxxxxxxxxxxxxxxxxxxxx/
> >
> > Signed-off-by: Ira Weiny <ira.weiny@xxxxxxxxx>
> >
> > ---
> > Changes from RFC:
> > Combine remount check for DAX_NEVER with DAX_ALWAYS
> > Update ext4_should_enable_dax()
>
> ...
>
> > @@ -2076,13 +2079,32 @@ static int handle_mount_opt(struct super_block *sb, char *opt, int token,
> > }
> > sbi->s_jquota_fmt = m->mount_opt;
> > #endif
> > - } else if (token == Opt_dax) {
> > + } else if (token == Opt_dax || token == Opt_dax_str) {
> > #ifdef CONFIG_FS_DAX
> > - ext4_msg(sb, KERN_WARNING,
> > - "DAX enabled. Warning: EXPERIMENTAL, use at your own risk");
> > - sbi->s_mount_opt |= m->mount_opt;
> > + char *tmp = match_strdup(&args[0]);
> > +
> > + if (!tmp || !strcmp(tmp, "always")) {
> > + ext4_msg(sb, KERN_WARNING,
> > + "DAX enabled. Warning: EXPERIMENTAL, use at your own risk");
> > + sbi->s_mount_opt |= EXT4_MOUNT_DAX_ALWAYS;
> > + sbi->s_mount_opt2 &= ~EXT4_MOUNT2_DAX_NEVER;
> > + } else if (!strcmp(tmp, "never")) {
> > + sbi->s_mount_opt2 |= EXT4_MOUNT2_DAX_NEVER;
> > + sbi->s_mount_opt &= ~EXT4_MOUNT_DAX_ALWAYS;
> > + } else if (!strcmp(tmp, "inode")) {
> > + sbi->s_mount_opt &= ~EXT4_MOUNT_DAX_ALWAYS;
> > + sbi->s_mount_opt2 &= ~EXT4_MOUNT2_DAX_NEVER;
> > + } else {
> > + ext4_msg(sb, KERN_WARNING, "DAX invalid option.");
> > + kfree(tmp);
> > + return -1;
> > + }
> > +
> > + kfree(tmp);
>
> As I wrote in my reply to previous version of this patch, I'd prefer if we
> handled this like e.g. 'data=' mount option. I don't think any unification
> in option parsing with XFS makes sence and I'd rather keep consistent how
> ext4 handles these 'enum' options.

Ok... I'm sorry I'll change this. Thanks for all the reviews!
Ira

>
> Honza
>
> --
> Jan Kara <jack@xxxxxxxx>
> SUSE Labs, CR