[PATCH v2 0/6] fs/ntfs3: Use new mount api and change some opts

[PATCH v2 0/6] fs/ntfs3: Use new mount api and change some opts

[Kari Argillander]

This series modify ntfs3 to use new mount api as Christoph Hellwig
wish for. And also it must be done at some point.
lore.kernel.org/linux-fsdevel/20210810090234.GA23732@lst.de/

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

Modify mount opt no_acl_rules so we can use new mount api feature so
new one is (no)acl_rules. I'm open to suggestion what should we call
this if this is not good.

I did testing some testing and there seems to be unnecesarry remount
flags. Probably copy paste from another driver. Christoph also
suggested that maybe we should not let user remount with every
possible parameter, but for now comment can do. This can be addressed
later imo. Of course we can talk about it and I will do it if we have
solution.

Xfstests also show same errors than before this patch series. We will
of course wait Paragon's results also because they might have some
tests that I cannot yeat run.

Hopefully Konstantin will also comment in some point. And we have to
remember that we do not even have v28 in review process which this is
based on. So no rush to review. I just feeled that this is quite
ready and also that Konstantin can comment newer version so thats why
"already" patch series v2.

Offtopic:
I have also started to make shutdown protocol to ntfs3 driver so that
we can use that somepoint for testing with xfstests. 

v2:
	- Rewrite this cover leter
	- Reorder noatime to first patch
	- NLS loading with string
	- Delete default_options function
	- Remove remount flags
	- Rename no_acl_rules mount option
	- Making code cleaner
	- Add comment that mount options should be tested

Kari Argillander (6):
  fs/ntfs3: Remove unnecesarry mount option noatime
  fs/ntfs3: Remove unnecesarry remount flag handling
  fs/ntfs3: Use new api for mounting
  fs/ntfs3: Make mount option nohidden more universal
  fs/ntfs3: Add iocharset= mount option as alias for nls=
  fs/ntfs3: Rename mount option no_acl_rules > (no)acl_rules

 Documentation/filesystems/ntfs3.rst |  10 +-
 fs/ntfs3/file.c                     |   2 +-
 fs/ntfs3/ntfs_fs.h                  |   3 +-
 fs/ntfs3/super.c                    | 413 ++++++++++++++--------------
 fs/ntfs3/xattr.c                    |   2 +-
 5 files changed, 213 insertions(+), 217 deletions(-)

-- 
2.25.1

[PATCH v2 1/6] fs/ntfs3: Remove unnecesarry mount option noatime

[Kari Argillander]

Remove unnecesarry mount option noatime because this will be handled
by VFS. Our option parser 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 6be13e256c1a..081ac875a61a 100644
--- a/fs/ntfs3/super.c
+++ b/fs/ntfs3/super.c
@@ -215,7 +215,6 @@ enum Opt {
 	Opt_nohidden,
 	Opt_showmeta,
 	Opt_acl,
-	Opt_noatime,
 	Opt_nls,
 	Opt_prealloc,
 	Opt_no_acs_rules,
@@ -234,7 +233,6 @@ static const match_table_t ntfs_tokens = {
 	{ Opt_sparse, "sparse" },
 	{ Opt_nohidden, "nohidden" },
 	{ Opt_acl, "acl" },
-	{ Opt_noatime, "noatime" },
 	{ Opt_showmeta, "showmeta" },
 	{ Opt_nls, "nls=%s" },
 	{ Opt_prealloc, "prealloc" },
@@ -325,9 +323,6 @@ static noinline int ntfs_parse_options(struct super_block *sb, char *options,
 			ntfs_err(sb, "support for ACL not compiled in!");
 			return -EINVAL;
 #endif
-		case Opt_noatime:
-			sb->s_flags |= SB_NOATIME;
-			break;
 		case Opt_showmeta:
 			opts->showmeta = 1;
 			break;
@@ -574,8 +569,6 @@ static int ntfs_show_options(struct seq_file *m, struct dentry *root)
 		seq_puts(m, ",prealloc");
 	if (sb->s_flags & SB_POSIXACL)
 		seq_puts(m, ",acl");
-	if (sb->s_flags & SB_NOATIME)
-		seq_puts(m, ",noatime");
 
 	return 0;
 }
-- 
2.25.1

Re: [PATCH v2 1/6] fs/ntfs3: Remove unnecesarry mount option noatime

[Christoph Hellwig]

On Thu, Aug 19, 2021 at 03:26:28AM +0300, Kari Argillander wrote:
> Remove unnecesarry mount option noatime because this will be handled
> by VFS. Our option parser will never get opt like this.
> 
> Signed-off-by: Kari Argillander <kari.argillander@gmail.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2 1/6] fs/ntfs3: Remove unnecesarry mount option noatime

[Christian Brauner]

On Thu, Aug 19, 2021 at 03:26:28AM +0300, Kari Argillander wrote:
> Remove unnecesarry mount option noatime because this will be handled
> by VFS. Our option parser will never get opt like this.
> 
> Signed-off-by: Kari Argillander <kari.argillander@gmail.com>
> ---

Looks good. Thanks!
Acked-by: Christian Brauner <christian.brauner@ubuntu.com>

[PATCH v2 2/6] fs/ntfs3: Remove unnecesarry remount flag handling

[Kari Argillander]

Remove unnecesarry remount flag handling. This does not do anything for
this driver. We have already set SB_NODIRATIME when we fill super. Also
noatime should be set from mount option. Now for some reson we try to
set it when remounting.

Lazytime part looks like it is copied from f2fs and there is own mount
parameter for it. That is why they use it. We do not set lazytime
anywhere in our code. So basically this just blocks lazytime when
remounting.

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

diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c
index 081ac875a61a..702da1437cfd 100644
--- a/fs/ntfs3/super.c
+++ b/fs/ntfs3/super.c
@@ -406,8 +406,6 @@ static int ntfs_remount(struct super_block *sb, int *flags, char *data)
 
 	clear_mount_options(&old_opts);
 
-	*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;
-- 
2.25.1

Re: [PATCH v2 2/6] fs/ntfs3: Remove unnecesarry remount flag handling

[Christoph Hellwig]

On Thu, Aug 19, 2021 at 03:26:29AM +0300, Kari Argillander wrote:
> Remove unnecesarry remount flag handling. This does not do anything for
> this driver. We have already set SB_NODIRATIME when we fill super. Also
> noatime should be set from mount option. Now for some reson we try to
> set it when remounting.
> 
> Lazytime part looks like it is copied from f2fs and there is own mount
> parameter for it. That is why they use it. We do not set lazytime
> anywhere in our code. So basically this just blocks lazytime when
> remounting.
> 
> Signed-off-by: Kari Argillander <kari.argillander@gmail.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2 2/6] fs/ntfs3: Remove unnecesarry remount flag handling

[Christian Brauner]

On Thu, Aug 19, 2021 at 03:26:29AM +0300, Kari Argillander wrote:
> Remove unnecesarry remount flag handling. This does not do anything for
> this driver. We have already set SB_NODIRATIME when we fill super. Also
> noatime should be set from mount option. Now for some reson we try to
> set it when remounting.
> 
> Lazytime part looks like it is copied from f2fs and there is own mount
> parameter for it. That is why they use it. We do not set lazytime
> anywhere in our code. So basically this just blocks lazytime when
> remounting.
> 
> Signed-off-by: Kari Argillander <kari.argillander@gmail.com>
> ---

Sounds good. Thanks!
Acked-by: Christian Brauner <christian.brauner@ubuntu.com>

[PATCH v2 3/6] 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
lore.kernel.org/linux-fsdevel/159646178122.1784947.11705396571718464082.stgit@warthog.procyon.org.uk/

Nls loading is changed a to load with string. This did make code also
little cleaner.

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/ntfs_fs.h |   1 +
 fs/ntfs3/super.c   | 392 +++++++++++++++++++++++----------------------
 2 files changed, 199 insertions(+), 194 deletions(-)

diff --git a/fs/ntfs3/ntfs_fs.h b/fs/ntfs3/ntfs_fs.h
index 0c3ac89c3115..1f07dd17c6c7 100644
--- a/fs/ntfs3/ntfs_fs.h
+++ b/fs/ntfs3/ntfs_fs.h
@@ -50,6 +50,7 @@
 
 struct ntfs_mount_options {
 	struct nls_table *nls;
+	char *nls_name;
 
 	kuid_t fs_uid;
 	kgid_t fs_gid;
diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c
index 702da1437cfd..39936a4ce831 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,8 +198,32 @@ void *ntfs_put_shared(void *ptr)
 	return ret;
 }
 
+/*
+ * 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)
+		nls = CONFIG_NLS_DEFAULT;
+
+	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)
 {
+	kfree(options->nls_name);
 	unload_nls(options->nls);
 }
 
@@ -221,202 +246,150 @@ 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_showmeta, "showmeta" },
-	{ Opt_nls, "nls=%s" },
-	{ Opt_prealloc, "prealloc" },
-	{ Opt_no_acs_rules, "no_acs_rules" },
-	{ Opt_err, NULL },
+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_no("showmeta",		Opt_showmeta),
+	fsparam_string("nls",			Opt_nls),
+	fsparam_flag_no("prealloc",		Opt_prealloc),
+	fsparam_flag("no_acs_rules",		Opt_no_acs_rules),
+	{}
 };
 
-static noinline int ntfs_parse_options(struct super_block *sb, char *options,
-				       int silent,
-				       struct ntfs_mount_options *opts)
+static int ntfs_fs_parse_param(struct fs_context *fc,
+			       struct fs_parameter *param)
 {
-	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;
-
-	while ((p = strsep(&options, ","))) {
-		int token;
+	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_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_showmeta:
+		opts->showmeta = result.negated ? 0 : 1;
+		break;
+	case Opt_nls:
+		opts->nls_name = param->string;
+		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;
 	}
-
-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;
-		}
-	}
-	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;
+	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);
+	struct ntfs_mount_options *new_opts = fc->s_fs_info;
+	int ro_rw;
 
-	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;
-
-	ro_rw = sb_rdonly(sb) && !(*flags & SB_RDONLY);
+	ro_rw = sb_rdonly(sb) && !(fc->sb_flags & SB_RDONLY);
 	if (ro_rw && (sbi->flags & NTFS_FLAGS_NEED_REPLAY)) {
-		ntfs_warn(
-			sb,
-			"Couldn't remount rw because journal is not replayed. Please umount/remount instead\n");
-		err = -EINVAL;
-		goto restore_opts;
+		errorf(fc, "ntfs3: Couldn't remount rw because journal is not replayed. Please umount/remount instead\n");
+		goto clear_new_fc;
 	}
 
 	sync_filesystem(sb);
 
 	if (ro_rw && (sbi->volume.flags & VOLUME_FLAG_DIRTY) &&
-	    !sbi->options.force) {
-		ntfs_warn(sb, "volume is dirty and \"force\" flag is not set!");
-		err = -EINVAL;
-		goto restore_opts;
+	    !new_opts->force) {
+		errorf(fc, "ntfs3: Volume is dirty and \"force\" flag is not set!");
+		goto clear_new_fc;
 	}
 
-	clear_mount_options(&old_opts);
+	/*
+	 * TODO: We should probably check some mount options does
+	 * they all work after remount. Example can we really change
+	 * nls. Remove this comment when all testing is done or
+	 * even better xfstest is made for it.
+	 */
 
-	ntfs_info(sb, "re-mounted. Opts: %s", orig_data);
-	err = 0;
-	goto out;
+	new_opts->nls = ntfs_load_nls(new_opts->nls_name);
+	if (IS_ERR(new_opts->nls)) {
+		errorf(fc, "ntfs3: Cannot load nls %s", new_opts->nls_name);
+		goto clear_new_fc;
+	}
 
-restore_opts:
 	clear_mount_options(&sbi->options);
-	memcpy(&sbi->options, &old_opts, sizeof(old_opts));
+	sbi->options = *new_opts;
+	return 0;
 
-out:
-	kfree(orig_data);
-	return err;
+clear_new_fc:
+	clear_mount_options(new_opts);
+	return -EINVAL;
 }
 
 static struct kmem_cache *ntfs_inode_cachep;
@@ -545,10 +518,8 @@ static int ntfs_show_options(struct seq_file *m, struct dentry *root)
 		seq_printf(m, ",fmask=%04o", ~opts->fs_fmask_inv);
 	if (opts->dmask)
 		seq_printf(m, ",dmask=%04o", ~opts->fs_dmask_inv);
-	if (opts->nls)
-		seq_printf(m, ",nls=%s", opts->nls->charset);
-	else
-		seq_puts(m, ",nls=utf8");
+	if (opts->nls_name)
+		seq_printf(m, ",nls=%s", opts->nls_name);
 	if (opts->sys_immutable)
 		seq_puts(m, ",sys_immutable");
 	if (opts->discard)
@@ -619,7 +590,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,
 };
 
@@ -883,10 +853,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);
@@ -905,11 +875,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"
@@ -921,10 +886,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 {
@@ -933,6 +894,14 @@ static int ntfs_fill_super(struct super_block *sb, void *data, int silent)
 			~(u64)(sbi->discard_granularity - 1);
 	}
 
+	sbi->options.nls = ntfs_load_nls(sbi->options.nls_name);
+	if (IS_ERR(sbi->options.nls)) {
+		ntfs_err(sb, "ntfs3: Cannot load nls %s",
+				sbi->options.nls_name);
+		err = PTR_ERR(sbi->options.nls);
+		goto out;
+	}
+
 	sb_set_blocksize(sb, PAGE_SIZE);
 
 	/* parse boot */
@@ -1400,19 +1369,54 @@ 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)
+{
+	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,
+};
+
+static int ntfs_init_fs_context(struct fs_context *fc)
 {
-	return mount_bdev(fs_type, flags, dev_name, data, ntfs_fill_super);
+	struct ntfs_sb_info *sbi;
+
+	sbi = ntfs_zalloc(sizeof(struct ntfs_sb_info));
+	if (!sbi)
+		return -ENOMEM;
+
+	/* Default options */
+	sbi->options.fs_uid = current_uid();
+	sbi->options.fs_gid = current_gid();
+	sbi->options.fs_fmask_inv = ~current_umask();
+	sbi->options.fs_dmask_inv = ~current_umask();
+
+	fc->s_fs_info = sbi;
+	fc->ops = &ntfs_context_ops;
+
+	return 0;
 }
 
 // clang-format off
 static struct file_system_type ntfs_fs_type = {
-	.owner		= THIS_MODULE,
-	.name		= "ntfs3",
-	.mount		= ntfs_mount,
-	.kill_sb	= kill_block_super,
-	.fs_flags	= FS_REQUIRES_DEV | FS_ALLOW_IDMAP,
+	.owner			= THIS_MODULE,
+	.name			= "ntfs3",
+	.init_fs_context	= ntfs_init_fs_context,
+	.parameters		= ntfs_fs_parameters,
+	.kill_sb		= kill_block_super,
+	.fs_flags		= FS_REQUIRES_DEV | FS_ALLOW_IDMAP,
 };
 // clang-format on
 
-- 
2.25.1

Re: [PATCH v2 3/6] fs/ntfs3: Use new api for mounting

[Pali Rohár]

Hello! I have there one comment:

On Thursday 19 August 2021 03:26:30 Kari Argillander wrote:
> @@ -545,10 +518,8 @@ static int ntfs_show_options(struct seq_file *m, struct dentry *root)
>  		seq_printf(m, ",fmask=%04o", ~opts->fs_fmask_inv);
>  	if (opts->dmask)
>  		seq_printf(m, ",dmask=%04o", ~opts->fs_dmask_inv);
> -	if (opts->nls)
> -		seq_printf(m, ",nls=%s", opts->nls->charset);
> -	else
> -		seq_puts(m, ",nls=utf8");
> +	if (opts->nls_name)
> +		seq_printf(m, ",nls=%s", opts->nls_name);

Please always print correct "nls=". Obviously ntfs driver (which
internally stores filenames in UTF-16) must always use some conversion
to null-term bytes. And if some kernel/driver default conversion is used
then userspace should know it, what exactly is used (e.g. to ensure that
would use correct encoding name argument of open(), stat()... syscalls).

>  	if (opts->sys_immutable)
>  		seq_puts(m, ",sys_immutable");
>  	if (opts->discard)
> @@ -619,7 +590,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,
>  };

Re: [PATCH v2 3/6] fs/ntfs3: Use new api for mounting

[Kari Argillander]

On Thu, Aug 19, 2021 at 10:18:28AM +0200, Pali Rohár wrote:
> Hello! I have there one comment:
> 
> On Thursday 19 August 2021 03:26:30 Kari Argillander wrote:
> > @@ -545,10 +518,8 @@ static int ntfs_show_options(struct seq_file *m, struct dentry *root)
> >  		seq_printf(m, ",fmask=%04o", ~opts->fs_fmask_inv);
> >  	if (opts->dmask)
> >  		seq_printf(m, ",dmask=%04o", ~opts->fs_dmask_inv);
> > -	if (opts->nls)
> > -		seq_printf(m, ",nls=%s", opts->nls->charset);
> > -	else
> > -		seq_puts(m, ",nls=utf8");
> > +	if (opts->nls_name)
> > +		seq_printf(m, ",nls=%s", opts->nls_name);
> 
> Please always print correct "nls=". Obviously ntfs driver (which
> internally stores filenames in UTF-16) must always use some conversion
> to null-term bytes. And if some kernel/driver default conversion is used
> then userspace should know it, what exactly is used (e.g. to ensure that
> would use correct encoding name argument of open(), stat()... syscalls).

Thanks. That was actually my intention, but it seems that there is bug
from my part and it will be fixed in v3. Thanks for reviewing.

> 
> >  	if (opts->sys_immutable)
> >  		seq_puts(m, ",sys_immutable");
> >  	if (opts->discard)
> > @@ -619,7 +590,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,
> >  };
> 

Re: [PATCH v2 3/6] fs/ntfs3: Use new api for mounting

[Kari Argillander]

On Thu, Aug 19, 2021 at 03:26:30AM +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
> lore.kernel.org/linux-fsdevel/159646178122.1784947.11705396571718464082.stgit@warthog.procyon.org.uk/
> 
> Nls loading is changed a to load with string. This did make code also
> little cleaner.
> 
> 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>

I will send new patches when Komarov has take a look of these. I have
found some bugs and how to improve things. Please take a look below my
second comment.

>  fs/ntfs3/super.c   | 392 +++++++++++++++++++++++----------------------

> +static void ntfs_fs_free(struct fs_context *fc)
> +{
> +	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,
> +};
> +
> +static int ntfs_init_fs_context(struct fs_context *fc)
>  {
> -	return mount_bdev(fs_type, flags, dev_name, data, ntfs_fill_super);
> +	struct ntfs_sb_info *sbi;
> +
> +	sbi = ntfs_zalloc(sizeof(struct ntfs_sb_info));
> +	if (!sbi)
> +		return -ENOMEM;
> +
> +	/* Default options */
> +	sbi->options.fs_uid = current_uid();
> +	sbi->options.fs_gid = current_gid();
> +	sbi->options.fs_fmask_inv = ~current_umask();
> +	sbi->options.fs_dmask_inv = ~current_umask();
> +
> +	fc->s_fs_info = sbi;
> +	fc->ops = &ntfs_context_ops;
> +
> +	return 0;
>  }
 
In this code I did not like that we make whole new sbi everytime.
Especially because we do zalloc. So when we regonfigure then new sbi
is allocated just for options. I notice that example xfs does allocate
their "sbi" everytime. Then I notice that example squashfs allocate
just mount options.

I have impression that we should allocate sbi if it first time so I
did "between code". I would like to do things like they are intended
with api so can Christoph comment that is this "right" thing to do
and is there any draw backs which I should know.

static void ntfs_fs_free(struct fs_context *fc)
{
	struct ntfs_mount_options *opts = fc->fs_private;
	struct ntfs_sb_info *sbi = fc->s_fs_info;

	if (sbi)
		put_ntfs(sbi);

	if (opts)
		clear_mount_options(opts);
}

static int ntfs_init_fs_context(struct fs_context *fc)
{
	struct ntfs_mount_options *opts;
	struct ntfs_sb_info *sbi = NULL;

	fc->ops = &ntfs_context_ops;

	opts = ntfs_zalloc(sizeof(struct ntfs_mount_options));
	if (!opts)
		return -ENOMEM;

	/* Default options. */
	opts->fs_uid = current_uid();
	opts->fs_gid = current_gid();
	opts->fs_fmask_inv = ~current_umask();
	opts->fs_dmask_inv = ~current_umask();

	fc->fs_private = opts;

	/* No need to initialize sbi if we just reconf. */
	if (fc->purpose == FS_CONTEXT_FOR_RECONFIGURE)
		return 0;

	sbi = ntfs_zalloc(sizeof(struct ntfs_sb_info));
	if (!sbi) {
		ntfs_free(opts);
		return -ENOMEM;
	}

	mutex_init(&sbi->compress.mtx_lznt);
#ifdef CONFIG_NTFS3_LZX_XPRESS
	mutex_init(&sbi->compress.mtx_xpress);
	mutex_init(&sbi->compress.mtx_lzx);
#endif

	sbi->options = opts;
	fc->s_fs_info = sbi;

	return 0;
}

Re: [PATCH v2 3/6] fs/ntfs3: Use new api for mounting

[Christoph Hellwig]

> +	/*
> +	 * TODO: We should probably check some mount options does
> +	 * they all work after remount. Example can we really change
> +	 * nls. Remove this comment when all testing is done or
> +	 * even better xfstest is made for it.
> +	 */

Instead of the TODO I would suggest a prep patch to drop changing of
any options in remount before this one and then only add them back
as needed and tested.

The mechanics of the conversion look good to me.

Re: [PATCH v2 3/6] fs/ntfs3: Use new api for mounting

[Kari Argillander]

On Tue, Aug 24, 2021 at 10:03:02AM +0200, Christoph Hellwig wrote:
> > +	/*
> > +	 * TODO: We should probably check some mount options does
> > +	 * they all work after remount. Example can we really change
> > +	 * nls. Remove this comment when all testing is done or
> > +	 * even better xfstest is made for it.
> > +	 */
> 
> Instead of the TODO I would suggest a prep patch to drop changing of
> any options in remount before this one and then only add them back
> as needed and tested.

This could be good option. I have actually tested nls and it will be
problem so we definitely drop that. I will wait what Konstantin has
to say about other.

> The mechanics of the conversion look good to me.

I have made quite few changes to make this series better and will
send v3 in the near future.

Main change is that we won't allocate sbi when remount. We can
allocate just options. Also won't let nls/iocharset change.

RE: [PATCH v2 3/6] fs/ntfs3: Use new api for mounting

[Konstantin Komarov]

> From: Kari Argillander <kari.argillander@gmail.com>
> Sent: Tuesday, August 24, 2021 11:22 AM
> To: Christoph Hellwig <hch@lst.de>
> Cc: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>; ntfs3@lists.linux.dev; linux-kernel@vger.kernel.org; linux-
> fsdevel@vger.kernel.org; Pali Rohár <pali@kernel.org>; Matthew Wilcox <willy@infradead.org>; Christian Brauner
> <christian.brauner@ubuntu.com>
> Subject: Re: [PATCH v2 3/6] fs/ntfs3: Use new api for mounting
> 
> On Tue, Aug 24, 2021 at 10:03:02AM +0200, Christoph Hellwig wrote:
> > > +	/*
> > > +	 * TODO: We should probably check some mount options does
> > > +	 * they all work after remount. Example can we really change
> > > +	 * nls. Remove this comment when all testing is done or
> > > +	 * even better xfstest is made for it.
> > > +	 */
> >
> > Instead of the TODO I would suggest a prep patch to drop changing of
> > any options in remount before this one and then only add them back
> > as needed and tested.
> 
> This could be good option. I have actually tested nls and it will be
> problem so we definitely drop that. I will wait what Konstantin has
> to say about other.
> 
> > The mechanics of the conversion look good to me.
> 
> I have made quite few changes to make this series better and will
> send v3 in the near future.
> 
> Main change is that we won't allocate sbi when remount. We can
> allocate just options. Also won't let nls/iocharset change.

Hi Kari, looking forward to have v3 series to pick up!

Best regards.

Re: [PATCH v2 3/6] fs/ntfs3: Use new api for mounting

[Christian Brauner]

On Thu, Aug 19, 2021 at 03:26:30AM +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
> lore.kernel.org/linux-fsdevel/159646178122.1784947.11705396571718464082.stgit@warthog.procyon.org.uk/
> 
> Nls loading is changed a to load with string. This did make code also
> little cleaner.
> 
> 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>
> ---

Looks mostly sane to me. Thanks!
Acked-by: Christian Brauner <christian.brauner@ubuntu.com>

>  fs/ntfs3/ntfs_fs.h |   1 +
>  fs/ntfs3/super.c   | 392 +++++++++++++++++++++++----------------------
>  2 files changed, 199 insertions(+), 194 deletions(-)
> 
> diff --git a/fs/ntfs3/ntfs_fs.h b/fs/ntfs3/ntfs_fs.h
> index 0c3ac89c3115..1f07dd17c6c7 100644
> --- a/fs/ntfs3/ntfs_fs.h
> +++ b/fs/ntfs3/ntfs_fs.h
> @@ -50,6 +50,7 @@
>  
>  struct ntfs_mount_options {
>  	struct nls_table *nls;
> +	char *nls_name;
>  
>  	kuid_t fs_uid;
>  	kgid_t fs_gid;
> diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c
> index 702da1437cfd..39936a4ce831 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,8 +198,32 @@ void *ntfs_put_shared(void *ptr)
>  	return ret;
>  }
>  
> +/*
> + * 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)
> +		nls = CONFIG_NLS_DEFAULT;
> +
> +	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)
>  {
> +	kfree(options->nls_name);
>  	unload_nls(options->nls);
>  }
>  
> @@ -221,202 +246,150 @@ 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_showmeta, "showmeta" },
> -	{ Opt_nls, "nls=%s" },
> -	{ Opt_prealloc, "prealloc" },
> -	{ Opt_no_acs_rules, "no_acs_rules" },
> -	{ Opt_err, NULL },
> +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_no("showmeta",		Opt_showmeta),
> +	fsparam_string("nls",			Opt_nls),
> +	fsparam_flag_no("prealloc",		Opt_prealloc),
> +	fsparam_flag("no_acs_rules",		Opt_no_acs_rules),
> +	{}
>  };
>  
> -static noinline int ntfs_parse_options(struct super_block *sb, char *options,
> -				       int silent,
> -				       struct ntfs_mount_options *opts)
> +static int ntfs_fs_parse_param(struct fs_context *fc,
> +			       struct fs_parameter *param)
>  {
> -	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;
> -
> -	while ((p = strsep(&options, ","))) {
> -		int token;
> +	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;

If you're only using opts->uid to check whether ->fs_*id is set then you
can probably simplify this. You're expressing the expectation that if
->fs_*id is set that it must be a valid *id. So you could initialize
->fs_*id to INVALID_*ID and then instead of having to check "if
(opts->uid)" in later code you can directly check "if (valid_uid(opts->fs_uid))".
Just a thought.

> +		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_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_showmeta:
> +		opts->showmeta = result.negated ? 0 : 1;
> +		break;
> +	case Opt_nls:
> +		opts->nls_name = param->string;
> +		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;
>  	}
> -
> -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;
> -		}
> -	}
> -	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;
> +	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);
> +	struct ntfs_mount_options *new_opts = fc->s_fs_info;
> +	int ro_rw;
>  
> -	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;
> -
> -	ro_rw = sb_rdonly(sb) && !(*flags & SB_RDONLY);
> +	ro_rw = sb_rdonly(sb) && !(fc->sb_flags & SB_RDONLY);
>  	if (ro_rw && (sbi->flags & NTFS_FLAGS_NEED_REPLAY)) {
> -		ntfs_warn(
> -			sb,
> -			"Couldn't remount rw because journal is not replayed. Please umount/remount instead\n");
> -		err = -EINVAL;
> -		goto restore_opts;
> +		errorf(fc, "ntfs3: Couldn't remount rw because journal is not replayed. Please umount/remount instead\n");
> +		goto clear_new_fc;
>  	}
>  
>  	sync_filesystem(sb);
>  
>  	if (ro_rw && (sbi->volume.flags & VOLUME_FLAG_DIRTY) &&
> -	    !sbi->options.force) {
> -		ntfs_warn(sb, "volume is dirty and \"force\" flag is not set!");
> -		err = -EINVAL;
> -		goto restore_opts;
> +	    !new_opts->force) {
> +		errorf(fc, "ntfs3: Volume is dirty and \"force\" flag is not set!");
> +		goto clear_new_fc;
>  	}
>  
> -	clear_mount_options(&old_opts);
> +	/*
> +	 * TODO: We should probably check some mount options does
> +	 * they all work after remount. Example can we really change
> +	 * nls. Remove this comment when all testing is done or
> +	 * even better xfstest is made for it.
> +	 */
>  
> -	ntfs_info(sb, "re-mounted. Opts: %s", orig_data);
> -	err = 0;
> -	goto out;
> +	new_opts->nls = ntfs_load_nls(new_opts->nls_name);
> +	if (IS_ERR(new_opts->nls)) {
> +		errorf(fc, "ntfs3: Cannot load nls %s", new_opts->nls_name);
> +		goto clear_new_fc;
> +	}
>  
> -restore_opts:
>  	clear_mount_options(&sbi->options);
> -	memcpy(&sbi->options, &old_opts, sizeof(old_opts));
> +	sbi->options = *new_opts;
> +	return 0;
>  
> -out:
> -	kfree(orig_data);
> -	return err;
> +clear_new_fc:
> +	clear_mount_options(new_opts);
> +	return -EINVAL;
>  }
>  
>  static struct kmem_cache *ntfs_inode_cachep;
> @@ -545,10 +518,8 @@ static int ntfs_show_options(struct seq_file *m, struct dentry *root)
>  		seq_printf(m, ",fmask=%04o", ~opts->fs_fmask_inv);
>  	if (opts->dmask)
>  		seq_printf(m, ",dmask=%04o", ~opts->fs_dmask_inv);
> -	if (opts->nls)
> -		seq_printf(m, ",nls=%s", opts->nls->charset);
> -	else
> -		seq_puts(m, ",nls=utf8");
> +	if (opts->nls_name)
> +		seq_printf(m, ",nls=%s", opts->nls_name);
>  	if (opts->sys_immutable)
>  		seq_puts(m, ",sys_immutable");
>  	if (opts->discard)
> @@ -619,7 +590,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,
>  };
>  
> @@ -883,10 +853,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);
> @@ -905,11 +875,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"
> @@ -921,10 +886,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) {
>  		;

Sidenote, why is this lonely semicolon here? Maybe you can rewrite this
so you don't need this branch at all.

>  	} else {
> @@ -933,6 +894,14 @@ static int ntfs_fill_super(struct super_block *sb, void *data, int silent)
>  			~(u64)(sbi->discard_granularity - 1);
>  	}
>  
> +	sbi->options.nls = ntfs_load_nls(sbi->options.nls_name);
> +	if (IS_ERR(sbi->options.nls)) {
> +		ntfs_err(sb, "ntfs3: Cannot load nls %s",
> +				sbi->options.nls_name);
> +		err = PTR_ERR(sbi->options.nls);
> +		goto out;
> +	}
> +
>  	sb_set_blocksize(sb, PAGE_SIZE);
>  
>  	/* parse boot */
> @@ -1400,19 +1369,54 @@ 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)
> +{
> +	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,
> +};
> +
> +static int ntfs_init_fs_context(struct fs_context *fc)
>  {
> -	return mount_bdev(fs_type, flags, dev_name, data, ntfs_fill_super);
> +	struct ntfs_sb_info *sbi;
> +
> +	sbi = ntfs_zalloc(sizeof(struct ntfs_sb_info));
> +	if (!sbi)
> +		return -ENOMEM;
> +
> +	/* Default options */
> +	sbi->options.fs_uid = current_uid();
> +	sbi->options.fs_gid = current_gid();
> +	sbi->options.fs_fmask_inv = ~current_umask();
> +	sbi->options.fs_dmask_inv = ~current_umask();
> +
> +	fc->s_fs_info = sbi;
> +	fc->ops = &ntfs_context_ops;
> +
> +	return 0;
>  }
>  
>  // clang-format off
>  static struct file_system_type ntfs_fs_type = {
> -	.owner		= THIS_MODULE,
> -	.name		= "ntfs3",
> -	.mount		= ntfs_mount,
> -	.kill_sb	= kill_block_super,
> -	.fs_flags	= FS_REQUIRES_DEV | FS_ALLOW_IDMAP,
> +	.owner			= THIS_MODULE,
> +	.name			= "ntfs3",
> +	.init_fs_context	= ntfs_init_fs_context,
> +	.parameters		= ntfs_fs_parameters,
> +	.kill_sb		= kill_block_super,
> +	.fs_flags		= FS_REQUIRES_DEV | FS_ALLOW_IDMAP,
>  };
>  // clang-format on
>  
> -- 
> 2.25.1
> 

Re: [PATCH v2 3/6] fs/ntfs3: Use new api for mounting

[Kari Argillander]

On Tue, Aug 24, 2021 at 01:32:17PM +0200, Christian Brauner wrote:
> On Thu, Aug 19, 2021 at 03:26:30AM +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
> > lore.kernel.org/linux-fsdevel/159646178122.1784947.11705396571718464082.stgit@warthog.procyon.org.uk/
> > 
> > Nls loading is changed a to load with string. This did make code also
> > little cleaner.
> > 
> > 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>
> > ---
> 
> Looks mostly sane to me. Thanks!
> Acked-by: Christian Brauner <christian.brauner@ubuntu.com>

Thanks for ack.

> 
> >  fs/ntfs3/ntfs_fs.h |   1 +
> >  fs/ntfs3/super.c   | 392 +++++++++++++++++++++++----------------------
> >  2 files changed, 199 insertions(+), 194 deletions(-)
> > 
> > diff --git a/fs/ntfs3/ntfs_fs.h b/fs/ntfs3/ntfs_fs.h
> > index 0c3ac89c3115..1f07dd17c6c7 100644
> > --- a/fs/ntfs3/ntfs_fs.h
> > +++ b/fs/ntfs3/ntfs_fs.h
> > @@ -50,6 +50,7 @@
> >  
> >  struct ntfs_mount_options {
> >  	struct nls_table *nls;
> > +	char *nls_name;
> >  
> >  	kuid_t fs_uid;
> >  	kgid_t fs_gid;
> > diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c
> > index 702da1437cfd..39936a4ce831 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,8 +198,32 @@ void *ntfs_put_shared(void *ptr)
> >  	return ret;
> >  }
> >  
> > +/*
> > + * 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)
> > +		nls = CONFIG_NLS_DEFAULT;
> > +
> > +	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)
> >  {
> > +	kfree(options->nls_name);
> >  	unload_nls(options->nls);
> >  }
> >  
> > @@ -221,202 +246,150 @@ 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_showmeta, "showmeta" },
> > -	{ Opt_nls, "nls=%s" },
> > -	{ Opt_prealloc, "prealloc" },
> > -	{ Opt_no_acs_rules, "no_acs_rules" },
> > -	{ Opt_err, NULL },
> > +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_no("showmeta",		Opt_showmeta),
> > +	fsparam_string("nls",			Opt_nls),
> > +	fsparam_flag_no("prealloc",		Opt_prealloc),
> > +	fsparam_flag("no_acs_rules",		Opt_no_acs_rules),
> > +	{}
> >  };
> >  
> > -static noinline int ntfs_parse_options(struct super_block *sb, char *options,
> > -				       int silent,
> > -				       struct ntfs_mount_options *opts)
> > +static int ntfs_fs_parse_param(struct fs_context *fc,
> > +			       struct fs_parameter *param)
> >  {
> > -	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;
> > -
> > -	while ((p = strsep(&options, ","))) {
> > -		int token;
> > +	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;
> 
> If you're only using opts->uid to check whether ->fs_*id is set then you
> can probably simplify this. You're expressing the expectation that if
> ->fs_*id is set that it must be a valid *id. So you could initialize
> ->fs_*id to INVALID_*ID and then instead of having to check "if
> (opts->uid)" in later code you can directly check "if (valid_uid(opts->fs_uid))".
> Just a thought.

Yeah opts->uid can be refactored away. It is only used to check if we
have changed fs_uid. Then show_options knows that we have change that
option.

There is couple options to refactor opts->*id away
	- show_options will check if (fs_*id != current_uid())
	- show_options will check if (fs_*id != GLOBAL_ROOT_UID)
	- show_options will always show fs_*id
		I probably prefer this one, because who will know know
		when driver is not showing option. Of course driver is
		not showing option when it is default but in this case
		default is not so easy to understand.

This is how other did it in show_options
adfs/exfat:
	if (!uid_eq(opts->fs_uid, GLOBAL_ROOT_UID))
ext2/ext4:
	if (!uid_eq(sbi->s_resuid, make_kuid(&init_user_ns, EXT2_DEF_RESUID)) ||
	le16_to_cpu(es->s_def_resuid) != EXT2_DEF_RESUID)
hfs/hpfs/ntfs:
	always print
jfs (no way like this, seems like bug):
	if (uid_valid(sbi->uid))
udf (same like ntfs3 right now):
	if (UDF_QUERY_FLAG(sb, UDF_FLAG_UID_SET))
 
> > +		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_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_showmeta:
> > +		opts->showmeta = result.negated ? 0 : 1;
> > +		break;
> > +	case Opt_nls:
> > +		opts->nls_name = param->string;
> > +		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;
> >  	}
> > -
> > -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;
> > -		}
> > -	}
> > -	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;
> > +	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);
> > +	struct ntfs_mount_options *new_opts = fc->s_fs_info;
> > +	int ro_rw;
> >  
> > -	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;
> > -
> > -	ro_rw = sb_rdonly(sb) && !(*flags & SB_RDONLY);
> > +	ro_rw = sb_rdonly(sb) && !(fc->sb_flags & SB_RDONLY);
> >  	if (ro_rw && (sbi->flags & NTFS_FLAGS_NEED_REPLAY)) {
> > -		ntfs_warn(
> > -			sb,
> > -			"Couldn't remount rw because journal is not replayed. Please umount/remount instead\n");
> > -		err = -EINVAL;
> > -		goto restore_opts;
> > +		errorf(fc, "ntfs3: Couldn't remount rw because journal is not replayed. Please umount/remount instead\n");
> > +		goto clear_new_fc;
> >  	}
> >  
> >  	sync_filesystem(sb);
> >  
> >  	if (ro_rw && (sbi->volume.flags & VOLUME_FLAG_DIRTY) &&
> > -	    !sbi->options.force) {
> > -		ntfs_warn(sb, "volume is dirty and \"force\" flag is not set!");
> > -		err = -EINVAL;
> > -		goto restore_opts;
> > +	    !new_opts->force) {
> > +		errorf(fc, "ntfs3: Volume is dirty and \"force\" flag is not set!");
> > +		goto clear_new_fc;
> >  	}
> >  
> > -	clear_mount_options(&old_opts);
> > +	/*
> > +	 * TODO: We should probably check some mount options does
> > +	 * they all work after remount. Example can we really change
> > +	 * nls. Remove this comment when all testing is done or
> > +	 * even better xfstest is made for it.
> > +	 */
> >  
> > -	ntfs_info(sb, "re-mounted. Opts: %s", orig_data);
> > -	err = 0;
> > -	goto out;
> > +	new_opts->nls = ntfs_load_nls(new_opts->nls_name);
> > +	if (IS_ERR(new_opts->nls)) {
> > +		errorf(fc, "ntfs3: Cannot load nls %s", new_opts->nls_name);
> > +		goto clear_new_fc;
> > +	}
> >  
> > -restore_opts:
> >  	clear_mount_options(&sbi->options);
> > -	memcpy(&sbi->options, &old_opts, sizeof(old_opts));
> > +	sbi->options = *new_opts;
> > +	return 0;
> >  
> > -out:
> > -	kfree(orig_data);
> > -	return err;
> > +clear_new_fc:
> > +	clear_mount_options(new_opts);
> > +	return -EINVAL;
> >  }
> >  
> >  static struct kmem_cache *ntfs_inode_cachep;
> > @@ -545,10 +518,8 @@ static int ntfs_show_options(struct seq_file *m, struct dentry *root)
> >  		seq_printf(m, ",fmask=%04o", ~opts->fs_fmask_inv);
> >  	if (opts->dmask)
> >  		seq_printf(m, ",dmask=%04o", ~opts->fs_dmask_inv);
> > -	if (opts->nls)
> > -		seq_printf(m, ",nls=%s", opts->nls->charset);
> > -	else
> > -		seq_puts(m, ",nls=utf8");
> > +	if (opts->nls_name)
> > +		seq_printf(m, ",nls=%s", opts->nls_name);
> >  	if (opts->sys_immutable)
> >  		seq_puts(m, ",sys_immutable");
> >  	if (opts->discard)
> > @@ -619,7 +590,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,
> >  };
> >  
> > @@ -883,10 +853,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);
> > @@ -905,11 +875,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"
> > @@ -921,10 +886,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) {
> >  		;
> 
> Sidenote, why is this lonely semicolon here? Maybe you can rewrite this
> so you don't need this branch at all.

I have patch series coming which will refactor fill_super and this will
be there. So I will not touch this in this series.

> 
> >  	} else {
> > @@ -933,6 +894,14 @@ static int ntfs_fill_super(struct super_block *sb, void *data, int silent)
> >  			~(u64)(sbi->discard_granularity - 1);
> >  	}
> >  
> > +	sbi->options.nls = ntfs_load_nls(sbi->options.nls_name);
> > +	if (IS_ERR(sbi->options.nls)) {
> > +		ntfs_err(sb, "ntfs3: Cannot load nls %s",
> > +				sbi->options.nls_name);
> > +		err = PTR_ERR(sbi->options.nls);
> > +		goto out;
> > +	}
> > +
> >  	sb_set_blocksize(sb, PAGE_SIZE);
> >  
> >  	/* parse boot */
> > @@ -1400,19 +1369,54 @@ 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)
> > +{
> > +	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,
> > +};
> > +
> > +static int ntfs_init_fs_context(struct fs_context *fc)
> >  {
> > -	return mount_bdev(fs_type, flags, dev_name, data, ntfs_fill_super);
> > +	struct ntfs_sb_info *sbi;
> > +
> > +	sbi = ntfs_zalloc(sizeof(struct ntfs_sb_info));
> > +	if (!sbi)
> > +		return -ENOMEM;
> > +
> > +	/* Default options */
> > +	sbi->options.fs_uid = current_uid();
> > +	sbi->options.fs_gid = current_gid();
> > +	sbi->options.fs_fmask_inv = ~current_umask();
> > +	sbi->options.fs_dmask_inv = ~current_umask();
> > +
> > +	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
> > 

[PATCH v2 4/6] 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.

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 39936a4ce831..8e86e1956486 100644
--- a/fs/ntfs3/super.c
+++ b/fs/ntfs3/super.c
@@ -256,7 +256,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),
@@ -317,7 +317,7 @@ static int ntfs_fs_parse_param(struct fs_context *fc,
 		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

Re: [PATCH v2 4/6] fs/ntfs3: Make mount option nohidden more universal

[Christoph Hellwig]

On Thu, Aug 19, 2021 at 03:26:31AM +0300, Kari Argillander wrote:
> 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.
> 
> Signed-off-by: Kari Argillander <kari.argillander@gmail.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2 4/6] fs/ntfs3: Make mount option nohidden more universal

[Christian Brauner]

On Thu, Aug 19, 2021 at 03:26:31AM +0300, Kari Argillander wrote:
> 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.
> 
> Signed-off-by: Kari Argillander <kari.argillander@gmail.com>
> ---

Sounds good. Thanks!
Acked-by: Christian Brauner <christian.brauner@ubuntu.com>

[PATCH v2 5/6] 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                    | 12 ++++++++----
 2 files changed, 10 insertions(+), 6 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 8e86e1956486..c3c07c181f15 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,
@@ -259,9 +259,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),
 	{}
 };
 
@@ -332,7 +336,7 @@ static int ntfs_fs_parse_param(struct fs_context *fc,
 	case Opt_showmeta:
 		opts->showmeta = result.negated ? 0 : 1;
 		break;
-	case Opt_nls:
+	case Opt_iocharset:
 		opts->nls_name = param->string;
 		param->string = NULL;
 		break;
@@ -519,7 +523,7 @@ 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_name)
-		seq_printf(m, ",nls=%s", opts->nls_name);
+		seq_printf(m, ",iocharset=%s", opts->nls_name);
 	if (opts->sys_immutable)
 		seq_puts(m, ",sys_immutable");
 	if (opts->discard)
-- 
2.25.1

Re: [PATCH v2 5/6] fs/ntfs3: Add iocharset= mount option as alias for nls=

[Pali Rohár]

On Thursday 19 August 2021 03:26:32 Kari Argillander wrote:
> 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                    | 12 ++++++++----
>  2 files changed, 10 insertions(+), 6 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 8e86e1956486..c3c07c181f15 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,
> @@ -259,9 +259,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),

Anyway, this is a new filesystem driver. Therefore, do we need to have
for it since beginning deprecated option?

>  	{}
>  };
>  
> @@ -332,7 +336,7 @@ static int ntfs_fs_parse_param(struct fs_context *fc,
>  	case Opt_showmeta:
>  		opts->showmeta = result.negated ? 0 : 1;
>  		break;
> -	case Opt_nls:
> +	case Opt_iocharset:
>  		opts->nls_name = param->string;
>  		param->string = NULL;
>  		break;
> @@ -519,7 +523,7 @@ 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_name)
> -		seq_printf(m, ",nls=%s", opts->nls_name);
> +		seq_printf(m, ",iocharset=%s", opts->nls_name);
>  	if (opts->sys_immutable)
>  		seq_puts(m, ",sys_immutable");
>  	if (opts->discard)
> -- 
> 2.25.1
> 

Re: [PATCH v2 5/6] fs/ntfs3: Add iocharset= mount option as alias for nls=

[Kari Argillander]

On Thu, Aug 19, 2021 at 10:26:58AM +0200, Pali Rohár wrote:
> On Thursday 19 August 2021 03:26:32 Kari Argillander wrote:
> > 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                    | 12 ++++++++----
> >  2 files changed, 10 insertions(+), 6 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 8e86e1956486..c3c07c181f15 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,
> > @@ -259,9 +259,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),
> 
> Anyway, this is a new filesystem driver. Therefore, do we need to have
> for it since beginning deprecated option?

I have also thought about this. In my mind this is new driver to our tree.
But is been available from Paragon. Their customers may migrate to this
so let's give them easy path to it. They also have free version and
there is many Linux user who will switch to this when this is available.

Another thing what I been thinking is that how we will switch from
ntfs->ntfs3. To give easy path to this driver then we should in some
point add ntfs driver mount options to this one. Maybe not totally
funtional, but so that mounting is possible. Current ntfs driver had nls
option so it makes sense to add it here. We might even like to think
ntfs-3g mount options because that is more used.

Of course we can just drop this. But I like that user experience is good
with kernel. And if we can make that little more pleasent with couple
line of trivial code then imo let's do it. We just need to make sure we
drop these in one point of time. It is too often these kind of things
will live in kernel "internity".

> 
> >  	{}
> >  };
> >  
> > @@ -332,7 +336,7 @@ static int ntfs_fs_parse_param(struct fs_context *fc,
> >  	case Opt_showmeta:
> >  		opts->showmeta = result.negated ? 0 : 1;
> >  		break;
> > -	case Opt_nls:
> > +	case Opt_iocharset:
> >  		opts->nls_name = param->string;
> >  		param->string = NULL;
> >  		break;
> > @@ -519,7 +523,7 @@ 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_name)
> > -		seq_printf(m, ",nls=%s", opts->nls_name);
> > +		seq_printf(m, ",iocharset=%s", opts->nls_name);
> >  	if (opts->sys_immutable)
> >  		seq_puts(m, ",sys_immutable");
> >  	if (opts->discard)
> > -- 
> > 2.25.1
> > 
> 

Re: [PATCH v2 5/6] fs/ntfs3: Add iocharset= mount option as alias for nls=

[Pali Rohár]

On Thursday 19 August 2021 12:45:32 Kari Argillander wrote:
> On Thu, Aug 19, 2021 at 10:26:58AM +0200, Pali Rohár wrote:
> > On Thursday 19 August 2021 03:26:32 Kari Argillander wrote:
> > > 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                    | 12 ++++++++----
> > >  2 files changed, 10 insertions(+), 6 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 8e86e1956486..c3c07c181f15 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,
> > > @@ -259,9 +259,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),
> > 
> > Anyway, this is a new filesystem driver. Therefore, do we need to have
> > for it since beginning deprecated option?
> 
> I have also thought about this. In my mind this is new driver to our tree.
> But is been available from Paragon. Their customers may migrate to this
> so let's give them easy path to it. They also have free version and
> there is many Linux user who will switch to this when this is available.
> 
> Another thing what I been thinking is that how we will switch from
> ntfs->ntfs3. To give easy path to this driver then we should in some
> point add ntfs driver mount options to this one. Maybe not totally
> funtional, but so that mounting is possible. Current ntfs driver had nls
> option so it makes sense to add it here. We might even like to think
> ntfs-3g mount options because that is more used.
> 
> Of course we can just drop this. But I like that user experience is good
> with kernel. And if we can make that little more pleasent with couple
> line of trivial code then imo let's do it. We just need to make sure we
> drop these in one point of time. It is too often these kind of things
> will live in kernel "internity".

Makes sense. Sometimes it is better to introduce legacy/deprecated code
also to the new one.

In case this ntfs3 driver is going to replace ntfs driver then it would
have to understand all options supported by ntfs, even those which are
already deprecated and also those which will be deprecated in future.
(Until there is a decision to drop it)

> > 
> > >  	{}
> > >  };
> > >  
> > > @@ -332,7 +336,7 @@ static int ntfs_fs_parse_param(struct fs_context *fc,
> > >  	case Opt_showmeta:
> > >  		opts->showmeta = result.negated ? 0 : 1;
> > >  		break;
> > > -	case Opt_nls:
> > > +	case Opt_iocharset:
> > >  		opts->nls_name = param->string;
> > >  		param->string = NULL;
> > >  		break;
> > > @@ -519,7 +523,7 @@ 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_name)
> > > -		seq_printf(m, ",nls=%s", opts->nls_name);
> > > +		seq_printf(m, ",iocharset=%s", opts->nls_name);
> > >  	if (opts->sys_immutable)
> > >  		seq_puts(m, ",sys_immutable");
> > >  	if (opts->discard)
> > > -- 
> > > 2.25.1
> > > 
> > 

[PATCH v2 6/6] fs/ntfs3: Rename mount option no_acl_rules > (no)acl_rules

[Kari Argillander]

Rename mount option no_acl_rules to noacl_rules. This allow us to use
possibility to mount with options noacl_rules or acl_rules.

Signed-off-by: Kari Argillander <kari.argillander@gmail.com>
---
 Documentation/filesystems/ntfs3.rst |  2 +-
 fs/ntfs3/file.c                     |  2 +-
 fs/ntfs3/ntfs_fs.h                  |  2 +-
 fs/ntfs3/super.c                    | 12 ++++++------
 fs/ntfs3/xattr.c                    |  2 +-
 5 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/Documentation/filesystems/ntfs3.rst b/Documentation/filesystems/ntfs3.rst
index ded706474825..bdc9dd5a9185 100644
--- a/Documentation/filesystems/ntfs3.rst
+++ b/Documentation/filesystems/ntfs3.rst
@@ -73,7 +73,7 @@ prealloc		Preallocate space for files excessively when file size is
 			increasing on writes. Decreases fragmentation in case of
 			parallel write operations to different files.
 
-no_acs_rules		"No access rules" mount option sets access rights for
+noacs_rules		"No access rules" mount option sets access rights for
 			files/folders to 777 and owner/group to root. This mount
 			option absorbs all other permissions:
 			- permissions change for files/folders will be reported
diff --git a/fs/ntfs3/file.c b/fs/ntfs3/file.c
index 59344985c2e8..de3c6c76ab7d 100644
--- a/fs/ntfs3/file.c
+++ b/fs/ntfs3/file.c
@@ -743,7 +743,7 @@ int ntfs3_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
 	umode_t mode = inode->i_mode;
 	int err;
 
-	if (sbi->options.no_acs_rules) {
+	if (sbi->options.noacs_rules) {
 		/* "no access rules" - force any changes of time etc. */
 		attr->ia_valid |= ATTR_FORCE;
 		/* and disable for editing some attributes */
diff --git a/fs/ntfs3/ntfs_fs.h b/fs/ntfs3/ntfs_fs.h
index 1f07dd17c6c7..bec51e6f476d 100644
--- a/fs/ntfs3/ntfs_fs.h
+++ b/fs/ntfs3/ntfs_fs.h
@@ -67,7 +67,7 @@ struct ntfs_mount_options {
 		showmeta : 1, /*show meta files*/
 		nohidden : 1, /*do not show hidden files*/
 		force : 1, /*rw mount dirty volume*/
-		no_acs_rules : 1, /*exclude acs rules*/
+		noacs_rules : 1, /*exclude acs rules*/
 		prealloc : 1 /*preallocate space when file is growing*/
 		;
 };
diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c
index c3c07c181f15..a94a094463ad 100644
--- a/fs/ntfs3/super.c
+++ b/fs/ntfs3/super.c
@@ -242,7 +242,7 @@ enum Opt {
 	Opt_acl,
 	Opt_iocharset,
 	Opt_prealloc,
-	Opt_no_acs_rules,
+	Opt_noacs_rules,
 	Opt_err,
 };
 
@@ -260,7 +260,7 @@ static const struct fs_parameter_spec ntfs_fs_parameters[] = {
 	fsparam_flag_no("acl",			Opt_acl),
 	fsparam_flag_no("showmeta",		Opt_showmeta),
 	fsparam_flag_no("prealloc",		Opt_prealloc),
-	fsparam_flag("no_acs_rules",		Opt_no_acs_rules),
+	fsparam_flag_no("acs_rules",		Opt_noacs_rules),
 	fsparam_string("iocharset",		Opt_iocharset),
 
 	__fsparam(fs_param_is_string,
@@ -343,8 +343,8 @@ static int ntfs_fs_parse_param(struct fs_context *fc,
 	case Opt_prealloc:
 		opts->prealloc = result.negated ? 0 : 1;
 		break;
-	case Opt_no_acs_rules:
-		opts->no_acs_rules = 1;
+	case Opt_noacs_rules:
+		opts->noacs_rules = result.negated ? 1 : 0;
 		break;
 	default:
 		/* Should not be here unless we forget add case. */
@@ -536,8 +536,8 @@ static int ntfs_show_options(struct seq_file *m, struct dentry *root)
 		seq_puts(m, ",nohidden");
 	if (opts->force)
 		seq_puts(m, ",force");
-	if (opts->no_acs_rules)
-		seq_puts(m, ",no_acs_rules");
+	if (opts->noacs_rules)
+		seq_puts(m, ",noacs_rules");
 	if (opts->prealloc)
 		seq_puts(m, ",prealloc");
 	if (sb->s_flags & SB_POSIXACL)
diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c
index 98871c895e77..e20e710a065f 100644
--- a/fs/ntfs3/xattr.c
+++ b/fs/ntfs3/xattr.c
@@ -774,7 +774,7 @@ int ntfs_acl_chmod(struct user_namespace *mnt_userns, struct inode *inode)
 int ntfs_permission(struct user_namespace *mnt_userns, struct inode *inode,
 		    int mask)
 {
-	if (ntfs_sb(inode->i_sb)->options.no_acs_rules) {
+	if (ntfs_sb(inode->i_sb)->options.noacs_rules) {
 		/* "no access rules" mode - allow all changes */
 		return 0;
 	}
-- 
2.25.1

Re: [PATCH v2 6/6] fs/ntfs3: Rename mount option no_acl_rules > (no)acl_rules

[Christoph Hellwig]

On Thu, Aug 19, 2021 at 03:26:33AM +0300, Kari Argillander wrote:
> Rename mount option no_acl_rules to noacl_rules. This allow us to use
> possibility to mount with options noacl_rules or acl_rules.
> 
> Signed-off-by: Kari Argillander <kari.argillander@gmail.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2 6/6] fs/ntfs3: Rename mount option no_acl_rules > (no)acl_rules

[Christian Brauner]

On Thu, Aug 19, 2021 at 03:26:33AM +0300, Kari Argillander wrote:
> Rename mount option no_acl_rules to noacl_rules. This allow us to use
> possibility to mount with options noacl_rules or acl_rules.
> 
> Signed-off-by: Kari Argillander <kari.argillander@gmail.com>
> ---

Looks good. Thanks!
Acked-by: Christian Brauner <christian.brauner@ubuntu.com>