(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

[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

[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: [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 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: 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 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 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.

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.

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

[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

[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

Re: your mail

[CGEL]

O
Sat, Aug 21, 2021 at 11:59:39AM +0300, Kari Argillander wrote:
> Bcc:
> Subject: Re: [PATCH] proc: prevent mount proc on same mountpoint in one pid
>  namespace
> Reply-To:
> In-Reply-To: <20210821083105.30336-1-yang.yang29@zte.com.cn>
> 
> On Sat, Aug 21, 2021 at 01:31:05AM -0700, cgel.zte@gmail.com wrote:
> > From: Yang Yang <yang.yang29@zte.com.cn>
> > 
> > Patch "proc: allow to mount many instances of proc in one pid namespace"
> > aims to mount many instances of proc on different mountpoint, see
> > tools/testing/selftests/proc/proc-multiple-procfs.c.
> > 
> > But there is a side-effects, user can mount many instances of proc on
> > the same mountpoint in one pid namespace, which is not allowed before.
> > This duplicate mount makes no sense but wastes memory and CPU, and user
> > may be confused why kernel allows it.
> > 
> > The logic of this patch is: when try to mount proc on /mnt, check if
> > there is a proc instance mount on /mnt in the same pid namespace. If
> > answer is yes, return -EBUSY.
> > 
> > Since this check can't be done in proc_get_tree(), which call
> > get_tree_nodev() and will create new super_block unconditionally.
> > And other nodev fs may faces the same case, so add a new hook in
> > fs_context_operations.
> > 
> > Reported-by: Zeal Robot <zealci@zte.com.cn>
> > Signed-off-by: Yang Yang <yang.yang29@zte.com.cn>
> > ---
> >  fs/namespace.c             |  9 +++++++++
> >  fs/proc/root.c             | 15 +++++++++++++++
> >  include/linux/fs_context.h |  1 +
> >  3 files changed, 25 insertions(+)
> > 
> > diff --git a/fs/namespace.c b/fs/namespace.c
> > index f79d9471cb76..84da649a70c5 100644
> > --- a/fs/namespace.c
> > +++ b/fs/namespace.c
> > @@ -2878,6 +2878,7 @@ static int do_new_mount_fc(struct fs_context *fc, struct path *mountpoint,
> >  static int do_new_mount(struct path *path, const char *fstype, int sb_flags,
> >  			int mnt_flags, const char *name, void *data)
> >  {
> > +	int (*check_mntpoint)(struct fs_context *fc, struct path *path);
> >  	struct file_system_type *type;
> >  	struct fs_context *fc;
> >  	const char *subtype = NULL;
> > @@ -2906,6 +2907,13 @@ static int do_new_mount(struct path *path, const char *fstype, int sb_flags,
> >  	if (IS_ERR(fc))
> >  		return PTR_ERR(fc);
> >  
> > +	/* check if there is a same super_block mount on path*/
> > +	check_mntpoint = fc->ops->check_mntpoint;
> > +	if (check_mntpoint)
> > +		err = check_mntpoint(fc, path);
> > +	if (err < 0)
> > +		goto err_fc;
> > +
> >  	if (subtype)
> >  		err = vfs_parse_fs_string(fc, "subtype",
> >  					  subtype, strlen(subtype));
> > @@ -2920,6 +2928,7 @@ static int do_new_mount(struct path *path, const char *fstype, int sb_flags,
> >  	if (!err)
> >  		err = do_new_mount_fc(fc, path, mnt_flags);
> >  
> > +err_fc:
> >  	put_fs_context(fc);
> >  	return err;
> >  }
> > diff --git a/fs/proc/root.c b/fs/proc/root.c
> > index c7e3b1350ef8..0971d6b0bec2 100644
> > --- a/fs/proc/root.c
> > +++ b/fs/proc/root.c
> > @@ -237,11 +237,26 @@ static void proc_fs_context_free(struct fs_context *fc)
> >  	kfree(ctx);
> >  }
> >  
> > +static int proc_check_mntpoint(struct fs_context *fc, struct path *path)
> > +{
> > +	struct super_block *mnt_sb = path->mnt->mnt_sb;
> > +	struct proc_fs_info *fs_info;
> > +
> > +	if (strcmp(mnt_sb->s_type->name, "proc") == 0) {
> > +		fs_info = mnt_sb->s_fs_info;
> > +		if (fs_info->pid_ns == task_active_pid_ns(current) &&
> > +		    path->mnt->mnt_root == path->dentry)
> > +			return -EBUSY;
> > +	}
> > +	return 0;
> > +}
> > +
> >  static const struct fs_context_operations proc_fs_context_ops = {
> >  	.free		= proc_fs_context_free,
> >  	.parse_param	= proc_parse_param,
> >  	.get_tree	= proc_get_tree,
> >  	.reconfigure	= proc_reconfigure,
> > +	.check_mntpoint	= proc_check_mntpoint,
> >  };
> >  
> >  static int proc_init_fs_context(struct fs_context *fc)
> > diff --git a/include/linux/fs_context.h b/include/linux/fs_context.h
> > index 6b54982fc5f3..090a05fb2d7d 100644
> > --- a/include/linux/fs_context.h
> > +++ b/include/linux/fs_context.h
> > @@ -119,6 +119,7 @@ struct fs_context_operations {
> >  	int (*parse_monolithic)(struct fs_context *fc, void *data);
> >  	int (*get_tree)(struct fs_context *fc);
> >  	int (*reconfigure)(struct fs_context *fc);
> > +	int (*check_mntpoint)(struct fs_context *fc, struct path *path);
> 
> Don't you think this should be it's own patch. It is after all internal
> api change. This also needs documentation. It would be confusing if
> someone convert to new mount api and there is one line which just
> address some proc stuff but even commit message does not address does
> every fs needs to add this. 
> 
> Documentation is very good shape right now and we are in face that
> everyone is migrating to use new mount api so everyting should be well
> documented.
> i
Thanks for your reply!

I will take commit message more carefully next time.
Sinece I am not quit sure about this patch, so I didn't write
Documentation for patch v1. AIViro had made it clear, so this 
patch is abondoned.
> >  };
> >  
> >  /*
> > -- 
> > 2.25.1
> > 

Re: your mail

[Jan Kara]

On Tue 03-05-11 18:43:48, Surbhi Palande wrote:
> On 05/03/2011 06:36 PM, Jan Kara wrote:
> >On Tue 03-05-11 16:56:57, Surbhi Palande wrote:
> >>On 05/03/2011 04:46 PM, Jan Kara wrote:
> >>>On Tue 03-05-11 16:08:36, Surbhi Palande wrote:
> >>
> >>Sorry for missing the subject line :(
> >>>>On munmap() zap_pte_range() is called which dirties the PTE dirty pages as
> >>>>Toshiyuki pointed out.
> >>>>
> >>>>zap_pte_range()
> >>>>   mapping->a_ops->set_page_dirty (= ext4_journalled_set_page_dirty)
> >>>>
> >>>>So, I think that it is here that we should do the checking for a ext4 F.S
> >>>>frozen state and also prevent a parallel ext4 F.S freeze from happening.
> >>>>
> >>>>Attaching a patch for initial review. Please do let me know your thoughts!
> >>>   This is definitely the wrong place. ->set_page_dirty() callbacks are
> >>>called with various locks held and the page need not be locked (thus
> >>>dereferencing page->mapping is oopsable). Moreover this particular callback
> >>>is called only in data=journal mode.
> >>Ok! Thanks for that!
> >>
> >>>
> >>>Believe me, the right place is page_mkwrite() - you have to catch the
> >>>read-only =>   read-write page transition. Once the page is mapped
> >>>read-write, you've already lost the race.
> >>
> >>My only point is:
> >>1) something should prevent the freeze from happening. We cant
> >>merely check the vfs_check_frozen()?
> >   Yes, I agree - see my other email with patches.
> >
> >>And this should be done where the page is marked dirty.Also, I
> >>thought that the page is marked read-write only in the page table in
> >>the __do_page_fault()? i.e the zap_pte_range() marks them dirty in
> >>the page cache? Is this understanding right?
> >   The page can become dirty either because it was written via standard
> >write - write_begin is responsible for reliable check here - or it was
> >written via mmap - here we rely on page_mkwrite to do a reliable check -
> >it is analogous to write_begin callback. There should be no other way
> >to dirty a page.
> >
> >With dirty bits it is a bit complicated. We have two of them in fact. One
> >in page table entry maintained by mmu and one in page structure maintained
> >by kernel. Some functions (such as zap_pte_range()) copy the dirty bits
> >from page table into struct page. This is a lazy process so page can in
> >principle have new data without a dirty bit set in struct page because we
> >have not yet copied the dirty bit from page table. Only at moments where it
> >is important (like when we want to unmap the page, or throw away the page,
> >or so), we make sure struct page and page table bits are in sync.
> >
> >Another subtle thing you need not be aware of it that when we clear page
> >dirty bit, we also writeprotect the page. So we are guaranteed to get a
> >page fault when the page is written to again.
> >
> >>IMHO, whatever code dirties the page in the page cache should call a
> >>F.S specific function and let it _prevent_ a fsfreeze while the page
> >>is getting dirtied, so that a freeze called after this point flushes
> >>this page!
> >   Agreed, that's what code in write_begin() and page_mkwrite() should
> >achieve.
> >								Honza
> Thanks a lot for the wonderful explanation :)
> 
> How about the revert : i.e calling  jbd2_journal_unlock_updates()
> from ext4_unfreeze() instead of the ext4_freeze()? Do you agree to
> that?
  Sorry, I don't agree with revert. We could talk about changing
jbd2_journal_unlock_updates() to not return with mutex held (and handle
synchronization of locked journal operations differently) as an alternative
to doing "freeze" reference counting. But returning with mutex held to user
space is no-go. It will cause problems in lockdep, violates kernel locking
rules, and generally is a bad programming ;).

								Honza

-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: your mail

[Surbhi Palande]

On 05/03/2011 06:36 PM, Jan Kara wrote:
> On Tue 03-05-11 16:56:57, Surbhi Palande wrote:
>> On 05/03/2011 04:46 PM, Jan Kara wrote:
>>> On Tue 03-05-11 16:08:36, Surbhi Palande wrote:
>>
>> Sorry for missing the subject line :(
>>>> On munmap() zap_pte_range() is called which dirties the PTE dirty pages as
>>>> Toshiyuki pointed out.
>>>>
>>>> zap_pte_range()
>>>>    mapping->a_ops->set_page_dirty (= ext4_journalled_set_page_dirty)
>>>>
>>>> So, I think that it is here that we should do the checking for a ext4 F.S
>>>> frozen state and also prevent a parallel ext4 F.S freeze from happening.
>>>>
>>>> Attaching a patch for initial review. Please do let me know your thoughts!
>>>    This is definitely the wrong place. ->set_page_dirty() callbacks are
>>> called with various locks held and the page need not be locked (thus
>>> dereferencing page->mapping is oopsable). Moreover this particular callback
>>> is called only in data=journal mode.
>> Ok! Thanks for that!
>>
>>>
>>> Believe me, the right place is page_mkwrite() - you have to catch the
>>> read-only =>   read-write page transition. Once the page is mapped
>>> read-write, you've already lost the race.
>>
>> My only point is:
>> 1) something should prevent the freeze from happening. We cant
>> merely check the vfs_check_frozen()?
>    Yes, I agree - see my other email with patches.
>
>> And this should be done where the page is marked dirty.Also, I
>> thought that the page is marked read-write only in the page table in
>> the __do_page_fault()? i.e the zap_pte_range() marks them dirty in
>> the page cache? Is this understanding right?
>    The page can become dirty either because it was written via standard
> write - write_begin is responsible for reliable check here - or it was
> written via mmap - here we rely on page_mkwrite to do a reliable check -
> it is analogous to write_begin callback. There should be no other way
> to dirty a page.
>
> With dirty bits it is a bit complicated. We have two of them in fact. One
> in page table entry maintained by mmu and one in page structure maintained
> by kernel. Some functions (such as zap_pte_range()) copy the dirty bits
> from page table into struct page. This is a lazy process so page can in
> principle have new data without a dirty bit set in struct page because we
> have not yet copied the dirty bit from page table. Only at moments where it
> is important (like when we want to unmap the page, or throw away the page,
> or so), we make sure struct page and page table bits are in sync.
>
> Another subtle thing you need not be aware of it that when we clear page
> dirty bit, we also writeprotect the page. So we are guaranteed to get a
> page fault when the page is written to again.
>
>> IMHO, whatever code dirties the page in the page cache should call a
>> F.S specific function and let it _prevent_ a fsfreeze while the page
>> is getting dirtied, so that a freeze called after this point flushes
>> this page!
>    Agreed, that's what code in write_begin() and page_mkwrite() should
> achieve.
> 								Honza
Thanks a lot for the wonderful explanation :)

How about the revert : i.e calling  jbd2_journal_unlock_updates() from 
ext4_unfreeze() instead of the ext4_freeze()? Do you agree to that?


Warm Regards,
Surbhi.

Re: your mail

[Jan Kara]

On Tue 03-05-11 16:56:57, Surbhi Palande wrote:
> On 05/03/2011 04:46 PM, Jan Kara wrote:
> >On Tue 03-05-11 16:08:36, Surbhi Palande wrote:
> 
> Sorry for missing the subject line :(
> >>On munmap() zap_pte_range() is called which dirties the PTE dirty pages as
> >>Toshiyuki pointed out.
> >>
> >>zap_pte_range()
> >>   mapping->a_ops->set_page_dirty (= ext4_journalled_set_page_dirty)
> >>
> >>So, I think that it is here that we should do the checking for a ext4 F.S
> >>frozen state and also prevent a parallel ext4 F.S freeze from happening.
> >>
> >>Attaching a patch for initial review. Please do let me know your thoughts!
> >   This is definitely the wrong place. ->set_page_dirty() callbacks are
> >called with various locks held and the page need not be locked (thus
> >dereferencing page->mapping is oopsable). Moreover this particular callback
> >is called only in data=journal mode.
> Ok! Thanks for that!
> 
> >
> >Believe me, the right place is page_mkwrite() - you have to catch the
> >read-only =>  read-write page transition. Once the page is mapped
> >read-write, you've already lost the race.
> 
> My only point is:
> 1) something should prevent the freeze from happening. We cant
> merely check the vfs_check_frozen()?
  Yes, I agree - see my other email with patches.

> And this should be done where the page is marked dirty.Also, I
> thought that the page is marked read-write only in the page table in
> the __do_page_fault()? i.e the zap_pte_range() marks them dirty in
> the page cache? Is this understanding right?
  The page can become dirty either because it was written via standard
write - write_begin is responsible for reliable check here - or it was
written via mmap - here we rely on page_mkwrite to do a reliable check -
it is analogous to write_begin callback. There should be no other way
to dirty a page.

With dirty bits it is a bit complicated. We have two of them in fact. One
in page table entry maintained by mmu and one in page structure maintained
by kernel. Some functions (such as zap_pte_range()) copy the dirty bits
from page table into struct page. This is a lazy process so page can in
principle have new data without a dirty bit set in struct page because we
have not yet copied the dirty bit from page table. Only at moments where it
is important (like when we want to unmap the page, or throw away the page,
or so), we make sure struct page and page table bits are in sync.

Another subtle thing you need not be aware of it that when we clear page
dirty bit, we also writeprotect the page. So we are guaranteed to get a
page fault when the page is written to again.

> IMHO, whatever code dirties the page in the page cache should call a
> F.S specific function and let it _prevent_ a fsfreeze while the page
> is getting dirtied, so that a freeze called after this point flushes
> this page!
  Agreed, that's what code in write_begin() and page_mkwrite() should
achieve.
								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: your mail

[Surbhi Palande]

On 05/03/2011 04:56 PM, Surbhi Palande wrote:
> On 05/03/2011 04:46 PM, Jan Kara wrote:
>> On Tue 03-05-11 16:08:36, Surbhi Palande wrote:
>
> Sorry for missing the subject line :(
>>> On munmap() zap_pte_range() is called which dirties the PTE dirty
>>> pages as
>>> Toshiyuki pointed out.
>>>
>>> zap_pte_range()
>>> mapping->a_ops->set_page_dirty (= ext4_journalled_set_page_dirty)
>>>
>>> So, I think that it is here that we should do the checking for a ext4
>>> F.S
>>> frozen state and also prevent a parallel ext4 F.S freeze from happening.
>>>
>>> Attaching a patch for initial review. Please do let me know your
>>> thoughts!
>> This is definitely the wrong place. ->set_page_dirty() callbacks are
>> called with various locks held and the page need not be locked (thus
>> dereferencing page->mapping is oopsable). Moreover this particular
>> callback
>> is called only in data=journal mode.
> Ok! Thanks for that!
>
>>
>> Believe me, the right place is page_mkwrite() - you have to catch the
>> read-only => read-write page transition. Once the page is mapped
>> read-write, you've already lost the race.
Also, we then need to prevent a munmap()/zap_pte_range() call from 
dirtying a mmapped file page when the F.S is frozen?

Warm Regards,
Surbhi.

>
> My only point is:
> 1) something should prevent the freeze from happening. We cant merely
> check the vfs_check_frozen()?
>
> And this should be done where the page is marked dirty.Also, I thought
> that the page is marked read-write only in the page table in the
> __do_page_fault()? i.e the zap_pte_range() marks them dirty in the page
> cache? Is this understanding right?
>
> IMHO, whatever code dirties the page in the page cache should call a F.S
> specific function and let it _prevent_ a fsfreeze while the page is
> getting dirtied, so that a freeze called after this point flushes this
> page!
>
> Warm Regards,
> Surbhi.
>
>
>
>
>
>
>
>
>
>
>>
>> Honza
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html

Re: your mail

[Surbhi Palande]

On 05/03/2011 04:46 PM, Jan Kara wrote:
> On Tue 03-05-11 16:08:36, Surbhi Palande wrote:

Sorry for missing the subject line :(
>> On munmap() zap_pte_range() is called which dirties the PTE dirty pages as
>> Toshiyuki pointed out.
>>
>> zap_pte_range()
>>    mapping->a_ops->set_page_dirty (= ext4_journalled_set_page_dirty)
>>
>> So, I think that it is here that we should do the checking for a ext4 F.S
>> frozen state and also prevent a parallel ext4 F.S freeze from happening.
>>
>> Attaching a patch for initial review. Please do let me know your thoughts!
>    This is definitely the wrong place. ->set_page_dirty() callbacks are
> called with various locks held and the page need not be locked (thus
> dereferencing page->mapping is oopsable). Moreover this particular callback
> is called only in data=journal mode.
Ok! Thanks for that!

>
> Believe me, the right place is page_mkwrite() - you have to catch the
> read-only =>  read-write page transition. Once the page is mapped
> read-write, you've already lost the race.

My only point is:
1) something should prevent the freeze from happening. We cant merely 
check the vfs_check_frozen()?

And this should be done where the page is marked dirty.Also, I thought 
that the page is marked read-write only in the page table in the 
__do_page_fault()? i.e the zap_pte_range() marks them dirty in the page 
cache? Is this understanding right?

IMHO, whatever code dirties the page in the page cache should call a F.S 
specific function and let it _prevent_ a fsfreeze while the page is 
getting dirtied, so that a freeze called after this point flushes this page!

Warm Regards,
Surbhi.










>
> 								Honza

Re: your mail

[Jan Kara]

On Tue 03-05-11 16:08:36, Surbhi Palande wrote:
> On munmap() zap_pte_range() is called which dirties the PTE dirty pages as
> Toshiyuki pointed out.
> 
> zap_pte_range()
>   mapping->a_ops->set_page_dirty (= ext4_journalled_set_page_dirty)  
> 
> So, I think that it is here that we should do the checking for a ext4 F.S
> frozen state and also prevent a parallel ext4 F.S freeze from happening.
> 
> Attaching a patch for initial review. Please do let me know your thoughts! 
  This is definitely the wrong place. ->set_page_dirty() callbacks are
called with various locks held and the page need not be locked (thus
dereferencing page->mapping is oopsable). Moreover this particular callback
is called only in data=journal mode.

Believe me, the right place is page_mkwrite() - you have to catch the
read-only => read-write page transition. Once the page is mapped
read-write, you've already lost the race.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: your mail

[Wu Fengguang]

On Tue, Jun 22, 2010 at 09:54:58PM +0800, Jan Kara wrote:
> On Tue 22-06-10 10:59:41, Wu Fengguang wrote:
> > > - use tagging also for WB_SYNC_NONE writeback - there's problem with an
> > >   interaction with wbc->nr_to_write. If we tag all dirty pages, we can
> > >   spend too much time tagging when we write only a few pages in the end
> > >   because of nr_to_write. If we tag only say nr_to_write pages, we may
> > >   not have enough pages tagged because some pages are written out by
> > >   someone else and so we would have to restart and tagging would become
> > 
> > This could be addressed by ignoring nr_to_write for the WB_SYNC_NONE
> > writeback triggered by sync(). write_cache_pages() already ignored
> > nr_to_write for WB_SYNC_ALL.
>   We could do that but frankly, I'm not very fond of adding more special
> cases to writeback code than strictly necessary...

So do me. However for this case we only need to broaden the special case test:

                        if (nr_to_write > 0) {
                                nr_to_write--;
                                if (nr_to_write == 0 &&
-                                   wbc->sync_mode == WB_SYNC_NONE) {
+                                   !wbc->for_sync) {

> > >   essentially useless. So my option is - switch to tagging for WB_SYNC_NONE
> > >   writeback if we can get rid of nr_to_write. But that's a story for
> > >   a different patch set.
> > 
> > Besides introducing overheads, it will be a policy change in which the
> > system loses control to somehow "throttle" writeback of huge files.
>   Yes, but if we guarantee we cannot livelock on a single file, do we care?
> Memory management does not care because it's getting rid of dirty pages
> which is what it wants. User might care but actually writing out files in
> the order they were dirtied (i.e., the order user written them) is quite
> natural so it's not a "surprising" behavior. And I don't think we can
> assume that data in those small files are more valuable than data in the
> large file and thus should be written earlier...

It could be a surprising behavior when, there is a 4GB dirty file and
100 small dirty files. The user may expect the 100 small dirty files
be synced to disk after 30s. However without nr_to_write, they could
be delayed by the 4GB file for 40 more seconds. Now if something goes
wrong in between and the small dirty files happen to include some
.c/.tex/.txt/... files. 

Small files are more likely your precious documents that are typed in
word-by-word and perhaps the most important data you want to protect.
Naturally you'll want them enjoy more priority than large files.

>   With the overhead you are right that tagging is more expensive than
> checking nr_to_write limit...

Thanks,
Fengguang

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: your mail

[Jan Kara]

On Tue 22-06-10 10:59:41, Wu Fengguang wrote:
> > - use tagging also for WB_SYNC_NONE writeback - there's problem with an
> >   interaction with wbc->nr_to_write. If we tag all dirty pages, we can
> >   spend too much time tagging when we write only a few pages in the end
> >   because of nr_to_write. If we tag only say nr_to_write pages, we may
> >   not have enough pages tagged because some pages are written out by
> >   someone else and so we would have to restart and tagging would become
> 
> This could be addressed by ignoring nr_to_write for the WB_SYNC_NONE
> writeback triggered by sync(). write_cache_pages() already ignored
> nr_to_write for WB_SYNC_ALL.
  We could do that but frankly, I'm not very fond of adding more special
cases to writeback code than strictly necessary...

> >   essentially useless. So my option is - switch to tagging for WB_SYNC_NONE
> >   writeback if we can get rid of nr_to_write. But that's a story for
> >   a different patch set.
> 
> Besides introducing overheads, it will be a policy change in which the
> system loses control to somehow "throttle" writeback of huge files.
  Yes, but if we guarantee we cannot livelock on a single file, do we care?
Memory management does not care because it's getting rid of dirty pages
which is what it wants. User might care but actually writing out files in
the order they were dirtied (i.e., the order user written them) is quite
natural so it's not a "surprising" behavior. And I don't think we can
assume that data in those small files are more valuable than data in the
large file and thus should be written earlier...
  With the overhead you are right that tagging is more expensive than
checking nr_to_write limit...

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: your mail

[Wu Fengguang]

> - use tagging also for WB_SYNC_NONE writeback - there's problem with an
>   interaction with wbc->nr_to_write. If we tag all dirty pages, we can
>   spend too much time tagging when we write only a few pages in the end
>   because of nr_to_write. If we tag only say nr_to_write pages, we may
>   not have enough pages tagged because some pages are written out by
>   someone else and so we would have to restart and tagging would become

This could be addressed by ignoring nr_to_write for the WB_SYNC_NONE
writeback triggered by sync(). write_cache_pages() already ignored
nr_to_write for WB_SYNC_ALL.

>   essentially useless. So my option is - switch to tagging for WB_SYNC_NONE
>   writeback if we can get rid of nr_to_write. But that's a story for
>   a different patch set.

Besides introducing overheads, it will be a policy change in which the
system loses control to somehow "throttle" writeback of huge files.

So it may be safer to enlarge nr_to_write instead of canceling it totally.

Thanks,
Fengguang

Re: your mail

[Dave Chinner]

On Wed, Jun 16, 2010 at 06:33:49PM +0200, Jan Kara wrote:
>   Hello,
> 
>   here is the fourth version of the writeback livelock avoidance patches
> for data integrity writes. To quickly summarize the idea: we tag dirty
> pages at the beginning of write_cache_pages with a new TOWRITE tag and
> then write only tagged pages to avoid parallel writers to livelock us.
> See changelogs of the patches for more details.
>   I have tested the patches with fsx and a test program I wrote which
> checks that if we crash after fsync, the data is indeed on disk.
>   If there are no more concerns, can these patches get merged?

Has it been run through xfstests? I'd suggest doing that at least
with XFS as there are several significant sync sanity tests for XFS
in the suite...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>