(no subject)

[Kari Argillander]

Date: Mon, 16 Aug 2021 04:08:46 +0300
Subject: [RFC PATCH 0/4] fs/ntfs3: Use new mount api and change some opts

This series modify ntfs3 to use new mount api as Christoph Hellwig wish
for.
https://lore.kernel.org/linux-fsdevel/20210810090234.GA23732@lst.de/

It also modify mount options noatime (not needed) and make new alias
for nls because kernel is changing to use it as described in here
https://lore.kernel.org/linux-fsdevel/20210808162453.1653-1-pali@kernel.org/

I would like really like to get fsparam_flag_no also for no_acs_rules
but then we have to make new name for it. Other possibility is to
modify mount api so it mount option can be no/no_. I think that would
maybe be good change. 

I did not quite like how I did nls table loading because now it always
first load default table and if user give option then default table is
dropped and if reconfigure is happening and this was same as before then
it is dropped. I try to make loading in fill_super and fs_reconfigure
but that just look ugly. This is quite readible so I leave it like this.
We also do not mount/remount so often that this probebly does not
matter. It seems that if new mount api had possibility to give default
value for mount option then there is not this kind of problem.

I would hope that these will added top of the now going ntfs3 patch
series. I do not have so many contributions to kernel yet and I would
like to get my name going there so that in future it would be easier to
contribute kernel.

Kari Argillander (4):
  fs/ntfs3: Use new api for mounting
  fs/ntfs3: Remove unnecesarry mount option noatime
  fs/ntfs3: Make mount option nohidden more universal
  fs/ntfs3: Add iocharset= mount option as alias for nls=

 Documentation/filesystems/ntfs3.rst |   4 -
 fs/ntfs3/super.c                    | 391 ++++++++++++++--------------
 2 files changed, 196 insertions(+), 199 deletions(-)

-- 
2.25.1

[RFC PATCH 1/4] fs/ntfs3: Use new api for mounting

[Kari Argillander]

We have now new mount api as described in Documentation/filesystems. We
should use it as it gives us some benefits which are desribed here
https://lore.kernel.org/linux-fsdevel/159646178122.1784947.11705396571718464082.stgit@warthog.procyon.org.uk/

Nls loading is changed a little bit because new api not have default
optioni for mount parameters. So we need to load nls table before and
change that if user specifie someting else.

Also try to use fsparam_flag_no as much as possible. This is just nice
little touch and is not mandatory but it should not make any harm. It
is just convenient that we can use example acl/noacl mount options.

Signed-off-by: Kari Argillander <kari.argillander@gmail.com>
---
 fs/ntfs3/super.c | 382 ++++++++++++++++++++++++-----------------------
 1 file changed, 193 insertions(+), 189 deletions(-)

diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c
index 6be13e256c1a..d805e0b31404 100644
--- a/fs/ntfs3/super.c
+++ b/fs/ntfs3/super.c
@@ -28,10 +28,11 @@
 #include <linux/buffer_head.h>
 #include <linux/exportfs.h>
 #include <linux/fs.h>
+#include <linux/fs_context.h>
+#include <linux/fs_parser.h>
 #include <linux/iversion.h>
 #include <linux/module.h>
 #include <linux/nls.h>
-#include <linux/parser.h>
 #include <linux/seq_file.h>
 #include <linux/statfs.h>
 
@@ -197,6 +198,30 @@ void *ntfs_put_shared(void *ptr)
 	return ret;
 }
 
+/*
+ * ntfs_load_nls
+ *
+ * Load nls table or if @nls is utf8 then return NULL because
+ * nls=utf8 is totally broken.
+ */
+static struct nls_table *ntfs_load_nls(char *nls)
+{
+	struct nls_table *ret;
+
+	if (!nls)
+		return ERR_PTR(-EINVAL);
+	if (strcmp(nls, "utf8"))
+		return NULL;
+	if (strcmp(nls, CONFIG_NLS_DEFAULT))
+		return load_nls_default();
+
+	ret = load_nls(nls);
+	if (!ret)
+		return ERR_PTR(-EINVAL);
+
+	return ret;
+}
+
 static inline void clear_mount_options(struct ntfs_mount_options *options)
 {
 	unload_nls(options->nls);
@@ -222,208 +247,164 @@ enum Opt {
 	Opt_err,
 };
 
-static const match_table_t ntfs_tokens = {
-	{ Opt_uid, "uid=%u" },
-	{ Opt_gid, "gid=%u" },
-	{ Opt_umask, "umask=%o" },
-	{ Opt_dmask, "dmask=%o" },
-	{ Opt_fmask, "fmask=%o" },
-	{ Opt_immutable, "sys_immutable" },
-	{ Opt_discard, "discard" },
-	{ Opt_force, "force" },
-	{ Opt_sparse, "sparse" },
-	{ Opt_nohidden, "nohidden" },
-	{ Opt_acl, "acl" },
-	{ Opt_noatime, "noatime" },
-	{ Opt_showmeta, "showmeta" },
-	{ Opt_nls, "nls=%s" },
-	{ Opt_prealloc, "prealloc" },
-	{ Opt_no_acs_rules, "no_acs_rules" },
-	{ Opt_err, NULL },
+// clang-format off
+static const struct fs_parameter_spec ntfs_fs_parameters[] = {
+	fsparam_u32("uid",			Opt_uid),
+	fsparam_u32("gid",			Opt_gid),
+	fsparam_u32oct("umask",			Opt_umask),
+	fsparam_u32oct("dmask",			Opt_dmask),
+	fsparam_u32oct("fmask",			Opt_fmask),
+	fsparam_flag_no("sys_immutable",	Opt_immutable),
+	fsparam_flag_no("discard",		Opt_discard),
+	fsparam_flag_no("force",		Opt_force),
+	fsparam_flag_no("sparse",		Opt_sparse),
+	fsparam_flag("nohidden",		Opt_nohidden),
+	fsparam_flag_no("acl",			Opt_acl),
+	fsparam_flag("noatime",			Opt_noatime),
+	fsparam_flag_no("showmeta",		Opt_showmeta),
+	fsparam_string("nls",			Opt_nls),
+	fsparam_flag_no("prealloc",		Opt_prealloc),
+	fsparam_flag("no_acs_rules",		Opt_no_acs_rules),
+	{}
 };
+// clang-format on
 
-static noinline int ntfs_parse_options(struct super_block *sb, char *options,
-				       int silent,
-				       struct ntfs_mount_options *opts)
+static void ntfs_default_options(struct ntfs_mount_options *opts)
 {
-	char *p;
-	substring_t args[MAX_OPT_ARGS];
-	int option;
-	char nls_name[30];
-	struct nls_table *nls;
-
 	opts->fs_uid = current_uid();
 	opts->fs_gid = current_gid();
-	opts->fs_fmask_inv = opts->fs_dmask_inv = ~current_umask();
-	nls_name[0] = 0;
-
-	if (!options)
-		goto out;
+	opts->fs_fmask_inv = ~current_umask();
+	opts->fs_dmask_inv = ~current_umask();
+	opts->nls = ntfs_load_nls(CONFIG_NLS_DEFAULT);
+}
 
-	while ((p = strsep(&options, ","))) {
-		int token;
+static int ntfs_fs_parse_param(struct fs_context *fc, struct fs_parameter *param)
+{
+	struct ntfs_sb_info *sbi = fc->s_fs_info;
+	struct ntfs_mount_options *opts = &sbi->options;
+	struct fs_parse_result result;
+	int opt;
 
-		if (!*p)
-			continue;
+	opt = fs_parse(fc, ntfs_fs_parameters, param, &result);
+	if (opt < 0)
+		return opt;
 
-		token = match_token(p, ntfs_tokens, args);
-		switch (token) {
-		case Opt_immutable:
-			opts->sys_immutable = 1;
-			break;
-		case Opt_uid:
-			if (match_int(&args[0], &option))
-				return -EINVAL;
-			opts->fs_uid = make_kuid(current_user_ns(), option);
-			if (!uid_valid(opts->fs_uid))
-				return -EINVAL;
-			opts->uid = 1;
-			break;
-		case Opt_gid:
-			if (match_int(&args[0], &option))
-				return -EINVAL;
-			opts->fs_gid = make_kgid(current_user_ns(), option);
-			if (!gid_valid(opts->fs_gid))
-				return -EINVAL;
-			opts->gid = 1;
-			break;
-		case Opt_umask:
-			if (match_octal(&args[0], &option))
-				return -EINVAL;
-			opts->fs_fmask_inv = opts->fs_dmask_inv = ~option;
-			opts->fmask = opts->dmask = 1;
-			break;
-		case Opt_dmask:
-			if (match_octal(&args[0], &option))
-				return -EINVAL;
-			opts->fs_dmask_inv = ~option;
-			opts->dmask = 1;
-			break;
-		case Opt_fmask:
-			if (match_octal(&args[0], &option))
-				return -EINVAL;
-			opts->fs_fmask_inv = ~option;
-			opts->fmask = 1;
-			break;
-		case Opt_discard:
-			opts->discard = 1;
-			break;
-		case Opt_force:
-			opts->force = 1;
-			break;
-		case Opt_sparse:
-			opts->sparse = 1;
-			break;
-		case Opt_nohidden:
-			opts->nohidden = 1;
-			break;
-		case Opt_acl:
+	switch (opt) {
+	case Opt_uid:
+		opts->fs_uid = make_kuid(current_user_ns(), result.uint_32);
+		if (!uid_valid(opts->fs_uid))
+			return -EINVAL;
+		opts->uid = 1;
+		break;
+	case Opt_gid:
+		opts->fs_gid = make_kgid(current_user_ns(), result.uint_32);
+		if (!gid_valid(opts->fs_gid))
+			return -EINVAL;
+		opts->gid = 1;
+		break;
+	case Opt_umask:
+		opts->fs_fmask_inv = ~result.uint_32;
+		opts->fs_dmask_inv = ~result.uint_32;
+		opts->fmask = 1;
+		opts->dmask = 1;
+		break;
+	case Opt_dmask:
+		opts->fs_dmask_inv = ~result.uint_32;
+		opts->dmask = 1;
+		break;
+	case Opt_fmask:
+		opts->fs_fmask_inv = ~result.uint_32;
+		opts->fmask = 1;
+		break;
+	case Opt_immutable:
+		opts->sys_immutable = result.negated ? 0 : 1;
+		break;
+	case Opt_discard:
+		opts->discard = result.negated ? 0 : 1;
+		break;
+	case Opt_force:
+		opts->force = result.negated ? 0 : 1;
+		break;
+	case Opt_sparse:
+		opts->sparse = result.negated ? 0 : 1;
+		break;
+	case Opt_nohidden:
+		opts->nohidden = 1;
+		break;
+	case Opt_acl:
+		if (!result.negated)
 #ifdef CONFIG_NTFS3_FS_POSIX_ACL
-			sb->s_flags |= SB_POSIXACL;
-			break;
+			fc->sb_flags |= SB_POSIXACL;
 #else
-			ntfs_err(sb, "support for ACL not compiled in!");
-			return -EINVAL;
+			return invalf(fc, "ntfs3: Support for ACL not compiled in!");
 #endif
-		case Opt_noatime:
-			sb->s_flags |= SB_NOATIME;
-			break;
-		case Opt_showmeta:
-			opts->showmeta = 1;
-			break;
-		case Opt_nls:
-			match_strlcpy(nls_name, &args[0], sizeof(nls_name));
-			break;
-		case Opt_prealloc:
-			opts->prealloc = 1;
-			break;
-		case Opt_no_acs_rules:
-			opts->no_acs_rules = 1;
-			break;
-		default:
-			if (!silent)
-				ntfs_err(
-					sb,
-					"Unrecognized mount option \"%s\" or missing value",
-					p);
-			//return -EINVAL;
+		else
+			fc->sb_flags &= ~SB_POSIXACL;
+		break;
+	case Opt_noatime:
+		fc->sb_flags |= SB_NOATIME;
+		break;
+	case Opt_showmeta:
+		opts->showmeta = result.negated ? 0 : 1;
+		break;
+	case Opt_nls:
+		unload_nls(opts->nls);
+
+		opts->nls = ntfs_load_nls(param->string);
+		if (IS_ERR(opts->nls)) {
+			return invalf(fc, "ntfs3: Cannot load nls %s",
+				      param->string);
 		}
-	}
 
-out:
-	if (!strcmp(nls_name[0] ? nls_name : CONFIG_NLS_DEFAULT, "utf8")) {
-		/* For UTF-8 use utf16s_to_utf8s/utf8s_to_utf16s instead of nls */
-		nls = NULL;
-	} else if (nls_name[0]) {
-		nls = load_nls(nls_name);
-		if (!nls) {
-			ntfs_err(sb, "failed to load \"%s\"", nls_name);
-			return -EINVAL;
-		}
-	} else {
-		nls = load_nls_default();
-		if (!nls) {
-			ntfs_err(sb, "failed to load default nls");
-			return -EINVAL;
-		}
+		param->string = NULL;
+		break;
+	case Opt_prealloc:
+		opts->prealloc = result.negated ? 0 : 1;
+		break;
+	case Opt_no_acs_rules:
+		opts->no_acs_rules = 1;
+		break;
+	default:
+		/* Should not be here unless we forget add case. */
+		return -EINVAL;
 	}
-	opts->nls = nls;
-
 	return 0;
 }
 
-static int ntfs_remount(struct super_block *sb, int *flags, char *data)
+static int ntfs_fs_reconfigure(struct fs_context *fc)
 {
-	int err, ro_rw;
+	int ro_rw;
+	struct super_block *sb = fc->root->d_sb;
 	struct ntfs_sb_info *sbi = sb->s_fs_info;
-	struct ntfs_mount_options old_opts;
-	char *orig_data = kstrdup(data, GFP_KERNEL);
-
-	if (data && !orig_data)
-		return -ENOMEM;
-
-	/* Store  original options */
-	memcpy(&old_opts, &sbi->options, sizeof(old_opts));
-	clear_mount_options(&sbi->options);
-	memset(&sbi->options, 0, sizeof(sbi->options));
-
-	err = ntfs_parse_options(sb, data, 0, &sbi->options);
-	if (err)
-		goto restore_opts;
+	struct ntfs_mount_options *new_opts = fc->s_fs_info;
+	int *flags = &fc->sb_flags;
 
 	ro_rw = sb_rdonly(sb) && !(*flags & SB_RDONLY);
 	if (ro_rw && (sbi->flags & NTFS_FLAGS_NEED_REPLAY)) {
-		ntfs_warn(
-			sb,
+		ntfs_warn(sb,
 			"Couldn't remount rw because journal is not replayed. Please umount/remount instead\n");
-		err = -EINVAL;
-		goto restore_opts;
+		goto clear_new_mount;
 	}
 
 	sync_filesystem(sb);
 
 	if (ro_rw && (sbi->volume.flags & VOLUME_FLAG_DIRTY) &&
-	    !sbi->options.force) {
+	    !new_opts->force) {
 		ntfs_warn(sb, "volume is dirty and \"force\" flag is not set!");
-		err = -EINVAL;
-		goto restore_opts;
+		goto clear_new_mount;
 	}
 
-	clear_mount_options(&old_opts);
+	*flags |= (*flags & ~SB_LAZYTIME) | (sb->s_flags & SB_LAZYTIME) |
+		  SB_NODIRATIME | SB_NOATIME;
 
-	*flags = (*flags & ~SB_LAZYTIME) | (sb->s_flags & SB_LAZYTIME) |
-		 SB_NODIRATIME | SB_NOATIME;
-	ntfs_info(sb, "re-mounted. Opts: %s", orig_data);
-	err = 0;
-	goto out;
-
-restore_opts:
 	clear_mount_options(&sbi->options);
-	memcpy(&sbi->options, &old_opts, sizeof(old_opts));
+	sbi->options = *new_opts;
 
-out:
-	kfree(orig_data);
-	return err;
+	return 0;
+
+clear_new_mount:
+	clear_mount_options(new_opts);
+	return -EINVAL;
 }
 
 static struct kmem_cache *ntfs_inode_cachep;
@@ -628,7 +609,6 @@ static const struct super_operations ntfs_sops = {
 	.statfs = ntfs_statfs,
 	.show_options = ntfs_show_options,
 	.sync_fs = ntfs_sync_fs,
-	.remount_fs = ntfs_remount,
 	.write_inode = ntfs3_write_inode,
 };
 
@@ -892,10 +872,10 @@ static int ntfs_init_from_boot(struct super_block *sb, u32 sector_size,
 }
 
 /* try to mount*/
-static int ntfs_fill_super(struct super_block *sb, void *data, int silent)
+static int ntfs_fill_super(struct super_block *sb, struct fs_context *fc)
 {
 	int err;
-	struct ntfs_sb_info *sbi;
+	struct ntfs_sb_info *sbi = sb->s_fs_info;
 	struct block_device *bdev = sb->s_bdev;
 	struct inode *bd_inode = bdev->bd_inode;
 	struct request_queue *rq = bdev_get_queue(bdev);
@@ -914,11 +894,6 @@ static int ntfs_fill_super(struct super_block *sb, void *data, int silent)
 
 	ref.high = 0;
 
-	sbi = ntfs_zalloc(sizeof(struct ntfs_sb_info));
-	if (!sbi)
-		return -ENOMEM;
-
-	sb->s_fs_info = sbi;
 	sbi->sb = sb;
 	sb->s_flags |= SB_NODIRATIME;
 	sb->s_magic = 0x7366746e; // "ntfs"
@@ -930,10 +905,6 @@ static int ntfs_fill_super(struct super_block *sb, void *data, int silent)
 	ratelimit_state_init(&sbi->msg_ratelimit, DEFAULT_RATELIMIT_INTERVAL,
 			     DEFAULT_RATELIMIT_BURST);
 
-	err = ntfs_parse_options(sb, data, silent, &sbi->options);
-	if (err)
-		goto out;
-
 	if (!rq || !blk_queue_discard(rq) || !rq->limits.discard_granularity) {
 		;
 	} else {
@@ -1409,19 +1380,52 @@ int ntfs_discard(struct ntfs_sb_info *sbi, CLST lcn, CLST len)
 	return err;
 }
 
-static struct dentry *ntfs_mount(struct file_system_type *fs_type, int flags,
-				 const char *dev_name, void *data)
+static int ntfs_fs_get_tree(struct fs_context *fc)
+{
+	return get_tree_bdev(fc, ntfs_fill_super);
+}
+
+static void ntfs_fs_free(struct fs_context *fc)
 {
-	return mount_bdev(fs_type, flags, dev_name, data, ntfs_fill_super);
+	struct ntfs_sb_info *sbi = fc->s_fs_info;
+
+	if (sbi)
+		put_ntfs(sbi);
+}
+
+static const struct fs_context_operations ntfs_context_ops = {
+	.parse_param	= ntfs_fs_parse_param,
+	.get_tree	= ntfs_fs_get_tree,
+	.reconfigure	= ntfs_fs_reconfigure,
+	.free		= ntfs_fs_free,
+};
+
+/*
+ * Set up the filesystem mount context.
+ */
+static int ntfs_init_fs_context(struct fs_context *fc)
+{
+	struct ntfs_sb_info *sbi;
+
+	sbi = ntfs_zalloc(sizeof(struct ntfs_sb_info));
+	if (!sbi)
+		return -ENOMEM;
+
+	ntfs_default_options(&sbi->options);
+
+	fc->s_fs_info = sbi;
+	fc->ops = &ntfs_context_ops;
+	return 0;
 }
 
 // clang-format off
 static struct file_system_type ntfs_fs_type = {
-	.owner		= THIS_MODULE,
-	.name		= "ntfs3",
-	.mount		= ntfs_mount,
-	.kill_sb	= kill_block_super,
-	.fs_flags	= FS_REQUIRES_DEV | FS_ALLOW_IDMAP,
+	.owner			= THIS_MODULE,
+	.name			= "ntfs3",
+	.init_fs_context	= ntfs_init_fs_context,
+	.parameters		= ntfs_fs_parameters,
+	.kill_sb		= kill_block_super,
+	.fs_flags		= FS_REQUIRES_DEV | FS_ALLOW_IDMAP,
 };
 // clang-format on
 
-- 
2.25.1

Re: [RFC PATCH 1/4] fs/ntfs3: Use new api for mounting

[Kari Argillander]

On Mon, Aug 16, 2021 at 05:47:00AM +0300, Kari Argillander wrote:
> We have now new mount api as described in Documentation/filesystems. We
> should use it as it gives us some benefits which are desribed here
> https://lore.kernel.org/linux-fsdevel/159646178122.1784947.11705396571718464082.stgit@warthog.procyon.org.uk/
> 
> Nls loading is changed a little bit because new api not have default
> optioni for mount parameters. So we need to load nls table before and
> change that if user specifie someting else.
> 
> Also try to use fsparam_flag_no as much as possible. This is just nice
> little touch and is not mandatory but it should not make any harm. It
> is just convenient that we can use example acl/noacl mount options.
 
I would like that if someone can comment can we do reconfigure so that
we change mount options? Can we example change iocharset and be ok after
that? I have look some other fs drivers and in my eyes it seems to be
quite random if driver should let reconfigure all parameters. Right now
code is that we can reconfigure every mount parameter but I do not know
if this is right call.

Re: [RFC PATCH 1/4] fs/ntfs3: Use new api for mounting

[Christoph Hellwig]

On Mon, Aug 16, 2021 at 06:23:51AM +0300, Kari Argillander wrote:
> > Nls loading is changed a little bit because new api not have default
> > optioni for mount parameters. So we need to load nls table before and
> > change that if user specifie someting else.
> > 
> > Also try to use fsparam_flag_no as much as possible. This is just nice
> > little touch and is not mandatory but it should not make any harm. It
> > is just convenient that we can use example acl/noacl mount options.
>  
> I would like that if someone can comment can we do reconfigure so that
> we change mount options? Can we example change iocharset and be ok after
> that? I have look some other fs drivers and in my eyes it seems to be
> quite random if driver should let reconfigure all parameters. Right now
> code is that we can reconfigure every mount parameter but I do not know
> if this is right call.

Reconfiguring non-trivial mount parameters is hard.  In general I'd
recommend to only allow reconfiguring paramters that

 a) have user demand for that
 b) you know what you're actually doing.

Something like the iocharset clearly isn't something that makes sense
to be changed.

Re: [RFC PATCH 1/4] fs/ntfs3: Use new api for mounting

[Kari Argillander]

On Mon, Aug 16, 2021 at 02:17:52PM +0200, Christoph Hellwig wrote:
> On Mon, Aug 16, 2021 at 06:23:51AM +0300, Kari Argillander wrote:
> > > Nls loading is changed a little bit because new api not have default
> > > optioni for mount parameters. So we need to load nls table before and
> > > change that if user specifie someting else.
> > > 
> > > Also try to use fsparam_flag_no as much as possible. This is just nice
> > > little touch and is not mandatory but it should not make any harm. It
> > > is just convenient that we can use example acl/noacl mount options.
> >  
> > I would like that if someone can comment can we do reconfigure so that
> > we change mount options? Can we example change iocharset and be ok after
> > that? I have look some other fs drivers and in my eyes it seems to be
> > quite random if driver should let reconfigure all parameters. Right now
> > code is that we can reconfigure every mount parameter but I do not know
> > if this is right call.
> 
> Reconfiguring non-trivial mount parameters is hard.  In general I'd
> recommend to only allow reconfiguring paramters that
> 
>  a) have user demand for that
>  b) you know what you're actually doing.
> 
> Something like the iocharset clearly isn't something that makes sense
> to be changed.

I will probably do this series so that nothing can be changed but that
there will be easy way to enable changing in code. So after I can send
small patch which will enable changing and I can test each option
separately. 

If Konstantin can comment if there is some parameters which have real
demand then I will of course implement those.

Thanks for comments.

Re: [RFC PATCH 1/4] fs/ntfs3: Use new api for mounting

[Christoph Hellwig]

> +/*
> + * ntfs_load_nls
> + *

No need to state the function name here.

> + * Load nls table or if @nls is utf8 then return NULL because
> + * nls=utf8 is totally broken.
> + */
> +static struct nls_table *ntfs_load_nls(char *nls)
> +{
> +	struct nls_table *ret;
> +
> +	if (!nls)
> +		return ERR_PTR(-EINVAL);
> +	if (strcmp(nls, "utf8"))
> +		return NULL;
> +	if (strcmp(nls, CONFIG_NLS_DEFAULT))
> +		return load_nls_default();
> +
> +	ret = load_nls(nls);
> +	if (!ret)
> +		return ERR_PTR(-EINVAL);
> +
> +	return ret;
> +}

This looks like something quite generic and not file system specific.
But I haven't found time to look at the series from Pali how this all
fits together.

> +// clang-format off

Please don't use C++ comments.  And we also should not put weird
formatter annotations into the kernel source anyway.

> +static void ntfs_default_options(struct ntfs_mount_options *opts)
>  {
>  	opts->fs_uid = current_uid();
>  	opts->fs_gid = current_gid();
> +	opts->fs_fmask_inv = ~current_umask();
> +	opts->fs_dmask_inv = ~current_umask();
> +	opts->nls = ntfs_load_nls(CONFIG_NLS_DEFAULT);
> +}

This function seems pretty pointless with a single trivial caller.

> +static int ntfs_fs_parse_param(struct fs_context *fc, struct fs_parameter *param)

Please avoid the overly long line.

> +		break;
> +	case Opt_showmeta:
> +		opts->showmeta = result.negated ? 0 : 1;
> +		break;
> +	case Opt_nls:
> +		unload_nls(opts->nls);
> +
> +		opts->nls = ntfs_load_nls(param->string);
> +		if (IS_ERR(opts->nls)) {
> +			return invalf(fc, "ntfs3: Cannot load nls %s",
> +				      param->string);
>  		}

So instead of unloading here, why not set keep a copy of the string
in the mount options structure and only load the actual table after
option parsing has finished?

> +     struct ntfs_mount_options *new_opts = fc->s_fs_info;

Does this rely on the mount_options being the first member in struct
ntfs_sb_info?  If so that is a landmine for future changes.

> +/*
> + * Set up the filesystem mount context.
> + */
> +static int ntfs_init_fs_context(struct fs_context *fc)
> +{
> +	struct ntfs_sb_info *sbi;
> +
> +	sbi = ntfs_zalloc(sizeof(struct ntfs_sb_info));

Not related to your patch, but why does ntfs3 have kmalloc wrappers
like this?

Re: [RFC PATCH 1/4] fs/ntfs3: Use new api for mounting

[Kari Argillander]

Thank you for taking time to review. I really appreciated it.

On Mon, Aug 16, 2021 at 02:36:19PM +0200, Christoph Hellwig wrote:
> > +/*
> > + * ntfs_load_nls
> > + *
> 
> No need to state the function name here.

This is current way of doing this in fs/ntfs3. I just like that things
are same kind in one driver. I agree that this may not be good way.

> > + * Load nls table or if @nls is utf8 then return NULL because
> > + * nls=utf8 is totally broken.
> > + */
> > +static struct nls_table *ntfs_load_nls(char *nls)
> > +{
> > +	struct nls_table *ret;
> > +
> > +	if (!nls)
> > +		return ERR_PTR(-EINVAL);
> > +	if (strcmp(nls, "utf8"))
> > +		return NULL;
> > +	if (strcmp(nls, CONFIG_NLS_DEFAULT))
> > +		return load_nls_default();
> > +
> > +	ret = load_nls(nls);
> > +	if (!ret)
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	return ret;
> > +}
> 
> This looks like something quite generic and not file system specific.
> But I haven't found time to look at the series from Pali how this all
> fits together.

It is quite generic I agree. Pali's series not implemeted any new way
doing this thing. In many cases Pali uses just load_nls and not
load_nls_default. This function basically use that if possible. It seems
that load_nls_default does not need error path so that's why it is nicer
to use.

One though is to implement api function load_nls_or_utf8(). Then we do not
need to test this utf8 stuff in all places.

> > +// clang-format off
> 
> Please don't use C++ comments.  And we also should not put weird
> formatter annotations into the kernel source anyway.

This is just a way ntfs3 do this but I agree totally and will take this
off. I did not even like it myself.

> > +static void ntfs_default_options(struct ntfs_mount_options *opts)
> >  {
> >  	opts->fs_uid = current_uid();
> >  	opts->fs_gid = current_gid();
> > +	opts->fs_fmask_inv = ~current_umask();
> > +	opts->fs_dmask_inv = ~current_umask();
> > +	opts->nls = ntfs_load_nls(CONFIG_NLS_DEFAULT);
> > +}
> 
> This function seems pretty pointless with a single trivial caller.

Yeah it is just because then no comment needed and other reason was that
I can but this closer to ntfs_fs_parse_param() so that when reading code
all parameter code is one place.

> > +static int ntfs_fs_parse_param(struct fs_context *fc, struct fs_parameter *param)
> 
> Please avoid the overly long line.

Thanks will fix.

> 
> > +		break;
> > +	case Opt_showmeta:
> > +		opts->showmeta = result.negated ? 0 : 1;
> > +		break;
> > +	case Opt_nls:
> > +		unload_nls(opts->nls);
> > +
> > +		opts->nls = ntfs_load_nls(param->string);
> > +		if (IS_ERR(opts->nls)) {
> > +			return invalf(fc, "ntfs3: Cannot load nls %s",
> > +				      param->string);
> >  		}
> 
> So instead of unloading here, why not set keep a copy of the string
> in the mount options structure and only load the actual table after
> option parsing has finished?

I did actually do this first but then I test this way and code get lot
cleaner. But I can totally change it back to "string loading".

> 
> > +     struct ntfs_mount_options *new_opts = fc->s_fs_info;
> 
> Does this rely on the mount_options being the first member in struct
> ntfs_sb_info?  If so that is a landmine for future changes.
> 
> > +/*
> > + * Set up the filesystem mount context.
> > + */
> > +static int ntfs_init_fs_context(struct fs_context *fc)
> > +{
> > +	struct ntfs_sb_info *sbi;
> > +
> > +	sbi = ntfs_zalloc(sizeof(struct ntfs_sb_info));
> 
> Not related to your patch, but why does ntfs3 have kmalloc wrappers
> like this?

I do not know. I actually also suggested changing this (link). This might
even confuse some static analyzer tools.
https://lore.kernel.org/linux-fsdevel/20210103231755.bcmyalz3maq4ama2@kari-VirtualBox/

Re: [RFC PATCH 1/4] fs/ntfs3: Use new api for mounting

[Pali Rohár]

On Monday 16 August 2021 16:14:17 Kari Argillander wrote:
> > > + * Load nls table or if @nls is utf8 then return NULL because
> > > + * nls=utf8 is totally broken.
> > > + */
> > > +static struct nls_table *ntfs_load_nls(char *nls)
> > > +{
> > > +	struct nls_table *ret;
> > > +
> > > +	if (!nls)
> > > +		return ERR_PTR(-EINVAL);
> > > +	if (strcmp(nls, "utf8"))
> > > +		return NULL;
> > > +	if (strcmp(nls, CONFIG_NLS_DEFAULT))
> > > +		return load_nls_default();
> > > +
> > > +	ret = load_nls(nls);
> > > +	if (!ret)
> > > +		return ERR_PTR(-EINVAL);
> > > +
> > > +	return ret;
> > > +}
> > 
> > This looks like something quite generic and not file system specific.
> > But I haven't found time to look at the series from Pali how this all
> > fits together.
> 
> It is quite generic I agree. Pali's series not implemeted any new way
> doing this thing. In many cases Pali uses just load_nls and not
> load_nls_default. This function basically use that if possible. It seems
> that load_nls_default does not need error path so that's why it is nicer
> to use.

Yes, I'm using what is currently available. But providing some helper
function should be a nice cleanup.

> One though is to implement api function load_nls_or_utf8(). Then we do not
> need to test this utf8 stuff in all places.

Beware that there are more cases which can happen:

- iocharset is not specified
  --> then driver default behavior is used
      --> it is either some fixed encoding (e.g. iso8859-1, utf8) or
          CONFIG_NLS_DEFAULT (*); so it should behave like iocharset is
          set to that fixed encoding or CONFIG_NLS_DEFAULT
- iocharset is set to utf8
  --> then native utf8* functions should be used instead of nls
- iocharset is set to CONFIG_NLS_DEFAULT
  --> then load_nls_default() should be used which is IIRC guaranteed to
      not fail
- iocharset is not set to utf8, neither to CONFIG_NLS_DEFAULT
  --> then load_nls(iocharset) should be used; this may fail

(*) - it is pity that not all fs drivers are using CONFIG_NLS_DEFAULT
      and some are using some their own fixed encoding... it just
      increase mess and "user surprise"

Re: [RFC PATCH 1/4] fs/ntfs3: Use new api for mounting

[Christian Brauner]

On Mon, Aug 16, 2021 at 05:47:00AM +0300, Kari Argillander wrote:
> We have now new mount api as described in Documentation/filesystems. We
> should use it as it gives us some benefits which are desribed here
> https://lore.kernel.org/linux-fsdevel/159646178122.1784947.11705396571718464082.stgit@warthog.procyon.org.uk/
> 
> Nls loading is changed a little bit because new api not have default
> optioni for mount parameters. So we need to load nls table before and
> change that if user specifie someting else.
> 
> Also try to use fsparam_flag_no as much as possible. This is just nice
> little touch and is not mandatory but it should not make any harm. It
> is just convenient that we can use example acl/noacl mount options.
> 
> Signed-off-by: Kari Argillander <kari.argillander@gmail.com>
> ---
>  fs/ntfs3/super.c | 382 ++++++++++++++++++++++++-----------------------
>  1 file changed, 193 insertions(+), 189 deletions(-)
> 
> diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c
> index 6be13e256c1a..d805e0b31404 100644
> --- a/fs/ntfs3/super.c
> +++ b/fs/ntfs3/super.c
> @@ -28,10 +28,11 @@
>  #include <linux/buffer_head.h>
>  #include <linux/exportfs.h>
>  #include <linux/fs.h>
> +#include <linux/fs_context.h>
> +#include <linux/fs_parser.h>
>  #include <linux/iversion.h>
>  #include <linux/module.h>
>  #include <linux/nls.h>
> -#include <linux/parser.h>
>  #include <linux/seq_file.h>
>  #include <linux/statfs.h>
>  
> @@ -197,6 +198,30 @@ void *ntfs_put_shared(void *ptr)
>  	return ret;
>  }
>  
> +/*
> + * ntfs_load_nls
> + *
> + * Load nls table or if @nls is utf8 then return NULL because
> + * nls=utf8 is totally broken.
> + */
> +static struct nls_table *ntfs_load_nls(char *nls)
> +{
> +	struct nls_table *ret;
> +
> +	if (!nls)
> +		return ERR_PTR(-EINVAL);
> +	if (strcmp(nls, "utf8"))
> +		return NULL;
> +	if (strcmp(nls, CONFIG_NLS_DEFAULT))
> +		return load_nls_default();
> +
> +	ret = load_nls(nls);
> +	if (!ret)
> +		return ERR_PTR(-EINVAL);
> +
> +	return ret;
> +}
> +
>  static inline void clear_mount_options(struct ntfs_mount_options *options)
>  {
>  	unload_nls(options->nls);
> @@ -222,208 +247,164 @@ enum Opt {
>  	Opt_err,
>  };
>  
> -static const match_table_t ntfs_tokens = {
> -	{ Opt_uid, "uid=%u" },
> -	{ Opt_gid, "gid=%u" },
> -	{ Opt_umask, "umask=%o" },
> -	{ Opt_dmask, "dmask=%o" },
> -	{ Opt_fmask, "fmask=%o" },
> -	{ Opt_immutable, "sys_immutable" },
> -	{ Opt_discard, "discard" },
> -	{ Opt_force, "force" },
> -	{ Opt_sparse, "sparse" },
> -	{ Opt_nohidden, "nohidden" },
> -	{ Opt_acl, "acl" },
> -	{ Opt_noatime, "noatime" },
> -	{ Opt_showmeta, "showmeta" },
> -	{ Opt_nls, "nls=%s" },
> -	{ Opt_prealloc, "prealloc" },
> -	{ Opt_no_acs_rules, "no_acs_rules" },
> -	{ Opt_err, NULL },
> +// clang-format off
> +static const struct fs_parameter_spec ntfs_fs_parameters[] = {
> +	fsparam_u32("uid",			Opt_uid),
> +	fsparam_u32("gid",			Opt_gid),
> +	fsparam_u32oct("umask",			Opt_umask),
> +	fsparam_u32oct("dmask",			Opt_dmask),
> +	fsparam_u32oct("fmask",			Opt_fmask),
> +	fsparam_flag_no("sys_immutable",	Opt_immutable),
> +	fsparam_flag_no("discard",		Opt_discard),
> +	fsparam_flag_no("force",		Opt_force),
> +	fsparam_flag_no("sparse",		Opt_sparse),
> +	fsparam_flag("nohidden",		Opt_nohidden),
> +	fsparam_flag_no("acl",			Opt_acl),
> +	fsparam_flag("noatime",			Opt_noatime),
> +	fsparam_flag_no("showmeta",		Opt_showmeta),
> +	fsparam_string("nls",			Opt_nls),
> +	fsparam_flag_no("prealloc",		Opt_prealloc),
> +	fsparam_flag("no_acs_rules",		Opt_no_acs_rules),
> +	{}
>  };
> +// clang-format on
>  
> -static noinline int ntfs_parse_options(struct super_block *sb, char *options,
> -				       int silent,
> -				       struct ntfs_mount_options *opts)
> +static void ntfs_default_options(struct ntfs_mount_options *opts)
>  {
> -	char *p;
> -	substring_t args[MAX_OPT_ARGS];
> -	int option;
> -	char nls_name[30];
> -	struct nls_table *nls;
> -
>  	opts->fs_uid = current_uid();
>  	opts->fs_gid = current_gid();
> -	opts->fs_fmask_inv = opts->fs_dmask_inv = ~current_umask();
> -	nls_name[0] = 0;
> -
> -	if (!options)
> -		goto out;
> +	opts->fs_fmask_inv = ~current_umask();
> +	opts->fs_dmask_inv = ~current_umask();
> +	opts->nls = ntfs_load_nls(CONFIG_NLS_DEFAULT);
> +}
>  
> -	while ((p = strsep(&options, ","))) {
> -		int token;
> +static int ntfs_fs_parse_param(struct fs_context *fc, struct fs_parameter *param)
> +{
> +	struct ntfs_sb_info *sbi = fc->s_fs_info;
> +	struct ntfs_mount_options *opts = &sbi->options;
> +	struct fs_parse_result result;
> +	int opt;
>  
> -		if (!*p)
> -			continue;
> +	opt = fs_parse(fc, ntfs_fs_parameters, param, &result);
> +	if (opt < 0)
> +		return opt;
>  
> -		token = match_token(p, ntfs_tokens, args);
> -		switch (token) {
> -		case Opt_immutable:
> -			opts->sys_immutable = 1;
> -			break;
> -		case Opt_uid:
> -			if (match_int(&args[0], &option))
> -				return -EINVAL;
> -			opts->fs_uid = make_kuid(current_user_ns(), option);
> -			if (!uid_valid(opts->fs_uid))
> -				return -EINVAL;
> -			opts->uid = 1;
> -			break;
> -		case Opt_gid:
> -			if (match_int(&args[0], &option))
> -				return -EINVAL;
> -			opts->fs_gid = make_kgid(current_user_ns(), option);
> -			if (!gid_valid(opts->fs_gid))
> -				return -EINVAL;
> -			opts->gid = 1;
> -			break;
> -		case Opt_umask:
> -			if (match_octal(&args[0], &option))
> -				return -EINVAL;
> -			opts->fs_fmask_inv = opts->fs_dmask_inv = ~option;
> -			opts->fmask = opts->dmask = 1;
> -			break;
> -		case Opt_dmask:
> -			if (match_octal(&args[0], &option))
> -				return -EINVAL;
> -			opts->fs_dmask_inv = ~option;
> -			opts->dmask = 1;
> -			break;
> -		case Opt_fmask:
> -			if (match_octal(&args[0], &option))
> -				return -EINVAL;
> -			opts->fs_fmask_inv = ~option;
> -			opts->fmask = 1;
> -			break;
> -		case Opt_discard:
> -			opts->discard = 1;
> -			break;
> -		case Opt_force:
> -			opts->force = 1;
> -			break;
> -		case Opt_sparse:
> -			opts->sparse = 1;
> -			break;
> -		case Opt_nohidden:
> -			opts->nohidden = 1;
> -			break;
> -		case Opt_acl:
> +	switch (opt) {
> +	case Opt_uid:
> +		opts->fs_uid = make_kuid(current_user_ns(), result.uint_32);
> +		if (!uid_valid(opts->fs_uid))
> +			return -EINVAL;
> +		opts->uid = 1;
> +		break;
> +	case Opt_gid:
> +		opts->fs_gid = make_kgid(current_user_ns(), result.uint_32);
> +		if (!gid_valid(opts->fs_gid))
> +			return -EINVAL;
> +		opts->gid = 1;
> +		break;
> +	case Opt_umask:
> +		opts->fs_fmask_inv = ~result.uint_32;
> +		opts->fs_dmask_inv = ~result.uint_32;
> +		opts->fmask = 1;
> +		opts->dmask = 1;
> +		break;
> +	case Opt_dmask:
> +		opts->fs_dmask_inv = ~result.uint_32;
> +		opts->dmask = 1;
> +		break;
> +	case Opt_fmask:
> +		opts->fs_fmask_inv = ~result.uint_32;
> +		opts->fmask = 1;
> +		break;
> +	case Opt_immutable:
> +		opts->sys_immutable = result.negated ? 0 : 1;
> +		break;
> +	case Opt_discard:
> +		opts->discard = result.negated ? 0 : 1;
> +		break;
> +	case Opt_force:
> +		opts->force = result.negated ? 0 : 1;
> +		break;
> +	case Opt_sparse:
> +		opts->sparse = result.negated ? 0 : 1;
> +		break;
> +	case Opt_nohidden:
> +		opts->nohidden = 1;
> +		break;
> +	case Opt_acl:
> +		if (!result.negated)
>  #ifdef CONFIG_NTFS3_FS_POSIX_ACL
> -			sb->s_flags |= SB_POSIXACL;
> -			break;
> +			fc->sb_flags |= SB_POSIXACL;
>  #else
> -			ntfs_err(sb, "support for ACL not compiled in!");
> -			return -EINVAL;
> +			return invalf(fc, "ntfs3: Support for ACL not compiled in!");
>  #endif
> -		case Opt_noatime:
> -			sb->s_flags |= SB_NOATIME;
> -			break;
> -		case Opt_showmeta:
> -			opts->showmeta = 1;
> -			break;
> -		case Opt_nls:
> -			match_strlcpy(nls_name, &args[0], sizeof(nls_name));
> -			break;
> -		case Opt_prealloc:
> -			opts->prealloc = 1;
> -			break;
> -		case Opt_no_acs_rules:
> -			opts->no_acs_rules = 1;
> -			break;
> -		default:
> -			if (!silent)
> -				ntfs_err(
> -					sb,
> -					"Unrecognized mount option \"%s\" or missing value",
> -					p);
> -			//return -EINVAL;
> +		else
> +			fc->sb_flags &= ~SB_POSIXACL;
> +		break;
> +	case Opt_noatime:
> +		fc->sb_flags |= SB_NOATIME;
> +		break;
> +	case Opt_showmeta:
> +		opts->showmeta = result.negated ? 0 : 1;
> +		break;
> +	case Opt_nls:
> +		unload_nls(opts->nls);
> +
> +		opts->nls = ntfs_load_nls(param->string);
> +		if (IS_ERR(opts->nls)) {
> +			return invalf(fc, "ntfs3: Cannot load nls %s",
> +				      param->string);
>  		}
> -	}
>  
> -out:
> -	if (!strcmp(nls_name[0] ? nls_name : CONFIG_NLS_DEFAULT, "utf8")) {
> -		/* For UTF-8 use utf16s_to_utf8s/utf8s_to_utf16s instead of nls */
> -		nls = NULL;
> -	} else if (nls_name[0]) {
> -		nls = load_nls(nls_name);
> -		if (!nls) {
> -			ntfs_err(sb, "failed to load \"%s\"", nls_name);
> -			return -EINVAL;
> -		}
> -	} else {
> -		nls = load_nls_default();
> -		if (!nls) {
> -			ntfs_err(sb, "failed to load default nls");
> -			return -EINVAL;
> -		}
> +		param->string = NULL;
> +		break;
> +	case Opt_prealloc:
> +		opts->prealloc = result.negated ? 0 : 1;
> +		break;
> +	case Opt_no_acs_rules:
> +		opts->no_acs_rules = 1;
> +		break;
> +	default:
> +		/* Should not be here unless we forget add case. */
> +		return -EINVAL;
>  	}
> -	opts->nls = nls;
> -
>  	return 0;
>  }
>  
> -static int ntfs_remount(struct super_block *sb, int *flags, char *data)
> +static int ntfs_fs_reconfigure(struct fs_context *fc)
>  {
> -	int err, ro_rw;
> +	int ro_rw;
> +	struct super_block *sb = fc->root->d_sb;
>  	struct ntfs_sb_info *sbi = sb->s_fs_info;
> -	struct ntfs_mount_options old_opts;
> -	char *orig_data = kstrdup(data, GFP_KERNEL);
> -
> -	if (data && !orig_data)
> -		return -ENOMEM;
> -
> -	/* Store  original options */
> -	memcpy(&old_opts, &sbi->options, sizeof(old_opts));
> -	clear_mount_options(&sbi->options);
> -	memset(&sbi->options, 0, sizeof(sbi->options));
> -
> -	err = ntfs_parse_options(sb, data, 0, &sbi->options);
> -	if (err)
> -		goto restore_opts;
> +	struct ntfs_mount_options *new_opts = fc->s_fs_info;
> +	int *flags = &fc->sb_flags;

Afaict this doesn't need to be a pointer anymore.
fscontext->reconfigure() doesn't have a int *flags parameter.

>  
>  	ro_rw = sb_rdonly(sb) && !(*flags & SB_RDONLY);
>  	if (ro_rw && (sbi->flags & NTFS_FLAGS_NEED_REPLAY)) {
> -		ntfs_warn(
> -			sb,
> +		ntfs_warn(sb,
>  			"Couldn't remount rw because journal is not replayed. Please umount/remount instead\n");
> -		err = -EINVAL;
> -		goto restore_opts;
> +		goto clear_new_mount;
>  	}
>  
>  	sync_filesystem(sb);
>  
>  	if (ro_rw && (sbi->volume.flags & VOLUME_FLAG_DIRTY) &&
> -	    !sbi->options.force) {
> +	    !new_opts->force) {
>  		ntfs_warn(sb, "volume is dirty and \"force\" flag is not set!");
> -		err = -EINVAL;
> -		goto restore_opts;
> +		goto clear_new_mount;
>  	}
>  
> -	clear_mount_options(&old_opts);
> +	*flags |= (*flags & ~SB_LAZYTIME) | (sb->s_flags & SB_LAZYTIME) |
> +		  SB_NODIRATIME | SB_NOATIME;
>  
> -	*flags = (*flags & ~SB_LAZYTIME) | (sb->s_flags & SB_LAZYTIME) |
> -		 SB_NODIRATIME | SB_NOATIME;
> -	ntfs_info(sb, "re-mounted. Opts: %s", orig_data);
> -	err = 0;
> -	goto out;
> -
> -restore_opts:
>  	clear_mount_options(&sbi->options);
> -	memcpy(&sbi->options, &old_opts, sizeof(old_opts));
> +	sbi->options = *new_opts;
>  
> -out:
> -	kfree(orig_data);
> -	return err;
> +	return 0;
> +
> +clear_new_mount:
> +	clear_mount_options(new_opts);
> +	return -EINVAL;
>  }
>  
>  static struct kmem_cache *ntfs_inode_cachep;
> @@ -628,7 +609,6 @@ static const struct super_operations ntfs_sops = {
>  	.statfs = ntfs_statfs,
>  	.show_options = ntfs_show_options,
>  	.sync_fs = ntfs_sync_fs,
> -	.remount_fs = ntfs_remount,
>  	.write_inode = ntfs3_write_inode,
>  };
>  
> @@ -892,10 +872,10 @@ static int ntfs_init_from_boot(struct super_block *sb, u32 sector_size,
>  }
>  
>  /* try to mount*/
> -static int ntfs_fill_super(struct super_block *sb, void *data, int silent)
> +static int ntfs_fill_super(struct super_block *sb, struct fs_context *fc)
>  {
>  	int err;
> -	struct ntfs_sb_info *sbi;
> +	struct ntfs_sb_info *sbi = sb->s_fs_info;
>  	struct block_device *bdev = sb->s_bdev;
>  	struct inode *bd_inode = bdev->bd_inode;
>  	struct request_queue *rq = bdev_get_queue(bdev);
> @@ -914,11 +894,6 @@ static int ntfs_fill_super(struct super_block *sb, void *data, int silent)
>  
>  	ref.high = 0;
>  
> -	sbi = ntfs_zalloc(sizeof(struct ntfs_sb_info));
> -	if (!sbi)
> -		return -ENOMEM;
> -
> -	sb->s_fs_info = sbi;
>  	sbi->sb = sb;
>  	sb->s_flags |= SB_NODIRATIME;
>  	sb->s_magic = 0x7366746e; // "ntfs"
> @@ -930,10 +905,6 @@ static int ntfs_fill_super(struct super_block *sb, void *data, int silent)
>  	ratelimit_state_init(&sbi->msg_ratelimit, DEFAULT_RATELIMIT_INTERVAL,
>  			     DEFAULT_RATELIMIT_BURST);
>  
> -	err = ntfs_parse_options(sb, data, silent, &sbi->options);
> -	if (err)
> -		goto out;
> -
>  	if (!rq || !blk_queue_discard(rq) || !rq->limits.discard_granularity) {
>  		;
>  	} else {
> @@ -1409,19 +1380,52 @@ int ntfs_discard(struct ntfs_sb_info *sbi, CLST lcn, CLST len)
>  	return err;
>  }
>  
> -static struct dentry *ntfs_mount(struct file_system_type *fs_type, int flags,
> -				 const char *dev_name, void *data)
> +static int ntfs_fs_get_tree(struct fs_context *fc)
> +{
> +	return get_tree_bdev(fc, ntfs_fill_super);
> +}
> +
> +static void ntfs_fs_free(struct fs_context *fc)
>  {
> -	return mount_bdev(fs_type, flags, dev_name, data, ntfs_fill_super);
> +	struct ntfs_sb_info *sbi = fc->s_fs_info;
> +
> +	if (sbi)
> +		put_ntfs(sbi);
> +}
> +
> +static const struct fs_context_operations ntfs_context_ops = {
> +	.parse_param	= ntfs_fs_parse_param,
> +	.get_tree	= ntfs_fs_get_tree,
> +	.reconfigure	= ntfs_fs_reconfigure,
> +	.free		= ntfs_fs_free,
> +};
> +
> +/*
> + * Set up the filesystem mount context.
> + */
> +static int ntfs_init_fs_context(struct fs_context *fc)
> +{
> +	struct ntfs_sb_info *sbi;
> +
> +	sbi = ntfs_zalloc(sizeof(struct ntfs_sb_info));
> +	if (!sbi)
> +		return -ENOMEM;
> +
> +	ntfs_default_options(&sbi->options);
> +
> +	fc->s_fs_info = sbi;
> +	fc->ops = &ntfs_context_ops;
> +	return 0;
>  }
>  
>  // clang-format off
>  static struct file_system_type ntfs_fs_type = {
> -	.owner		= THIS_MODULE,
> -	.name		= "ntfs3",
> -	.mount		= ntfs_mount,
> -	.kill_sb	= kill_block_super,
> -	.fs_flags	= FS_REQUIRES_DEV | FS_ALLOW_IDMAP,
> +	.owner			= THIS_MODULE,
> +	.name			= "ntfs3",
> +	.init_fs_context	= ntfs_init_fs_context,
> +	.parameters		= ntfs_fs_parameters,
> +	.kill_sb		= kill_block_super,
> +	.fs_flags		= FS_REQUIRES_DEV | FS_ALLOW_IDMAP,

If you add idmapped mount support right away please try to make sure and
enable ntfs3 with xfstests if that's all possible.

Christian

Re: [RFC PATCH 1/4] fs/ntfs3: Use new api for mounting

[Kari Argillander]

On Mon, Aug 16, 2021 at 03:40:30PM +0200, Christian Brauner wrote:
> On Mon, Aug 16, 2021 at 05:47:00AM +0300, Kari Argillander wrote:
> > We have now new mount api as described in Documentation/filesystems. We
> > should use it as it gives us some benefits which are desribed here
> > https://lore.kernel.org/linux-fsdevel/159646178122.1784947.11705396571718464082.stgit@warthog.procyon.org.uk/
> > 
> > Nls loading is changed a little bit because new api not have default
> > optioni for mount parameters. So we need to load nls table before and
> > change that if user specifie someting else.
> > 
> > Also try to use fsparam_flag_no as much as possible. This is just nice
> > little touch and is not mandatory but it should not make any harm. It
> > is just convenient that we can use example acl/noacl mount options.
> > 
> > Signed-off-by: Kari Argillander <kari.argillander@gmail.com>
> > ---
> >  fs/ntfs3/super.c | 382 ++++++++++++++++++++++++-----------------------
> >  1 file changed, 193 insertions(+), 189 deletions(-)
> > 
> > diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c
> > index 6be13e256c1a..d805e0b31404 100644
> > --- a/fs/ntfs3/super.c
> > +++ b/fs/ntfs3/super.c
> > @@ -28,10 +28,11 @@
> >  #include <linux/buffer_head.h>
> >  #include <linux/exportfs.h>
> >  #include <linux/fs.h>
> > +#include <linux/fs_context.h>
> > +#include <linux/fs_parser.h>
> >  #include <linux/iversion.h>
> >  #include <linux/module.h>
> >  #include <linux/nls.h>
> > -#include <linux/parser.h>
> >  #include <linux/seq_file.h>
> >  #include <linux/statfs.h>
> >  
> > @@ -197,6 +198,30 @@ void *ntfs_put_shared(void *ptr)
> >  	return ret;
> >  }
> >  
> > +/*
> > + * ntfs_load_nls
> > + *
> > + * Load nls table or if @nls is utf8 then return NULL because
> > + * nls=utf8 is totally broken.
> > + */
> > +static struct nls_table *ntfs_load_nls(char *nls)
> > +{
> > +	struct nls_table *ret;
> > +
> > +	if (!nls)
> > +		return ERR_PTR(-EINVAL);
> > +	if (strcmp(nls, "utf8"))
> > +		return NULL;
> > +	if (strcmp(nls, CONFIG_NLS_DEFAULT))
> > +		return load_nls_default();
> > +
> > +	ret = load_nls(nls);
> > +	if (!ret)
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	return ret;
> > +}
> > +
> >  static inline void clear_mount_options(struct ntfs_mount_options *options)
> >  {
> >  	unload_nls(options->nls);
> > @@ -222,208 +247,164 @@ enum Opt {
> >  	Opt_err,
> >  };
> >  
> > -static const match_table_t ntfs_tokens = {
> > -	{ Opt_uid, "uid=%u" },
> > -	{ Opt_gid, "gid=%u" },
> > -	{ Opt_umask, "umask=%o" },
> > -	{ Opt_dmask, "dmask=%o" },
> > -	{ Opt_fmask, "fmask=%o" },
> > -	{ Opt_immutable, "sys_immutable" },
> > -	{ Opt_discard, "discard" },
> > -	{ Opt_force, "force" },
> > -	{ Opt_sparse, "sparse" },
> > -	{ Opt_nohidden, "nohidden" },
> > -	{ Opt_acl, "acl" },
> > -	{ Opt_noatime, "noatime" },
> > -	{ Opt_showmeta, "showmeta" },
> > -	{ Opt_nls, "nls=%s" },
> > -	{ Opt_prealloc, "prealloc" },
> > -	{ Opt_no_acs_rules, "no_acs_rules" },
> > -	{ Opt_err, NULL },
> > +// clang-format off
> > +static const struct fs_parameter_spec ntfs_fs_parameters[] = {
> > +	fsparam_u32("uid",			Opt_uid),
> > +	fsparam_u32("gid",			Opt_gid),
> > +	fsparam_u32oct("umask",			Opt_umask),
> > +	fsparam_u32oct("dmask",			Opt_dmask),
> > +	fsparam_u32oct("fmask",			Opt_fmask),
> > +	fsparam_flag_no("sys_immutable",	Opt_immutable),
> > +	fsparam_flag_no("discard",		Opt_discard),
> > +	fsparam_flag_no("force",		Opt_force),
> > +	fsparam_flag_no("sparse",		Opt_sparse),
> > +	fsparam_flag("nohidden",		Opt_nohidden),
> > +	fsparam_flag_no("acl",			Opt_acl),
> > +	fsparam_flag("noatime",			Opt_noatime),
> > +	fsparam_flag_no("showmeta",		Opt_showmeta),
> > +	fsparam_string("nls",			Opt_nls),
> > +	fsparam_flag_no("prealloc",		Opt_prealloc),
> > +	fsparam_flag("no_acs_rules",		Opt_no_acs_rules),
> > +	{}
> >  };
> > +// clang-format on
> >  
> > -static noinline int ntfs_parse_options(struct super_block *sb, char *options,
> > -				       int silent,
> > -				       struct ntfs_mount_options *opts)
> > +static void ntfs_default_options(struct ntfs_mount_options *opts)
> >  {
> > -	char *p;
> > -	substring_t args[MAX_OPT_ARGS];
> > -	int option;
> > -	char nls_name[30];
> > -	struct nls_table *nls;
> > -
> >  	opts->fs_uid = current_uid();
> >  	opts->fs_gid = current_gid();
> > -	opts->fs_fmask_inv = opts->fs_dmask_inv = ~current_umask();
> > -	nls_name[0] = 0;
> > -
> > -	if (!options)
> > -		goto out;
> > +	opts->fs_fmask_inv = ~current_umask();
> > +	opts->fs_dmask_inv = ~current_umask();
> > +	opts->nls = ntfs_load_nls(CONFIG_NLS_DEFAULT);
> > +}
> >  
> > -	while ((p = strsep(&options, ","))) {
> > -		int token;
> > +static int ntfs_fs_parse_param(struct fs_context *fc, struct fs_parameter *param)
> > +{
> > +	struct ntfs_sb_info *sbi = fc->s_fs_info;
> > +	struct ntfs_mount_options *opts = &sbi->options;
> > +	struct fs_parse_result result;
> > +	int opt;
> >  
> > -		if (!*p)
> > -			continue;
> > +	opt = fs_parse(fc, ntfs_fs_parameters, param, &result);
> > +	if (opt < 0)
> > +		return opt;
> >  
> > -		token = match_token(p, ntfs_tokens, args);
> > -		switch (token) {
> > -		case Opt_immutable:
> > -			opts->sys_immutable = 1;
> > -			break;
> > -		case Opt_uid:
> > -			if (match_int(&args[0], &option))
> > -				return -EINVAL;
> > -			opts->fs_uid = make_kuid(current_user_ns(), option);
> > -			if (!uid_valid(opts->fs_uid))
> > -				return -EINVAL;
> > -			opts->uid = 1;
> > -			break;
> > -		case Opt_gid:
> > -			if (match_int(&args[0], &option))
> > -				return -EINVAL;
> > -			opts->fs_gid = make_kgid(current_user_ns(), option);
> > -			if (!gid_valid(opts->fs_gid))
> > -				return -EINVAL;
> > -			opts->gid = 1;
> > -			break;
> > -		case Opt_umask:
> > -			if (match_octal(&args[0], &option))
> > -				return -EINVAL;
> > -			opts->fs_fmask_inv = opts->fs_dmask_inv = ~option;
> > -			opts->fmask = opts->dmask = 1;
> > -			break;
> > -		case Opt_dmask:
> > -			if (match_octal(&args[0], &option))
> > -				return -EINVAL;
> > -			opts->fs_dmask_inv = ~option;
> > -			opts->dmask = 1;
> > -			break;
> > -		case Opt_fmask:
> > -			if (match_octal(&args[0], &option))
> > -				return -EINVAL;
> > -			opts->fs_fmask_inv = ~option;
> > -			opts->fmask = 1;
> > -			break;
> > -		case Opt_discard:
> > -			opts->discard = 1;
> > -			break;
> > -		case Opt_force:
> > -			opts->force = 1;
> > -			break;
> > -		case Opt_sparse:
> > -			opts->sparse = 1;
> > -			break;
> > -		case Opt_nohidden:
> > -			opts->nohidden = 1;
> > -			break;
> > -		case Opt_acl:
> > +	switch (opt) {
> > +	case Opt_uid:
> > +		opts->fs_uid = make_kuid(current_user_ns(), result.uint_32);
> > +		if (!uid_valid(opts->fs_uid))
> > +			return -EINVAL;
> > +		opts->uid = 1;
> > +		break;
> > +	case Opt_gid:
> > +		opts->fs_gid = make_kgid(current_user_ns(), result.uint_32);
> > +		if (!gid_valid(opts->fs_gid))
> > +			return -EINVAL;
> > +		opts->gid = 1;
> > +		break;
> > +	case Opt_umask:
> > +		opts->fs_fmask_inv = ~result.uint_32;
> > +		opts->fs_dmask_inv = ~result.uint_32;
> > +		opts->fmask = 1;
> > +		opts->dmask = 1;
> > +		break;
> > +	case Opt_dmask:
> > +		opts->fs_dmask_inv = ~result.uint_32;
> > +		opts->dmask = 1;
> > +		break;
> > +	case Opt_fmask:
> > +		opts->fs_fmask_inv = ~result.uint_32;
> > +		opts->fmask = 1;
> > +		break;
> > +	case Opt_immutable:
> > +		opts->sys_immutable = result.negated ? 0 : 1;
> > +		break;
> > +	case Opt_discard:
> > +		opts->discard = result.negated ? 0 : 1;
> > +		break;
> > +	case Opt_force:
> > +		opts->force = result.negated ? 0 : 1;
> > +		break;
> > +	case Opt_sparse:
> > +		opts->sparse = result.negated ? 0 : 1;
> > +		break;
> > +	case Opt_nohidden:
> > +		opts->nohidden = 1;
> > +		break;
> > +	case Opt_acl:
> > +		if (!result.negated)
> >  #ifdef CONFIG_NTFS3_FS_POSIX_ACL
> > -			sb->s_flags |= SB_POSIXACL;
> > -			break;
> > +			fc->sb_flags |= SB_POSIXACL;
> >  #else
> > -			ntfs_err(sb, "support for ACL not compiled in!");
> > -			return -EINVAL;
> > +			return invalf(fc, "ntfs3: Support for ACL not compiled in!");
> >  #endif
> > -		case Opt_noatime:
> > -			sb->s_flags |= SB_NOATIME;
> > -			break;
> > -		case Opt_showmeta:
> > -			opts->showmeta = 1;
> > -			break;
> > -		case Opt_nls:
> > -			match_strlcpy(nls_name, &args[0], sizeof(nls_name));
> > -			break;
> > -		case Opt_prealloc:
> > -			opts->prealloc = 1;
> > -			break;
> > -		case Opt_no_acs_rules:
> > -			opts->no_acs_rules = 1;
> > -			break;
> > -		default:
> > -			if (!silent)
> > -				ntfs_err(
> > -					sb,
> > -					"Unrecognized mount option \"%s\" or missing value",
> > -					p);
> > -			//return -EINVAL;
> > +		else
> > +			fc->sb_flags &= ~SB_POSIXACL;
> > +		break;
> > +	case Opt_noatime:
> > +		fc->sb_flags |= SB_NOATIME;
> > +		break;
> > +	case Opt_showmeta:
> > +		opts->showmeta = result.negated ? 0 : 1;
> > +		break;
> > +	case Opt_nls:
> > +		unload_nls(opts->nls);
> > +
> > +		opts->nls = ntfs_load_nls(param->string);
> > +		if (IS_ERR(opts->nls)) {
> > +			return invalf(fc, "ntfs3: Cannot load nls %s",
> > +				      param->string);
> >  		}
> > -	}
> >  
> > -out:
> > -	if (!strcmp(nls_name[0] ? nls_name : CONFIG_NLS_DEFAULT, "utf8")) {
> > -		/* For UTF-8 use utf16s_to_utf8s/utf8s_to_utf16s instead of nls */
> > -		nls = NULL;
> > -	} else if (nls_name[0]) {
> > -		nls = load_nls(nls_name);
> > -		if (!nls) {
> > -			ntfs_err(sb, "failed to load \"%s\"", nls_name);
> > -			return -EINVAL;
> > -		}
> > -	} else {
> > -		nls = load_nls_default();
> > -		if (!nls) {
> > -			ntfs_err(sb, "failed to load default nls");
> > -			return -EINVAL;
> > -		}
> > +		param->string = NULL;
> > +		break;
> > +	case Opt_prealloc:
> > +		opts->prealloc = result.negated ? 0 : 1;
> > +		break;
> > +	case Opt_no_acs_rules:
> > +		opts->no_acs_rules = 1;
> > +		break;
> > +	default:
> > +		/* Should not be here unless we forget add case. */
> > +		return -EINVAL;
> >  	}
> > -	opts->nls = nls;
> > -
> >  	return 0;
> >  }
> >  
> > -static int ntfs_remount(struct super_block *sb, int *flags, char *data)
> > +static int ntfs_fs_reconfigure(struct fs_context *fc)
> >  {
> > -	int err, ro_rw;
> > +	int ro_rw;
> > +	struct super_block *sb = fc->root->d_sb;
> >  	struct ntfs_sb_info *sbi = sb->s_fs_info;
> > -	struct ntfs_mount_options old_opts;
> > -	char *orig_data = kstrdup(data, GFP_KERNEL);
> > -
> > -	if (data && !orig_data)
> > -		return -ENOMEM;
> > -
> > -	/* Store  original options */
> > -	memcpy(&old_opts, &sbi->options, sizeof(old_opts));
> > -	clear_mount_options(&sbi->options);
> > -	memset(&sbi->options, 0, sizeof(sbi->options));
> > -
> > -	err = ntfs_parse_options(sb, data, 0, &sbi->options);
> > -	if (err)
> > -		goto restore_opts;
> > +	struct ntfs_mount_options *new_opts = fc->s_fs_info;
> > +	int *flags = &fc->sb_flags;
> 
> Afaict this doesn't need to be a pointer anymore.
> fscontext->reconfigure() doesn't have a int *flags parameter.

Yeah.

> >  
> >  	ro_rw = sb_rdonly(sb) && !(*flags & SB_RDONLY);
> >  	if (ro_rw && (sbi->flags & NTFS_FLAGS_NEED_REPLAY)) {
> > -		ntfs_warn(
> > -			sb,
> > +		ntfs_warn(sb,
> >  			"Couldn't remount rw because journal is not replayed. Please umount/remount instead\n");
> > -		err = -EINVAL;
> > -		goto restore_opts;
> > +		goto clear_new_mount;
> >  	}
> >  
> >  	sync_filesystem(sb);
> >  
> >  	if (ro_rw && (sbi->volume.flags & VOLUME_FLAG_DIRTY) &&
> > -	    !sbi->options.force) {
> > +	    !new_opts->force) {
> >  		ntfs_warn(sb, "volume is dirty and \"force\" flag is not set!");
> > -		err = -EINVAL;
> > -		goto restore_opts;
> > +		goto clear_new_mount;
> >  	}
> >  
> > -	clear_mount_options(&old_opts);
> > +	*flags |= (*flags & ~SB_LAZYTIME) | (sb->s_flags & SB_LAZYTIME) |
> > +		  SB_NODIRATIME | SB_NOATIME;
> >  
> > -	*flags = (*flags & ~SB_LAZYTIME) | (sb->s_flags & SB_LAZYTIME) |
> > -		 SB_NODIRATIME | SB_NOATIME;
> > -	ntfs_info(sb, "re-mounted. Opts: %s", orig_data);
> > -	err = 0;
> > -	goto out;
> > -
> > -restore_opts:
> >  	clear_mount_options(&sbi->options);
> > -	memcpy(&sbi->options, &old_opts, sizeof(old_opts));
> > +	sbi->options = *new_opts;
> >  
> > -out:
> > -	kfree(orig_data);
> > -	return err;
> > +	return 0;
> > +
> > +clear_new_mount:
> > +	clear_mount_options(new_opts);
> > +	return -EINVAL;
> >  }
> >  
> >  static struct kmem_cache *ntfs_inode_cachep;
> > @@ -628,7 +609,6 @@ static const struct super_operations ntfs_sops = {
> >  	.statfs = ntfs_statfs,
> >  	.show_options = ntfs_show_options,
> >  	.sync_fs = ntfs_sync_fs,
> > -	.remount_fs = ntfs_remount,
> >  	.write_inode = ntfs3_write_inode,
> >  };
> >  
> > @@ -892,10 +872,10 @@ static int ntfs_init_from_boot(struct super_block *sb, u32 sector_size,
> >  }
> >  
> >  /* try to mount*/
> > -static int ntfs_fill_super(struct super_block *sb, void *data, int silent)
> > +static int ntfs_fill_super(struct super_block *sb, struct fs_context *fc)
> >  {
> >  	int err;
> > -	struct ntfs_sb_info *sbi;
> > +	struct ntfs_sb_info *sbi = sb->s_fs_info;
> >  	struct block_device *bdev = sb->s_bdev;
> >  	struct inode *bd_inode = bdev->bd_inode;
> >  	struct request_queue *rq = bdev_get_queue(bdev);
> > @@ -914,11 +894,6 @@ static int ntfs_fill_super(struct super_block *sb, void *data, int silent)
> >  
> >  	ref.high = 0;
> >  
> > -	sbi = ntfs_zalloc(sizeof(struct ntfs_sb_info));
> > -	if (!sbi)
> > -		return -ENOMEM;
> > -
> > -	sb->s_fs_info = sbi;
> >  	sbi->sb = sb;
> >  	sb->s_flags |= SB_NODIRATIME;
> >  	sb->s_magic = 0x7366746e; // "ntfs"
> > @@ -930,10 +905,6 @@ static int ntfs_fill_super(struct super_block *sb, void *data, int silent)
> >  	ratelimit_state_init(&sbi->msg_ratelimit, DEFAULT_RATELIMIT_INTERVAL,
> >  			     DEFAULT_RATELIMIT_BURST);
> >  
> > -	err = ntfs_parse_options(sb, data, silent, &sbi->options);
> > -	if (err)
> > -		goto out;
> > -
> >  	if (!rq || !blk_queue_discard(rq) || !rq->limits.discard_granularity) {
> >  		;
> >  	} else {
> > @@ -1409,19 +1380,52 @@ int ntfs_discard(struct ntfs_sb_info *sbi, CLST lcn, CLST len)
> >  	return err;
> >  }
> >  
> > -static struct dentry *ntfs_mount(struct file_system_type *fs_type, int flags,
> > -				 const char *dev_name, void *data)
> > +static int ntfs_fs_get_tree(struct fs_context *fc)
> > +{
> > +	return get_tree_bdev(fc, ntfs_fill_super);
> > +}
> > +
> > +static void ntfs_fs_free(struct fs_context *fc)
> >  {
> > -	return mount_bdev(fs_type, flags, dev_name, data, ntfs_fill_super);
> > +	struct ntfs_sb_info *sbi = fc->s_fs_info;
> > +
> > +	if (sbi)
> > +		put_ntfs(sbi);
> > +}
> > +
> > +static const struct fs_context_operations ntfs_context_ops = {
> > +	.parse_param	= ntfs_fs_parse_param,
> > +	.get_tree	= ntfs_fs_get_tree,
> > +	.reconfigure	= ntfs_fs_reconfigure,
> > +	.free		= ntfs_fs_free,
> > +};
> > +
> > +/*
> > + * Set up the filesystem mount context.
> > + */
> > +static int ntfs_init_fs_context(struct fs_context *fc)
> > +{
> > +	struct ntfs_sb_info *sbi;
> > +
> > +	sbi = ntfs_zalloc(sizeof(struct ntfs_sb_info));
> > +	if (!sbi)
> > +		return -ENOMEM;
> > +
> > +	ntfs_default_options(&sbi->options);
> > +
> > +	fc->s_fs_info = sbi;
> > +	fc->ops = &ntfs_context_ops;
> > +	return 0;
> >  }
> >  
> >  // clang-format off
> >  static struct file_system_type ntfs_fs_type = {
> > -	.owner		= THIS_MODULE,
> > -	.name		= "ntfs3",
> > -	.mount		= ntfs_mount,
> > -	.kill_sb	= kill_block_super,
> > -	.fs_flags	= FS_REQUIRES_DEV | FS_ALLOW_IDMAP,

Note FS_ALLOW_IDMAP here.

> > +	.owner			= THIS_MODULE,
> > +	.name			= "ntfs3",
> > +	.init_fs_context	= ntfs_init_fs_context,
> > +	.parameters		= ntfs_fs_parameters,
> > +	.kill_sb		= kill_block_super,
> > +	.fs_flags		= FS_REQUIRES_DEV | FS_ALLOW_IDMAP,
> 
> If you add idmapped mount support right away please try to make sure and
> enable ntfs3 with xfstests if that's all possible.

This is already implemented by Komarov in new series. He haven't yet
sended it to mailing list. It is already in linux-next.

Komarov has also stated that they use xfstests (link). And he has showed
test results.
https://lore.kernel.org/linux-fsdevel/0e175373cef24e2abe76e203bb36d260@paragon-software.com/

I have tested these with kvm-xfstests which has support for ntfs3 made
by Theodore Ts'o. Current results:

./kvm-xfstests -c ntfs3 -X "generic/014,generic/129,generic/405,generic/476" -g auto
ntfs3/default: 662 tests, 43 failures, 207 skipped, 7442 seconds
  Failures: generic/012 generic/013 generic/016 generic/021 
    generic/022 generic/040 generic/041 generic/065 generic/066 
    generic/092 generic/094 generic/104 generic/130 generic/177 
    generic/225 generic/228 generic/240 generic/255 generic/258 
    generic/316 generic/321 generic/322 generic/335 generic/336 
    generic/341 generic/342 generic/343 generic/348 generic/360 
    generic/401 generic/423 generic/475 generic/483 generic/498 
    generic/510 generic/526 generic/527 generic/538 generic/551 
    generic/552 generic/631 generic/633 generic/640 
Totals: 455 tests, 207 skipped, 43 failures, 0 errors, 7088s

ACL support seems broken. Notice that many of these are in group "punch"
and punch a hole for regular files was added in "V28" but maybe that
does not work correctly as showed by xfstests.

There is also on going patch set by Theodore for support ntfs3 and fuse
based filesystem in straight in xfstests (link). And after that gets
there then we can ask 01 project to include ntfs3 to their testing.
And hopefully Paragon will send some ntfs related test to upstream.
https://lore.kernel.org/fstests/YQoVXWRFGeY19onQ@mit.edu/

Re: [RFC PATCH 1/4] fs/ntfs3: Use new api for mounting

[Christian Brauner]

On Mon, Aug 16, 2021 at 04:59:27PM +0300, Kari Argillander wrote:
> On Mon, Aug 16, 2021 at 03:40:30PM +0200, Christian Brauner wrote:
> > On Mon, Aug 16, 2021 at 05:47:00AM +0300, Kari Argillander wrote:
> > > We have now new mount api as described in Documentation/filesystems. We
> > > should use it as it gives us some benefits which are desribed here
> > > https://lore.kernel.org/linux-fsdevel/159646178122.1784947.11705396571718464082.stgit@warthog.procyon.org.uk/
> > > 
> > > Nls loading is changed a little bit because new api not have default
> > > optioni for mount parameters. So we need to load nls table before and
> > > change that if user specifie someting else.
> > > 
> > > Also try to use fsparam_flag_no as much as possible. This is just nice
> > > little touch and is not mandatory but it should not make any harm. It
> > > is just convenient that we can use example acl/noacl mount options.
> > > 
> > > Signed-off-by: Kari Argillander <kari.argillander@gmail.com>
> > > ---
> > >  fs/ntfs3/super.c | 382 ++++++++++++++++++++++++-----------------------
> > >  1 file changed, 193 insertions(+), 189 deletions(-)
> > > 
> > > diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c
> > > index 6be13e256c1a..d805e0b31404 100644
> > > --- a/fs/ntfs3/super.c
> > > +++ b/fs/ntfs3/super.c
> > > @@ -28,10 +28,11 @@
> > >  #include <linux/buffer_head.h>
> > >  #include <linux/exportfs.h>
> > >  #include <linux/fs.h>
> > > +#include <linux/fs_context.h>
> > > +#include <linux/fs_parser.h>
> > >  #include <linux/iversion.h>
> > >  #include <linux/module.h>
> > >  #include <linux/nls.h>
> > > -#include <linux/parser.h>
> > >  #include <linux/seq_file.h>
> > >  #include <linux/statfs.h>
> > >  
> > > @@ -197,6 +198,30 @@ void *ntfs_put_shared(void *ptr)
> > >  	return ret;
> > >  }
> > >  
> > > +/*
> > > + * ntfs_load_nls
> > > + *
> > > + * Load nls table or if @nls is utf8 then return NULL because
> > > + * nls=utf8 is totally broken.
> > > + */
> > > +static struct nls_table *ntfs_load_nls(char *nls)
> > > +{
> > > +	struct nls_table *ret;
> > > +
> > > +	if (!nls)
> > > +		return ERR_PTR(-EINVAL);
> > > +	if (strcmp(nls, "utf8"))
> > > +		return NULL;
> > > +	if (strcmp(nls, CONFIG_NLS_DEFAULT))
> > > +		return load_nls_default();
> > > +
> > > +	ret = load_nls(nls);
> > > +	if (!ret)
> > > +		return ERR_PTR(-EINVAL);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > >  static inline void clear_mount_options(struct ntfs_mount_options *options)
> > >  {
> > >  	unload_nls(options->nls);
> > > @@ -222,208 +247,164 @@ enum Opt {
> > >  	Opt_err,
> > >  };
> > >  
> > > -static const match_table_t ntfs_tokens = {
> > > -	{ Opt_uid, "uid=%u" },
> > > -	{ Opt_gid, "gid=%u" },
> > > -	{ Opt_umask, "umask=%o" },
> > > -	{ Opt_dmask, "dmask=%o" },
> > > -	{ Opt_fmask, "fmask=%o" },
> > > -	{ Opt_immutable, "sys_immutable" },
> > > -	{ Opt_discard, "discard" },
> > > -	{ Opt_force, "force" },
> > > -	{ Opt_sparse, "sparse" },
> > > -	{ Opt_nohidden, "nohidden" },
> > > -	{ Opt_acl, "acl" },
> > > -	{ Opt_noatime, "noatime" },
> > > -	{ Opt_showmeta, "showmeta" },
> > > -	{ Opt_nls, "nls=%s" },
> > > -	{ Opt_prealloc, "prealloc" },
> > > -	{ Opt_no_acs_rules, "no_acs_rules" },
> > > -	{ Opt_err, NULL },
> > > +// clang-format off
> > > +static const struct fs_parameter_spec ntfs_fs_parameters[] = {
> > > +	fsparam_u32("uid",			Opt_uid),
> > > +	fsparam_u32("gid",			Opt_gid),
> > > +	fsparam_u32oct("umask",			Opt_umask),
> > > +	fsparam_u32oct("dmask",			Opt_dmask),
> > > +	fsparam_u32oct("fmask",			Opt_fmask),
> > > +	fsparam_flag_no("sys_immutable",	Opt_immutable),
> > > +	fsparam_flag_no("discard",		Opt_discard),
> > > +	fsparam_flag_no("force",		Opt_force),
> > > +	fsparam_flag_no("sparse",		Opt_sparse),
> > > +	fsparam_flag("nohidden",		Opt_nohidden),
> > > +	fsparam_flag_no("acl",			Opt_acl),
> > > +	fsparam_flag("noatime",			Opt_noatime),
> > > +	fsparam_flag_no("showmeta",		Opt_showmeta),
> > > +	fsparam_string("nls",			Opt_nls),
> > > +	fsparam_flag_no("prealloc",		Opt_prealloc),
> > > +	fsparam_flag("no_acs_rules",		Opt_no_acs_rules),
> > > +	{}
> > >  };
> > > +// clang-format on
> > >  
> > > -static noinline int ntfs_parse_options(struct super_block *sb, char *options,
> > > -				       int silent,
> > > -				       struct ntfs_mount_options *opts)
> > > +static void ntfs_default_options(struct ntfs_mount_options *opts)
> > >  {
> > > -	char *p;
> > > -	substring_t args[MAX_OPT_ARGS];
> > > -	int option;
> > > -	char nls_name[30];
> > > -	struct nls_table *nls;
> > > -
> > >  	opts->fs_uid = current_uid();
> > >  	opts->fs_gid = current_gid();
> > > -	opts->fs_fmask_inv = opts->fs_dmask_inv = ~current_umask();
> > > -	nls_name[0] = 0;
> > > -
> > > -	if (!options)
> > > -		goto out;
> > > +	opts->fs_fmask_inv = ~current_umask();
> > > +	opts->fs_dmask_inv = ~current_umask();
> > > +	opts->nls = ntfs_load_nls(CONFIG_NLS_DEFAULT);
> > > +}
> > >  
> > > -	while ((p = strsep(&options, ","))) {
> > > -		int token;
> > > +static int ntfs_fs_parse_param(struct fs_context *fc, struct fs_parameter *param)
> > > +{
> > > +	struct ntfs_sb_info *sbi = fc->s_fs_info;
> > > +	struct ntfs_mount_options *opts = &sbi->options;
> > > +	struct fs_parse_result result;
> > > +	int opt;
> > >  
> > > -		if (!*p)
> > > -			continue;
> > > +	opt = fs_parse(fc, ntfs_fs_parameters, param, &result);
> > > +	if (opt < 0)
> > > +		return opt;
> > >  
> > > -		token = match_token(p, ntfs_tokens, args);
> > > -		switch (token) {
> > > -		case Opt_immutable:
> > > -			opts->sys_immutable = 1;
> > > -			break;
> > > -		case Opt_uid:
> > > -			if (match_int(&args[0], &option))
> > > -				return -EINVAL;
> > > -			opts->fs_uid = make_kuid(current_user_ns(), option);
> > > -			if (!uid_valid(opts->fs_uid))
> > > -				return -EINVAL;
> > > -			opts->uid = 1;
> > > -			break;
> > > -		case Opt_gid:
> > > -			if (match_int(&args[0], &option))
> > > -				return -EINVAL;
> > > -			opts->fs_gid = make_kgid(current_user_ns(), option);
> > > -			if (!gid_valid(opts->fs_gid))
> > > -				return -EINVAL;
> > > -			opts->gid = 1;
> > > -			break;
> > > -		case Opt_umask:
> > > -			if (match_octal(&args[0], &option))
> > > -				return -EINVAL;
> > > -			opts->fs_fmask_inv = opts->fs_dmask_inv = ~option;
> > > -			opts->fmask = opts->dmask = 1;
> > > -			break;
> > > -		case Opt_dmask:
> > > -			if (match_octal(&args[0], &option))
> > > -				return -EINVAL;
> > > -			opts->fs_dmask_inv = ~option;
> > > -			opts->dmask = 1;
> > > -			break;
> > > -		case Opt_fmask:
> > > -			if (match_octal(&args[0], &option))
> > > -				return -EINVAL;
> > > -			opts->fs_fmask_inv = ~option;
> > > -			opts->fmask = 1;
> > > -			break;
> > > -		case Opt_discard:
> > > -			opts->discard = 1;
> > > -			break;
> > > -		case Opt_force:
> > > -			opts->force = 1;
> > > -			break;
> > > -		case Opt_sparse:
> > > -			opts->sparse = 1;
> > > -			break;
> > > -		case Opt_nohidden:
> > > -			opts->nohidden = 1;
> > > -			break;
> > > -		case Opt_acl:
> > > +	switch (opt) {
> > > +	case Opt_uid:
> > > +		opts->fs_uid = make_kuid(current_user_ns(), result.uint_32);
> > > +		if (!uid_valid(opts->fs_uid))
> > > +			return -EINVAL;
> > > +		opts->uid = 1;
> > > +		break;
> > > +	case Opt_gid:
> > > +		opts->fs_gid = make_kgid(current_user_ns(), result.uint_32);
> > > +		if (!gid_valid(opts->fs_gid))
> > > +			return -EINVAL;
> > > +		opts->gid = 1;
> > > +		break;
> > > +	case Opt_umask:
> > > +		opts->fs_fmask_inv = ~result.uint_32;
> > > +		opts->fs_dmask_inv = ~result.uint_32;
> > > +		opts->fmask = 1;
> > > +		opts->dmask = 1;
> > > +		break;
> > > +	case Opt_dmask:
> > > +		opts->fs_dmask_inv = ~result.uint_32;
> > > +		opts->dmask = 1;
> > > +		break;
> > > +	case Opt_fmask:
> > > +		opts->fs_fmask_inv = ~result.uint_32;
> > > +		opts->fmask = 1;
> > > +		break;
> > > +	case Opt_immutable:
> > > +		opts->sys_immutable = result.negated ? 0 : 1;
> > > +		break;
> > > +	case Opt_discard:
> > > +		opts->discard = result.negated ? 0 : 1;
> > > +		break;
> > > +	case Opt_force:
> > > +		opts->force = result.negated ? 0 : 1;
> > > +		break;
> > > +	case Opt_sparse:
> > > +		opts->sparse = result.negated ? 0 : 1;
> > > +		break;
> > > +	case Opt_nohidden:
> > > +		opts->nohidden = 1;
> > > +		break;
> > > +	case Opt_acl:
> > > +		if (!result.negated)
> > >  #ifdef CONFIG_NTFS3_FS_POSIX_ACL
> > > -			sb->s_flags |= SB_POSIXACL;
> > > -			break;
> > > +			fc->sb_flags |= SB_POSIXACL;
> > >  #else
> > > -			ntfs_err(sb, "support for ACL not compiled in!");
> > > -			return -EINVAL;
> > > +			return invalf(fc, "ntfs3: Support for ACL not compiled in!");
> > >  #endif
> > > -		case Opt_noatime:
> > > -			sb->s_flags |= SB_NOATIME;
> > > -			break;
> > > -		case Opt_showmeta:
> > > -			opts->showmeta = 1;
> > > -			break;
> > > -		case Opt_nls:
> > > -			match_strlcpy(nls_name, &args[0], sizeof(nls_name));
> > > -			break;
> > > -		case Opt_prealloc:
> > > -			opts->prealloc = 1;
> > > -			break;
> > > -		case Opt_no_acs_rules:
> > > -			opts->no_acs_rules = 1;
> > > -			break;
> > > -		default:
> > > -			if (!silent)
> > > -				ntfs_err(
> > > -					sb,
> > > -					"Unrecognized mount option \"%s\" or missing value",
> > > -					p);
> > > -			//return -EINVAL;
> > > +		else
> > > +			fc->sb_flags &= ~SB_POSIXACL;
> > > +		break;
> > > +	case Opt_noatime:
> > > +		fc->sb_flags |= SB_NOATIME;
> > > +		break;
> > > +	case Opt_showmeta:
> > > +		opts->showmeta = result.negated ? 0 : 1;
> > > +		break;
> > > +	case Opt_nls:
> > > +		unload_nls(opts->nls);
> > > +
> > > +		opts->nls = ntfs_load_nls(param->string);
> > > +		if (IS_ERR(opts->nls)) {
> > > +			return invalf(fc, "ntfs3: Cannot load nls %s",
> > > +				      param->string);
> > >  		}
> > > -	}
> > >  
> > > -out:
> > > -	if (!strcmp(nls_name[0] ? nls_name : CONFIG_NLS_DEFAULT, "utf8")) {
> > > -		/* For UTF-8 use utf16s_to_utf8s/utf8s_to_utf16s instead of nls */
> > > -		nls = NULL;
> > > -	} else if (nls_name[0]) {
> > > -		nls = load_nls(nls_name);
> > > -		if (!nls) {
> > > -			ntfs_err(sb, "failed to load \"%s\"", nls_name);
> > > -			return -EINVAL;
> > > -		}
> > > -	} else {
> > > -		nls = load_nls_default();
> > > -		if (!nls) {
> > > -			ntfs_err(sb, "failed to load default nls");
> > > -			return -EINVAL;
> > > -		}
> > > +		param->string = NULL;
> > > +		break;
> > > +	case Opt_prealloc:
> > > +		opts->prealloc = result.negated ? 0 : 1;
> > > +		break;
> > > +	case Opt_no_acs_rules:
> > > +		opts->no_acs_rules = 1;
> > > +		break;
> > > +	default:
> > > +		/* Should not be here unless we forget add case. */
> > > +		return -EINVAL;
> > >  	}
> > > -	opts->nls = nls;
> > > -
> > >  	return 0;
> > >  }
> > >  
> > > -static int ntfs_remount(struct super_block *sb, int *flags, char *data)
> > > +static int ntfs_fs_reconfigure(struct fs_context *fc)
> > >  {
> > > -	int err, ro_rw;
> > > +	int ro_rw;
> > > +	struct super_block *sb = fc->root->d_sb;
> > >  	struct ntfs_sb_info *sbi = sb->s_fs_info;
> > > -	struct ntfs_mount_options old_opts;
> > > -	char *orig_data = kstrdup(data, GFP_KERNEL);
> > > -
> > > -	if (data && !orig_data)
> > > -		return -ENOMEM;
> > > -
> > > -	/* Store  original options */
> > > -	memcpy(&old_opts, &sbi->options, sizeof(old_opts));
> > > -	clear_mount_options(&sbi->options);
> > > -	memset(&sbi->options, 0, sizeof(sbi->options));
> > > -
> > > -	err = ntfs_parse_options(sb, data, 0, &sbi->options);
> > > -	if (err)
> > > -		goto restore_opts;
> > > +	struct ntfs_mount_options *new_opts = fc->s_fs_info;
> > > +	int *flags = &fc->sb_flags;
> > 
> > Afaict this doesn't need to be a pointer anymore.
> > fscontext->reconfigure() doesn't have a int *flags parameter.
> 
> Yeah.
> 
> > >  
> > >  	ro_rw = sb_rdonly(sb) && !(*flags & SB_RDONLY);
> > >  	if (ro_rw && (sbi->flags & NTFS_FLAGS_NEED_REPLAY)) {
> > > -		ntfs_warn(
> > > -			sb,
> > > +		ntfs_warn(sb,
> > >  			"Couldn't remount rw because journal is not replayed. Please umount/remount instead\n");
> > > -		err = -EINVAL;
> > > -		goto restore_opts;
> > > +		goto clear_new_mount;
> > >  	}
> > >  
> > >  	sync_filesystem(sb);
> > >  
> > >  	if (ro_rw && (sbi->volume.flags & VOLUME_FLAG_DIRTY) &&
> > > -	    !sbi->options.force) {
> > > +	    !new_opts->force) {
> > >  		ntfs_warn(sb, "volume is dirty and \"force\" flag is not set!");
> > > -		err = -EINVAL;
> > > -		goto restore_opts;
> > > +		goto clear_new_mount;
> > >  	}
> > >  
> > > -	clear_mount_options(&old_opts);
> > > +	*flags |= (*flags & ~SB_LAZYTIME) | (sb->s_flags & SB_LAZYTIME) |
> > > +		  SB_NODIRATIME | SB_NOATIME;
> > >  
> > > -	*flags = (*flags & ~SB_LAZYTIME) | (sb->s_flags & SB_LAZYTIME) |
> > > -		 SB_NODIRATIME | SB_NOATIME;
> > > -	ntfs_info(sb, "re-mounted. Opts: %s", orig_data);
> > > -	err = 0;
> > > -	goto out;
> > > -
> > > -restore_opts:
> > >  	clear_mount_options(&sbi->options);
> > > -	memcpy(&sbi->options, &old_opts, sizeof(old_opts));
> > > +	sbi->options = *new_opts;
> > >  
> > > -out:
> > > -	kfree(orig_data);
> > > -	return err;
> > > +	return 0;
> > > +
> > > +clear_new_mount:
> > > +	clear_mount_options(new_opts);
> > > +	return -EINVAL;
> > >  }
> > >  
> > >  static struct kmem_cache *ntfs_inode_cachep;
> > > @@ -628,7 +609,6 @@ static const struct super_operations ntfs_sops = {
> > >  	.statfs = ntfs_statfs,
> > >  	.show_options = ntfs_show_options,
> > >  	.sync_fs = ntfs_sync_fs,
> > > -	.remount_fs = ntfs_remount,
> > >  	.write_inode = ntfs3_write_inode,
> > >  };
> > >  
> > > @@ -892,10 +872,10 @@ static int ntfs_init_from_boot(struct super_block *sb, u32 sector_size,
> > >  }
> > >  
> > >  /* try to mount*/
> > > -static int ntfs_fill_super(struct super_block *sb, void *data, int silent)
> > > +static int ntfs_fill_super(struct super_block *sb, struct fs_context *fc)
> > >  {
> > >  	int err;
> > > -	struct ntfs_sb_info *sbi;
> > > +	struct ntfs_sb_info *sbi = sb->s_fs_info;
> > >  	struct block_device *bdev = sb->s_bdev;
> > >  	struct inode *bd_inode = bdev->bd_inode;
> > >  	struct request_queue *rq = bdev_get_queue(bdev);
> > > @@ -914,11 +894,6 @@ static int ntfs_fill_super(struct super_block *sb, void *data, int silent)
> > >  
> > >  	ref.high = 0;
> > >  
> > > -	sbi = ntfs_zalloc(sizeof(struct ntfs_sb_info));
> > > -	if (!sbi)
> > > -		return -ENOMEM;
> > > -
> > > -	sb->s_fs_info = sbi;
> > >  	sbi->sb = sb;
> > >  	sb->s_flags |= SB_NODIRATIME;
> > >  	sb->s_magic = 0x7366746e; // "ntfs"
> > > @@ -930,10 +905,6 @@ static int ntfs_fill_super(struct super_block *sb, void *data, int silent)
> > >  	ratelimit_state_init(&sbi->msg_ratelimit, DEFAULT_RATELIMIT_INTERVAL,
> > >  			     DEFAULT_RATELIMIT_BURST);
> > >  
> > > -	err = ntfs_parse_options(sb, data, silent, &sbi->options);
> > > -	if (err)
> > > -		goto out;
> > > -
> > >  	if (!rq || !blk_queue_discard(rq) || !rq->limits.discard_granularity) {
> > >  		;
> > >  	} else {
> > > @@ -1409,19 +1380,52 @@ int ntfs_discard(struct ntfs_sb_info *sbi, CLST lcn, CLST len)
> > >  	return err;
> > >  }
> > >  
> > > -static struct dentry *ntfs_mount(struct file_system_type *fs_type, int flags,
> > > -				 const char *dev_name, void *data)
> > > +static int ntfs_fs_get_tree(struct fs_context *fc)
> > > +{
> > > +	return get_tree_bdev(fc, ntfs_fill_super);
> > > +}
> > > +
> > > +static void ntfs_fs_free(struct fs_context *fc)
> > >  {
> > > -	return mount_bdev(fs_type, flags, dev_name, data, ntfs_fill_super);
> > > +	struct ntfs_sb_info *sbi = fc->s_fs_info;
> > > +
> > > +	if (sbi)
> > > +		put_ntfs(sbi);
> > > +}
> > > +
> > > +static const struct fs_context_operations ntfs_context_ops = {
> > > +	.parse_param	= ntfs_fs_parse_param,
> > > +	.get_tree	= ntfs_fs_get_tree,
> > > +	.reconfigure	= ntfs_fs_reconfigure,
> > > +	.free		= ntfs_fs_free,
> > > +};
> > > +
> > > +/*
> > > + * Set up the filesystem mount context.
> > > + */
> > > +static int ntfs_init_fs_context(struct fs_context *fc)
> > > +{
> > > +	struct ntfs_sb_info *sbi;
> > > +
> > > +	sbi = ntfs_zalloc(sizeof(struct ntfs_sb_info));
> > > +	if (!sbi)
> > > +		return -ENOMEM;
> > > +
> > > +	ntfs_default_options(&sbi->options);
> > > +
> > > +	fc->s_fs_info = sbi;
> > > +	fc->ops = &ntfs_context_ops;
> > > +	return 0;
> > >  }
> > >  
> > >  // clang-format off
> > >  static struct file_system_type ntfs_fs_type = {
> > > -	.owner		= THIS_MODULE,
> > > -	.name		= "ntfs3",
> > > -	.mount		= ntfs_mount,
> > > -	.kill_sb	= kill_block_super,
> > > -	.fs_flags	= FS_REQUIRES_DEV | FS_ALLOW_IDMAP,
> 
> Note FS_ALLOW_IDMAP here.
> 
> > > +	.owner			= THIS_MODULE,
> > > +	.name			= "ntfs3",
> > > +	.init_fs_context	= ntfs_init_fs_context,
> > > +	.parameters		= ntfs_fs_parameters,
> > > +	.kill_sb		= kill_block_super,
> > > +	.fs_flags		= FS_REQUIRES_DEV | FS_ALLOW_IDMAP,
> > 
> > If you add idmapped mount support right away please try to make sure and
> > enable ntfs3 with xfstests if that's all possible.
> 
> This is already implemented by Komarov in new series. He haven't yet
> sended it to mailing list. It is already in linux-next.
> 
> Komarov has also stated that they use xfstests (link). And he has showed
> test results.
> https://lore.kernel.org/linux-fsdevel/0e175373cef24e2abe76e203bb36d260@paragon-software.com/
> 
> I have tested these with kvm-xfstests which has support for ntfs3 made
> by Theodore Ts'o. Current results:
> 
> ./kvm-xfstests -c ntfs3 -X "generic/014,generic/129,generic/405,generic/476" -g auto
> ntfs3/default: 662 tests, 43 failures, 207 skipped, 7442 seconds
>   Failures: generic/012 generic/013 generic/016 generic/021 
>     generic/022 generic/040 generic/041 generic/065 generic/066 
>     generic/092 generic/094 generic/104 generic/130 generic/177 
>     generic/225 generic/228 generic/240 generic/255 generic/258 
>     generic/316 generic/321 generic/322 generic/335 generic/336 
>     generic/341 generic/342 generic/343 generic/348 generic/360 
>     generic/401 generic/423 generic/475 generic/483 generic/498 
>     generic/510 generic/526 generic/527 generic/538 generic/551 
>     generic/552 generic/631 generic/633 generic/640 
> Totals: 455 tests, 207 skipped, 43 failures, 0 errors, 7088s
> 
> ACL support seems broken. Notice that many of these are in group "punch"
> and punch a hole for regular files was added in "V28" but maybe that
> does not work correctly as showed by xfstests.
> 
> There is also on going patch set by Theodore for support ntfs3 and fuse
> based filesystem in straight in xfstests (link). And after that gets
> there then we can ask 01 project to include ntfs3 to their testing.
> And hopefully Paragon will send some ntfs related test to upstream.
> https://lore.kernel.org/fstests/YQoVXWRFGeY19onQ@mit.edu/

Ok, cool. I can help you fix the failures in generic/633 and generic/640
once that work has progressed.

Christian

[RFC PATCH 2/4] fs/ntfs3: Remove unnecesarry mount option noatime

[Kari Argillander]

Remove unnecesarry mount option noatime because this will be handled
by VFS. Our function ntfs_parse_param will never get opt like
this.

Signed-off-by: Kari Argillander <kari.argillander@gmail.com>
---
 Documentation/filesystems/ntfs3.rst | 4 ----
 fs/ntfs3/super.c                    | 7 -------
 2 files changed, 11 deletions(-)

diff --git a/Documentation/filesystems/ntfs3.rst b/Documentation/filesystems/ntfs3.rst
index ffe9ea0c1499..af7158de6fde 100644
--- a/Documentation/filesystems/ntfs3.rst
+++ b/Documentation/filesystems/ntfs3.rst
@@ -85,10 +85,6 @@ acl			Support POSIX ACLs (Access Control Lists). Effective if
 			supported by Kernel. Not to be confused with NTFS ACLs.
 			The option specified as acl enables support for POSIX ACLs.
 
-noatime			All files and directories will not update their last access
-			time attribute if a partition is mounted with this parameter.
-			This option can speed up file system operation.
-
 ===============================================================================
 
 ToDo list
diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c
index d805e0b31404..e4e2bd0ebfe6 100644
--- a/fs/ntfs3/super.c
+++ b/fs/ntfs3/super.c
@@ -240,7 +240,6 @@ enum Opt {
 	Opt_nohidden,
 	Opt_showmeta,
 	Opt_acl,
-	Opt_noatime,
 	Opt_nls,
 	Opt_prealloc,
 	Opt_no_acs_rules,
@@ -260,7 +259,6 @@ static const struct fs_parameter_spec ntfs_fs_parameters[] = {
 	fsparam_flag_no("sparse",		Opt_sparse),
 	fsparam_flag("nohidden",		Opt_nohidden),
 	fsparam_flag_no("acl",			Opt_acl),
-	fsparam_flag("noatime",			Opt_noatime),
 	fsparam_flag_no("showmeta",		Opt_showmeta),
 	fsparam_string("nls",			Opt_nls),
 	fsparam_flag_no("prealloc",		Opt_prealloc),
@@ -341,9 +339,6 @@ static int ntfs_fs_parse_param(struct fs_context *fc, struct fs_parameter *param
 		else
 			fc->sb_flags &= ~SB_POSIXACL;
 		break;
-	case Opt_noatime:
-		fc->sb_flags |= SB_NOATIME;
-		break;
 	case Opt_showmeta:
 		opts->showmeta = result.negated ? 0 : 1;
 		break;
@@ -555,8 +550,6 @@ static int ntfs_show_options(struct seq_file *m, struct dentry *root)
 		seq_puts(m, ",prealloc");
 	if (sb->s_flags & SB_POSIXACL)
 		seq_puts(m, ",acl");
-	if (sb->s_flags & SB_NOATIME)
-		seq_puts(m, ",noatime");
 
 	return 0;
 }
-- 
2.25.1

Re: [RFC PATCH 2/4] fs/ntfs3: Remove unnecesarry mount option noatime

[Christoph Hellwig]

On Mon, Aug 16, 2021 at 05:47:01AM +0300, Kari Argillander wrote:
> Remove unnecesarry mount option noatime because this will be handled
> by VFS. Our function ntfs_parse_param will never get opt like
> this.

Looks good, but I'd move this to the front of your series.

Re: [RFC PATCH 2/4] fs/ntfs3: Remove unnecesarry mount option noatime

[Kari Argillander]

On Mon, Aug 16, 2021 at 02:18:18PM +0200, Christoph Hellwig wrote:
> On Mon, Aug 16, 2021 at 05:47:01AM +0300, Kari Argillander wrote:
> > Remove unnecesarry mount option noatime because this will be handled
> > by VFS. Our function ntfs_parse_param will never get opt like
> > this.
> 
> Looks good, but I'd move this to the front of your series.

I agree. I will do this.

[RFC PATCH 3/4] fs/ntfs3: Make mount option nohidden more universal

[Kari Argillander]

If we call Opt_nohidden with just keyword hidden, then we can use
hidden/nohidden when mounting. We already use this method for almoust
all other parameters so it is just logical that this will use same
method.

We still have to think what to do with no_acl_rules.

Signed-off-by: Kari Argillander <kari.argillander@gmail.com>
---
 fs/ntfs3/super.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c
index e4e2bd0ebfe6..2a4866c2a512 100644
--- a/fs/ntfs3/super.c
+++ b/fs/ntfs3/super.c
@@ -257,7 +257,7 @@ static const struct fs_parameter_spec ntfs_fs_parameters[] = {
 	fsparam_flag_no("discard",		Opt_discard),
 	fsparam_flag_no("force",		Opt_force),
 	fsparam_flag_no("sparse",		Opt_sparse),
-	fsparam_flag("nohidden",		Opt_nohidden),
+	fsparam_flag_no("hidden",		Opt_nohidden),
 	fsparam_flag_no("acl",			Opt_acl),
 	fsparam_flag_no("showmeta",		Opt_showmeta),
 	fsparam_string("nls",			Opt_nls),
@@ -327,7 +327,7 @@ static int ntfs_fs_parse_param(struct fs_context *fc, struct fs_parameter *param
 		opts->sparse = result.negated ? 0 : 1;
 		break;
 	case Opt_nohidden:
-		opts->nohidden = 1;
+		opts->nohidden = result.negated ? 1 : 0;
 		break;
 	case Opt_acl:
 		if (!result.negated)
-- 
2.25.1

[RFC PATCH 4/4] fs/ntfs3: Add iocharset= mount option as alias for nls=

[Kari Argillander]

Other fs drivers are using iocharset= mount option for specifying charset.
So add it also for ntfs3 and mark old nls= mount option as deprecated.

Signed-off-by: Kari Argillander <kari.argillander@gmail.com>
---
 Documentation/filesystems/ntfs3.rst |  4 ++--
 fs/ntfs3/super.c                    | 14 +++++++++-----
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/Documentation/filesystems/ntfs3.rst b/Documentation/filesystems/ntfs3.rst
index af7158de6fde..ded706474825 100644
--- a/Documentation/filesystems/ntfs3.rst
+++ b/Documentation/filesystems/ntfs3.rst
@@ -32,12 +32,12 @@ generic ones.
 
 ===============================================================================
 
-nls=name		This option informs the driver how to interpret path
+iocharset=name		This option informs the driver how to interpret path
 			strings and translate them to Unicode and back. If
 			this option is not set, the default codepage will be
 			used (CONFIG_NLS_DEFAULT).
 			Examples:
-				'nls=utf8'
+				'iocharset=utf8'
 
 uid=
 gid=
diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c
index 2a4866c2a512..886c495d2f5c 100644
--- a/fs/ntfs3/super.c
+++ b/fs/ntfs3/super.c
@@ -240,7 +240,7 @@ enum Opt {
 	Opt_nohidden,
 	Opt_showmeta,
 	Opt_acl,
-	Opt_nls,
+	Opt_iocharset,
 	Opt_prealloc,
 	Opt_no_acs_rules,
 	Opt_err,
@@ -260,9 +260,13 @@ static const struct fs_parameter_spec ntfs_fs_parameters[] = {
 	fsparam_flag_no("hidden",		Opt_nohidden),
 	fsparam_flag_no("acl",			Opt_acl),
 	fsparam_flag_no("showmeta",		Opt_showmeta),
-	fsparam_string("nls",			Opt_nls),
 	fsparam_flag_no("prealloc",		Opt_prealloc),
 	fsparam_flag("no_acs_rules",		Opt_no_acs_rules),
+	fsparam_string("iocharset",		Opt_iocharset),
+
+	__fsparam(fs_param_is_string,
+		  "nls", Opt_iocharset,
+		  fs_param_deprecated, NULL),
 	{}
 };
 // clang-format on
@@ -342,7 +346,7 @@ static int ntfs_fs_parse_param(struct fs_context *fc, struct fs_parameter *param
 	case Opt_showmeta:
 		opts->showmeta = result.negated ? 0 : 1;
 		break;
-	case Opt_nls:
+	case Opt_iocharset:
 		unload_nls(opts->nls);
 
 		opts->nls = ntfs_load_nls(param->string);
@@ -529,9 +533,9 @@ static int ntfs_show_options(struct seq_file *m, struct dentry *root)
 	if (opts->dmask)
 		seq_printf(m, ",dmask=%04o", ~opts->fs_dmask_inv);
 	if (opts->nls)
-		seq_printf(m, ",nls=%s", opts->nls->charset);
+		seq_printf(m, ",iocharset=%s", opts->nls->charset);
 	else
-		seq_puts(m, ",nls=utf8");
+		seq_puts(m, ",iocharset=utf8");
 	if (opts->sys_immutable)
 		seq_puts(m, ",sys_immutable");
 	if (opts->discard)
-- 
2.25.1

Re: [RFC PATCH 0/4] fs/ntfs3: Use new mount api and change some opts

[Kari Argillander]

As I screw up subject with this one I resend this with replay.

On Mon, Aug 16, 2021 at 05:46:59AM +0300, Kari Argillander wrote:
> This series modify ntfs3 to use new mount api as Christoph Hellwig wish
> for.
> https://lore.kernel.org/linux-fsdevel/20210810090234.GA23732@lst.de/
> 
> It also modify mount options noatime (not needed) and make new alias
> for nls because kernel is changing to use it as described in here
> https://lore.kernel.org/linux-fsdevel/20210808162453.1653-1-pali@kernel.org/
> 
> I would like really like to get fsparam_flag_no also for no_acs_rules
> but then we have to make new name for it. Other possibility is to
> modify mount api so it mount option can be no/no_. I think that would
> maybe be good change. 
> 
> I did not quite like how I did nls table loading because now it always
> first load default table and if user give option then default table is
> dropped and if reconfigure is happening and this was same as before then
> it is dropped. I try to make loading in fill_super and fs_reconfigure
> but that just look ugly. This is quite readible so I leave it like this.
> We also do not mount/remount so often that this probebly does not
> matter. It seems that if new mount api had possibility to give default
> value for mount option then there is not this kind of problem.
> 
> I would hope that these will added top of the now going ntfs3 patch
> series. I do not have so many contributions to kernel yet and I would
> like to get my name going there so that in future it would be easier to
> contribute kernel.
> 
> Kari Argillander (4):
>   fs/ntfs3: Use new api for mounting
>   fs/ntfs3: Remove unnecesarry mount option noatime
>   fs/ntfs3: Make mount option nohidden more universal
>   fs/ntfs3: Add iocharset= mount option as alias for nls=
> 
>  Documentation/filesystems/ntfs3.rst |   4 -
>  fs/ntfs3/super.c                    | 391 ++++++++++++++--------------
>  2 files changed, 196 insertions(+), 199 deletions(-)
> 
> -- 
> 2.25.1
> 
> 

Re: your mail

[Christoph Hellwig]

On Mon, Aug 16, 2021 at 05:46:59AM +0300, Kari Argillander wrote:
> I would like really like to get fsparam_flag_no also for no_acs_rules
> but then we have to make new name for it. Other possibility is to
> modify mount api so it mount option can be no/no_. I think that would
> maybe be good change. 

I don't think adding another no_ alias is a good idea.  I'd suggest
to just rename the existing flag before the ntfs3 driver ever hits
mainline.

Re: [RFC PATCH 0/4] fs/ntfs3: Use new mount api and change some opts

[Kari Argillander]

On Mon, Aug 16, 2021 at 02:27:21PM +0200, Christoph Hellwig wrote:
> On Mon, Aug 16, 2021 at 05:46:59AM +0300, Kari Argillander wrote:
> > I would like really like to get fsparam_flag_no also for no_acs_rules
> > but then we have to make new name for it. Other possibility is to
> > modify mount api so it mount option can be no/no_. I think that would
> > maybe be good change. 
> 
> I don't think adding another no_ alias is a good idea.  I'd suggest
> to just rename the existing flag before the ntfs3 driver ever hits
> mainline.

Konstantin can suggest what should we call this.