[REPOST PATCH v3 00/16] xfs: mount API patch series

[REPOST PATCH v3 00/16] xfs: mount API patch series

[Ian Kent]

Appologies for posting a stale series, here is the correct one.

This patch series adds support to xfs for the new kernel mount API
as described in the LWN article at https://lwn.net/Articles/780267/.

In the article there's a lengthy description of the reasons for
adopting the API and problems expected to be resolved by using it.

The series has been applied to the repository located at
git://git.kernel.org/pub/scm/fs/xfs/xfs-linux.git, and built and
some simple tests run on it.

Other things that continue to cause me concern:

- Message logging.
  There is error logging done in the VFS by the mount-api code, some
  is VFS specific while some is file system specific. This can lead
  to duplicated and often inconsistent logging.

  The mount-api feature of saving error message text to the mount
  context for later retrival by fsopen()/fsconfig()/fsmount() users
  is the reason the mount-api log macros are present. But, at the
  moment (last time I looked), these macros will either log the
  error message or save it to the mount context, there's not yet
  a way to modify this behaviour which can lead to messages,
  possibly needed for debug purposes, being lost. There's also
  the pr_xxx() log functions (not a problem for xfs AFAICS) that
  aren't aware of the mount context.

  Consequently I've tried to keep logging as it is in xfs and
  also tried to avoid duplicate log messages, deferring mount
  context messaging until more work has been done on mount-api
  convesions so that further discussions can take place.

- That large comment about mount option handling.
  I'm not sure what to do about that large comment. On one hand
  I think there's useful information there and don't really want
  to para-phrase it (largely) out of existence, not knowing of
  a sensible place to put it so I can refer to it instead.

Changes for v3:
- fix struct xfs_fs_context initialization in xfs_parseargs().
- move call to xfs_validate_params() below label "done".
- if allocation of xfs_mount fails return ENOMEM immediately.
- remove erroneous kfree(mp) in xfs_fill_super().
- move the removal of xfs_fs_remount() and xfs_test_remount_options()
  to the switch to mount api patch.
- retain original usage of distinct <option>, no<option> usage.
- fix line length and a white space problem in xfs_parseargs().
- defer introduction of struct fs_context_operations until mount
  api implementation.
- don't use a new line for the void parameter of xfs_mount_alloc().
- check for -ENOPARAM in xfs_parse_param() to report invalid options
  using the options switch (to avoid double entry log messages).
- remove obolete mount option biosize.
- try and make comment in xfs_fc_free() more understandable.

Changes for v2:
- changed .name to uppercase in fs_parameter_description to ensure
  consistent error log messages between the vfs parser and the xfs
  parameter parser.
- clearify comment above xfs_parse_param() about when possibly
  allocated mp->m_logname or mp->m_rtname are freed.
- factor out xfs_remount_rw() and xfs_remount_ro() from  xfs_remount().
- changed xfs_mount_alloc() to not set super block in xfs_mount so it
  can be re-used when switching to the mount-api.
- fixed don't check for NULL when calling kfree() in xfs_fc_free().
- refactored xfs_parseargs() in an attempt to highlight the code
  that actually changes in converting to use the new mount api.
- dropped xfs-mount-api-rename-xfs_fill_super.patch, it didn't seem
  necessary.
- move comment about remount difficulties above xfs_reconfigure()
  and increase line length to try and make the comment managable.

Al Viro has sent a pull request to Linus for the patch containing
the vfs_get_block_super() just recently so there will be conflicts
if this series is merged without dropping that patch.
---

David Howells (1):
      vfs: Create fs_context-aware mount_bdev() replacement

Ian Kent (15):
      xfs: remove very old mount option
      xfs: mount-api - add fs parameter description
      xfs: mount-api - refactor suffix_kstrtoint()
      xfs: mount-api - refactor xfs_parseags()
      xfs: mount-api - make xfs_parse_param() take context .parse_param() args
      xfs: mount-api - move xfs_parseargs() validation to a helper
      xfs: mount-api - refactor xfs_fs_fill_super()
      xfs: mount-api - add xfs_get_tree()
      xfs: mount-api - add xfs_remount_rw() helper
      xfs: mount-api - add xfs_remount_ro() helper
      xfs: mount api - add xfs_reconfigure()
      xfs: mount-api - add xfs_fc_free()
      xfs: mount-api - dont set sb in xfs_mount_alloc()
      xfs: mount-api - switch to new mount-api
      xfs: mount-api - remove legacy mount functions


 fs/fs_context.c            |    2 
 fs/super.c                 |  111 +++++
 fs/xfs/xfs_super.c         |  960 ++++++++++++++++++++++++--------------------
 include/linux/fs_context.h |    9 
 4 files changed, 642 insertions(+), 440 deletions(-)

--
Ian

[REPOST PATCH v3 01/16] vfs: Create fs_context-aware mount_bdev() replacement

[Ian Kent]

From: David Howells <dhowells@redhat.com>

Create a function, vfs_get_block_super(), that is fs_context-aware and a
replacement for mount_bdev().  It caches the block device pointer and file
open mode in the fs_context struct so that this information can be passed
into sget_fc()'s test and set functions.

Signed-off-by: David Howells <dhowells@redhat.com>
cc: Jens Axboe <axboe@kernel.dk>
cc: linux-block@vger.kernel.org
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/fs_context.c            |    2 +
 fs/super.c                 |  111 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/fs_context.h |    9 ++++
 3 files changed, 121 insertions(+), 1 deletion(-)

diff --git a/fs/fs_context.c b/fs/fs_context.c
index 103643c68e3f..270ecae32216 100644
--- a/fs/fs_context.c
+++ b/fs/fs_context.c
@@ -501,6 +501,8 @@ void put_fs_context(struct fs_context *fc)
 
 	if (fc->need_free && fc->ops && fc->ops->free)
 		fc->ops->free(fc);
+	if (fc->dev_destructor)
+		fc->dev_destructor(fc);
 
 	security_free_mnt_opts(&fc->security);
 	put_net(fc->net_ns);
diff --git a/fs/super.c b/fs/super.c
index 113c58f19425..95cf8a2d5b25 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -1215,6 +1215,110 @@ int get_tree_single(struct fs_context *fc,
 EXPORT_SYMBOL(get_tree_single);
 
 #ifdef CONFIG_BLOCK
+static void fc_bdev_destructor(struct fs_context *fc)
+{
+	if (fc->bdev) {
+		blkdev_put(fc->bdev, fc->bdev_mode);
+		fc->bdev = NULL;
+	}
+}
+
+static int set_bdev_super_fc(struct super_block *s, struct fs_context *fc)
+{
+	s->s_mode = fc->bdev_mode;
+	s->s_bdev = fc->bdev;
+	s->s_dev = s->s_bdev->bd_dev;
+	s->s_bdi = bdi_get(s->s_bdev->bd_bdi);
+	fc->bdev = NULL;
+	return 0;
+}
+
+static int test_bdev_super_fc(struct super_block *s, struct fs_context *fc)
+{
+	return s->s_bdev == fc->bdev;
+}
+
+/**
+ * vfs_get_block_super - Get a superblock based on a single block device
+ * @fc: The filesystem context holding the parameters
+ * @keying: How to distinguish superblocks
+ * @fill_super: Helper to initialise a new superblock
+ */
+int vfs_get_block_super(struct fs_context *fc,
+			int (*fill_super)(struct super_block *,
+					  struct fs_context *))
+{
+	struct block_device *bdev;
+	struct super_block *s;
+	int error = 0;
+
+	fc->bdev_mode = FMODE_READ | FMODE_EXCL;
+	if (!(fc->sb_flags & SB_RDONLY))
+		fc->bdev_mode |= FMODE_WRITE;
+
+	if (!fc->source)
+		return invalf(fc, "No source specified");
+
+	bdev = blkdev_get_by_path(fc->source, fc->bdev_mode, fc->fs_type);
+	if (IS_ERR(bdev)) {
+		errorf(fc, "%s: Can't open blockdev", fc->source);
+		return PTR_ERR(bdev);
+	}
+
+	fc->dev_destructor = fc_bdev_destructor;
+	fc->bdev = bdev;
+
+	/* Once the superblock is inserted into the list by sget_fc(), s_umount
+	 * will protect the lockfs code from trying to start a snapshot while
+	 * we are mounting
+	 */
+	mutex_lock(&bdev->bd_fsfreeze_mutex);
+	if (bdev->bd_fsfreeze_count > 0) {
+		mutex_unlock(&bdev->bd_fsfreeze_mutex);
+		warnf(fc, "%pg: Can't mount, blockdev is frozen", bdev);
+		return -EBUSY;
+	}
+
+	fc->sb_flags |= SB_NOSEC;
+	s = sget_fc(fc, test_bdev_super_fc, set_bdev_super_fc);
+	mutex_unlock(&bdev->bd_fsfreeze_mutex);
+	if (IS_ERR(s))
+		return PTR_ERR(s);
+
+	if (s->s_root) {
+		/* Don't summarily change the RO/RW state. */
+		if ((fc->sb_flags ^ s->s_flags) & SB_RDONLY) {
+			warnf(fc, "%pg: Can't mount, would change RO state", bdev);
+			error = -EBUSY;
+			goto error_sb;
+		}
+
+		/* Leave fc->bdev to fc_bdev_destructor() to clean up to avoid
+		 * locking conflicts.
+		 */
+	} else {
+		snprintf(s->s_id, sizeof(s->s_id), "%pg", bdev);
+		sb_set_blocksize(s, block_size(bdev));
+		error = fill_super(s, fc);
+		if (error)
+			goto error_sb;
+
+		s->s_flags |= SB_ACTIVE;
+		bdev->bd_super = s;
+	}
+
+	BUG_ON(fc->root);
+	fc->root = dget(s->s_root);
+	return 0;
+
+error_sb:
+	deactivate_locked_super(s);
+	/* Leave fc->bdev to fc_bdev_destructor() to clean up */
+	return error;
+}
+EXPORT_SYMBOL(vfs_get_block_super);
+
+
 static int set_bdev_super(struct super_block *s, void *data)
 {
 	s->s_bdev = data;
@@ -1414,8 +1518,13 @@ int vfs_get_tree(struct fs_context *fc)
 	 * on the superblock.
 	 */
 	error = fc->ops->get_tree(fc);
-	if (error < 0)
+	if (error < 0) {
+		if (fc->dev_destructor) {
+			fc->dev_destructor(fc);
+			fc->dev_destructor = NULL;
+		}
 		return error;
+	}
 
 	if (!fc->root) {
 		pr_err("Filesystem %s get_tree() didn't set fc->root\n",
diff --git a/include/linux/fs_context.h b/include/linux/fs_context.h
index 7c6fe3d47fa6..ed5b4349671e 100644
--- a/include/linux/fs_context.h
+++ b/include/linux/fs_context.h
@@ -88,6 +88,9 @@ struct fs_context {
 	struct mutex		uapi_mutex;	/* Userspace access mutex */
 	struct file_system_type	*fs_type;
 	void			*fs_private;	/* The filesystem's context */
+	union {
+		struct block_device *bdev;	/* The backing blockdev (if applicable) */
+	};
 	struct dentry		*root;		/* The root and superblock */
 	struct user_namespace	*user_ns;	/* The user namespace for this mount */
 	struct net		*net_ns;	/* The network namespace for this mount */
@@ -97,6 +100,7 @@ struct fs_context {
 	const char		*subtype;	/* The subtype to set on the superblock */
 	void			*security;	/* Linux S&M options */
 	void			*s_fs_info;	/* Proposed s_fs_info */
+	fmode_t			bdev_mode;	/* File open mode for bdev */
 	unsigned int		sb_flags;	/* Proposed superblock flags (SB_*) */
 	unsigned int		sb_flags_mask;	/* Superblock flags that were changed */
 	unsigned int		s_iflags;	/* OR'd with sb->s_iflags */
@@ -105,6 +109,7 @@ struct fs_context {
 	enum fs_context_phase	phase:8;	/* The phase the context is in */
 	bool			need_free:1;	/* Need to call ops->free() */
 	bool			global:1;	/* Goes into &init_user_ns */
+	void (*dev_destructor)(struct fs_context *fc); /* For block or mtd */
 };
 
 struct fs_context_operations {
@@ -154,6 +159,10 @@ extern int get_tree_single(struct fs_context *fc,
 			 int (*fill_super)(struct super_block *sb,
 					   struct fs_context *fc));
 
+extern int vfs_get_block_super(struct fs_context *fc,
+			       int (*fill_super)(struct super_block *sb,
+						 struct fs_context *fc));
+
 extern const struct file_operations fscontext_fops;
 
 /*

Re: [REPOST PATCH v3 01/16] vfs: Create fs_context-aware mount_bdev() replacement

[Al Viro]

On Tue, Sep 24, 2019 at 09:22:05PM +0800, Ian Kent wrote:
> From: David Howells <dhowells@redhat.com>
> 
> Create a function, vfs_get_block_super(), that is fs_context-aware and a
> replacement for mount_bdev().  It caches the block device pointer and file
> open mode in the fs_context struct so that this information can be passed
> into sget_fc()'s test and set functions.

NAK.  Use get_tree_bdev() instead.

Re: [REPOST PATCH v3 01/16] vfs: Create fs_context-aware mount_bdev() replacement

[Ian Kent]

On Tue, 2019-09-24 at 22:33 +0100, Al Viro wrote:
> On Tue, Sep 24, 2019 at 09:22:05PM +0800, Ian Kent wrote:
> > From: David Howells <dhowells@redhat.com>
> > 
> > Create a function, vfs_get_block_super(), that is fs_context-aware
> > and a
> > replacement for mount_bdev().  It caches the block device pointer
> > and file
> > open mode in the fs_context struct so that this information can be
> > passed
> > into sget_fc()'s test and set functions.
> 
> NAK.  Use get_tree_bdev() instead.

Thanks Al, will do.

Ian

[REPOST PATCH v3 02/16] xfs: remove very old mount option

[Ian Kent]

It appears the biosize mount option hasn't been documented as
a vilid option since 2005.

So remove it.

Signed-off-by: Ian Kent <raven@themaw.net>
---
 fs/xfs/xfs_super.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index f9450235533c..1010097354a6 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -51,7 +51,7 @@ static struct xfs_kobj xfs_dbg_kobj;	/* global debug sysfs attrs */
  * Table driven mount option parser.
  */
 enum {
-	Opt_logbufs, Opt_logbsize, Opt_logdev, Opt_rtdev, Opt_biosize,
+	Opt_logbufs, Opt_logbsize, Opt_logdev, Opt_rtdev,
 	Opt_wsync, Opt_noalign, Opt_swalloc, Opt_sunit, Opt_swidth, Opt_nouuid,
 	Opt_grpid, Opt_nogrpid, Opt_bsdgroups, Opt_sysvgroups,
 	Opt_allocsize, Opt_norecovery, Opt_inode64, Opt_inode32, Opt_ikeep,
@@ -67,7 +67,6 @@ static const match_table_t tokens = {
 	{Opt_logbsize,	"logbsize=%s"},	/* size of XFS log buffers */
 	{Opt_logdev,	"logdev=%s"},	/* log device */
 	{Opt_rtdev,	"rtdev=%s"},	/* realtime I/O device */
-	{Opt_biosize,	"biosize=%u"},	/* log2 of preferred buffered io size */
 	{Opt_wsync,	"wsync"},	/* safe-mode nfs compatible mount */
 	{Opt_noalign,	"noalign"},	/* turn off stripe alignment */
 	{Opt_swalloc,	"swalloc"},	/* turn on stripe width allocation */
@@ -229,7 +228,6 @@ xfs_parseargs(
 				return -ENOMEM;
 			break;
 		case Opt_allocsize:
-		case Opt_biosize:
 			if (suffix_kstrtoint(args, 10, &iosize))
 				return -EINVAL;
 			iosizelog = ffs(iosize) - 1;

[REPOST PATCH v3 03/16] xfs: mount-api - add fs parameter description

[Ian Kent]

The new mount-api uses an array of struct fs_parameter_spec for
parameter parsing, create this table populated with the xfs mount
parameters.

The new mount-api table definition is wider than the token based
parameter table and interleaving the option description comments
between each table line is much less readable than adding them to
the end of each table entry. So add the option description comment
to each entry line even though it causes quite a few of the entries
to be longer than 80 characters.

Signed-off-by: Ian Kent <raven@themaw.net>
---
 fs/xfs/xfs_super.c |   50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 1010097354a6..9c1ce3d70c08 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -38,6 +38,8 @@
 
 #include <linux/magic.h>
 #include <linux/parser.h>
+#include <linux/fs_context.h>
+#include <linux/fs_parser.h>
 
 static const struct super_operations xfs_super_operations;
 struct bio_set xfs_ioend_bioset;
@@ -108,6 +110,54 @@ static const match_table_t tokens = {
 	{Opt_err,	NULL},
 };
 
+static const struct fs_parameter_spec xfs_param_specs[] = {
+ fsparam_u32	("logbufs",    Opt_logbufs),   /* number of XFS log buffers */
+ fsparam_string ("logbsize",   Opt_logbsize),  /* size of XFS log buffers */
+ fsparam_string ("logdev",     Opt_logdev),    /* log device */
+ fsparam_string ("rtdev",      Opt_rtdev),     /* realtime I/O device */
+ fsparam_flag	("wsync",      Opt_wsync),     /* safe-mode nfs compatible mount */
+ fsparam_flag	("noalign",    Opt_noalign),   /* turn off stripe alignment */
+ fsparam_flag	("swalloc",    Opt_swalloc),   /* turn on stripe width allocation */
+ fsparam_u32	("sunit",      Opt_sunit),     /* data volume stripe unit */
+ fsparam_u32	("swidth",     Opt_swidth),    /* data volume stripe width */
+ fsparam_flag	("nouuid",     Opt_nouuid),    /* ignore filesystem UUID */
+ fsparam_flag   ("grpid",      Opt_grpid),     /* group-ID from parent directory */
+ fsparam_flag   ("nogrpid",    Opt_nogrpid),   /* no group-ID from parent directory */
+ fsparam_flag	("bsdgroups",  Opt_bsdgroups), /* group-ID from parent directory */
+ fsparam_flag	("sysvgroups", Opt_sysvgroups),/* group-ID from current process */
+ fsparam_string ("allocsize",  Opt_allocsize), /* preferred allocation size */
+ fsparam_flag	("norecovery", Opt_norecovery),/* don't run XFS recovery */
+ fsparam_flag	("inode64",    Opt_inode64),   /* inodes can be allocated anywhere */
+ fsparam_flag	("inode32",    Opt_inode32),   /* inode allocation limited to XFS_MAXINUMBER_32 */
+ fsparam_flag   ("ikeep",      Opt_ikeep),     /* do not free empty inode clusters */
+ fsparam_flag   ("noikeep",    Opt_noikeep),   /* free empty inode clusters */
+ fsparam_flag   ("largeio",    Opt_largeio),   /* report large I/O sizes in stat() */
+ fsparam_flag   ("nolargeio",  Opt_nolargeio), /* do not report large I/O sizes in stat() */
+ fsparam_flag   ("attr2",      Opt_attr2),     /* do use attr2 attribute format */
+ fsparam_flag   ("noattr2",    Opt_noattr2),   /* do not use attr2 attribute format */
+ fsparam_flag	("filestreams",Opt_filestreams), /* use filestreams allocator */
+ fsparam_flag   ("quota",      Opt_quota),     /* disk quotas (user) */
+ fsparam_flag   ("noquota",    Opt_noquota),   /* no quotas */
+ fsparam_flag	("usrquota",   Opt_usrquota),  /* user quota enabled */
+ fsparam_flag	("grpquota",   Opt_grpquota),  /* group quota enabled */
+ fsparam_flag	("prjquota",   Opt_prjquota),  /* project quota enabled */
+ fsparam_flag	("uquota",     Opt_uquota),    /* user quota (IRIX variant) */
+ fsparam_flag	("gquota",     Opt_gquota),    /* group quota (IRIX variant) */
+ fsparam_flag	("pquota",     Opt_pquota),    /* project quota (IRIX variant) */
+ fsparam_flag	("uqnoenforce",Opt_uqnoenforce), /* user quota limit enforcement */
+ fsparam_flag	("gqnoenforce",Opt_gqnoenforce), /* group quota limit enforcement */
+ fsparam_flag	("pqnoenforce",Opt_pqnoenforce), /* project quota limit enforcement */
+ fsparam_flag	("qnoenforce", Opt_qnoenforce),  /* same as uqnoenforce */
+ fsparam_flag   ("discard",    Opt_discard),   /* Discard unused blocks */
+ fsparam_flag   ("nodiscard",  Opt_nodiscard), /* Do not discard unused blocks */
+ fsparam_flag	("dax",	       Opt_dax),       /* Enable direct access to bdev pages */
+ {}
+};
+
+static const struct fs_parameter_description xfs_fs_parameters = {
+	.name		= "XFS",
+	.specs		= xfs_param_specs,
+};
 
 STATIC int
 suffix_kstrtoint(const substring_t *s, unsigned int base, int *res)

[REPOST PATCH v3 04/16] xfs: mount-api - refactor suffix_kstrtoint()

[Ian Kent]

The mount-api doesn't have a "human unit" parse type yet so
the options that have values like "10k" etc. still need to
be converted by the fs.

But the value comes to the fs as a string (not a substring_t
type) so there's a need to change the conversion function to
take a character string instead.

After refactoring xfs_parseargs() and changing it to use
xfs_parse_param() match_kstrtoint() will no longer be used
and will be removed.

Signed-off-by: Ian Kent <raven@themaw.net>
---
 fs/xfs/xfs_super.c |   22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 9c1ce3d70c08..6a16750b1314 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -160,13 +160,13 @@ static const struct fs_parameter_description xfs_fs_parameters = {
 };
 
 STATIC int
-suffix_kstrtoint(const substring_t *s, unsigned int base, int *res)
+suffix_kstrtoint(const char *s, unsigned int base, int *res)
 {
 	int	last, shift_left_factor = 0, _res;
 	char	*value;
 	int	ret = 0;
 
-	value = match_strdup(s);
+	value = kstrdup(s, GFP_KERNEL);
 	if (!value)
 		return -ENOMEM;
 
@@ -191,6 +191,20 @@ suffix_kstrtoint(const substring_t *s, unsigned int base, int *res)
 	return ret;
 }
 
+STATIC int
+match_kstrtoint(const substring_t *s, unsigned int base, int *res)
+{
+	const char	*value;
+	int ret;
+
+	value = match_strdup(s);
+	if (!value)
+		return -ENOMEM;
+	ret = suffix_kstrtoint(value, base, res);
+	kfree(value);
+	return ret;
+}
+
 /*
  * This function fills in xfs_mount_t fields based on mount args.
  * Note: the superblock has _not_ yet been read in.
@@ -262,7 +276,7 @@ xfs_parseargs(
 				return -EINVAL;
 			break;
 		case Opt_logbsize:
-			if (suffix_kstrtoint(args, 10, &mp->m_logbsize))
+			if (match_kstrtoint(args, 10, &mp->m_logbsize))
 				return -EINVAL;
 			break;
 		case Opt_logdev:
@@ -278,7 +292,7 @@ xfs_parseargs(
 				return -ENOMEM;
 			break;
 		case Opt_allocsize:
-			if (suffix_kstrtoint(args, 10, &iosize))
+			if (match_kstrtoint(args, 10, &iosize))
 				return -EINVAL;
 			iosizelog = ffs(iosize) - 1;
 			break;

[REPOST PATCH v3 05/16] xfs: mount-api - refactor xfs_parseags()

[Ian Kent]

Refactor xfs_parseags(), move the entire token case block to a
separate function in an attempt to highlight the code that
actually changes in converting to use the new mount api.

The only changes are what's needed to communicate the variables
dsunit, dswidth and iosizelog back to xfs_parseags().

Signed-off-by: Ian Kent <raven@themaw.net>
---
 fs/xfs/xfs_super.c |  290 ++++++++++++++++++++++++++++------------------------
 1 file changed, 155 insertions(+), 135 deletions(-)

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 6a16750b1314..b04aebab69ab 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -205,6 +205,156 @@ match_kstrtoint(const substring_t *s, unsigned int base, int *res)
 	return ret;
 }
 
+STATIC int
+xfs_parse_param(
+	int			token,
+	char			*p,
+	substring_t		*args,
+	struct xfs_mount	*mp,
+	int			*dsunit,
+	int			*dswidth,
+	uint8_t			*iosizelog)
+{
+	int			iosize = 0;
+
+	switch (token) {
+	case Opt_logbufs:
+		if (match_int(args, &mp->m_logbufs))
+			return -EINVAL;
+		break;
+	case Opt_logbsize:
+		if (match_kstrtoint(args, 10, &mp->m_logbsize))
+			return -EINVAL;
+		break;
+	case Opt_logdev:
+		kfree(mp->m_logname);
+		mp->m_logname = match_strdup(args);
+		if (!mp->m_logname)
+			return -ENOMEM;
+		break;
+	case Opt_rtdev:
+		kfree(mp->m_rtname);
+		mp->m_rtname = match_strdup(args);
+		if (!mp->m_rtname)
+			return -ENOMEM;
+		break;
+	case Opt_allocsize:
+		if (match_kstrtoint(args, 10, &iosize))
+			return -EINVAL;
+		*iosizelog = ffs(iosize) - 1;
+		break;
+	case Opt_grpid:
+	case Opt_bsdgroups:
+		mp->m_flags |= XFS_MOUNT_GRPID;
+		break;
+	case Opt_nogrpid:
+	case Opt_sysvgroups:
+		mp->m_flags &= ~XFS_MOUNT_GRPID;
+		break;
+	case Opt_wsync:
+		mp->m_flags |= XFS_MOUNT_WSYNC;
+		break;
+	case Opt_norecovery:
+		mp->m_flags |= XFS_MOUNT_NORECOVERY;
+		break;
+	case Opt_noalign:
+		mp->m_flags |= XFS_MOUNT_NOALIGN;
+		break;
+	case Opt_swalloc:
+		mp->m_flags |= XFS_MOUNT_SWALLOC;
+		break;
+	case Opt_sunit:
+		if (match_int(args, dsunit))
+			return -EINVAL;
+		break;
+	case Opt_swidth:
+		if (match_int(args, dswidth))
+			return -EINVAL;
+		break;
+	case Opt_inode32:
+		mp->m_flags |= XFS_MOUNT_SMALL_INUMS;
+		break;
+	case Opt_inode64:
+		mp->m_flags &= ~XFS_MOUNT_SMALL_INUMS;
+		break;
+	case Opt_nouuid:
+		mp->m_flags |= XFS_MOUNT_NOUUID;
+		break;
+	case Opt_ikeep:
+		mp->m_flags |= XFS_MOUNT_IKEEP;
+		break;
+	case Opt_noikeep:
+		mp->m_flags &= ~XFS_MOUNT_IKEEP;
+		break;
+	case Opt_largeio:
+		mp->m_flags &= ~XFS_MOUNT_COMPAT_IOSIZE;
+		break;
+	case Opt_nolargeio:
+		mp->m_flags |= XFS_MOUNT_COMPAT_IOSIZE;
+		break;
+	case Opt_attr2:
+		mp->m_flags |= XFS_MOUNT_ATTR2;
+		break;
+	case Opt_noattr2:
+		mp->m_flags &= ~XFS_MOUNT_ATTR2;
+		mp->m_flags |= XFS_MOUNT_NOATTR2;
+		break;
+	case Opt_filestreams:
+		mp->m_flags |= XFS_MOUNT_FILESTREAMS;
+		break;
+	case Opt_noquota:
+		mp->m_qflags &= ~XFS_ALL_QUOTA_ACCT;
+		mp->m_qflags &= ~XFS_ALL_QUOTA_ENFD;
+		mp->m_qflags &= ~XFS_ALL_QUOTA_ACTIVE;
+		break;
+	case Opt_quota:
+	case Opt_uquota:
+	case Opt_usrquota:
+		mp->m_qflags |= (XFS_UQUOTA_ACCT | XFS_UQUOTA_ACTIVE |
+				 XFS_UQUOTA_ENFD);
+		break;
+	case Opt_qnoenforce:
+	case Opt_uqnoenforce:
+		mp->m_qflags |= (XFS_UQUOTA_ACCT | XFS_UQUOTA_ACTIVE);
+		mp->m_qflags &= ~XFS_UQUOTA_ENFD;
+		break;
+	case Opt_pquota:
+	case Opt_prjquota:
+		mp->m_qflags |= (XFS_PQUOTA_ACCT | XFS_PQUOTA_ACTIVE |
+				 XFS_PQUOTA_ENFD);
+		break;
+	case Opt_pqnoenforce:
+		mp->m_qflags |= (XFS_PQUOTA_ACCT | XFS_PQUOTA_ACTIVE);
+		mp->m_qflags &= ~XFS_PQUOTA_ENFD;
+		break;
+	case Opt_gquota:
+	case Opt_grpquota:
+		mp->m_qflags |= (XFS_GQUOTA_ACCT | XFS_GQUOTA_ACTIVE |
+				 XFS_GQUOTA_ENFD);
+		break;
+	case Opt_gqnoenforce:
+		mp->m_qflags |= (XFS_GQUOTA_ACCT | XFS_GQUOTA_ACTIVE);
+		mp->m_qflags &= ~XFS_GQUOTA_ENFD;
+		break;
+	case Opt_discard:
+		mp->m_flags |= XFS_MOUNT_DISCARD;
+		break;
+	case Opt_nodiscard:
+		mp->m_flags &= ~XFS_MOUNT_DISCARD;
+		break;
+#ifdef CONFIG_FS_DAX
+	case Opt_dax:
+		mp->m_flags |= XFS_MOUNT_DAX;
+		break;
+#endif
+	default:
+		xfs_warn(mp, "unknown mount option [%s].", p);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 /*
  * This function fills in xfs_mount_t fields based on mount args.
  * Note: the superblock has _not_ yet been read in.
@@ -226,7 +376,6 @@ xfs_parseargs(
 	substring_t		args[MAX_OPT_ARGS];
 	int			dsunit = 0;
 	int			dswidth = 0;
-	int			iosize = 0;
 	uint8_t			iosizelog = 0;
 
 	/*
@@ -265,145 +414,16 @@ xfs_parseargs(
 
 	while ((p = strsep(&options, ",")) != NULL) {
 		int		token;
+		int		ret;
 
 		if (!*p)
 			continue;
 
 		token = match_token(p, tokens, args);
-		switch (token) {
-		case Opt_logbufs:
-			if (match_int(args, &mp->m_logbufs))
-				return -EINVAL;
-			break;
-		case Opt_logbsize:
-			if (match_kstrtoint(args, 10, &mp->m_logbsize))
-				return -EINVAL;
-			break;
-		case Opt_logdev:
-			kfree(mp->m_logname);
-			mp->m_logname = match_strdup(args);
-			if (!mp->m_logname)
-				return -ENOMEM;
-			break;
-		case Opt_rtdev:
-			kfree(mp->m_rtname);
-			mp->m_rtname = match_strdup(args);
-			if (!mp->m_rtname)
-				return -ENOMEM;
-			break;
-		case Opt_allocsize:
-			if (match_kstrtoint(args, 10, &iosize))
-				return -EINVAL;
-			iosizelog = ffs(iosize) - 1;
-			break;
-		case Opt_grpid:
-		case Opt_bsdgroups:
-			mp->m_flags |= XFS_MOUNT_GRPID;
-			break;
-		case Opt_nogrpid:
-		case Opt_sysvgroups:
-			mp->m_flags &= ~XFS_MOUNT_GRPID;
-			break;
-		case Opt_wsync:
-			mp->m_flags |= XFS_MOUNT_WSYNC;
-			break;
-		case Opt_norecovery:
-			mp->m_flags |= XFS_MOUNT_NORECOVERY;
-			break;
-		case Opt_noalign:
-			mp->m_flags |= XFS_MOUNT_NOALIGN;
-			break;
-		case Opt_swalloc:
-			mp->m_flags |= XFS_MOUNT_SWALLOC;
-			break;
-		case Opt_sunit:
-			if (match_int(args, &dsunit))
-				return -EINVAL;
-			break;
-		case Opt_swidth:
-			if (match_int(args, &dswidth))
-				return -EINVAL;
-			break;
-		case Opt_inode32:
-			mp->m_flags |= XFS_MOUNT_SMALL_INUMS;
-			break;
-		case Opt_inode64:
-			mp->m_flags &= ~XFS_MOUNT_SMALL_INUMS;
-			break;
-		case Opt_nouuid:
-			mp->m_flags |= XFS_MOUNT_NOUUID;
-			break;
-		case Opt_ikeep:
-			mp->m_flags |= XFS_MOUNT_IKEEP;
-			break;
-		case Opt_noikeep:
-			mp->m_flags &= ~XFS_MOUNT_IKEEP;
-			break;
-		case Opt_largeio:
-			mp->m_flags &= ~XFS_MOUNT_COMPAT_IOSIZE;
-			break;
-		case Opt_nolargeio:
-			mp->m_flags |= XFS_MOUNT_COMPAT_IOSIZE;
-			break;
-		case Opt_attr2:
-			mp->m_flags |= XFS_MOUNT_ATTR2;
-			break;
-		case Opt_noattr2:
-			mp->m_flags &= ~XFS_MOUNT_ATTR2;
-			mp->m_flags |= XFS_MOUNT_NOATTR2;
-			break;
-		case Opt_filestreams:
-			mp->m_flags |= XFS_MOUNT_FILESTREAMS;
-			break;
-		case Opt_noquota:
-			mp->m_qflags &= ~XFS_ALL_QUOTA_ACCT;
-			mp->m_qflags &= ~XFS_ALL_QUOTA_ENFD;
-			mp->m_qflags &= ~XFS_ALL_QUOTA_ACTIVE;
-			break;
-		case Opt_quota:
-		case Opt_uquota:
-		case Opt_usrquota:
-			mp->m_qflags |= (XFS_UQUOTA_ACCT | XFS_UQUOTA_ACTIVE |
-					 XFS_UQUOTA_ENFD);
-			break;
-		case Opt_qnoenforce:
-		case Opt_uqnoenforce:
-			mp->m_qflags |= (XFS_UQUOTA_ACCT | XFS_UQUOTA_ACTIVE);
-			mp->m_qflags &= ~XFS_UQUOTA_ENFD;
-			break;
-		case Opt_pquota:
-		case Opt_prjquota:
-			mp->m_qflags |= (XFS_PQUOTA_ACCT | XFS_PQUOTA_ACTIVE |
-					 XFS_PQUOTA_ENFD);
-			break;
-		case Opt_pqnoenforce:
-			mp->m_qflags |= (XFS_PQUOTA_ACCT | XFS_PQUOTA_ACTIVE);
-			mp->m_qflags &= ~XFS_PQUOTA_ENFD;
-			break;
-		case Opt_gquota:
-		case Opt_grpquota:
-			mp->m_qflags |= (XFS_GQUOTA_ACCT | XFS_GQUOTA_ACTIVE |
-					 XFS_GQUOTA_ENFD);
-			break;
-		case Opt_gqnoenforce:
-			mp->m_qflags |= (XFS_GQUOTA_ACCT | XFS_GQUOTA_ACTIVE);
-			mp->m_qflags &= ~XFS_GQUOTA_ENFD;
-			break;
-		case Opt_discard:
-			mp->m_flags |= XFS_MOUNT_DISCARD;
-			break;
-		case Opt_nodiscard:
-			mp->m_flags &= ~XFS_MOUNT_DISCARD;
-			break;
-#ifdef CONFIG_FS_DAX
-		case Opt_dax:
-			mp->m_flags |= XFS_MOUNT_DAX;
-			break;
-#endif
-		default:
-			xfs_warn(mp, "unknown mount option [%s].", p);
-			return -EINVAL;
-		}
+		ret = xfs_parse_param(token, p, args, mp,
+				      &dsunit, &dswidth, &iosizelog);
+		if (ret)
+			return ret;
 	}
 
 	/*

[REPOST PATCH v3 06/16] xfs: mount-api - make xfs_parse_param() take context .parse_param() args

[Ian Kent]

Make xfs_parse_param() take arguments of the fs context operation
.parse_param() in preparation for switching to use the file system
mount context for mount.

The function fc_parse() only uses the file system context (fc here)
when calling log macros warnf() and invalf() which in turn check
only the fc->log field to determine if the message should be saved
to a context buffer (for later retrival by userspace) or logged
using printk().

Also the temporary function match_kstrtoint() is now unused, remove it.

Signed-off-by: Ian Kent <raven@themaw.net>
---
 fs/xfs/xfs_super.c |  137 +++++++++++++++++++++++++++++++---------------------
 1 file changed, 81 insertions(+), 56 deletions(-)

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index b04aebab69ab..6792d46fa0be 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -191,57 +191,60 @@ suffix_kstrtoint(const char *s, unsigned int base, int *res)
 	return ret;
 }
 
-STATIC int
-match_kstrtoint(const substring_t *s, unsigned int base, int *res)
-{
-	const char	*value;
-	int ret;
-
-	value = match_strdup(s);
-	if (!value)
-		return -ENOMEM;
-	ret = suffix_kstrtoint(value, base, res);
-	kfree(value);
-	return ret;
-}
+struct xfs_fs_context {
+	int	dsunit;
+	int	dswidth;
+	uint8_t	iosizelog;
+};
 
 STATIC int
 xfs_parse_param(
-	int			token,
-	char			*p,
-	substring_t		*args,
-	struct xfs_mount	*mp,
-	int			*dsunit,
-	int			*dswidth,
-	uint8_t			*iosizelog)
+	struct fs_context	*fc,
+	struct fs_parameter	*param)
 {
+	struct xfs_fs_context	*ctx = fc->fs_private;
+	struct xfs_mount	*mp = fc->s_fs_info;
+	struct fs_parse_result	result;
 	int			iosize = 0;
+	int			opt;
 
-	switch (token) {
+	opt = fs_parse(fc, &xfs_fs_parameters, param, &result);
+	if (opt < 0) {
+		/*
+		 * If fs_parse() returns -ENOPARAM and the parameter
+		 * is "source" the VFS needs to handle this option
+		 * in order to boot otherwise use the default case
+		 * below to handle invalid options.
+		 */
+		if (opt != -ENOPARAM ||
+		    strcmp(param->key, "source") == 0)
+			return opt;
+	}
+
+	switch (opt) {
 	case Opt_logbufs:
-		if (match_int(args, &mp->m_logbufs))
-			return -EINVAL;
+		mp->m_logbufs = result.uint_32;
 		break;
 	case Opt_logbsize:
-		if (match_kstrtoint(args, 10, &mp->m_logbsize))
+		if (suffix_kstrtoint(param->string, 10, &mp->m_logbsize))
 			return -EINVAL;
 		break;
 	case Opt_logdev:
 		kfree(mp->m_logname);
-		mp->m_logname = match_strdup(args);
+		mp->m_logname = kstrdup(param->string, GFP_KERNEL);
 		if (!mp->m_logname)
 			return -ENOMEM;
 		break;
 	case Opt_rtdev:
 		kfree(mp->m_rtname);
-		mp->m_rtname = match_strdup(args);
+		mp->m_rtname = kstrdup(param->string, GFP_KERNEL);
 		if (!mp->m_rtname)
 			return -ENOMEM;
 		break;
 	case Opt_allocsize:
-		if (match_kstrtoint(args, 10, &iosize))
+		if (suffix_kstrtoint(param->string, 10, &iosize))
 			return -EINVAL;
-		*iosizelog = ffs(iosize) - 1;
+		ctx->iosizelog = ffs(iosize) - 1;
 		break;
 	case Opt_grpid:
 	case Opt_bsdgroups:
@@ -264,12 +267,10 @@ xfs_parse_param(
 		mp->m_flags |= XFS_MOUNT_SWALLOC;
 		break;
 	case Opt_sunit:
-		if (match_int(args, dsunit))
-			return -EINVAL;
+		ctx->dsunit = result.uint_32;
 		break;
 	case Opt_swidth:
-		if (match_int(args, dswidth))
-			return -EINVAL;
+		ctx->dswidth = result.uint_32;
 		break;
 	case Opt_inode32:
 		mp->m_flags |= XFS_MOUNT_SMALL_INUMS;
@@ -348,7 +349,7 @@ xfs_parse_param(
 		break;
 #endif
 	default:
-		xfs_warn(mp, "unknown mount option [%s].", p);
+		xfs_warn(mp, "unknown mount option [%s].", param->key);
 		return -EINVAL;
 	}
 
@@ -373,10 +374,16 @@ xfs_parseargs(
 {
 	const struct super_block *sb = mp->m_super;
 	char			*p;
-	substring_t		args[MAX_OPT_ARGS];
-	int			dsunit = 0;
-	int			dswidth = 0;
-	uint8_t			iosizelog = 0;
+	struct fs_context	fc;
+	struct xfs_fs_context	context;
+	struct xfs_fs_context	*ctx;
+	int			ret;
+
+	memset(&fc, 0, sizeof(fc));
+	memset(&context, 0, sizeof(context));
+	fc.fs_private = &context;
+	ctx = &context;
+	fc.s_fs_info = mp;
 
 	/*
 	 * set up the mount name first so all the errors will refer to the
@@ -413,16 +420,33 @@ xfs_parseargs(
 		goto done;
 
 	while ((p = strsep(&options, ",")) != NULL) {
-		int		token;
-		int		ret;
+		struct fs_parameter	param;
+		char			*value;
 
 		if (!*p)
 			continue;
 
-		token = match_token(p, tokens, args);
-		ret = xfs_parse_param(token, p, args, mp,
-				      &dsunit, &dswidth, &iosizelog);
-		if (ret)
+		param.key = p;
+		param.type = fs_value_is_string;
+		param.string = NULL;
+		param.size = 0;
+
+		value = strchr(p, '=');
+		if (value) {
+			*value++ = 0;
+			param.size = strlen(value);
+			if (param.size > 0) {
+				param.string = kmemdup_nul(value,
+							   param.size,
+							   GFP_KERNEL);
+				if (!param.string)
+					return -ENOMEM;
+			}
+		}
+
+		ret = xfs_parse_param(&fc, &param);
+		kfree(param.string);
+		if (ret < 0)
 			return ret;
 	}
 
@@ -435,7 +459,8 @@ xfs_parseargs(
 		return -EINVAL;
 	}
 
-	if ((mp->m_flags & XFS_MOUNT_NOALIGN) && (dsunit || dswidth)) {
+	if ((mp->m_flags & XFS_MOUNT_NOALIGN) &&
+	    (ctx->dsunit || ctx->dswidth)) {
 		xfs_warn(mp,
 	"sunit and swidth options incompatible with the noalign option");
 		return -EINVAL;
@@ -448,28 +473,28 @@ xfs_parseargs(
 	}
 #endif
 
-	if ((dsunit && !dswidth) || (!dsunit && dswidth)) {
+	if ((ctx->dsunit && !ctx->dswidth) || (!ctx->dsunit && ctx->dswidth)) {
 		xfs_warn(mp, "sunit and swidth must be specified together");
 		return -EINVAL;
 	}
 
-	if (dsunit && (dswidth % dsunit != 0)) {
+	if (ctx->dsunit && (ctx->dswidth % ctx->dsunit != 0)) {
 		xfs_warn(mp,
 	"stripe width (%d) must be a multiple of the stripe unit (%d)",
-			dswidth, dsunit);
+			ctx->dswidth, ctx->dsunit);
 		return -EINVAL;
 	}
 
 done:
-	if (dsunit && !(mp->m_flags & XFS_MOUNT_NOALIGN)) {
+	if (ctx->dsunit && !(mp->m_flags & XFS_MOUNT_NOALIGN)) {
 		/*
 		 * At this point the superblock has not been read
 		 * in, therefore we do not know the block size.
 		 * Before the mount call ends we will convert
 		 * these to FSBs.
 		 */
-		mp->m_dalign = dsunit;
-		mp->m_swidth = dswidth;
+		mp->m_dalign = ctx->dsunit;
+		mp->m_swidth = ctx->dswidth;
 	}
 
 	if (mp->m_logbufs != -1 &&
@@ -491,18 +516,18 @@ xfs_parseargs(
 		return -EINVAL;
 	}
 
-	if (iosizelog) {
-		if (iosizelog > XFS_MAX_IO_LOG ||
-		    iosizelog < XFS_MIN_IO_LOG) {
+	if (ctx->iosizelog) {
+		if (ctx->iosizelog > XFS_MAX_IO_LOG ||
+		    ctx->iosizelog < XFS_MIN_IO_LOG) {
 			xfs_warn(mp, "invalid log iosize: %d [not %d-%d]",
-				iosizelog, XFS_MIN_IO_LOG,
+				ctx->iosizelog, XFS_MIN_IO_LOG,
 				XFS_MAX_IO_LOG);
 			return -EINVAL;
 		}
 
 		mp->m_flags |= XFS_MOUNT_DFLT_IOSIZE;
-		mp->m_readio_log = iosizelog;
-		mp->m_writeio_log = iosizelog;
+		mp->m_readio_log = ctx->iosizelog;
+		mp->m_writeio_log = ctx->iosizelog;
 	}
 
 	return 0;

Re: [REPOST PATCH v3 06/16] xfs: mount-api - make xfs_parse_param() take context .parse_param() args

[Brian Foster]

On Tue, Sep 24, 2019 at 09:22:33PM +0800, Ian Kent wrote:
> Make xfs_parse_param() take arguments of the fs context operation
> .parse_param() in preparation for switching to use the file system
> mount context for mount.
> 
> The function fc_parse() only uses the file system context (fc here)
> when calling log macros warnf() and invalf() which in turn check
> only the fc->log field to determine if the message should be saved
> to a context buffer (for later retrival by userspace) or logged
> using printk().
> 
> Also the temporary function match_kstrtoint() is now unused, remove it.
> 
> Signed-off-by: Ian Kent <raven@themaw.net>
> ---
>  fs/xfs/xfs_super.c |  137 +++++++++++++++++++++++++++++++---------------------
>  1 file changed, 81 insertions(+), 56 deletions(-)
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index b04aebab69ab..6792d46fa0be 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -191,57 +191,60 @@ suffix_kstrtoint(const char *s, unsigned int base, int *res)
>  	return ret;
>  }
>  
...
>  
>  STATIC int
>  xfs_parse_param(
...
> -	switch (token) {
> +	opt = fs_parse(fc, &xfs_fs_parameters, param, &result);
> +	if (opt < 0) {
> +		/*
> +		 * If fs_parse() returns -ENOPARAM and the parameter
> +		 * is "source" the VFS needs to handle this option
> +		 * in order to boot otherwise use the default case
> +		 * below to handle invalid options.
> +		 */
> +		if (opt != -ENOPARAM ||
> +		    strcmp(param->key, "source") == 0)
> +			return opt;

Same question as before on this bit..

...
> @@ -373,10 +374,16 @@ xfs_parseargs(
>  {
>  	const struct super_block *sb = mp->m_super;
>  	char			*p;
> -	substring_t		args[MAX_OPT_ARGS];
> -	int			dsunit = 0;
> -	int			dswidth = 0;
> -	uint8_t			iosizelog = 0;
> +	struct fs_context	fc;
> +	struct xfs_fs_context	context;
> +	struct xfs_fs_context	*ctx;
> +	int			ret;
> +
> +	memset(&fc, 0, sizeof(fc));
> +	memset(&context, 0, sizeof(context));
> +	fc.fs_private = &context;
> +	ctx = &context;

I think you mentioned this ctx/context pattern would be removed from
v2..?

Brian

> +	fc.s_fs_info = mp;
>  
>  	/*
>  	 * set up the mount name first so all the errors will refer to the
> @@ -413,16 +420,33 @@ xfs_parseargs(
>  		goto done;
>  
>  	while ((p = strsep(&options, ",")) != NULL) {
> -		int		token;
> -		int		ret;
> +		struct fs_parameter	param;
> +		char			*value;
>  
>  		if (!*p)
>  			continue;
>  
> -		token = match_token(p, tokens, args);
> -		ret = xfs_parse_param(token, p, args, mp,
> -				      &dsunit, &dswidth, &iosizelog);
> -		if (ret)
> +		param.key = p;
> +		param.type = fs_value_is_string;
> +		param.string = NULL;
> +		param.size = 0;
> +
> +		value = strchr(p, '=');
> +		if (value) {
> +			*value++ = 0;
> +			param.size = strlen(value);
> +			if (param.size > 0) {
> +				param.string = kmemdup_nul(value,
> +							   param.size,
> +							   GFP_KERNEL);
> +				if (!param.string)
> +					return -ENOMEM;
> +			}
> +		}
> +
> +		ret = xfs_parse_param(&fc, &param);
> +		kfree(param.string);
> +		if (ret < 0)
>  			return ret;
>  	}
>  
> @@ -435,7 +459,8 @@ xfs_parseargs(
>  		return -EINVAL;
>  	}
>  
> -	if ((mp->m_flags & XFS_MOUNT_NOALIGN) && (dsunit || dswidth)) {
> +	if ((mp->m_flags & XFS_MOUNT_NOALIGN) &&
> +	    (ctx->dsunit || ctx->dswidth)) {
>  		xfs_warn(mp,
>  	"sunit and swidth options incompatible with the noalign option");
>  		return -EINVAL;
> @@ -448,28 +473,28 @@ xfs_parseargs(
>  	}
>  #endif
>  
> -	if ((dsunit && !dswidth) || (!dsunit && dswidth)) {
> +	if ((ctx->dsunit && !ctx->dswidth) || (!ctx->dsunit && ctx->dswidth)) {
>  		xfs_warn(mp, "sunit and swidth must be specified together");
>  		return -EINVAL;
>  	}
>  
> -	if (dsunit && (dswidth % dsunit != 0)) {
> +	if (ctx->dsunit && (ctx->dswidth % ctx->dsunit != 0)) {
>  		xfs_warn(mp,
>  	"stripe width (%d) must be a multiple of the stripe unit (%d)",
> -			dswidth, dsunit);
> +			ctx->dswidth, ctx->dsunit);
>  		return -EINVAL;
>  	}
>  
>  done:
> -	if (dsunit && !(mp->m_flags & XFS_MOUNT_NOALIGN)) {
> +	if (ctx->dsunit && !(mp->m_flags & XFS_MOUNT_NOALIGN)) {
>  		/*
>  		 * At this point the superblock has not been read
>  		 * in, therefore we do not know the block size.
>  		 * Before the mount call ends we will convert
>  		 * these to FSBs.
>  		 */
> -		mp->m_dalign = dsunit;
> -		mp->m_swidth = dswidth;
> +		mp->m_dalign = ctx->dsunit;
> +		mp->m_swidth = ctx->dswidth;
>  	}
>  
>  	if (mp->m_logbufs != -1 &&
> @@ -491,18 +516,18 @@ xfs_parseargs(
>  		return -EINVAL;
>  	}
>  
> -	if (iosizelog) {
> -		if (iosizelog > XFS_MAX_IO_LOG ||
> -		    iosizelog < XFS_MIN_IO_LOG) {
> +	if (ctx->iosizelog) {
> +		if (ctx->iosizelog > XFS_MAX_IO_LOG ||
> +		    ctx->iosizelog < XFS_MIN_IO_LOG) {
>  			xfs_warn(mp, "invalid log iosize: %d [not %d-%d]",
> -				iosizelog, XFS_MIN_IO_LOG,
> +				ctx->iosizelog, XFS_MIN_IO_LOG,
>  				XFS_MAX_IO_LOG);
>  			return -EINVAL;
>  		}
>  
>  		mp->m_flags |= XFS_MOUNT_DFLT_IOSIZE;
> -		mp->m_readio_log = iosizelog;
> -		mp->m_writeio_log = iosizelog;
> +		mp->m_readio_log = ctx->iosizelog;
> +		mp->m_writeio_log = ctx->iosizelog;
>  	}
>  
>  	return 0;
> 

Re: [REPOST PATCH v3 06/16] xfs: mount-api - make xfs_parse_param() take context .parse_param() args

[Ian Kent]

On Tue, 2019-09-24 at 10:37 -0400, Brian Foster wrote:
> On Tue, Sep 24, 2019 at 09:22:33PM +0800, Ian Kent wrote:
> > Make xfs_parse_param() take arguments of the fs context operation
> > .parse_param() in preparation for switching to use the file system
> > mount context for mount.
> > 
> > The function fc_parse() only uses the file system context (fc here)
> > when calling log macros warnf() and invalf() which in turn check
> > only the fc->log field to determine if the message should be saved
> > to a context buffer (for later retrival by userspace) or logged
> > using printk().
> > 
> > Also the temporary function match_kstrtoint() is now unused, remove
> > it.
> > 
> > Signed-off-by: Ian Kent <raven@themaw.net>
> > ---
> >  fs/xfs/xfs_super.c |  137 +++++++++++++++++++++++++++++++---------
> > ------------
> >  1 file changed, 81 insertions(+), 56 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > index b04aebab69ab..6792d46fa0be 100644
> > --- a/fs/xfs/xfs_super.c
> > +++ b/fs/xfs/xfs_super.c
> > @@ -191,57 +191,60 @@ suffix_kstrtoint(const char *s, unsigned int
> > base, int *res)
> >  	return ret;
> >  }
> >  
> ...
> >  
> >  STATIC int
> >  xfs_parse_param(
> ...
> > -	switch (token) {
> > +	opt = fs_parse(fc, &xfs_fs_parameters, param, &result);
> > +	if (opt < 0) {
> > +		/*
> > +		 * If fs_parse() returns -ENOPARAM and the parameter
> > +		 * is "source" the VFS needs to handle this option
> > +		 * in order to boot otherwise use the default case
> > +		 * below to handle invalid options.
> > +		 */
> > +		if (opt != -ENOPARAM ||
> > +		    strcmp(param->key, "source") == 0)
> > +			return opt;
> 
> Same question as before on this bit..

Your comment was:
Why is this not something that is handled in core mount-api code? Every
filesystem needs this logic in order to be a rootfs..?

I looked at the VFS code and was tempted to change it but it's all too
easy to prevent the system from booting.

The way the VFS looks to me it needs to give the file system a chance
to handle the "source" option, if the file system ->parse_param()
doesn't handle the option it "must" return -ENOPARAM so the VFS will
test for and handle the "source" option.

Having returned -ENOPARAM either the option is "source" or it's a
real unknown option.

The choices are:
1) If it is the "source" option we will get a false positive unknown
parameter message logged by our ->parse_param().
2) if it isn't the "source" option we will get an unknown parameter
message from our ->parse_param() and an additional inconsistent
format unknown parameter message from the VFS.
3) Check for the "source" parameter in our ->parse_param() and
return without issuing a message so the VFS can handle the option,
no duplicate message and no inconsistent logging.

Suggestions on how to handle this better, a VFS patch perhaps?
Suggestions David, Al?

> ...
> > @@ -373,10 +374,16 @@ xfs_parseargs(
> >  {
> >  	const struct super_block *sb = mp->m_super;
> >  	char			*p;
> > -	substring_t		args[MAX_OPT_ARGS];
> > -	int			dsunit = 0;
> > -	int			dswidth = 0;
> > -	uint8_t			iosizelog = 0;
> > +	struct fs_context	fc;
> > +	struct xfs_fs_context	context;
> > +	struct xfs_fs_context	*ctx;
> > +	int			ret;
> > +
> > +	memset(&fc, 0, sizeof(fc));
> > +	memset(&context, 0, sizeof(context));
> > +	fc.fs_private = &context;
> > +	ctx = &context;
> 
> I think you mentioned this ctx/context pattern would be removed from
> v2..?

Except that, to minimise code churn the ctx variable is needed
because it will be used in the end result.

I don't much like prefixing those references with &context even
if some happen to go away later on, the additional local variable
is clearer to read and provides usage consistency for the reader
over the changes.

Ian
> 
> Brian
> 
> > +	fc.s_fs_info = mp;
> >  
> >  	/*
> >  	 * set up the mount name first so all the errors will refer to
> > the
> > @@ -413,16 +420,33 @@ xfs_parseargs(
> >  		goto done;
> >  
> >  	while ((p = strsep(&options, ",")) != NULL) {
> > -		int		token;
> > -		int		ret;
> > +		struct fs_parameter	param;
> > +		char			*value;
> >  
> >  		if (!*p)
> >  			continue;
> >  
> > -		token = match_token(p, tokens, args);
> > -		ret = xfs_parse_param(token, p, args, mp,
> > -				      &dsunit, &dswidth, &iosizelog);
> > -		if (ret)
> > +		param.key = p;
> > +		param.type = fs_value_is_string;
> > +		param.string = NULL;
> > +		param.size = 0;
> > +
> > +		value = strchr(p, '=');
> > +		if (value) {
> > +			*value++ = 0;
> > +			param.size = strlen(value);
> > +			if (param.size > 0) {
> > +				param.string = kmemdup_nul(value,
> > +							   param.size,
> > +							   GFP_KERNEL);
> > +				if (!param.string)
> > +					return -ENOMEM;
> > +			}
> > +		}
> > +
> > +		ret = xfs_parse_param(&fc, &param);
> > +		kfree(param.string);
> > +		if (ret < 0)
> >  			return ret;
> >  	}
> >  
> > @@ -435,7 +459,8 @@ xfs_parseargs(
> >  		return -EINVAL;
> >  	}
> >  
> > -	if ((mp->m_flags & XFS_MOUNT_NOALIGN) && (dsunit || dswidth)) {
> > +	if ((mp->m_flags & XFS_MOUNT_NOALIGN) &&
> > +	    (ctx->dsunit || ctx->dswidth)) {
> >  		xfs_warn(mp,
> >  	"sunit and swidth options incompatible with the noalign
> > option");
> >  		return -EINVAL;
> > @@ -448,28 +473,28 @@ xfs_parseargs(
> >  	}
> >  #endif
> >  
> > -	if ((dsunit && !dswidth) || (!dsunit && dswidth)) {
> > +	if ((ctx->dsunit && !ctx->dswidth) || (!ctx->dsunit && ctx-
> > >dswidth)) {
> >  		xfs_warn(mp, "sunit and swidth must be specified
> > together");
> >  		return -EINVAL;
> >  	}
> >  
> > -	if (dsunit && (dswidth % dsunit != 0)) {
> > +	if (ctx->dsunit && (ctx->dswidth % ctx->dsunit != 0)) {
> >  		xfs_warn(mp,
> >  	"stripe width (%d) must be a multiple of the stripe unit (%d)",
> > -			dswidth, dsunit);
> > +			ctx->dswidth, ctx->dsunit);
> >  		return -EINVAL;
> >  	}
> >  
> >  done:
> > -	if (dsunit && !(mp->m_flags & XFS_MOUNT_NOALIGN)) {
> > +	if (ctx->dsunit && !(mp->m_flags & XFS_MOUNT_NOALIGN)) {
> >  		/*
> >  		 * At this point the superblock has not been read
> >  		 * in, therefore we do not know the block size.
> >  		 * Before the mount call ends we will convert
> >  		 * these to FSBs.
> >  		 */
> > -		mp->m_dalign = dsunit;
> > -		mp->m_swidth = dswidth;
> > +		mp->m_dalign = ctx->dsunit;
> > +		mp->m_swidth = ctx->dswidth;
> >  	}
> >  
> >  	if (mp->m_logbufs != -1 &&
> > @@ -491,18 +516,18 @@ xfs_parseargs(
> >  		return -EINVAL;
> >  	}
> >  
> > -	if (iosizelog) {
> > -		if (iosizelog > XFS_MAX_IO_LOG ||
> > -		    iosizelog < XFS_MIN_IO_LOG) {
> > +	if (ctx->iosizelog) {
> > +		if (ctx->iosizelog > XFS_MAX_IO_LOG ||
> > +		    ctx->iosizelog < XFS_MIN_IO_LOG) {
> >  			xfs_warn(mp, "invalid log iosize: %d [not %d-
> > %d]",
> > -				iosizelog, XFS_MIN_IO_LOG,
> > +				ctx->iosizelog, XFS_MIN_IO_LOG,
> >  				XFS_MAX_IO_LOG);
> >  			return -EINVAL;
> >  		}
> >  
> >  		mp->m_flags |= XFS_MOUNT_DFLT_IOSIZE;
> > -		mp->m_readio_log = iosizelog;
> > -		mp->m_writeio_log = iosizelog;
> > +		mp->m_readio_log = ctx->iosizelog;
> > +		mp->m_writeio_log = ctx->iosizelog;
> >  	}
> >  
> >  	return 0;
> > 

Re: [REPOST PATCH v3 06/16] xfs: mount-api - make xfs_parse_param() take context .parse_param() args

[Brian Foster]

On Wed, Sep 25, 2019 at 08:20:25AM +0800, Ian Kent wrote:
> On Tue, 2019-09-24 at 10:37 -0400, Brian Foster wrote:
> > On Tue, Sep 24, 2019 at 09:22:33PM +0800, Ian Kent wrote:
> > > Make xfs_parse_param() take arguments of the fs context operation
> > > .parse_param() in preparation for switching to use the file system
> > > mount context for mount.
> > > 
> > > The function fc_parse() only uses the file system context (fc here)
> > > when calling log macros warnf() and invalf() which in turn check
> > > only the fc->log field to determine if the message should be saved
> > > to a context buffer (for later retrival by userspace) or logged
> > > using printk().
> > > 
> > > Also the temporary function match_kstrtoint() is now unused, remove
> > > it.
> > > 
> > > Signed-off-by: Ian Kent <raven@themaw.net>
> > > ---
> > >  fs/xfs/xfs_super.c |  137 +++++++++++++++++++++++++++++++---------
> > > ------------
> > >  1 file changed, 81 insertions(+), 56 deletions(-)
> > > 
> > > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > > index b04aebab69ab..6792d46fa0be 100644
> > > --- a/fs/xfs/xfs_super.c
> > > +++ b/fs/xfs/xfs_super.c
> > > @@ -191,57 +191,60 @@ suffix_kstrtoint(const char *s, unsigned int
> > > base, int *res)
> > >  	return ret;
> > >  }
> > >  
> > ...
> > >  
> > >  STATIC int
> > >  xfs_parse_param(
> > ...
> > > -	switch (token) {
> > > +	opt = fs_parse(fc, &xfs_fs_parameters, param, &result);
> > > +	if (opt < 0) {
> > > +		/*
> > > +		 * If fs_parse() returns -ENOPARAM and the parameter
> > > +		 * is "source" the VFS needs to handle this option
> > > +		 * in order to boot otherwise use the default case
> > > +		 * below to handle invalid options.
> > > +		 */
> > > +		if (opt != -ENOPARAM ||
> > > +		    strcmp(param->key, "source") == 0)
> > > +			return opt;
> > 
> > Same question as before on this bit..
> 
> Your comment was:
> Why is this not something that is handled in core mount-api code? Every
> filesystem needs this logic in order to be a rootfs..?
> 
> I looked at the VFS code and was tempted to change it but it's all too
> easy to prevent the system from booting.
> 

Ok, so I'm not terribly familiar with the core mount code in the first
place. Can you elaborate a bit on the where the whole "source" thing
comes from and why/how it's necessary to boot?

> The way the VFS looks to me it needs to give the file system a chance
> to handle the "source" option, if the file system ->parse_param()
> doesn't handle the option it "must" return -ENOPARAM so the VFS will
> test for and handle the "source" option.
> 

Do any existing filesystems handle this option? By handle, I mean
actually have to make some change, set some option, etc.

> Having returned -ENOPARAM either the option is "source" or it's a
> real unknown option.
> 
> The choices are:
> 1) If it is the "source" option we will get a false positive unknown
> parameter message logged by our ->parse_param().
> 2) if it isn't the "source" option we will get an unknown parameter
> message from our ->parse_param() and an additional inconsistent
> format unknown parameter message from the VFS.
> 3) Check for the "source" parameter in our ->parse_param() and
> return without issuing a message so the VFS can handle the option,
> no duplicate message and no inconsistent logging.
> 

Hmm.. so we definitely don't want spurious unknown parameter messages,
but I don't see how that leaves #3 as the only other option.

Is param-key already filled in at that point? If so, couldn't we set a
flag or something on the context structure to signal that we don't care
about the source option, so let the vfs handle it however it needs to?
If not, another option could be to define a helper function or something
that the fs can call to determine whether an -ENOPARAM key is some
global option to be handled by a higher layer and so to not throw a
warning or whatever. That has the same logic as this patch, but is still
better than open-coding "source" key checks all over the place IMO. 

BTW, this all implies there is some reason for an fs to handle the
"source" option, so what happens if one actually does? ISTM the
->parse_param() callout would return 0 and vfs_parse_fs_param() would
skip its own update of fc->source. Hm?

Brian

> Suggestions on how to handle this better, a VFS patch perhaps?
> Suggestions David, Al?
> 
> > ...
> > > @@ -373,10 +374,16 @@ xfs_parseargs(
> > >  {
> > >  	const struct super_block *sb = mp->m_super;
> > >  	char			*p;
> > > -	substring_t		args[MAX_OPT_ARGS];
> > > -	int			dsunit = 0;
> > > -	int			dswidth = 0;
> > > -	uint8_t			iosizelog = 0;
> > > +	struct fs_context	fc;
> > > +	struct xfs_fs_context	context;
> > > +	struct xfs_fs_context	*ctx;
> > > +	int			ret;
> > > +
> > > +	memset(&fc, 0, sizeof(fc));
> > > +	memset(&context, 0, sizeof(context));
> > > +	fc.fs_private = &context;
> > > +	ctx = &context;
> > 
> > I think you mentioned this ctx/context pattern would be removed from
> > v2..?
> 
> Except that, to minimise code churn the ctx variable is needed
> because it will be used in the end result.
> 
> I don't much like prefixing those references with &context even
> if some happen to go away later on, the additional local variable
> is clearer to read and provides usage consistency for the reader
> over the changes.
> 
> Ian
> > 
> > Brian
> > 
> > > +	fc.s_fs_info = mp;
> > >  
> > >  	/*
> > >  	 * set up the mount name first so all the errors will refer to
> > > the
> > > @@ -413,16 +420,33 @@ xfs_parseargs(
> > >  		goto done;
> > >  
> > >  	while ((p = strsep(&options, ",")) != NULL) {
> > > -		int		token;
> > > -		int		ret;
> > > +		struct fs_parameter	param;
> > > +		char			*value;
> > >  
> > >  		if (!*p)
> > >  			continue;
> > >  
> > > -		token = match_token(p, tokens, args);
> > > -		ret = xfs_parse_param(token, p, args, mp,
> > > -				      &dsunit, &dswidth, &iosizelog);
> > > -		if (ret)
> > > +		param.key = p;
> > > +		param.type = fs_value_is_string;
> > > +		param.string = NULL;
> > > +		param.size = 0;
> > > +
> > > +		value = strchr(p, '=');
> > > +		if (value) {
> > > +			*value++ = 0;
> > > +			param.size = strlen(value);
> > > +			if (param.size > 0) {
> > > +				param.string = kmemdup_nul(value,
> > > +							   param.size,
> > > +							   GFP_KERNEL);
> > > +				if (!param.string)
> > > +					return -ENOMEM;
> > > +			}
> > > +		}
> > > +
> > > +		ret = xfs_parse_param(&fc, &param);
> > > +		kfree(param.string);
> > > +		if (ret < 0)
> > >  			return ret;
> > >  	}
> > >  
> > > @@ -435,7 +459,8 @@ xfs_parseargs(
> > >  		return -EINVAL;
> > >  	}
> > >  
> > > -	if ((mp->m_flags & XFS_MOUNT_NOALIGN) && (dsunit || dswidth)) {
> > > +	if ((mp->m_flags & XFS_MOUNT_NOALIGN) &&
> > > +	    (ctx->dsunit || ctx->dswidth)) {
> > >  		xfs_warn(mp,
> > >  	"sunit and swidth options incompatible with the noalign
> > > option");
> > >  		return -EINVAL;
> > > @@ -448,28 +473,28 @@ xfs_parseargs(
> > >  	}
> > >  #endif
> > >  
> > > -	if ((dsunit && !dswidth) || (!dsunit && dswidth)) {
> > > +	if ((ctx->dsunit && !ctx->dswidth) || (!ctx->dsunit && ctx-
> > > >dswidth)) {
> > >  		xfs_warn(mp, "sunit and swidth must be specified
> > > together");
> > >  		return -EINVAL;
> > >  	}
> > >  
> > > -	if (dsunit && (dswidth % dsunit != 0)) {
> > > +	if (ctx->dsunit && (ctx->dswidth % ctx->dsunit != 0)) {
> > >  		xfs_warn(mp,
> > >  	"stripe width (%d) must be a multiple of the stripe unit (%d)",
> > > -			dswidth, dsunit);
> > > +			ctx->dswidth, ctx->dsunit);
> > >  		return -EINVAL;
> > >  	}
> > >  
> > >  done:
> > > -	if (dsunit && !(mp->m_flags & XFS_MOUNT_NOALIGN)) {
> > > +	if (ctx->dsunit && !(mp->m_flags & XFS_MOUNT_NOALIGN)) {
> > >  		/*
> > >  		 * At this point the superblock has not been read
> > >  		 * in, therefore we do not know the block size.
> > >  		 * Before the mount call ends we will convert
> > >  		 * these to FSBs.
> > >  		 */
> > > -		mp->m_dalign = dsunit;
> > > -		mp->m_swidth = dswidth;
> > > +		mp->m_dalign = ctx->dsunit;
> > > +		mp->m_swidth = ctx->dswidth;
> > >  	}
> > >  
> > >  	if (mp->m_logbufs != -1 &&
> > > @@ -491,18 +516,18 @@ xfs_parseargs(
> > >  		return -EINVAL;
> > >  	}
> > >  
> > > -	if (iosizelog) {
> > > -		if (iosizelog > XFS_MAX_IO_LOG ||
> > > -		    iosizelog < XFS_MIN_IO_LOG) {
> > > +	if (ctx->iosizelog) {
> > > +		if (ctx->iosizelog > XFS_MAX_IO_LOG ||
> > > +		    ctx->iosizelog < XFS_MIN_IO_LOG) {
> > >  			xfs_warn(mp, "invalid log iosize: %d [not %d-
> > > %d]",
> > > -				iosizelog, XFS_MIN_IO_LOG,
> > > +				ctx->iosizelog, XFS_MIN_IO_LOG,
> > >  				XFS_MAX_IO_LOG);
> > >  			return -EINVAL;
> > >  		}
> > >  
> > >  		mp->m_flags |= XFS_MOUNT_DFLT_IOSIZE;
> > > -		mp->m_readio_log = iosizelog;
> > > -		mp->m_writeio_log = iosizelog;
> > > +		mp->m_readio_log = ctx->iosizelog;
> > > +		mp->m_writeio_log = ctx->iosizelog;
> > >  	}
> > >  
> > >  	return 0;
> > > 
> 

Re: [REPOST PATCH v3 06/16] xfs: mount-api - make xfs_parse_param() take context .parse_param() args

[Ian Kent]

On Wed, 2019-09-25 at 10:33 -0400, Brian Foster wrote:
> On Wed, Sep 25, 2019 at 08:20:25AM +0800, Ian Kent wrote:
> > On Tue, 2019-09-24 at 10:37 -0400, Brian Foster wrote:
> > > On Tue, Sep 24, 2019 at 09:22:33PM +0800, Ian Kent wrote:
> > > > Make xfs_parse_param() take arguments of the fs context
> > > > operation
> > > > .parse_param() in preparation for switching to use the file
> > > > system
> > > > mount context for mount.
> > > > 
> > > > The function fc_parse() only uses the file system context (fc
> > > > here)
> > > > when calling log macros warnf() and invalf() which in turn
> > > > check
> > > > only the fc->log field to determine if the message should be
> > > > saved
> > > > to a context buffer (for later retrival by userspace) or logged
> > > > using printk().
> > > > 
> > > > Also the temporary function match_kstrtoint() is now unused,
> > > > remove
> > > > it.
> > > > 
> > > > Signed-off-by: Ian Kent <raven@themaw.net>
> > > > ---
> > > >  fs/xfs/xfs_super.c |  137 +++++++++++++++++++++++++++++++-----
> > > > ----
> > > > ------------
> > > >  1 file changed, 81 insertions(+), 56 deletions(-)
> > > > 
> > > > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > > > index b04aebab69ab..6792d46fa0be 100644
> > > > --- a/fs/xfs/xfs_super.c
> > > > +++ b/fs/xfs/xfs_super.c
> > > > @@ -191,57 +191,60 @@ suffix_kstrtoint(const char *s, unsigned
> > > > int
> > > > base, int *res)
> > > >  	return ret;
> > > >  }
> > > >  
> > > ...
> > > >  
> > > >  STATIC int
> > > >  xfs_parse_param(
> > > ...
> > > > -	switch (token) {
> > > > +	opt = fs_parse(fc, &xfs_fs_parameters, param, &result);
> > > > +	if (opt < 0) {
> > > > +		/*
> > > > +		 * If fs_parse() returns -ENOPARAM and the
> > > > parameter
> > > > +		 * is "source" the VFS needs to handle this
> > > > option
> > > > +		 * in order to boot otherwise use the default
> > > > case
> > > > +		 * below to handle invalid options.
> > > > +		 */
> > > > +		if (opt != -ENOPARAM ||
> > > > +		    strcmp(param->key, "source") == 0)
> > > > +			return opt;
> > > 
> > > Same question as before on this bit..
> > 
> > Your comment was:
> > Why is this not something that is handled in core mount-api code?
> > Every
> > filesystem needs this logic in order to be a rootfs..?
> > 
> > I looked at the VFS code and was tempted to change it but it's all
> > too
> > easy to prevent the system from booting.
> > 
> 
> Ok, so I'm not terribly familiar with the core mount code in the
> first
> place. Can you elaborate a bit on the where the whole "source" thing
> comes from and why/how it's necessary to boot?

Your not alone.

I've pondered over the VFS mount code fairly often over the years
and I've not seen it before either.

About all I know is it's needed for rootfs, so I guess it's needed
to resolve the boot device when no file system is yet mounted and
a normal path walk can't be done.

> 
> > The way the VFS looks to me it needs to give the file system a
> > chance
> > to handle the "source" option, if the file system ->parse_param()
> > doesn't handle the option it "must" return -ENOPARAM so the VFS
> > will
> > test for and handle the "source" option.
> > 
> 
> Do any existing filesystems handle this option? By handle, I mean
> actually have to make some change, set some option, etc.

AFAIK very few file systems handle the option (and I suspect
virtually none until perhaps recently) as David mentioned a
couple that do yesterday on IRC.

Apparently there are a couple of file systems that want to
take a non-standard mount source and resolve it to a kernel
usable source for mounting.

I'm really not familiar with the details either so I'm making
assumptions which might not be correct.

> 
> > Having returned -ENOPARAM either the option is "source" or it's a
> > real unknown option.
> > 
> > The choices are:
> > 1) If it is the "source" option we will get a false positive
> > unknown
> > parameter message logged by our ->parse_param().
> > 2) if it isn't the "source" option we will get an unknown parameter
> > message from our ->parse_param() and an additional inconsistent
> > format unknown parameter message from the VFS.
> > 3) Check for the "source" parameter in our ->parse_param() and
> > return without issuing a message so the VFS can handle the option,
> > no duplicate message and no inconsistent logging.
> > 
> 
> Hmm.. so we definitely don't want spurious unknown parameter
> messages,
> but I don't see how that leaves #3 as the only other option.

My reading of the the code (the mount-api code) is that if the
.parse_param() method is defined it gives it a chance to handle
the parameter whatever it is. Then .parase_param() must return
-ENOPARAM for the VFS parameter handling function to then check
for the "source" parameter, handle it or issue an unknown parameter
message.

So, in order to return something other than -ENOPARAM, thereby
avoiding the extra message, that must be done only if the parameter
is not "source".

The problem is most file systems won't handle that parameter and
can reasonably be expected to issue an error message when they
encounter it but there are a couple that will want to handle it
so .parse_param() must be given a chance to do so.

So it's not a problem specific to xfs.

> 
> Is param-key already filled in at that point? If so, couldn't we set
> a
> flag or something on the context structure to signal that we don't
> care
> about the source option, so let the vfs handle it however it needs
> to?

Maybe.

> If not, another option could be to define a helper function or
> something
> that the fs can call to determine whether an -ENOPARAM key is some
> global option to be handled by a higher layer and so to not throw a
> warning or whatever. That has the same logic as this patch, but is
> still
> better than open-coding "source" key checks all over the place IMO. 

Maybe an additional fs_context_purpose needs to be defined, maybe
FS_CONTEXT_FOR_ROOTFS for example.

That's probably the cleanest way to handle it, not sure it would
properly cover the cases though.

That wouldn't be an entirely trivial change so David and Al would
likely need to get involved and Linus would need to be willing to
accept it.

> 
> BTW, this all implies there is some reason for an fs to handle the
> "source" option, so what happens if one actually does? ISTM the
> ->parse_param() callout would return 0 and vfs_parse_fs_param() would
> skip its own update of fc->source. Hm?

As I understand it that's not a problem because the file system
will need to have converted the parameter value to some "source"
value usable by the kernel.

> 
> Brian
> 
> > Suggestions on how to handle this better, a VFS patch perhaps?
> > Suggestions David, Al?
> > 
> > > ...
> > > > @@ -373,10 +374,16 @@ xfs_parseargs(
> > > >  {
> > > >  	const struct super_block *sb = mp->m_super;
> > > >  	char			*p;
> > > > -	substring_t		args[MAX_OPT_ARGS];
> > > > -	int			dsunit = 0;
> > > > -	int			dswidth = 0;
> > > > -	uint8_t			iosizelog = 0;
> > > > +	struct fs_context	fc;
> > > > +	struct xfs_fs_context	context;
> > > > +	struct xfs_fs_context	*ctx;
> > > > +	int			ret;
> > > > +
> > > > +	memset(&fc, 0, sizeof(fc));
> > > > +	memset(&context, 0, sizeof(context));
> > > > +	fc.fs_private = &context;
> > > > +	ctx = &context;
> > > 
> > > I think you mentioned this ctx/context pattern would be removed
> > > from
> > > v2..?
> > 
> > Except that, to minimise code churn the ctx variable is needed
> > because it will be used in the end result.
> > 
> > I don't much like prefixing those references with &context even
> > if some happen to go away later on, the additional local variable
> > is clearer to read and provides usage consistency for the reader
> > over the changes.
> > 
> > Ian
> > > Brian
> > > 
> > > > +	fc.s_fs_info = mp;
> > > >  
> > > >  	/*
> > > >  	 * set up the mount name first so all the errors will
> > > > refer to
> > > > the
> > > > @@ -413,16 +420,33 @@ xfs_parseargs(
> > > >  		goto done;
> > > >  
> > > >  	while ((p = strsep(&options, ",")) != NULL) {
> > > > -		int		token;
> > > > -		int		ret;
> > > > +		struct fs_parameter	param;
> > > > +		char			*value;
> > > >  
> > > >  		if (!*p)
> > > >  			continue;
> > > >  
> > > > -		token = match_token(p, tokens, args);
> > > > -		ret = xfs_parse_param(token, p, args, mp,
> > > > -				      &dsunit, &dswidth,
> > > > &iosizelog);
> > > > -		if (ret)
> > > > +		param.key = p;
> > > > +		param.type = fs_value_is_string;
> > > > +		param.string = NULL;
> > > > +		param.size = 0;
> > > > +
> > > > +		value = strchr(p, '=');
> > > > +		if (value) {
> > > > +			*value++ = 0;
> > > > +			param.size = strlen(value);
> > > > +			if (param.size > 0) {
> > > > +				param.string =
> > > > kmemdup_nul(value,
> > > > +							   para
> > > > m.size,
> > > > +							   GFP_
> > > > KERNEL);
> > > > +				if (!param.string)
> > > > +					return -ENOMEM;
> > > > +			}
> > > > +		}
> > > > +
> > > > +		ret = xfs_parse_param(&fc, &param);
> > > > +		kfree(param.string);
> > > > +		if (ret < 0)
> > > >  			return ret;
> > > >  	}
> > > >  
> > > > @@ -435,7 +459,8 @@ xfs_parseargs(
> > > >  		return -EINVAL;
> > > >  	}
> > > >  
> > > > -	if ((mp->m_flags & XFS_MOUNT_NOALIGN) && (dsunit ||
> > > > dswidth)) {
> > > > +	if ((mp->m_flags & XFS_MOUNT_NOALIGN) &&
> > > > +	    (ctx->dsunit || ctx->dswidth)) {
> > > >  		xfs_warn(mp,
> > > >  	"sunit and swidth options incompatible with the noalign
> > > > option");
> > > >  		return -EINVAL;
> > > > @@ -448,28 +473,28 @@ xfs_parseargs(
> > > >  	}
> > > >  #endif
> > > >  
> > > > -	if ((dsunit && !dswidth) || (!dsunit && dswidth)) {
> > > > +	if ((ctx->dsunit && !ctx->dswidth) || (!ctx->dsunit &&
> > > > ctx-
> > > > > dswidth)) {
> > > >  		xfs_warn(mp, "sunit and swidth must be
> > > > specified
> > > > together");
> > > >  		return -EINVAL;
> > > >  	}
> > > >  
> > > > -	if (dsunit && (dswidth % dsunit != 0)) {
> > > > +	if (ctx->dsunit && (ctx->dswidth % ctx->dsunit != 0)) {
> > > >  		xfs_warn(mp,
> > > >  	"stripe width (%d) must be a multiple of the stripe
> > > > unit (%d)",
> > > > -			dswidth, dsunit);
> > > > +			ctx->dswidth, ctx->dsunit);
> > > >  		return -EINVAL;
> > > >  	}
> > > >  
> > > >  done:
> > > > -	if (dsunit && !(mp->m_flags & XFS_MOUNT_NOALIGN)) {
> > > > +	if (ctx->dsunit && !(mp->m_flags & XFS_MOUNT_NOALIGN))
> > > > {
> > > >  		/*
> > > >  		 * At this point the superblock has not been
> > > > read
> > > >  		 * in, therefore we do not know the block size.
> > > >  		 * Before the mount call ends we will convert
> > > >  		 * these to FSBs.
> > > >  		 */
> > > > -		mp->m_dalign = dsunit;
> > > > -		mp->m_swidth = dswidth;
> > > > +		mp->m_dalign = ctx->dsunit;
> > > > +		mp->m_swidth = ctx->dswidth;
> > > >  	}
> > > >  
> > > >  	if (mp->m_logbufs != -1 &&
> > > > @@ -491,18 +516,18 @@ xfs_parseargs(
> > > >  		return -EINVAL;
> > > >  	}
> > > >  
> > > > -	if (iosizelog) {
> > > > -		if (iosizelog > XFS_MAX_IO_LOG ||
> > > > -		    iosizelog < XFS_MIN_IO_LOG) {
> > > > +	if (ctx->iosizelog) {
> > > > +		if (ctx->iosizelog > XFS_MAX_IO_LOG ||
> > > > +		    ctx->iosizelog < XFS_MIN_IO_LOG) {
> > > >  			xfs_warn(mp, "invalid log iosize: %d
> > > > [not %d-
> > > > %d]",
> > > > -				iosizelog, XFS_MIN_IO_LOG,
> > > > +				ctx->iosizelog, XFS_MIN_IO_LOG,
> > > >  				XFS_MAX_IO_LOG);
> > > >  			return -EINVAL;
> > > >  		}
> > > >  
> > > >  		mp->m_flags |= XFS_MOUNT_DFLT_IOSIZE;
> > > > -		mp->m_readio_log = iosizelog;
> > > > -		mp->m_writeio_log = iosizelog;
> > > > +		mp->m_readio_log = ctx->iosizelog;
> > > > +		mp->m_writeio_log = ctx->iosizelog;
> > > >  	}
> > > >  
> > > >  	return 0;
> > > > 

Re: [REPOST PATCH v3 06/16] xfs: mount-api - make xfs_parse_param() take context .parse_param() args

[Al Viro]

On Thu, Sep 26, 2019 at 10:57:48AM +0800, Ian Kent wrote:

> > Ok, so I'm not terribly familiar with the core mount code in the
> > first
> > place. Can you elaborate a bit on the where the whole "source" thing
> > comes from and why/how it's necessary to boot?

device name.  And there's nothing special about boot.

> Your not alone.
> 
> I've pondered over the VFS mount code fairly often over the years
> and I've not seen it before either.
> 
> About all I know is it's needed for rootfs, so I guess it's needed
> to resolve the boot device when no file system is yet mounted and
> a normal path walk can't be done.

Bollocks.  Absolute root is ramfs or tmpfs and we do have mknod done
there.

Root filesystem is a complete red herring.

> Maybe an additional fs_context_purpose needs to be defined, maybe
> FS_CONTEXT_FOR_ROOTFS for example.

NO.  ->purpose should go away, not be extended.  And again, that
has fuck-all to do with rootfs.

> That's probably the cleanest way to handle it, not sure it would
> properly cover the cases though.
> 
> That wouldn't be an entirely trivial change so David and Al would
> likely need to get involved and Linus would need to be willing to
> accept it.

> > BTW, this all implies there is some reason for an fs to handle the
> > "source" option, so what happens if one actually does? ISTM the
> > ->parse_param() callout would return 0 and vfs_parse_fs_param() would
> > skip its own update of fc->source. Hm?
> 
> As I understand it that's not a problem because the file system
> will need to have converted the parameter value to some "source"
> value usable by the kernel.
 
For fsck sake...  It's where the first argument of mount(2) goes.
As in, "/dev/sda4" when you say mount -t xfs /dev/sda4 /mnt
Or "10.1.1.18:/srv/nfs/mirrors" in
mount -t nfs4 10.1.1.18:/srv/nfs/mirrors /home/mirrors/public \
	 -o rsize=8192,wsize=8192,proto=tcp,ro

Note that it's not in any real sense different from any other
option - its interpretation is entirely up to fs type.  The
only reason why it's separate is historical - way, way back
there had been no filesystem types and mount(2) got just
a block device pathname + mountpoint + one flag (ro/rw).
No (string) options either.  There has been a long and nasty
history, considerably older than Linux, including such things
as magical binary structures (each for its own fs type - check
how e.g.  OSF is doing that).

But accidents of calling conventions aside, device name is just
another fs option.  And device name is a misnomer - witness
what e.g. NFS is doing.  Keeping it special for new API
was pointless, so we have this in vfs_kern_mount():
        fc = fs_context_for_mount(type, flags);
        if (IS_ERR(fc))
                return ERR_CAST(fc);

        if (name)
                ret = vfs_parse_fs_string(fc, "source",
                                          name, strlen(name));
        if (!ret)
                ret = parse_monolithic_mount_data(fc, data);  
        if (!ret)
                mnt = fc_mount(fc);
        else   
                mnt = ERR_PTR(ret);

        put_fs_context(fc); 

and sys_mount() ends up passing the first argument as 'name' to
vfs_kern_mount().  That's it - nothing more to it.

Re: [REPOST PATCH v3 06/16] xfs: mount-api - make xfs_parse_param() take context .parse_param() args

[Ian Kent]

On Thu, 2019-09-26 at 10:57 +0800, Ian Kent wrote:
> On Wed, 2019-09-25 at 10:33 -0400, Brian Foster wrote:
> > On Wed, Sep 25, 2019 at 08:20:25AM +0800, Ian Kent wrote:
> > > On Tue, 2019-09-24 at 10:37 -0400, Brian Foster wrote:
> > > > On Tue, Sep 24, 2019 at 09:22:33PM +0800, Ian Kent wrote:
> > > > > Make xfs_parse_param() take arguments of the fs context
> > > > > operation
> > > > > .parse_param() in preparation for switching to use the file
> > > > > system
> > > > > mount context for mount.
> > > > > 
> > > > > The function fc_parse() only uses the file system context (fc
> > > > > here)
> > > > > when calling log macros warnf() and invalf() which in turn
> > > > > check
> > > > > only the fc->log field to determine if the message should be
> > > > > saved
> > > > > to a context buffer (for later retrival by userspace) or
> > > > > logged
> > > > > using printk().
> > > > > 
> > > > > Also the temporary function match_kstrtoint() is now unused,
> > > > > remove
> > > > > it.
> > > > > 
> > > > > Signed-off-by: Ian Kent <raven@themaw.net>
> > > > > ---
> > > > >  fs/xfs/xfs_super.c |  137 +++++++++++++++++++++++++++++++---
> > > > > --
> > > > > ----
> > > > > ------------
> > > > >  1 file changed, 81 insertions(+), 56 deletions(-)
> > > > > 
> > > > > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > > > > index b04aebab69ab..6792d46fa0be 100644
> > > > > --- a/fs/xfs/xfs_super.c
> > > > > +++ b/fs/xfs/xfs_super.c
> > > > > @@ -191,57 +191,60 @@ suffix_kstrtoint(const char *s,
> > > > > unsigned
> > > > > int
> > > > > base, int *res)
> > > > >  	return ret;
> > > > >  }
> > > > >  
> > > > ...
> > > > >  
> > > > >  STATIC int
> > > > >  xfs_parse_param(
> > > > ...
> > > > > -	switch (token) {
> > > > > +	opt = fs_parse(fc, &xfs_fs_parameters, param, &result);
> > > > > +	if (opt < 0) {
> > > > > +		/*
> > > > > +		 * If fs_parse() returns -ENOPARAM and the
> > > > > parameter
> > > > > +		 * is "source" the VFS needs to handle this
> > > > > option
> > > > > +		 * in order to boot otherwise use the default
> > > > > case
> > > > > +		 * below to handle invalid options.
> > > > > +		 */
> > > > > +		if (opt != -ENOPARAM ||
> > > > > +		    strcmp(param->key, "source") == 0)
> > > > > +			return opt;
> > > > 
> > > > Same question as before on this bit..
> > > 
> > > Your comment was:
> > > Why is this not something that is handled in core mount-api code?
> > > Every
> > > filesystem needs this logic in order to be a rootfs..?
> > > 
> > > I looked at the VFS code and was tempted to change it but it's
> > > all
> > > too
> > > easy to prevent the system from booting.
> > > 
> > 
> > Ok, so I'm not terribly familiar with the core mount code in the
> > first
> > place. Can you elaborate a bit on the where the whole "source"
> > thing
> > comes from and why/how it's necessary to boot?
> 
> Your not alone.
> 
> I've pondered over the VFS mount code fairly often over the years
> and I've not seen it before either.
> 
> About all I know is it's needed for rootfs, so I guess it's needed
> to resolve the boot device when no file system is yet mounted and
> a normal path walk can't be done.
> 
> > > The way the VFS looks to me it needs to give the file system a
> > > chance
> > > to handle the "source" option, if the file system ->parse_param()
> > > doesn't handle the option it "must" return -ENOPARAM so the VFS
> > > will
> > > test for and handle the "source" option.
> > > 
> > 
> > Do any existing filesystems handle this option? By handle, I mean
> > actually have to make some change, set some option, etc.
> 
> AFAIK very few file systems handle the option (and I suspect
> virtually none until perhaps recently) as David mentioned a
> couple that do yesterday on IRC.
> 
> Apparently there are a couple of file systems that want to
> take a non-standard mount source and resolve it to a kernel
> usable source for mounting.
> 
> I'm really not familiar with the details either so I'm making
> assumptions which might not be correct.
> 
> > > Having returned -ENOPARAM either the option is "source" or it's a
> > > real unknown option.
> > > 
> > > The choices are:
> > > 1) If it is the "source" option we will get a false positive
> > > unknown
> > > parameter message logged by our ->parse_param().
> > > 2) if it isn't the "source" option we will get an unknown
> > > parameter
> > > message from our ->parse_param() and an additional inconsistent
> > > format unknown parameter message from the VFS.
> > > 3) Check for the "source" parameter in our ->parse_param() and
> > > return without issuing a message so the VFS can handle the
> > > option,
> > > no duplicate message and no inconsistent logging.
> > > 
> > 
> > Hmm.. so we definitely don't want spurious unknown parameter
> > messages,
> > but I don't see how that leaves #3 as the only other option.
> 
> My reading of the the code (the mount-api code) is that if the
> .parse_param() method is defined it gives it a chance to handle
> the parameter whatever it is. Then .parase_param() must return
> -ENOPARAM for the VFS parameter handling function to then check
> for the "source" parameter, handle it or issue an unknown parameter
> message.
> 
> So, in order to return something other than -ENOPARAM, thereby
> avoiding the extra message, that must be done only if the parameter
> is not "source".
> 
> The problem is most file systems won't handle that parameter and
> can reasonably be expected to issue an error message when they
> encounter it but there are a couple that will want to handle it
> so .parse_param() must be given a chance to do so.
> 
> So it's not a problem specific to xfs.

But let's not forget why this is being done.

The reason is the "XFS: <message>" vs. "xfs: <message>" inconsistency
and the lack of kernel log messages if the fsxxx() system calls are
being used to perform the mount.

The other possibility is to do:
opt = fs_parse(fc, &xfs_fs_parameters, param, &result);
if (opt < 0)
	return opt;

and accept that the "default:" case that issues an error message for
an unknown parameter will never be reached and unknown parameter
messages will use fs_type ->name (lower case) for the log message.
And also accept that if the fsxxx() system calls are being used to
perform the mount parameter parsing messages won't be logged to
the kernel log at all.

The gfs2 mount-api code does this.

> 
> > Is param-key already filled in at that point? If so, couldn't we
> > set
> > a
> > flag or something on the context structure to signal that we don't
> > care
> > about the source option, so let the vfs handle it however it needs
> > to?
> 
> Maybe.
> 
> > If not, another option could be to define a helper function or
> > something
> > that the fs can call to determine whether an -ENOPARAM key is some
> > global option to be handled by a higher layer and so to not throw a
> > warning or whatever. That has the same logic as this patch, but is
> > still
> > better than open-coding "source" key checks all over the place
> > IMO. 
> 
> Maybe an additional fs_context_purpose needs to be defined, maybe
> FS_CONTEXT_FOR_ROOTFS for example.
> 
> That's probably the cleanest way to handle it, not sure it would
> properly cover the cases though.
> 
> That wouldn't be an entirely trivial change so David and Al would
> likely need to get involved and Linus would need to be willing to
> accept it.
> 
> > BTW, this all implies there is some reason for an fs to handle the
> > "source" option, so what happens if one actually does? ISTM the
> > ->parse_param() callout would return 0 and vfs_parse_fs_param()
> > would
> > skip its own update of fc->source. Hm?
> 
> As I understand it that's not a problem because the file system
> will need to have converted the parameter value to some "source"
> value usable by the kernel.
> 
> > Brian
> > 
> > > Suggestions on how to handle this better, a VFS patch perhaps?
> > > Suggestions David, Al?
> > > 
> > > > ...
> > > > > @@ -373,10 +374,16 @@ xfs_parseargs(
> > > > >  {
> > > > >  	const struct super_block *sb = mp->m_super;
> > > > >  	char			*p;
> > > > > -	substring_t		args[MAX_OPT_ARGS];
> > > > > -	int			dsunit = 0;
> > > > > -	int			dswidth = 0;
> > > > > -	uint8_t			iosizelog = 0;
> > > > > +	struct fs_context	fc;
> > > > > +	struct xfs_fs_context	context;
> > > > > +	struct xfs_fs_context	*ctx;
> > > > > +	int			ret;
> > > > > +
> > > > > +	memset(&fc, 0, sizeof(fc));
> > > > > +	memset(&context, 0, sizeof(context));
> > > > > +	fc.fs_private = &context;
> > > > > +	ctx = &context;
> > > > 
> > > > I think you mentioned this ctx/context pattern would be removed
> > > > from
> > > > v2..?
> > > 
> > > Except that, to minimise code churn the ctx variable is needed
> > > because it will be used in the end result.
> > > 
> > > I don't much like prefixing those references with &context even
> > > if some happen to go away later on, the additional local variable
> > > is clearer to read and provides usage consistency for the reader
> > > over the changes.
> > > 
> > > Ian
> > > > Brian
> > > > 
> > > > > +	fc.s_fs_info = mp;
> > > > >  
> > > > >  	/*
> > > > >  	 * set up the mount name first so all the errors will
> > > > > refer to
> > > > > the
> > > > > @@ -413,16 +420,33 @@ xfs_parseargs(
> > > > >  		goto done;
> > > > >  
> > > > >  	while ((p = strsep(&options, ",")) != NULL) {
> > > > > -		int		token;
> > > > > -		int		ret;
> > > > > +		struct fs_parameter	param;
> > > > > +		char			*value;
> > > > >  
> > > > >  		if (!*p)
> > > > >  			continue;
> > > > >  
> > > > > -		token = match_token(p, tokens, args);
> > > > > -		ret = xfs_parse_param(token, p, args, mp,
> > > > > -				      &dsunit, &dswidth,
> > > > > &iosizelog);
> > > > > -		if (ret)
> > > > > +		param.key = p;
> > > > > +		param.type = fs_value_is_string;
> > > > > +		param.string = NULL;
> > > > > +		param.size = 0;
> > > > > +
> > > > > +		value = strchr(p, '=');
> > > > > +		if (value) {
> > > > > +			*value++ = 0;
> > > > > +			param.size = strlen(value);
> > > > > +			if (param.size > 0) {
> > > > > +				param.string =
> > > > > kmemdup_nul(value,
> > > > > +							   para
> > > > > m.size,
> > > > > +							   GFP_
> > > > > KERNEL);
> > > > > +				if (!param.string)
> > > > > +					return -ENOMEM;
> > > > > +			}
> > > > > +		}
> > > > > +
> > > > > +		ret = xfs_parse_param(&fc, &param);
> > > > > +		kfree(param.string);
> > > > > +		if (ret < 0)
> > > > >  			return ret;
> > > > >  	}
> > > > >  
> > > > > @@ -435,7 +459,8 @@ xfs_parseargs(
> > > > >  		return -EINVAL;
> > > > >  	}
> > > > >  
> > > > > -	if ((mp->m_flags & XFS_MOUNT_NOALIGN) && (dsunit ||
> > > > > dswidth)) {
> > > > > +	if ((mp->m_flags & XFS_MOUNT_NOALIGN) &&
> > > > > +	    (ctx->dsunit || ctx->dswidth)) {
> > > > >  		xfs_warn(mp,
> > > > >  	"sunit and swidth options incompatible with the noalign
> > > > > option");
> > > > >  		return -EINVAL;
> > > > > @@ -448,28 +473,28 @@ xfs_parseargs(
> > > > >  	}
> > > > >  #endif
> > > > >  
> > > > > -	if ((dsunit && !dswidth) || (!dsunit && dswidth)) {
> > > > > +	if ((ctx->dsunit && !ctx->dswidth) || (!ctx->dsunit &&
> > > > > ctx-
> > > > > > dswidth)) {
> > > > >  		xfs_warn(mp, "sunit and swidth must be
> > > > > specified
> > > > > together");
> > > > >  		return -EINVAL;
> > > > >  	}
> > > > >  
> > > > > -	if (dsunit && (dswidth % dsunit != 0)) {
> > > > > +	if (ctx->dsunit && (ctx->dswidth % ctx->dsunit != 0)) {
> > > > >  		xfs_warn(mp,
> > > > >  	"stripe width (%d) must be a multiple of the stripe
> > > > > unit (%d)",
> > > > > -			dswidth, dsunit);
> > > > > +			ctx->dswidth, ctx->dsunit);
> > > > >  		return -EINVAL;
> > > > >  	}
> > > > >  
> > > > >  done:
> > > > > -	if (dsunit && !(mp->m_flags & XFS_MOUNT_NOALIGN)) {
> > > > > +	if (ctx->dsunit && !(mp->m_flags & XFS_MOUNT_NOALIGN))
> > > > > {
> > > > >  		/*
> > > > >  		 * At this point the superblock has not been
> > > > > read
> > > > >  		 * in, therefore we do not know the block size.
> > > > >  		 * Before the mount call ends we will convert
> > > > >  		 * these to FSBs.
> > > > >  		 */
> > > > > -		mp->m_dalign = dsunit;
> > > > > -		mp->m_swidth = dswidth;
> > > > > +		mp->m_dalign = ctx->dsunit;
> > > > > +		mp->m_swidth = ctx->dswidth;
> > > > >  	}
> > > > >  
> > > > >  	if (mp->m_logbufs != -1 &&
> > > > > @@ -491,18 +516,18 @@ xfs_parseargs(
> > > > >  		return -EINVAL;
> > > > >  	}
> > > > >  
> > > > > -	if (iosizelog) {
> > > > > -		if (iosizelog > XFS_MAX_IO_LOG ||
> > > > > -		    iosizelog < XFS_MIN_IO_LOG) {
> > > > > +	if (ctx->iosizelog) {
> > > > > +		if (ctx->iosizelog > XFS_MAX_IO_LOG ||
> > > > > +		    ctx->iosizelog < XFS_MIN_IO_LOG) {
> > > > >  			xfs_warn(mp, "invalid log iosize: %d
> > > > > [not %d-
> > > > > %d]",
> > > > > -				iosizelog, XFS_MIN_IO_LOG,
> > > > > +				ctx->iosizelog, XFS_MIN_IO_LOG,
> > > > >  				XFS_MAX_IO_LOG);
> > > > >  			return -EINVAL;
> > > > >  		}
> > > > >  
> > > > >  		mp->m_flags |= XFS_MOUNT_DFLT_IOSIZE;
> > > > > -		mp->m_readio_log = iosizelog;
> > > > > -		mp->m_writeio_log = iosizelog;
> > > > > +		mp->m_readio_log = ctx->iosizelog;
> > > > > +		mp->m_writeio_log = ctx->iosizelog;
> > > > >  	}
> > > > >  
> > > > >  	return 0;
> > > > > 

Re: [REPOST PATCH v3 06/16] xfs: mount-api - make xfs_parse_param() take context .parse_param() args

[Al Viro]

On Tue, Sep 24, 2019 at 09:22:33PM +0800, Ian Kent wrote:

> +	opt = fs_parse(fc, &xfs_fs_parameters, param, &result);
> +	if (opt < 0) {
> +		/*
> +		 * If fs_parse() returns -ENOPARAM and the parameter
> +		 * is "source" the VFS needs to handle this option
> +		 * in order to boot otherwise use the default case
> +		 * below to handle invalid options.
> +		 */
> +		if (opt != -ENOPARAM ||
> +		    strcmp(param->key, "source") == 0)
> +			return opt;

Just return opt; here and be done with that.  The comment is bloody
misleading - for one thing, "in order to boot" is really "in order to
mount anything", and the only reason for the kludge is that the
default for "source" (in vfs_parse_fs_param(), triggered in case
when -ENOPARAM had been returned by ->parse_param()) won't get triggered
if you insist on reporting _all_ unknown options on your own.

> +	}

>  	default:
> -		xfs_warn(mp, "unknown mount option [%s].", p);
> +		xfs_warn(mp, "unknown mount option [%s].", param->key);
>  		return -EINVAL;

... here, instead of letting the same vfs_parse_fs_param() handle
the warning.

Or you could add Opt_source for handling that, with equivalent of that
fallback (namely,
                if (param->type != fs_value_is_string)
                        return invalf(fc, "VFS: Non-string source");
                if (fc->source)
                        return invalf(fc, "VFS: Multiple sources");
                fc->source = param->string;
                param->string = NULL;
                return 0;
) done in your ->parse_param().

Re: [REPOST PATCH v3 06/16] xfs: mount-api - make xfs_parse_param() take context .parse_param() args

[Ian Kent]

On Thu, 2019-09-26 at 05:14 +0100, Al Viro wrote:
> On Tue, Sep 24, 2019 at 09:22:33PM +0800, Ian Kent wrote:
> 
> > +	opt = fs_parse(fc, &xfs_fs_parameters, param, &result);
> > +	if (opt < 0) {
> > +		/*
> > +		 * If fs_parse() returns -ENOPARAM and the parameter
> > +		 * is "source" the VFS needs to handle this option
> > +		 * in order to boot otherwise use the default case
> > +		 * below to handle invalid options.
> > +		 */
> > +		if (opt != -ENOPARAM ||
> > +		    strcmp(param->key, "source") == 0)
> > +			return opt;
> 
> Just return opt; here and be done with that.  The comment is bloody
> misleading - for one thing, "in order to boot" is really "in order to
> mount anything", and the only reason for the kludge is that the
> default for "source" (in vfs_parse_fs_param(), triggered in case
> when -ENOPARAM had been returned by ->parse_param()) won't get
> triggered
> if you insist on reporting _all_ unknown options on your own.
> 
> > +	}
> >  	default:
> > -		xfs_warn(mp, "unknown mount option [%s].", p);
> > +		xfs_warn(mp, "unknown mount option [%s].", param->key);
> >  		return -EINVAL;
> 
> ... here, instead of letting the same vfs_parse_fs_param() handle
> the warning.
> 
> Or you could add Opt_source for handling that, with equivalent of
> that
> fallback (namely,
>                 if (param->type != fs_value_is_string)
>                         return invalf(fc, "VFS: Non-string source");
>                 if (fc->source)
>                         return invalf(fc, "VFS: Multiple sources");
>                 fc->source = param->string;
>                 param->string = NULL;
>                 return 0;
> ) done in your ->parse_param().

Either of those makes sense to me.

The only other thing relevant to either case is messages not going
to the kernel log if fsconfig() is being used which could make problem
resolution more difficult.

Any objection to changing logfc() to always log to the kernel log
and save messages to the context if fc->log is non-null rather than
the either or behaviour we have now?

Ian

Re: [REPOST PATCH v3 06/16] xfs: mount-api - make xfs_parse_param() take context .parse_param() args

[Ian Kent]

On Thu, 2019-09-26 at 15:06 +0800, Ian Kent wrote:
> On Thu, 2019-09-26 at 05:14 +0100, Al Viro wrote:
> > On Tue, Sep 24, 2019 at 09:22:33PM +0800, Ian Kent wrote:
> > 
> > > +	opt = fs_parse(fc, &xfs_fs_parameters, param, &result);
> > > +	if (opt < 0) {
> > > +		/*
> > > +		 * If fs_parse() returns -ENOPARAM and the parameter
> > > +		 * is "source" the VFS needs to handle this option
> > > +		 * in order to boot otherwise use the default case
> > > +		 * below to handle invalid options.
> > > +		 */
> > > +		if (opt != -ENOPARAM ||
> > > +		    strcmp(param->key, "source") == 0)
> > > +			return opt;
> > 
> > Just return opt; here and be done with that.  The comment is bloody
> > misleading - for one thing, "in order to boot" is really "in order
> > to
> > mount anything", and the only reason for the kludge is that the
> > default for "source" (in vfs_parse_fs_param(), triggered in case
> > when -ENOPARAM had been returned by ->parse_param()) won't get
> > triggered
> > if you insist on reporting _all_ unknown options on your own.
> > 
> > > +	}
> > >  	default:
> > > -		xfs_warn(mp, "unknown mount option [%s].", p);
> > > +		xfs_warn(mp, "unknown mount option [%s].", param->key);
> > >  		return -EINVAL;
> > 
> > ... here, instead of letting the same vfs_parse_fs_param() handle
> > the warning.
> > 
> > Or you could add Opt_source for handling that, with equivalent of
> > that
> > fallback (namely,
> >                 if (param->type != fs_value_is_string)
> >                         return invalf(fc, "VFS: Non-string
> > source");
> >                 if (fc->source)
> >                         return invalf(fc, "VFS: Multiple sources");
> >                 fc->source = param->string;
> >                 param->string = NULL;
> >                 return 0;
> > ) done in your ->parse_param().
> 
> Either of those makes sense to me.
> 
> The only other thing relevant to either case is messages not going
> to the kernel log if fsconfig() is being used which could make
> problem
> resolution more difficult.
> 
> Any objection to changing logfc() to always log to the kernel log
> and save messages to the context if fc->log is non-null rather than
> the either or behaviour we have now?

Actually, forget about this.

That "e", "w" and "i" will attract inconsistent log formatting
comments.

Probably simplest to just add an xfs log macro to log to the
kernel log and also use the mount-api log macro if context ->log
is non-null.

> 
> Ian
> 

Re: [REPOST PATCH v3 06/16] xfs: mount-api - make xfs_parse_param() take context .parse_param() args

[David Howells]

Ian Kent <raven@themaw.net> wrote:

> The only other thing relevant to either case is messages not going
> to the kernel log if fsconfig() is being used which could make problem
> resolution more difficult.

Maybe we should just remove this entirely.  It's only partially capable since
I wasn't allowed to add a per-task message buffer, so it can't help you with
interior mounts - as done by NFS - or automounts.

David

[REPOST PATCH v3 07/16] xfs: mount-api - move xfs_parseargs() validation to a helper

[Ian Kent]

Move the validation code of xfs_parseargs() into a helper for later
use within the mount context methods.

Signed-off-by: Ian Kent <raven@themaw.net>
---
 fs/xfs/xfs_super.c |  148 +++++++++++++++++++++++++++++++++-------------------
 1 file changed, 94 insertions(+), 54 deletions(-)

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 6792d46fa0be..cfda58dd3822 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -356,6 +356,97 @@ xfs_parse_param(
 	return 0;
 }
 
+STATIC int
+xfs_validate_params(
+	struct xfs_mount        *mp,
+	struct xfs_fs_context   *ctx,
+	bool			nooptions)
+{
+	if (nooptions)
+		goto noopts;
+
+	/*
+	 * no recovery flag requires a read-only mount
+	 */
+	if ((mp->m_flags & XFS_MOUNT_NORECOVERY) &&
+	    !(mp->m_flags & XFS_MOUNT_RDONLY)) {
+		xfs_warn(mp, "no-recovery mounts must be read-only.");
+		return -EINVAL;
+	}
+
+	if ((mp->m_flags & XFS_MOUNT_NOALIGN) && (ctx->dsunit || ctx->dswidth)) {
+		xfs_warn(mp,
+	"sunit and swidth options incompatible with the noalign option");
+		return -EINVAL;
+	}
+
+#ifndef CONFIG_XFS_QUOTA
+	if (XFS_IS_QUOTA_RUNNING(mp)) {
+		xfs_warn(mp, "quota support not available in this kernel.");
+		return -EINVAL;
+	}
+#endif
+
+	if ((ctx->dsunit && !ctx->dswidth) || (!ctx->dsunit && ctx->dswidth)) {
+		xfs_warn(mp, "sunit and swidth must be specified together");
+		return -EINVAL;
+	}
+
+	if (ctx->dsunit && (ctx->dswidth % ctx->dsunit != 0)) {
+		xfs_warn(mp,
+	"stripe width (%d) must be a multiple of the stripe unit (%d)",
+			ctx->dswidth, ctx->dsunit);
+		return -EINVAL;
+	}
+
+noopts:
+	if (ctx->dsunit && !(mp->m_flags & XFS_MOUNT_NOALIGN)) {
+		/*
+		 * At this point the superblock has not been read
+		 * in, therefore we do not know the block size.
+		 * Before the mount call ends we will convert
+		 * these to FSBs.
+		 */
+		mp->m_dalign = ctx->dsunit;
+		mp->m_swidth = ctx->dswidth;
+	}
+
+	if (mp->m_logbufs != -1 &&
+	    mp->m_logbufs != 0 &&
+	    (mp->m_logbufs < XLOG_MIN_ICLOGS ||
+	     mp->m_logbufs > XLOG_MAX_ICLOGS)) {
+		xfs_warn(mp, "invalid logbufs value: %d [not %d-%d]",
+			mp->m_logbufs, XLOG_MIN_ICLOGS, XLOG_MAX_ICLOGS);
+		return -EINVAL;
+	}
+	if (mp->m_logbsize != -1 &&
+	    mp->m_logbsize !=  0 &&
+	    (mp->m_logbsize < XLOG_MIN_RECORD_BSIZE ||
+	     mp->m_logbsize > XLOG_MAX_RECORD_BSIZE ||
+	     !is_power_of_2(mp->m_logbsize))) {
+		xfs_warn(mp,
+			"invalid logbufsize: %d [not 16k,32k,64k,128k or 256k]",
+			mp->m_logbsize);
+		return -EINVAL;
+	}
+
+	if (ctx->iosizelog) {
+		if (ctx->iosizelog > XFS_MAX_IO_LOG ||
+		    ctx->iosizelog < XFS_MIN_IO_LOG) {
+			xfs_warn(mp, "invalid log iosize: %d [not %d-%d]",
+				ctx->iosizelog, XFS_MIN_IO_LOG,
+				XFS_MAX_IO_LOG);
+			return -EINVAL;
+		}
+
+		mp->m_flags |= XFS_MOUNT_DFLT_IOSIZE;
+		mp->m_readio_log = ctx->iosizelog;
+		mp->m_writeio_log = ctx->iosizelog;
+	}
+
+	return 0;
+}
+
 /*
  * This function fills in xfs_mount_t fields based on mount args.
  * Note: the superblock has _not_ yet been read in.
@@ -447,16 +538,7 @@ xfs_parseargs(
 		ret = xfs_parse_param(&fc, &param);
 		kfree(param.string);
 		if (ret < 0)
-			return ret;
-	}
-
-	/*
-	 * no recovery flag requires a read-only mount
-	 */
-	if ((mp->m_flags & XFS_MOUNT_NORECOVERY) &&
-	    !(mp->m_flags & XFS_MOUNT_RDONLY)) {
-		xfs_warn(mp, "no-recovery mounts must be read-only.");
-		return -EINVAL;
+			goto done;
 	}
 
 	if ((mp->m_flags & XFS_MOUNT_NOALIGN) &&
@@ -486,51 +568,9 @@ xfs_parseargs(
 	}
 
 done:
-	if (ctx->dsunit && !(mp->m_flags & XFS_MOUNT_NOALIGN)) {
-		/*
-		 * At this point the superblock has not been read
-		 * in, therefore we do not know the block size.
-		 * Before the mount call ends we will convert
-		 * these to FSBs.
-		 */
-		mp->m_dalign = ctx->dsunit;
-		mp->m_swidth = ctx->dswidth;
-	}
-
-	if (mp->m_logbufs != -1 &&
-	    mp->m_logbufs != 0 &&
-	    (mp->m_logbufs < XLOG_MIN_ICLOGS ||
-	     mp->m_logbufs > XLOG_MAX_ICLOGS)) {
-		xfs_warn(mp, "invalid logbufs value: %d [not %d-%d]",
-			mp->m_logbufs, XLOG_MIN_ICLOGS, XLOG_MAX_ICLOGS);
-		return -EINVAL;
-	}
-	if (mp->m_logbsize != -1 &&
-	    mp->m_logbsize !=  0 &&
-	    (mp->m_logbsize < XLOG_MIN_RECORD_BSIZE ||
-	     mp->m_logbsize > XLOG_MAX_RECORD_BSIZE ||
-	     !is_power_of_2(mp->m_logbsize))) {
-		xfs_warn(mp,
-			"invalid logbufsize: %d [not 16k,32k,64k,128k or 256k]",
-			mp->m_logbsize);
-		return -EINVAL;
-	}
+	ret = xfs_validate_params(mp, &context, false);
 
-	if (ctx->iosizelog) {
-		if (ctx->iosizelog > XFS_MAX_IO_LOG ||
-		    ctx->iosizelog < XFS_MIN_IO_LOG) {
-			xfs_warn(mp, "invalid log iosize: %d [not %d-%d]",
-				ctx->iosizelog, XFS_MIN_IO_LOG,
-				XFS_MAX_IO_LOG);
-			return -EINVAL;
-		}
-
-		mp->m_flags |= XFS_MOUNT_DFLT_IOSIZE;
-		mp->m_readio_log = ctx->iosizelog;
-		mp->m_writeio_log = ctx->iosizelog;
-	}
-
-	return 0;
+	return ret;
 }
 
 struct proc_xfs_info {

Re: [REPOST PATCH v3 07/16] xfs: mount-api - move xfs_parseargs() validation to a helper

[Brian Foster]

On Tue, Sep 24, 2019 at 09:22:38PM +0800, Ian Kent wrote:
> Move the validation code of xfs_parseargs() into a helper for later
> use within the mount context methods.
> 
> Signed-off-by: Ian Kent <raven@themaw.net>
> ---
>  fs/xfs/xfs_super.c |  148 +++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 94 insertions(+), 54 deletions(-)
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 6792d46fa0be..cfda58dd3822 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -356,6 +356,97 @@ xfs_parse_param(
>  	return 0;
>  }
>  
> +STATIC int
> +xfs_validate_params(
> +	struct xfs_mount        *mp,
> +	struct xfs_fs_context   *ctx,
> +	bool			nooptions)
> +{
> +	if (nooptions)
> +		goto noopts;
> +
> +	/*
> +	 * no recovery flag requires a read-only mount
> +	 */
> +	if ((mp->m_flags & XFS_MOUNT_NORECOVERY) &&
> +	    !(mp->m_flags & XFS_MOUNT_RDONLY)) {
> +		xfs_warn(mp, "no-recovery mounts must be read-only.");
> +		return -EINVAL;
> +	}
> +
> +	if ((mp->m_flags & XFS_MOUNT_NOALIGN) && (ctx->dsunit || ctx->dswidth)) {

Long line ^.

> +		xfs_warn(mp,
> +	"sunit and swidth options incompatible with the noalign option");
> +		return -EINVAL;
> +	}
> +
...
> @@ -447,16 +538,7 @@ xfs_parseargs(
>  		ret = xfs_parse_param(&fc, &param);
>  		kfree(param.string);
>  		if (ret < 0)
> -			return ret;
> -	}
> -
> -	/*
> -	 * no recovery flag requires a read-only mount
> -	 */
> -	if ((mp->m_flags & XFS_MOUNT_NORECOVERY) &&
> -	    !(mp->m_flags & XFS_MOUNT_RDONLY)) {
> -		xfs_warn(mp, "no-recovery mounts must be read-only.");
> -		return -EINVAL;
> +			goto done;

Isn't this supposed to just return the error?

Brian

>  	}
>  
>  	if ((mp->m_flags & XFS_MOUNT_NOALIGN) &&
> @@ -486,51 +568,9 @@ xfs_parseargs(
>  	}
>  
>  done:
> -	if (ctx->dsunit && !(mp->m_flags & XFS_MOUNT_NOALIGN)) {
> -		/*
> -		 * At this point the superblock has not been read
> -		 * in, therefore we do not know the block size.
> -		 * Before the mount call ends we will convert
> -		 * these to FSBs.
> -		 */
> -		mp->m_dalign = ctx->dsunit;
> -		mp->m_swidth = ctx->dswidth;
> -	}
> -
> -	if (mp->m_logbufs != -1 &&
> -	    mp->m_logbufs != 0 &&
> -	    (mp->m_logbufs < XLOG_MIN_ICLOGS ||
> -	     mp->m_logbufs > XLOG_MAX_ICLOGS)) {
> -		xfs_warn(mp, "invalid logbufs value: %d [not %d-%d]",
> -			mp->m_logbufs, XLOG_MIN_ICLOGS, XLOG_MAX_ICLOGS);
> -		return -EINVAL;
> -	}
> -	if (mp->m_logbsize != -1 &&
> -	    mp->m_logbsize !=  0 &&
> -	    (mp->m_logbsize < XLOG_MIN_RECORD_BSIZE ||
> -	     mp->m_logbsize > XLOG_MAX_RECORD_BSIZE ||
> -	     !is_power_of_2(mp->m_logbsize))) {
> -		xfs_warn(mp,
> -			"invalid logbufsize: %d [not 16k,32k,64k,128k or 256k]",
> -			mp->m_logbsize);
> -		return -EINVAL;
> -	}
> +	ret = xfs_validate_params(mp, &context, false);
>  
> -	if (ctx->iosizelog) {
> -		if (ctx->iosizelog > XFS_MAX_IO_LOG ||
> -		    ctx->iosizelog < XFS_MIN_IO_LOG) {
> -			xfs_warn(mp, "invalid log iosize: %d [not %d-%d]",
> -				ctx->iosizelog, XFS_MIN_IO_LOG,
> -				XFS_MAX_IO_LOG);
> -			return -EINVAL;
> -		}
> -
> -		mp->m_flags |= XFS_MOUNT_DFLT_IOSIZE;
> -		mp->m_readio_log = ctx->iosizelog;
> -		mp->m_writeio_log = ctx->iosizelog;
> -	}
> -
> -	return 0;
> +	return ret;
>  }
>  
>  struct proc_xfs_info {
> 

Re: [REPOST PATCH v3 07/16] xfs: mount-api - move xfs_parseargs() validation to a helper

[Ian Kent]

On Tue, 2019-09-24 at 10:37 -0400, Brian Foster wrote:
> On Tue, Sep 24, 2019 at 09:22:38PM +0800, Ian Kent wrote:
> > Move the validation code of xfs_parseargs() into a helper for later
> > use within the mount context methods.
> > 
> > Signed-off-by: Ian Kent <raven@themaw.net>
> > ---
> >  fs/xfs/xfs_super.c |  148 +++++++++++++++++++++++++++++++++-------
> > ------------
> >  1 file changed, 94 insertions(+), 54 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > index 6792d46fa0be..cfda58dd3822 100644
> > --- a/fs/xfs/xfs_super.c
> > +++ b/fs/xfs/xfs_super.c
> > @@ -356,6 +356,97 @@ xfs_parse_param(
> >  	return 0;
> >  }
> >  
> > +STATIC int
> > +xfs_validate_params(
> > +	struct xfs_mount        *mp,
> > +	struct xfs_fs_context   *ctx,
> > +	bool			nooptions)
> > +{
> > +	if (nooptions)
> > +		goto noopts;
> > +
> > +	/*
> > +	 * no recovery flag requires a read-only mount
> > +	 */
> > +	if ((mp->m_flags & XFS_MOUNT_NORECOVERY) &&
> > +	    !(mp->m_flags & XFS_MOUNT_RDONLY)) {
> > +		xfs_warn(mp, "no-recovery mounts must be read-only.");
> > +		return -EINVAL;
> > +	}
> > +
> > +	if ((mp->m_flags & XFS_MOUNT_NOALIGN) && (ctx->dsunit || ctx-
> > >dswidth)) {
> 
> Long line ^.

Oh, I thought I fixed those long lines.
I'll sort that out.

> 
> > +		xfs_warn(mp,
> > +	"sunit and swidth options incompatible with the noalign
> > option");
> > +		return -EINVAL;
> > +	}
> > +
> ...
> > @@ -447,16 +538,7 @@ xfs_parseargs(
> >  		ret = xfs_parse_param(&fc, &param);
> >  		kfree(param.string);
> >  		if (ret < 0)
> > -			return ret;
> > -	}
> > -
> > -	/*
> > -	 * no recovery flag requires a read-only mount
> > -	 */
> > -	if ((mp->m_flags & XFS_MOUNT_NORECOVERY) &&
> > -	    !(mp->m_flags & XFS_MOUNT_RDONLY)) {
> > -		xfs_warn(mp, "no-recovery mounts must be read-only.");
> > -		return -EINVAL;
> > +			goto done;
> 
> Isn't this supposed to just return the error?

Yes, I think your right ... I'll fix that too.

Ian
> 
> Brian
> 
> >  	}
> >  
> >  	if ((mp->m_flags & XFS_MOUNT_NOALIGN) &&
> > @@ -486,51 +568,9 @@ xfs_parseargs(
> >  	}
> >  
> >  done:
> > -	if (ctx->dsunit && !(mp->m_flags & XFS_MOUNT_NOALIGN)) {
> > -		/*
> > -		 * At this point the superblock has not been read
> > -		 * in, therefore we do not know the block size.
> > -		 * Before the mount call ends we will convert
> > -		 * these to FSBs.
> > -		 */
> > -		mp->m_dalign = ctx->dsunit;
> > -		mp->m_swidth = ctx->dswidth;
> > -	}
> > -
> > -	if (mp->m_logbufs != -1 &&
> > -	    mp->m_logbufs != 0 &&
> > -	    (mp->m_logbufs < XLOG_MIN_ICLOGS ||
> > -	     mp->m_logbufs > XLOG_MAX_ICLOGS)) {
> > -		xfs_warn(mp, "invalid logbufs value: %d [not %d-%d]",
> > -			mp->m_logbufs, XLOG_MIN_ICLOGS,
> > XLOG_MAX_ICLOGS);
> > -		return -EINVAL;
> > -	}
> > -	if (mp->m_logbsize != -1 &&
> > -	    mp->m_logbsize !=  0 &&
> > -	    (mp->m_logbsize < XLOG_MIN_RECORD_BSIZE ||
> > -	     mp->m_logbsize > XLOG_MAX_RECORD_BSIZE ||
> > -	     !is_power_of_2(mp->m_logbsize))) {
> > -		xfs_warn(mp,
> > -			"invalid logbufsize: %d [not 16k,32k,64k,128k
> > or 256k]",
> > -			mp->m_logbsize);
> > -		return -EINVAL;
> > -	}
> > +	ret = xfs_validate_params(mp, &context, false);
> >  
> > -	if (ctx->iosizelog) {
> > -		if (ctx->iosizelog > XFS_MAX_IO_LOG ||
> > -		    ctx->iosizelog < XFS_MIN_IO_LOG) {
> > -			xfs_warn(mp, "invalid log iosize: %d [not %d-
> > %d]",
> > -				ctx->iosizelog, XFS_MIN_IO_LOG,
> > -				XFS_MAX_IO_LOG);
> > -			return -EINVAL;
> > -		}
> > -
> > -		mp->m_flags |= XFS_MOUNT_DFLT_IOSIZE;
> > -		mp->m_readio_log = ctx->iosizelog;
> > -		mp->m_writeio_log = ctx->iosizelog;
> > -	}
> > -
> > -	return 0;
> > +	return ret;
> >  }
> >  
> >  struct proc_xfs_info {
> > 

[REPOST PATCH v3 08/16] xfs: mount-api - refactor xfs_fs_fill_super()

[Ian Kent]

Much of the code in xfs_fs_fill_super() will be used by the fill super
function of the new mount-api.

So refactor the common code into a helper in an attempt to show what's
actually changed.

Signed-off-by: Ian Kent <raven@themaw.net>
---
 fs/xfs/xfs_super.c |   64 +++++++++++++++++++++++++++++++++-------------------
 1 file changed, 41 insertions(+), 23 deletions(-)

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index cfda58dd3822..ea3640ffd8f5 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1720,27 +1720,14 @@ xfs_mount_alloc(
 
 
 STATIC int
-xfs_fs_fill_super(
-	struct super_block	*sb,
-	void			*data,
+__xfs_fs_fill_super(
+	struct xfs_mount	*mp,
 	int			silent)
 {
+	struct super_block	*sb = mp->m_super;
 	struct inode		*root;
-	struct xfs_mount	*mp = NULL;
-	int			flags = 0, error = -ENOMEM;
-
-	/*
-	 * allocate mp and do all low-level struct initializations before we
-	 * attach it to the super
-	 */
-	mp = xfs_mount_alloc(sb);
-	if (!mp)
-		goto out;
-	sb->s_fs_info = mp;
-
-	error = xfs_parseargs(mp, (char *)data);
-	if (error)
-		goto out_free_fsname;
+	int			flags = 0;
+	int			error;
 
 	sb_min_blocksize(sb, BBSIZE);
 	sb->s_xattr = xfs_xattr_handlers;
@@ -1767,7 +1754,7 @@ xfs_fs_fill_super(
 
 	error = xfs_open_devices(mp);
 	if (error)
-		goto out_free_fsname;
+		goto out;
 
 	error = xfs_init_mount_workqueues(mp);
 	if (error)
@@ -1902,10 +1889,6 @@ xfs_fs_fill_super(
 	xfs_destroy_mount_workqueues(mp);
  out_close_devices:
 	xfs_close_devices(mp);
- out_free_fsname:
-	sb->s_fs_info = NULL;
-	xfs_free_fsname(mp);
-	kfree(mp);
  out:
 	return error;
 
@@ -1915,6 +1898,41 @@ xfs_fs_fill_super(
 	goto out_free_sb;
 }
 
+STATIC int
+xfs_fs_fill_super(
+	struct super_block	*sb,
+	void			*data,
+	int			silent)
+{
+	struct xfs_mount	*mp = NULL;
+	int			error = -ENOMEM;
+
+	/*
+	 * allocate mp and do all low-level struct initializations before we
+	 * attach it to the super
+	 */
+	mp = xfs_mount_alloc(sb);
+	if (!mp)
+		return -ENOMEM;
+	sb->s_fs_info = mp;
+
+	error = xfs_parseargs(mp, (char *)data);
+	if (error)
+		goto out_free_fsname;
+
+	error = __xfs_fs_fill_super(mp, silent);
+	if (error)
+		goto out_free_fsname;
+
+	return 0;
+
+ out_free_fsname:
+	sb->s_fs_info = NULL;
+	xfs_free_fsname(mp);
+	kfree(mp);
+	return error;
+}
+
 STATIC void
 xfs_fs_put_super(
 	struct super_block	*sb)

Re: [REPOST PATCH v3 08/16] xfs: mount-api - refactor xfs_fs_fill_super()

[Brian Foster]

On Tue, Sep 24, 2019 at 09:22:43PM +0800, Ian Kent wrote:
> Much of the code in xfs_fs_fill_super() will be used by the fill super
> function of the new mount-api.
> 
> So refactor the common code into a helper in an attempt to show what's
> actually changed.
> 
> Signed-off-by: Ian Kent <raven@themaw.net>
> ---
>  fs/xfs/xfs_super.c |   64 +++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 41 insertions(+), 23 deletions(-)
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index cfda58dd3822..ea3640ffd8f5 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
...
> @@ -1915,6 +1898,41 @@ xfs_fs_fill_super(
>  	goto out_free_sb;
>  }
>  
> +STATIC int
> +xfs_fs_fill_super(
> +	struct super_block	*sb,
> +	void			*data,
> +	int			silent)
> +{
> +	struct xfs_mount	*mp = NULL;
> +	int			error = -ENOMEM;

Both variable initializations are unnecessary. With those fixed:

Reviewed-by: Brian Foster <bfoster@redhat.com>

> +
> +	/*
> +	 * allocate mp and do all low-level struct initializations before we
> +	 * attach it to the super
> +	 */
> +	mp = xfs_mount_alloc(sb);
> +	if (!mp)
> +		return -ENOMEM;
> +	sb->s_fs_info = mp;
> +
> +	error = xfs_parseargs(mp, (char *)data);
> +	if (error)
> +		goto out_free_fsname;
> +
> +	error = __xfs_fs_fill_super(mp, silent);
> +	if (error)
> +		goto out_free_fsname;
> +
> +	return 0;
> +
> + out_free_fsname:
> +	sb->s_fs_info = NULL;
> +	xfs_free_fsname(mp);
> +	kfree(mp);
> +	return error;
> +}
> +
>  STATIC void
>  xfs_fs_put_super(
>  	struct super_block	*sb)
> 

[REPOST PATCH v3 09/16] xfs: mount-api - add xfs_get_tree()

[Ian Kent]

Add the fs_context_operations method .get_tree that validates
mount options and fills the super block as previously done
by the file_system_type .mount method.

Signed-off-by: Ian Kent <raven@themaw.net>
---
 fs/xfs/xfs_super.c |   50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index ea3640ffd8f5..6f9fe92b4e21 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1933,6 +1933,51 @@ xfs_fs_fill_super(
 	return error;
 }
 
+STATIC int
+xfs_fill_super(
+	struct super_block	*sb,
+	struct fs_context	*fc)
+{
+	struct xfs_fs_context	*ctx = fc->fs_private;
+	struct xfs_mount	*mp = sb->s_fs_info;
+	int			silent = fc->sb_flags & SB_SILENT;
+	int			error = -ENOMEM;
+
+	mp->m_super = sb;
+
+	/*
+	 * set up the mount name first so all the errors will refer to the
+	 * correct device.
+	 */
+	mp->m_fsname = kstrndup(sb->s_id, MAXNAMELEN, GFP_KERNEL);
+	if (!mp->m_fsname)
+		return -ENOMEM;
+	mp->m_fsname_len = strlen(mp->m_fsname) + 1;
+
+	error = xfs_validate_params(mp, ctx, false);
+	if (error)
+		goto out_free_fsname;
+
+	error = __xfs_fs_fill_super(mp, silent);
+	if (error)
+		goto out_free_fsname;
+
+	return 0;
+
+ out_free_fsname:
+	sb->s_fs_info = NULL;
+	xfs_free_fsname(mp);
+
+	return error;
+}
+
+STATIC int
+xfs_get_tree(
+	struct fs_context	*fc)
+{
+	return vfs_get_block_super(fc, xfs_fill_super);
+}
+
 STATIC void
 xfs_fs_put_super(
 	struct super_block	*sb)
@@ -2003,6 +2048,11 @@ static const struct super_operations xfs_super_operations = {
 	.free_cached_objects	= xfs_fs_free_cached_objects,
 };
 
+static const struct fs_context_operations xfs_context_ops = {
+	.parse_param = xfs_parse_param,
+	.get_tree    = xfs_get_tree,
+};
+
 static struct file_system_type xfs_fs_type = {
 	.owner			= THIS_MODULE,
 	.name			= "xfs",

Re: [REPOST PATCH v3 09/16] xfs: mount-api - add xfs_get_tree()

[Brian Foster]

On Tue, Sep 24, 2019 at 09:22:49PM +0800, Ian Kent wrote:
> Add the fs_context_operations method .get_tree that validates
> mount options and fills the super block as previously done
> by the file_system_type .mount method.
> 
> Signed-off-by: Ian Kent <raven@themaw.net>
> ---
>  fs/xfs/xfs_super.c |   50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index ea3640ffd8f5..6f9fe92b4e21 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1933,6 +1933,51 @@ xfs_fs_fill_super(
>  	return error;
>  }
>  
> +STATIC int
> +xfs_fill_super(
> +	struct super_block	*sb,
> +	struct fs_context	*fc)
> +{
> +	struct xfs_fs_context	*ctx = fc->fs_private;
> +	struct xfs_mount	*mp = sb->s_fs_info;
> +	int			silent = fc->sb_flags & SB_SILENT;
> +	int			error = -ENOMEM;
> +
> +	mp->m_super = sb;
> +
> +	/*
> +	 * set up the mount name first so all the errors will refer to the
> +	 * correct device.
> +	 */
> +	mp->m_fsname = kstrndup(sb->s_id, MAXNAMELEN, GFP_KERNEL);
> +	if (!mp->m_fsname)
> +		return -ENOMEM;
> +	mp->m_fsname_len = strlen(mp->m_fsname) + 1;
> +
> +	error = xfs_validate_params(mp, ctx, false);
> +	if (error)
> +		goto out_free_fsname;
> +
> +	error = __xfs_fs_fill_super(mp, silent);
> +	if (error)
> +		goto out_free_fsname;
> +
> +	return 0;
> +
> + out_free_fsname:
> +	sb->s_fs_info = NULL;
> +	xfs_free_fsname(mp);
> +

I'm still not following the (intended) lifecycle of mp here. Looking
ahead in the series, we allocate mp in xfs_init_fs_context() and set
some state. It looks like at some point we grow an xfs_fc_free()
callback that frees mp, but that doesn't exist as of yet. So is that a
memory leak as of this patch?

We also call xfs_free_fsname() here (which doesn't reset pointers to
NULL) and open-code kfree()'s of a couple of the same fields in
xfs_fc_free(). Those look like double frees to me.

Hmm.. I guess I'm kind of wondering why we lift the mp alloc out of the
fill super call in the first place. At a glance, it doesn't look like we
do anything in that xfs_init_fs_context() call that we couldn't do a bit
later..

Brian

> +	return error;
> +}
> +
> +STATIC int
> +xfs_get_tree(
> +	struct fs_context	*fc)
> +{
> +	return vfs_get_block_super(fc, xfs_fill_super);
> +}
> +
>  STATIC void
>  xfs_fs_put_super(
>  	struct super_block	*sb)
> @@ -2003,6 +2048,11 @@ static const struct super_operations xfs_super_operations = {
>  	.free_cached_objects	= xfs_fs_free_cached_objects,
>  };
>  
> +static const struct fs_context_operations xfs_context_ops = {
> +	.parse_param = xfs_parse_param,
> +	.get_tree    = xfs_get_tree,
> +};
> +
>  static struct file_system_type xfs_fs_type = {
>  	.owner			= THIS_MODULE,
>  	.name			= "xfs",
> 

Re: [REPOST PATCH v3 09/16] xfs: mount-api - add xfs_get_tree()

[Ian Kent]

On Tue, 2019-09-24 at 10:38 -0400, Brian Foster wrote:
> On Tue, Sep 24, 2019 at 09:22:49PM +0800, Ian Kent wrote:
> > Add the fs_context_operations method .get_tree that validates
> > mount options and fills the super block as previously done
> > by the file_system_type .mount method.
> > 
> > Signed-off-by: Ian Kent <raven@themaw.net>
> > ---
> >  fs/xfs/xfs_super.c |   50
> > ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 50 insertions(+)
> > 
> > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > index ea3640ffd8f5..6f9fe92b4e21 100644
> > --- a/fs/xfs/xfs_super.c
> > +++ b/fs/xfs/xfs_super.c
> > @@ -1933,6 +1933,51 @@ xfs_fs_fill_super(
> >  	return error;
> >  }
> >  
> > +STATIC int
> > +xfs_fill_super(
> > +	struct super_block	*sb,
> > +	struct fs_context	*fc)
> > +{
> > +	struct xfs_fs_context	*ctx = fc->fs_private;
> > +	struct xfs_mount	*mp = sb->s_fs_info;
> > +	int			silent = fc->sb_flags & SB_SILENT;
> > +	int			error = -ENOMEM;
> > +
> > +	mp->m_super = sb;
> > +
> > +	/*
> > +	 * set up the mount name first so all the errors will refer to
> > the
> > +	 * correct device.
> > +	 */
> > +	mp->m_fsname = kstrndup(sb->s_id, MAXNAMELEN, GFP_KERNEL);
> > +	if (!mp->m_fsname)
> > +		return -ENOMEM;
> > +	mp->m_fsname_len = strlen(mp->m_fsname) + 1;
> > +
> > +	error = xfs_validate_params(mp, ctx, false);
> > +	if (error)
> > +		goto out_free_fsname;
> > +
> > +	error = __xfs_fs_fill_super(mp, silent);
> > +	if (error)
> > +		goto out_free_fsname;
> > +
> > +	return 0;
> > +
> > + out_free_fsname:
> > +	sb->s_fs_info = NULL;
> > +	xfs_free_fsname(mp);
> > +
> 
> I'm still not following the (intended) lifecycle of mp here. Looking
> ahead in the series, we allocate mp in xfs_init_fs_context() and set
> some state. It looks like at some point we grow an xfs_fc_free()
> callback that frees mp, but that doesn't exist as of yet. So is that
> a
> memory leak as of this patch?
> 
> We also call xfs_free_fsname() here (which doesn't reset pointers to
> NULL) and open-code kfree()'s of a couple of the same fields in
> xfs_fc_free(). Those look like double frees to me.
> 
> Hmm.. I guess I'm kind of wondering why we lift the mp alloc out of
> the
> fill super call in the first place. At a glance, it doesn't look like
> we
> do anything in that xfs_init_fs_context() call that we couldn't do a
> bit
> later..

Umm ... yes ...

I think I've got the active code path right ...

At this point .mount == xfs_fs_mount() which will calls
xfs_fs_fill_super() to fill the super block.

xfs_fs_fill_super() allocates the super block info struct and sets
it in the super block private info field, then calls xfs_parseargs()
which still allocates mp->m_fsname at this point, to accomodate a
similar free pattern in xfs_test_remount_options().

It then calls __xfs_fs_fill_super() which doesn't touch those fsname
fields or mp to fit in with what will be done later.

If an error occurs both the fsname fields (xfs_free_fsname()) and mp
are freed by the main caller, xfs_fs_fill_super().

I think that process is ok.

The mount api process that isn't active yet is a bit different.

The context (ctx), a temporary working space, is allocated then saved
in the mount context (fc) and the super block info is also allocated
and saved in the mount context in it's field of the same name as the
private super block info field, s_fs_info.

The function xfs_fill_super() is called as a result of the .get_tree()
mount context operation to fill the super block.

During this process, when the VFS successfully allocates the super
block s_fs_info is set in the super block and the mount context
field set to NULL. From this point freeing the private super block
info becomes part of usual freeing of the super block with the super
operation .kill_sb().

But if the super block allocation fails then the mount context
s_fs_info field remains set and is the responsibility of the
mount context operations .fc_free() method to clean up.

Now the VFS calls to xfs_fill_super() after this.

I should have been able to leave xfs_fill_super() it as it
was with:
        sb->s_fs_info = NULL;
        xfs_free_fsname(mp);
        kfree(mp);
and that should have been ok but it wasn't, there was some sort of
allocation problem, possibly a double free, causing a crash.

Strictly speaking this cleanup process should be carried out by
either the mount context .fc_free() or super operation .kill_sb()
and that's what I want to do.

So I'm not sure the allocation time and the place this is done
can (or should) be done differently.

And that freeing on error exit from xfs_fill_super() is definitely
wrong now! Ha, and I didn't see any crashes myself when I tested
it ... maybe I need a reproducer ...

Ian

> 
> Brian
> 
> > +	return error;
> > +}
> > +
> > +STATIC int
> > +xfs_get_tree(
> > +	struct fs_context	*fc)
> > +{
> > +	return vfs_get_block_super(fc, xfs_fill_super);
> > +}
> > +
> >  STATIC void
> >  xfs_fs_put_super(
> >  	struct super_block	*sb)
> > @@ -2003,6 +2048,11 @@ static const struct super_operations
> > xfs_super_operations = {
> >  	.free_cached_objects	= xfs_fs_free_cached_objects,
> >  };
> >  
> > +static const struct fs_context_operations xfs_context_ops = {
> > +	.parse_param = xfs_parse_param,
> > +	.get_tree    = xfs_get_tree,
> > +};
> > +
> >  static struct file_system_type xfs_fs_type = {
> >  	.owner			= THIS_MODULE,
> >  	.name			= "xfs",
> > 

Re: [REPOST PATCH v3 09/16] xfs: mount-api - add xfs_get_tree()

[Ian Kent]

On Wed, 2019-09-25 at 15:42 +0800, Ian Kent wrote:
> On Tue, 2019-09-24 at 10:38 -0400, Brian Foster wrote:
> > On Tue, Sep 24, 2019 at 09:22:49PM +0800, Ian Kent wrote:
> > > Add the fs_context_operations method .get_tree that validates
> > > mount options and fills the super block as previously done
> > > by the file_system_type .mount method.
> > > 
> > > Signed-off-by: Ian Kent <raven@themaw.net>
> > > ---
> > >  fs/xfs/xfs_super.c |   50
> > > ++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 50 insertions(+)
> > > 
> > > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > > index ea3640ffd8f5..6f9fe92b4e21 100644
> > > --- a/fs/xfs/xfs_super.c
> > > +++ b/fs/xfs/xfs_super.c
> > > @@ -1933,6 +1933,51 @@ xfs_fs_fill_super(
> > >  	return error;
> > >  }
> > >  
> > > +STATIC int
> > > +xfs_fill_super(
> > > +	struct super_block	*sb,
> > > +	struct fs_context	*fc)
> > > +{
> > > +	struct xfs_fs_context	*ctx = fc->fs_private;
> > > +	struct xfs_mount	*mp = sb->s_fs_info;
> > > +	int			silent = fc->sb_flags & SB_SILENT;
> > > +	int			error = -ENOMEM;
> > > +
> > > +	mp->m_super = sb;
> > > +
> > > +	/*
> > > +	 * set up the mount name first so all the errors will refer to
> > > the
> > > +	 * correct device.
> > > +	 */
> > > +	mp->m_fsname = kstrndup(sb->s_id, MAXNAMELEN, GFP_KERNEL);
> > > +	if (!mp->m_fsname)
> > > +		return -ENOMEM;
> > > +	mp->m_fsname_len = strlen(mp->m_fsname) + 1;
> > > +
> > > +	error = xfs_validate_params(mp, ctx, false);
> > > +	if (error)
> > > +		goto out_free_fsname;
> > > +
> > > +	error = __xfs_fs_fill_super(mp, silent);
> > > +	if (error)
> > > +		goto out_free_fsname;
> > > +
> > > +	return 0;
> > > +
> > > + out_free_fsname:
> > > +	sb->s_fs_info = NULL;
> > > +	xfs_free_fsname(mp);
> > > +
> > 
> > I'm still not following the (intended) lifecycle of mp here.
> > Looking
> > ahead in the series, we allocate mp in xfs_init_fs_context() and
> > set
> > some state. It looks like at some point we grow an xfs_fc_free()
> > callback that frees mp, but that doesn't exist as of yet. So is
> > that
> > a
> > memory leak as of this patch?
> > 
> > We also call xfs_free_fsname() here (which doesn't reset pointers
> > to
> > NULL) and open-code kfree()'s of a couple of the same fields in
> > xfs_fc_free(). Those look like double frees to me.
> > 
> > Hmm.. I guess I'm kind of wondering why we lift the mp alloc out of
> > the
> > fill super call in the first place. At a glance, it doesn't look
> > like
> > we
> > do anything in that xfs_init_fs_context() call that we couldn't do
> > a
> > bit
> > later..
> 
> Umm ... yes ...
> 
> I think I've got the active code path right ...
> 
> At this point .mount == xfs_fs_mount() which will calls
> xfs_fs_fill_super() to fill the super block.
> 
> xfs_fs_fill_super() allocates the super block info struct and sets
> it in the super block private info field, then calls xfs_parseargs()
> which still allocates mp->m_fsname at this point, to accomodate a
> similar free pattern in xfs_test_remount_options().
> 
> It then calls __xfs_fs_fill_super() which doesn't touch those fsname
> fields or mp to fit in with what will be done later.
> 
> If an error occurs both the fsname fields (xfs_free_fsname()) and mp
> are freed by the main caller, xfs_fs_fill_super().
> 
> I think that process is ok.
> 
> The mount api process that isn't active yet is a bit different.
> 
> The context (ctx), a temporary working space, is allocated then saved
> in the mount context (fc) and the super block info is also allocated
> and saved in the mount context in it's field of the same name as the
> private super block info field, s_fs_info.
> 
> The function xfs_fill_super() is called as a result of the
> .get_tree()
> mount context operation to fill the super block.
> 
> During this process, when the VFS successfully allocates the super
> block s_fs_info is set in the super block and the mount context
> field set to NULL. From this point freeing the private super block
> info becomes part of usual freeing of the super block with the super
> operation .kill_sb().
> 
> But if the super block allocation fails then the mount context
> s_fs_info field remains set and is the responsibility of the
> mount context operations .fc_free() method to clean up.
> 
> Now the VFS calls to xfs_fill_super() after this.
> 
> I should have been able to leave xfs_fill_super() it as it
> was with:
>         sb->s_fs_info = NULL;
>         xfs_free_fsname(mp);
>         kfree(mp);
> and that should have been ok but it wasn't, there was some sort of
> allocation problem, possibly a double free, causing a crash.
> 
> Strictly speaking this cleanup process should be carried out by
> either the mount context .fc_free() or super operation .kill_sb()
> and that's what I want to do.

Umm ... but I can't actually do that ...

Looking back at xfs I realize that the filling of the super
block is meant to leave nothing allocated and set
sb->s_fs_info = NULL on error so that ->put_super() won't try
and cleanup a whole bunch of stuff that hasn't been done.

Which brings me back to what I originally had above ... which
we believe doesn't work ?

> 
> So I'm not sure the allocation time and the place this is done
> can (or should) be done differently.
> 
> And that freeing on error exit from xfs_fill_super() is definitely
> wrong now! Ha, and I didn't see any crashes myself when I tested
> it ... maybe I need a reproducer ...
> 
> Ian
> 
> > Brian
> > 
> > > +	return error;
> > > +}
> > > +
> > > +STATIC int
> > > +xfs_get_tree(
> > > +	struct fs_context	*fc)
> > > +{
> > > +	return vfs_get_block_super(fc, xfs_fill_super);
> > > +}
> > > +
> > >  STATIC void
> > >  xfs_fs_put_super(
> > >  	struct super_block	*sb)
> > > @@ -2003,6 +2048,11 @@ static const struct super_operations
> > > xfs_super_operations = {
> > >  	.free_cached_objects	= xfs_fs_free_cached_objects,
> > >  };
> > >  
> > > +static const struct fs_context_operations xfs_context_ops = {
> > > +	.parse_param = xfs_parse_param,
> > > +	.get_tree    = xfs_get_tree,
> > > +};
> > > +
> > >  static struct file_system_type xfs_fs_type = {
> > >  	.owner			= THIS_MODULE,
> > >  	.name			= "xfs",
> > > 

Re: [REPOST PATCH v3 09/16] xfs: mount-api - add xfs_get_tree()

[Brian Foster]

On Wed, Sep 25, 2019 at 04:07:08PM +0800, Ian Kent wrote:
> On Wed, 2019-09-25 at 15:42 +0800, Ian Kent wrote:
> > On Tue, 2019-09-24 at 10:38 -0400, Brian Foster wrote:
> > > On Tue, Sep 24, 2019 at 09:22:49PM +0800, Ian Kent wrote:
> > > > Add the fs_context_operations method .get_tree that validates
> > > > mount options and fills the super block as previously done
> > > > by the file_system_type .mount method.
> > > > 
> > > > Signed-off-by: Ian Kent <raven@themaw.net>
> > > > ---
> > > >  fs/xfs/xfs_super.c |   50
> > > > ++++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 50 insertions(+)
> > > > 
> > > > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > > > index ea3640ffd8f5..6f9fe92b4e21 100644
> > > > --- a/fs/xfs/xfs_super.c
> > > > +++ b/fs/xfs/xfs_super.c
> > > > @@ -1933,6 +1933,51 @@ xfs_fs_fill_super(
> > > >  	return error;
> > > >  }
> > > >  
> > > > +STATIC int
> > > > +xfs_fill_super(
> > > > +	struct super_block	*sb,
> > > > +	struct fs_context	*fc)
> > > > +{
> > > > +	struct xfs_fs_context	*ctx = fc->fs_private;
> > > > +	struct xfs_mount	*mp = sb->s_fs_info;
> > > > +	int			silent = fc->sb_flags & SB_SILENT;
> > > > +	int			error = -ENOMEM;
> > > > +
> > > > +	mp->m_super = sb;
> > > > +
> > > > +	/*
> > > > +	 * set up the mount name first so all the errors will refer to
> > > > the
> > > > +	 * correct device.
> > > > +	 */
> > > > +	mp->m_fsname = kstrndup(sb->s_id, MAXNAMELEN, GFP_KERNEL);
> > > > +	if (!mp->m_fsname)
> > > > +		return -ENOMEM;
> > > > +	mp->m_fsname_len = strlen(mp->m_fsname) + 1;
> > > > +
> > > > +	error = xfs_validate_params(mp, ctx, false);
> > > > +	if (error)
> > > > +		goto out_free_fsname;
> > > > +
> > > > +	error = __xfs_fs_fill_super(mp, silent);
> > > > +	if (error)
> > > > +		goto out_free_fsname;
> > > > +
> > > > +	return 0;
> > > > +
> > > > + out_free_fsname:
> > > > +	sb->s_fs_info = NULL;
> > > > +	xfs_free_fsname(mp);
> > > > +
> > > 
> > > I'm still not following the (intended) lifecycle of mp here.
> > > Looking
> > > ahead in the series, we allocate mp in xfs_init_fs_context() and
> > > set
> > > some state. It looks like at some point we grow an xfs_fc_free()
> > > callback that frees mp, but that doesn't exist as of yet. So is
> > > that
> > > a
> > > memory leak as of this patch?
> > > 
> > > We also call xfs_free_fsname() here (which doesn't reset pointers
> > > to
> > > NULL) and open-code kfree()'s of a couple of the same fields in
> > > xfs_fc_free(). Those look like double frees to me.
> > > 
> > > Hmm.. I guess I'm kind of wondering why we lift the mp alloc out of
> > > the
> > > fill super call in the first place. At a glance, it doesn't look
> > > like
> > > we
> > > do anything in that xfs_init_fs_context() call that we couldn't do
> > > a
> > > bit
> > > later..
> > 
> > Umm ... yes ...
> > 
> > I think I've got the active code path right ...
> > 
> > At this point .mount == xfs_fs_mount() which will calls
> > xfs_fs_fill_super() to fill the super block.
> > 
> > xfs_fs_fill_super() allocates the super block info struct and sets
> > it in the super block private info field, then calls xfs_parseargs()
> > which still allocates mp->m_fsname at this point, to accomodate a
> > similar free pattern in xfs_test_remount_options().
> > 
> > It then calls __xfs_fs_fill_super() which doesn't touch those fsname
> > fields or mp to fit in with what will be done later.
> > 
> > If an error occurs both the fsname fields (xfs_free_fsname()) and mp
> > are freed by the main caller, xfs_fs_fill_super().
> > 
> > I think that process is ok.
> > 
> > The mount api process that isn't active yet is a bit different.
> > 
> > The context (ctx), a temporary working space, is allocated then saved
> > in the mount context (fc) and the super block info is also allocated
> > and saved in the mount context in it's field of the same name as the
> > private super block info field, s_fs_info.
> > 
> > The function xfs_fill_super() is called as a result of the
> > .get_tree()
> > mount context operation to fill the super block.
> > 
> > During this process, when the VFS successfully allocates the super
> > block s_fs_info is set in the super block and the mount context
> > field set to NULL. From this point freeing the private super block
> > info becomes part of usual freeing of the super block with the super
> > operation .kill_sb().
> > 
> > But if the super block allocation fails then the mount context
> > s_fs_info field remains set and is the responsibility of the
> > mount context operations .fc_free() method to clean up.
> > 
> > Now the VFS calls to xfs_fill_super() after this.
> > 
> > I should have been able to leave xfs_fill_super() it as it
> > was with:
> >         sb->s_fs_info = NULL;
> >         xfs_free_fsname(mp);
> >         kfree(mp);
> > and that should have been ok but it wasn't, there was some sort of
> > allocation problem, possibly a double free, causing a crash.
> > 
> > Strictly speaking this cleanup process should be carried out by
> > either the mount context .fc_free() or super operation .kill_sb()
> > and that's what I want to do.
> 
> Umm ... but I can't actually do that ...
> 
> Looking back at xfs I realize that the filling of the super
> block is meant to leave nothing allocated and set
> sb->s_fs_info = NULL on error so that ->put_super() won't try
> and cleanup a whole bunch of stuff that hasn't been done.
> 
> Which brings me back to what I originally had above ... which
> we believe doesn't work ?
> 

It looks like perhaps the assignment of sb->s_fs_info was lost as well?
Skipping to the end, I see xfs_init_fs_context() alloc mp and assign
fc->s_fs_info. xfs_get_tree() leads to xfs_fill_super(), which somehow
gets mp from sb->s_fs_info (not fc->...), but then resets sb->s_fs_info
on error and frees the names, leaving fs->s_fs_info so presumably
xfs_fc_free() can free mp along with a couple of the names (again). I
can't really make heads or tails of what this is even attempting to do.

That aside, it's not clear to me why the new code can't follow a similar
pattern as the old code with regard to allocation. Allocate mp in
xfs_fill_super() and set up sb/fc pointers, reset pointers and free mp
on error return. Otherwise, xfs_fc_free() checks for fc->s_fs_info !=
NULL and frees mp from there. Is there some reason we can't continue to
do that?

Brian

> > 
> > So I'm not sure the allocation time and the place this is done
> > can (or should) be done differently.
> > 
> > And that freeing on error exit from xfs_fill_super() is definitely
> > wrong now! Ha, and I didn't see any crashes myself when I tested
> > it ... maybe I need a reproducer ...
> > 
> > Ian
> > 
> > > Brian
> > > 
> > > > +	return error;
> > > > +}
> > > > +
> > > > +STATIC int
> > > > +xfs_get_tree(
> > > > +	struct fs_context	*fc)
> > > > +{
> > > > +	return vfs_get_block_super(fc, xfs_fill_super);
> > > > +}
> > > > +
> > > >  STATIC void
> > > >  xfs_fs_put_super(
> > > >  	struct super_block	*sb)
> > > > @@ -2003,6 +2048,11 @@ static const struct super_operations
> > > > xfs_super_operations = {
> > > >  	.free_cached_objects	= xfs_fs_free_cached_objects,
> > > >  };
> > > >  
> > > > +static const struct fs_context_operations xfs_context_ops = {
> > > > +	.parse_param = xfs_parse_param,
> > > > +	.get_tree    = xfs_get_tree,
> > > > +};
> > > > +
> > > >  static struct file_system_type xfs_fs_type = {
> > > >  	.owner			= THIS_MODULE,
> > > >  	.name			= "xfs",
> > > > 
> 

Re: [REPOST PATCH v3 09/16] xfs: mount-api - add xfs_get_tree()

[Ian Kent]

On Wed, 2019-09-25 at 10:34 -0400, Brian Foster wrote:
> On Wed, Sep 25, 2019 at 04:07:08PM +0800, Ian Kent wrote:
> > On Wed, 2019-09-25 at 15:42 +0800, Ian Kent wrote:
> > > On Tue, 2019-09-24 at 10:38 -0400, Brian Foster wrote:
> > > > On Tue, Sep 24, 2019 at 09:22:49PM +0800, Ian Kent wrote:
> > > > > Add the fs_context_operations method .get_tree that validates
> > > > > mount options and fills the super block as previously done
> > > > > by the file_system_type .mount method.
> > > > > 
> > > > > Signed-off-by: Ian Kent <raven@themaw.net>
> > > > > ---
> > > > >  fs/xfs/xfs_super.c |   50
> > > > > ++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > >  1 file changed, 50 insertions(+)
> > > > > 
> > > > > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > > > > index ea3640ffd8f5..6f9fe92b4e21 100644
> > > > > --- a/fs/xfs/xfs_super.c
> > > > > +++ b/fs/xfs/xfs_super.c
> > > > > @@ -1933,6 +1933,51 @@ xfs_fs_fill_super(
> > > > >  	return error;
> > > > >  }
> > > > >  
> > > > > +STATIC int
> > > > > +xfs_fill_super(
> > > > > +	struct super_block	*sb,
> > > > > +	struct fs_context	*fc)
> > > > > +{
> > > > > +	struct xfs_fs_context	*ctx = fc->fs_private;
> > > > > +	struct xfs_mount	*mp = sb->s_fs_info;
> > > > > +	int			silent = fc->sb_flags &
> > > > > SB_SILENT;
> > > > > +	int			error = -ENOMEM;
> > > > > +
> > > > > +	mp->m_super = sb;
> > > > > +
> > > > > +	/*
> > > > > +	 * set up the mount name first so all the errors will
> > > > > refer to
> > > > > the
> > > > > +	 * correct device.
> > > > > +	 */
> > > > > +	mp->m_fsname = kstrndup(sb->s_id, MAXNAMELEN,
> > > > > GFP_KERNEL);
> > > > > +	if (!mp->m_fsname)
> > > > > +		return -ENOMEM;
> > > > > +	mp->m_fsname_len = strlen(mp->m_fsname) + 1;
> > > > > +
> > > > > +	error = xfs_validate_params(mp, ctx, false);
> > > > > +	if (error)
> > > > > +		goto out_free_fsname;
> > > > > +
> > > > > +	error = __xfs_fs_fill_super(mp, silent);
> > > > > +	if (error)
> > > > > +		goto out_free_fsname;
> > > > > +
> > > > > +	return 0;
> > > > > +
> > > > > + out_free_fsname:
> > > > > +	sb->s_fs_info = NULL;
> > > > > +	xfs_free_fsname(mp);
> > > > > +
> > > > 
> > > > I'm still not following the (intended) lifecycle of mp here.
> > > > Looking
> > > > ahead in the series, we allocate mp in xfs_init_fs_context()
> > > > and
> > > > set
> > > > some state. It looks like at some point we grow an
> > > > xfs_fc_free()
> > > > callback that frees mp, but that doesn't exist as of yet. So is
> > > > that
> > > > a
> > > > memory leak as of this patch?
> > > > 
> > > > We also call xfs_free_fsname() here (which doesn't reset
> > > > pointers
> > > > to
> > > > NULL) and open-code kfree()'s of a couple of the same fields in
> > > > xfs_fc_free(). Those look like double frees to me.
> > > > 
> > > > Hmm.. I guess I'm kind of wondering why we lift the mp alloc
> > > > out of
> > > > the
> > > > fill super call in the first place. At a glance, it doesn't
> > > > look
> > > > like
> > > > we
> > > > do anything in that xfs_init_fs_context() call that we couldn't
> > > > do
> > > > a
> > > > bit
> > > > later..
> > > 
> > > Umm ... yes ...
> > > 
> > > I think I've got the active code path right ...
> > > 
> > > At this point .mount == xfs_fs_mount() which will calls
> > > xfs_fs_fill_super() to fill the super block.
> > > 
> > > xfs_fs_fill_super() allocates the super block info struct and
> > > sets
> > > it in the super block private info field, then calls
> > > xfs_parseargs()
> > > which still allocates mp->m_fsname at this point, to accomodate a
> > > similar free pattern in xfs_test_remount_options().
> > > 
> > > It then calls __xfs_fs_fill_super() which doesn't touch those
> > > fsname
> > > fields or mp to fit in with what will be done later.
> > > 
> > > If an error occurs both the fsname fields (xfs_free_fsname()) and
> > > mp
> > > are freed by the main caller, xfs_fs_fill_super().
> > > 
> > > I think that process is ok.
> > > 
> > > The mount api process that isn't active yet is a bit different.
> > > 
> > > The context (ctx), a temporary working space, is allocated then
> > > saved
> > > in the mount context (fc) and the super block info is also
> > > allocated
> > > and saved in the mount context in it's field of the same name as
> > > the
> > > private super block info field, s_fs_info.
> > > 
> > > The function xfs_fill_super() is called as a result of the
> > > .get_tree()
> > > mount context operation to fill the super block.
> > > 
> > > During this process, when the VFS successfully allocates the
> > > super
> > > block s_fs_info is set in the super block and the mount context
> > > field set to NULL. From this point freeing the private super
> > > block
> > > info becomes part of usual freeing of the super block with the
> > > super
> > > operation .kill_sb().
> > > 
> > > But if the super block allocation fails then the mount context
> > > s_fs_info field remains set and is the responsibility of the
> > > mount context operations .fc_free() method to clean up.
> > > 
> > > Now the VFS calls to xfs_fill_super() after this.
> > > 
> > > I should have been able to leave xfs_fill_super() it as it
> > > was with:
> > >         sb->s_fs_info = NULL;
> > >         xfs_free_fsname(mp);
> > >         kfree(mp);
> > > and that should have been ok but it wasn't, there was some sort
> > > of
> > > allocation problem, possibly a double free, causing a crash.
> > > 
> > > Strictly speaking this cleanup process should be carried out by
> > > either the mount context .fc_free() or super operation .kill_sb()
> > > and that's what I want to do.
> > 
> > Umm ... but I can't actually do that ...
> > 
> > Looking back at xfs I realize that the filling of the super
> > block is meant to leave nothing allocated and set
> > sb->s_fs_info = NULL on error so that ->put_super() won't try
> > and cleanup a whole bunch of stuff that hasn't been done.
> > 
> > Which brings me back to what I originally had above ... which
> > we believe doesn't work ?
> > 
> 
> It looks like perhaps the assignment of sb->s_fs_info was lost as
> well?
> Skipping to the end, I see xfs_init_fs_context() alloc mp and assign
> fc->s_fs_info. xfs_get_tree() leads to xfs_fill_super(), which
> somehow
> gets mp from sb->s_fs_info (not fc->...), but then resets sb-
> >s_fs_info
> on error and frees the names, leaving fs->s_fs_info so presumably
> xfs_fc_free() can free mp along with a couple of the names (again). I
> can't really make heads or tails of what this is even attempting to
> do.

Ha, it seems a bit mysterious, but it's actually much simpler
than it appears.

> 
> That aside, it's not clear to me why the new code can't follow a
> similar
> pattern as the old code with regard to allocation. Allocate mp in
> xfs_fill_super() and set up sb/fc pointers, reset pointers and free
> mp
> on error return. Otherwise, xfs_fc_free() checks for fc->s_fs_info !=
> NULL and frees mp from there. Is there some reason we can't continue
> to
> do that?

I think not without a fairly significant re-design.

The main difference is the mount-api will allocate the super
block later than the old mount code.

Basically, if file system parameter parsing fails the super
block won't get allocated.

So the super block isn't available during parameter parsing
but the file system private data structure may be needed for
it, so it comes from the file system context at that point.

When the super block is successfully allocated the file system
private data structure is set in the super block (and the field
NULLed in the context) and things progress much the same as
before from that point.

That's the essential difference in the process AFAICS.

By the time fill_super() is called everything is set and you
should be able to proceed almost the same as before.

Ian

> Brian
> 
> > > So I'm not sure the allocation time and the place this is done
> > > can (or should) be done differently.
> > > 
> > > And that freeing on error exit from xfs_fill_super() is
> > > definitely
> > > wrong now! Ha, and I didn't see any crashes myself when I tested
> > > it ... maybe I need a reproducer ...
> > > 
> > > Ian
> > > 
> > > > Brian
> > > > 
> > > > > +	return error;
> > > > > +}
> > > > > +
> > > > > +STATIC int
> > > > > +xfs_get_tree(
> > > > > +	struct fs_context	*fc)
> > > > > +{
> > > > > +	return vfs_get_block_super(fc, xfs_fill_super);
> > > > > +}
> > > > > +
> > > > >  STATIC void
> > > > >  xfs_fs_put_super(
> > > > >  	struct super_block	*sb)
> > > > > @@ -2003,6 +2048,11 @@ static const struct super_operations
> > > > > xfs_super_operations = {
> > > > >  	.free_cached_objects	= xfs_fs_free_cached_objects,
> > > > >  };
> > > > >  
> > > > > +static const struct fs_context_operations xfs_context_ops =
> > > > > {
> > > > > +	.parse_param = xfs_parse_param,
> > > > > +	.get_tree    = xfs_get_tree,
> > > > > +};
> > > > > +
> > > > >  static struct file_system_type xfs_fs_type = {
> > > > >  	.owner			= THIS_MODULE,
> > > > >  	.name			= "xfs",
> > > > > 

Re: [REPOST PATCH v3 09/16] xfs: mount-api - add xfs_get_tree()

[Brian Foster]

On Thu, Sep 26, 2019 at 11:27:40AM +0800, Ian Kent wrote:
> On Wed, 2019-09-25 at 10:34 -0400, Brian Foster wrote:
> > On Wed, Sep 25, 2019 at 04:07:08PM +0800, Ian Kent wrote:
> > > On Wed, 2019-09-25 at 15:42 +0800, Ian Kent wrote:
> > > > On Tue, 2019-09-24 at 10:38 -0400, Brian Foster wrote:
> > > > > On Tue, Sep 24, 2019 at 09:22:49PM +0800, Ian Kent wrote:
> > > > > > Add the fs_context_operations method .get_tree that validates
> > > > > > mount options and fills the super block as previously done
> > > > > > by the file_system_type .mount method.
> > > > > > 
> > > > > > Signed-off-by: Ian Kent <raven@themaw.net>
> > > > > > ---
> > > > > >  fs/xfs/xfs_super.c |   50
> > > > > > ++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > > >  1 file changed, 50 insertions(+)
> > > > > > 
> > > > > > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > > > > > index ea3640ffd8f5..6f9fe92b4e21 100644
> > > > > > --- a/fs/xfs/xfs_super.c
> > > > > > +++ b/fs/xfs/xfs_super.c
> > > > > > @@ -1933,6 +1933,51 @@ xfs_fs_fill_super(
> > > > > >  	return error;
> > > > > >  }
> > > > > >  
> > > > > > +STATIC int
> > > > > > +xfs_fill_super(
> > > > > > +	struct super_block	*sb,
> > > > > > +	struct fs_context	*fc)
> > > > > > +{
> > > > > > +	struct xfs_fs_context	*ctx = fc->fs_private;
> > > > > > +	struct xfs_mount	*mp = sb->s_fs_info;
> > > > > > +	int			silent = fc->sb_flags &
> > > > > > SB_SILENT;
> > > > > > +	int			error = -ENOMEM;
> > > > > > +
> > > > > > +	mp->m_super = sb;
> > > > > > +
> > > > > > +	/*
> > > > > > +	 * set up the mount name first so all the errors will
> > > > > > refer to
> > > > > > the
> > > > > > +	 * correct device.
> > > > > > +	 */
> > > > > > +	mp->m_fsname = kstrndup(sb->s_id, MAXNAMELEN,
> > > > > > GFP_KERNEL);
> > > > > > +	if (!mp->m_fsname)
> > > > > > +		return -ENOMEM;
> > > > > > +	mp->m_fsname_len = strlen(mp->m_fsname) + 1;
> > > > > > +
> > > > > > +	error = xfs_validate_params(mp, ctx, false);
> > > > > > +	if (error)
> > > > > > +		goto out_free_fsname;
> > > > > > +
> > > > > > +	error = __xfs_fs_fill_super(mp, silent);
> > > > > > +	if (error)
> > > > > > +		goto out_free_fsname;
> > > > > > +
> > > > > > +	return 0;
> > > > > > +
> > > > > > + out_free_fsname:
> > > > > > +	sb->s_fs_info = NULL;
> > > > > > +	xfs_free_fsname(mp);
> > > > > > +
> > > > > 
> > > > > I'm still not following the (intended) lifecycle of mp here.
> > > > > Looking
> > > > > ahead in the series, we allocate mp in xfs_init_fs_context()
> > > > > and
> > > > > set
> > > > > some state. It looks like at some point we grow an
> > > > > xfs_fc_free()
> > > > > callback that frees mp, but that doesn't exist as of yet. So is
> > > > > that
> > > > > a
> > > > > memory leak as of this patch?
> > > > > 
> > > > > We also call xfs_free_fsname() here (which doesn't reset
> > > > > pointers
> > > > > to
> > > > > NULL) and open-code kfree()'s of a couple of the same fields in
> > > > > xfs_fc_free(). Those look like double frees to me.
> > > > > 
> > > > > Hmm.. I guess I'm kind of wondering why we lift the mp alloc
> > > > > out of
> > > > > the
> > > > > fill super call in the first place. At a glance, it doesn't
> > > > > look
> > > > > like
> > > > > we
> > > > > do anything in that xfs_init_fs_context() call that we couldn't
> > > > > do
> > > > > a
> > > > > bit
> > > > > later..
> > > > 
> > > > Umm ... yes ...
> > > > 
> > > > I think I've got the active code path right ...
> > > > 
> > > > At this point .mount == xfs_fs_mount() which will calls
> > > > xfs_fs_fill_super() to fill the super block.
> > > > 
> > > > xfs_fs_fill_super() allocates the super block info struct and
> > > > sets
> > > > it in the super block private info field, then calls
> > > > xfs_parseargs()
> > > > which still allocates mp->m_fsname at this point, to accomodate a
> > > > similar free pattern in xfs_test_remount_options().
> > > > 
> > > > It then calls __xfs_fs_fill_super() which doesn't touch those
> > > > fsname
> > > > fields or mp to fit in with what will be done later.
> > > > 
> > > > If an error occurs both the fsname fields (xfs_free_fsname()) and
> > > > mp
> > > > are freed by the main caller, xfs_fs_fill_super().
> > > > 
> > > > I think that process is ok.
> > > > 
> > > > The mount api process that isn't active yet is a bit different.
> > > > 
> > > > The context (ctx), a temporary working space, is allocated then
> > > > saved
> > > > in the mount context (fc) and the super block info is also
> > > > allocated
> > > > and saved in the mount context in it's field of the same name as
> > > > the
> > > > private super block info field, s_fs_info.
> > > > 
> > > > The function xfs_fill_super() is called as a result of the
> > > > .get_tree()
> > > > mount context operation to fill the super block.
> > > > 
> > > > During this process, when the VFS successfully allocates the
> > > > super
> > > > block s_fs_info is set in the super block and the mount context
> > > > field set to NULL. From this point freeing the private super
> > > > block
> > > > info becomes part of usual freeing of the super block with the
> > > > super
> > > > operation .kill_sb().
> > > > 
> > > > But if the super block allocation fails then the mount context
> > > > s_fs_info field remains set and is the responsibility of the
> > > > mount context operations .fc_free() method to clean up.
> > > > 
> > > > Now the VFS calls to xfs_fill_super() after this.
> > > > 
> > > > I should have been able to leave xfs_fill_super() it as it
> > > > was with:
> > > >         sb->s_fs_info = NULL;
> > > >         xfs_free_fsname(mp);
> > > >         kfree(mp);
> > > > and that should have been ok but it wasn't, there was some sort
> > > > of
> > > > allocation problem, possibly a double free, causing a crash.
> > > > 
> > > > Strictly speaking this cleanup process should be carried out by
> > > > either the mount context .fc_free() or super operation .kill_sb()
> > > > and that's what I want to do.
> > > 
> > > Umm ... but I can't actually do that ...
> > > 
> > > Looking back at xfs I realize that the filling of the super
> > > block is meant to leave nothing allocated and set
> > > sb->s_fs_info = NULL on error so that ->put_super() won't try
> > > and cleanup a whole bunch of stuff that hasn't been done.
> > > 
> > > Which brings me back to what I originally had above ... which
> > > we believe doesn't work ?
> > > 
> > 
> > It looks like perhaps the assignment of sb->s_fs_info was lost as
> > well?
> > Skipping to the end, I see xfs_init_fs_context() alloc mp and assign
> > fc->s_fs_info. xfs_get_tree() leads to xfs_fill_super(), which
> > somehow
> > gets mp from sb->s_fs_info (not fc->...), but then resets sb-
> > >s_fs_info
> > on error and frees the names, leaving fs->s_fs_info so presumably
> > xfs_fc_free() can free mp along with a couple of the names (again). I
> > can't really make heads or tails of what this is even attempting to
> > do.
> 
> Ha, it seems a bit mysterious, but it's actually much simpler
> than it appears.
> 

Feel free to explain any of the above..? Where do you currently assign
sb->s_fs_info, for example?

> > 
> > That aside, it's not clear to me why the new code can't follow a
> > similar
> > pattern as the old code with regard to allocation. Allocate mp in
> > xfs_fill_super() and set up sb/fc pointers, reset pointers and free
> > mp
> > on error return. Otherwise, xfs_fc_free() checks for fc->s_fs_info !=
> > NULL and frees mp from there. Is there some reason we can't continue
> > to
> > do that?
> 
> I think not without a fairly significant re-design.
> 
> The main difference is the mount-api will allocate the super
> block later than the old mount code.
> 
> Basically, if file system parameter parsing fails the super
> block won't get allocated.
> 
> So the super block isn't available during parameter parsing
> but the file system private data structure may be needed for
> it, so it comes from the file system context at that point.
> 
> When the super block is successfully allocated the file system
> private data structure is set in the super block (and the field
> NULLed in the context) and things progress much the same as
> before from that point.
> 
> That's the essential difference in the process AFAICS.
> 

I see. This is probably something that should be noted in the commit
log (that the ordering changes from before such that we need to allocate
mp a bit earlier). This is reasonable because even though the current
code allocs mp in the fill_super callback, we parse arguments
immediately after the mp allocation and don't otherwise rely on the sb
in that code.

If I follow correctly, it sounds like perhaps we need to separate the
management of sb->s_fs_info from the "ownership" of mp. For example,
allocate mp, assign fc->s_fs_info and free via xfs_fc_free() as you do
now. In the xfs_fill_super() callback, pull mp from fc->s_fs_info and
assign it to sb->s_fs_info. If we fail at this point, reset
sb->s_fs_info to NULL and let the fc infrastructure deal with freeing mp
in its own callback.

What I'm not clear on is whether something like xfs_fs_put_super()
should still free mp as well. Once the filesystem successfully mounts,
are we still going to see an xfs_fc_free() callback, or is this all just
transient mount path stuff? If the former, perhaps put_super() should
also not free mp and just reset its own ->s_fs_info reference. If the
latter, then I guess we just need to understand at what point during a
successful mount responsibility to free transfers from one place to the
other. Thoughts?

Brian

> By the time fill_super() is called everything is set and you
> should be able to proceed almost the same as before.
> 
> Ian
> 
> > Brian
> > 
> > > > So I'm not sure the allocation time and the place this is done
> > > > can (or should) be done differently.
> > > > 
> > > > And that freeing on error exit from xfs_fill_super() is
> > > > definitely
> > > > wrong now! Ha, and I didn't see any crashes myself when I tested
> > > > it ... maybe I need a reproducer ...
> > > > 
> > > > Ian
> > > > 
> > > > > Brian
> > > > > 
> > > > > > +	return error;
> > > > > > +}
> > > > > > +
> > > > > > +STATIC int
> > > > > > +xfs_get_tree(
> > > > > > +	struct fs_context	*fc)
> > > > > > +{
> > > > > > +	return vfs_get_block_super(fc, xfs_fill_super);
> > > > > > +}
> > > > > > +
> > > > > >  STATIC void
> > > > > >  xfs_fs_put_super(
> > > > > >  	struct super_block	*sb)
> > > > > > @@ -2003,6 +2048,11 @@ static const struct super_operations
> > > > > > xfs_super_operations = {
> > > > > >  	.free_cached_objects	= xfs_fs_free_cached_objects,
> > > > > >  };
> > > > > >  
> > > > > > +static const struct fs_context_operations xfs_context_ops =
> > > > > > {
> > > > > > +	.parse_param = xfs_parse_param,
> > > > > > +	.get_tree    = xfs_get_tree,
> > > > > > +};
> > > > > > +
> > > > > >  static struct file_system_type xfs_fs_type = {
> > > > > >  	.owner			= THIS_MODULE,
> > > > > >  	.name			= "xfs",
> > > > > > 
> 

Re: [REPOST PATCH v3 09/16] xfs: mount-api - add xfs_get_tree()

[Ian Kent]

On Thu, 2019-09-26 at 07:14 -0400, Brian Foster wrote:
> On Thu, Sep 26, 2019 at 11:27:40AM +0800, Ian Kent wrote:
> > On Wed, 2019-09-25 at 10:34 -0400, Brian Foster wrote:
> > > On Wed, Sep 25, 2019 at 04:07:08PM +0800, Ian Kent wrote:
> > > > On Wed, 2019-09-25 at 15:42 +0800, Ian Kent wrote:
> > > > > On Tue, 2019-09-24 at 10:38 -0400, Brian Foster wrote:
> > > > > > On Tue, Sep 24, 2019 at 09:22:49PM +0800, Ian Kent wrote:
> > > > > > > Add the fs_context_operations method .get_tree that
> > > > > > > validates
> > > > > > > mount options and fills the super block as previously
> > > > > > > done
> > > > > > > by the file_system_type .mount method.
> > > > > > > 
> > > > > > > Signed-off-by: Ian Kent <raven@themaw.net>
> > > > > > > ---
> > > > > > >  fs/xfs/xfs_super.c |   50
> > > > > > > ++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > > > >  1 file changed, 50 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > > > > > > index ea3640ffd8f5..6f9fe92b4e21 100644
> > > > > > > --- a/fs/xfs/xfs_super.c
> > > > > > > +++ b/fs/xfs/xfs_super.c
> > > > > > > @@ -1933,6 +1933,51 @@ xfs_fs_fill_super(
> > > > > > >  	return error;
> > > > > > >  }
> > > > > > >  
> > > > > > > +STATIC int
> > > > > > > +xfs_fill_super(
> > > > > > > +	struct super_block	*sb,
> > > > > > > +	struct fs_context	*fc)
> > > > > > > +{
> > > > > > > +	struct xfs_fs_context	*ctx = fc->fs_private;
> > > > > > > +	struct xfs_mount	*mp = sb->s_fs_info;
> > > > > > > +	int			silent = fc->sb_flags &
> > > > > > > SB_SILENT;
> > > > > > > +	int			error = -ENOMEM;
> > > > > > > +
> > > > > > > +	mp->m_super = sb;
> > > > > > > +
> > > > > > > +	/*
> > > > > > > +	 * set up the mount name first so all the errors will
> > > > > > > refer to
> > > > > > > the
> > > > > > > +	 * correct device.
> > > > > > > +	 */
> > > > > > > +	mp->m_fsname = kstrndup(sb->s_id, MAXNAMELEN,
> > > > > > > GFP_KERNEL);
> > > > > > > +	if (!mp->m_fsname)
> > > > > > > +		return -ENOMEM;
> > > > > > > +	mp->m_fsname_len = strlen(mp->m_fsname) + 1;
> > > > > > > +
> > > > > > > +	error = xfs_validate_params(mp, ctx, false);
> > > > > > > +	if (error)
> > > > > > > +		goto out_free_fsname;
> > > > > > > +
> > > > > > > +	error = __xfs_fs_fill_super(mp, silent);
> > > > > > > +	if (error)
> > > > > > > +		goto out_free_fsname;
> > > > > > > +
> > > > > > > +	return 0;
> > > > > > > +
> > > > > > > + out_free_fsname:
> > > > > > > +	sb->s_fs_info = NULL;
> > > > > > > +	xfs_free_fsname(mp);
> > > > > > > +
> > > > > > 
> > > > > > I'm still not following the (intended) lifecycle of mp
> > > > > > here.
> > > > > > Looking
> > > > > > ahead in the series, we allocate mp in
> > > > > > xfs_init_fs_context()
> > > > > > and
> > > > > > set
> > > > > > some state. It looks like at some point we grow an
> > > > > > xfs_fc_free()
> > > > > > callback that frees mp, but that doesn't exist as of yet.
> > > > > > So is
> > > > > > that
> > > > > > a
> > > > > > memory leak as of this patch?
> > > > > > 
> > > > > > We also call xfs_free_fsname() here (which doesn't reset
> > > > > > pointers
> > > > > > to
> > > > > > NULL) and open-code kfree()'s of a couple of the same
> > > > > > fields in
> > > > > > xfs_fc_free(). Those look like double frees to me.
> > > > > > 
> > > > > > Hmm.. I guess I'm kind of wondering why we lift the mp
> > > > > > alloc
> > > > > > out of
> > > > > > the
> > > > > > fill super call in the first place. At a glance, it doesn't
> > > > > > look
> > > > > > like
> > > > > > we
> > > > > > do anything in that xfs_init_fs_context() call that we
> > > > > > couldn't
> > > > > > do
> > > > > > a
> > > > > > bit
> > > > > > later..
> > > > > 
> > > > > Umm ... yes ...
> > > > > 
> > > > > I think I've got the active code path right ...
> > > > > 
> > > > > At this point .mount == xfs_fs_mount() which will calls
> > > > > xfs_fs_fill_super() to fill the super block.
> > > > > 
> > > > > xfs_fs_fill_super() allocates the super block info struct and
> > > > > sets
> > > > > it in the super block private info field, then calls
> > > > > xfs_parseargs()
> > > > > which still allocates mp->m_fsname at this point, to
> > > > > accomodate a
> > > > > similar free pattern in xfs_test_remount_options().
> > > > > 
> > > > > It then calls __xfs_fs_fill_super() which doesn't touch those
> > > > > fsname
> > > > > fields or mp to fit in with what will be done later.
> > > > > 
> > > > > If an error occurs both the fsname fields (xfs_free_fsname())
> > > > > and
> > > > > mp
> > > > > are freed by the main caller, xfs_fs_fill_super().
> > > > > 
> > > > > I think that process is ok.
> > > > > 
> > > > > The mount api process that isn't active yet is a bit
> > > > > different.
> > > > > 
> > > > > The context (ctx), a temporary working space, is allocated
> > > > > then
> > > > > saved
> > > > > in the mount context (fc) and the super block info is also
> > > > > allocated
> > > > > and saved in the mount context in it's field of the same name
> > > > > as
> > > > > the
> > > > > private super block info field, s_fs_info.
> > > > > 
> > > > > The function xfs_fill_super() is called as a result of the
> > > > > .get_tree()
> > > > > mount context operation to fill the super block.
> > > > > 
> > > > > During this process, when the VFS successfully allocates the
> > > > > super
> > > > > block s_fs_info is set in the super block and the mount
> > > > > context
> > > > > field set to NULL. From this point freeing the private super
> > > > > block
> > > > > info becomes part of usual freeing of the super block with
> > > > > the
> > > > > super
> > > > > operation .kill_sb().
> > > > > 
> > > > > But if the super block allocation fails then the mount
> > > > > context
> > > > > s_fs_info field remains set and is the responsibility of the
> > > > > mount context operations .fc_free() method to clean up.
> > > > > 
> > > > > Now the VFS calls to xfs_fill_super() after this.
> > > > > 
> > > > > I should have been able to leave xfs_fill_super() it as it
> > > > > was with:
> > > > >         sb->s_fs_info = NULL;
> > > > >         xfs_free_fsname(mp);
> > > > >         kfree(mp);
> > > > > and that should have been ok but it wasn't, there was some
> > > > > sort
> > > > > of
> > > > > allocation problem, possibly a double free, causing a crash.
> > > > > 
> > > > > Strictly speaking this cleanup process should be carried out
> > > > > by
> > > > > either the mount context .fc_free() or super operation
> > > > > .kill_sb()
> > > > > and that's what I want to do.
> > > > 
> > > > Umm ... but I can't actually do that ...
> > > > 
> > > > Looking back at xfs I realize that the filling of the super
> > > > block is meant to leave nothing allocated and set
> > > > sb->s_fs_info = NULL on error so that ->put_super() won't try
> > > > and cleanup a whole bunch of stuff that hasn't been done.
> > > > 
> > > > Which brings me back to what I originally had above ... which
> > > > we believe doesn't work ?
> > > > 
> > > 
> > > It looks like perhaps the assignment of sb->s_fs_info was lost as
> > > well?
> > > Skipping to the end, I see xfs_init_fs_context() alloc mp and
> > > assign
> > > fc->s_fs_info. xfs_get_tree() leads to xfs_fill_super(), which
> > > somehow
> > > gets mp from sb->s_fs_info (not fc->...), but then resets sb-
> > > > s_fs_info
> > > on error and frees the names, leaving fs->s_fs_info so presumably
> > > xfs_fc_free() can free mp along with a couple of the names
> > > (again). I
> > > can't really make heads or tails of what this is even attempting
> > > to
> > > do.
> > 
> > Ha, it seems a bit mysterious, but it's actually much simpler
> > than it appears.
> > 
> 
> Feel free to explain any of the above..? Where do you currently
> assign
> sb->s_fs_info, for example?

The VFS does the assignment, see below.

> 
> > > That aside, it's not clear to me why the new code can't follow a
> > > similar
> > > pattern as the old code with regard to allocation. Allocate mp in
> > > xfs_fill_super() and set up sb/fc pointers, reset pointers and
> > > free
> > > mp
> > > on error return. Otherwise, xfs_fc_free() checks for fc-
> > > >s_fs_info !=
> > > NULL and frees mp from there. Is there some reason we can't
> > > continue
> > > to
> > > do that?
> > 
> > I think not without a fairly significant re-design.
> > 
> > The main difference is the mount-api will allocate the super
> > block later than the old mount code.
> > 
> > Basically, if file system parameter parsing fails the super
> > block won't get allocated.
> > 
> > So the super block isn't available during parameter parsing
> > but the file system private data structure may be needed for
> > it, so it comes from the file system context at that point.
> > 
> > When the super block is successfully allocated the file system
> > private data structure is set in the super block (and the field
> > NULLed in the context) and things progress much the same as
> > before from that point.
> > 
> > That's the essential difference in the process AFAICS.
> > 
> 
> I see. This is probably something that should be noted in the commit
> log (that the ordering changes from before such that we need to
> allocate
> mp a bit earlier). This is reasonable because even though the current
> code allocs mp in the fill_super callback, we parse arguments
> immediately after the mp allocation and don't otherwise rely on the
> sb
> in that code.
> 
> If I follow correctly, it sounds like perhaps we need to separate the
> management of sb->s_fs_info from the "ownership" of mp. For example,
> allocate mp, assign fc->s_fs_info and free via xfs_fc_free() as you
> do
> now. In the xfs_fill_super() callback, pull mp from fc->s_fs_info and
> assign it to sb->s_fs_info. If we fail at this point, reset
> sb->s_fs_info to NULL and let the fc infrastructure deal with freeing
> mp
> in its own callback.

The mount api callbacks are for use during setup and don't really
apply once that's done by the mount api.

Lets describe the life cycle of s_fs_info by going through the call
path from the xfs_get_tree() fs context operation.

At this point the context has been setup by .init_fs_context ( aka.
xfs_init_fs_context()), and fc->s_fs_info == mp and parameter parsing
has been done.

Then the .get_tree() method gets called (aka. fs_get_tree) and all
this does (now) is call get_tree_bdev() as Al requested.

Even though that's changed it still ends up calling similar VFS
functions to what it did before.

Now:
int get_tree_bdev(struct fs_context *fc,
                int (*fill_super)(struct super_block *,
                                  struct fs_context *))
{

skip ... (the usual stuff until we get to)

        fc->sb_flags |= SB_NOSEC;
        fc->sget_key = bdev;
        s = sget_fc(fc, test_bdev_super_fc, set_bdev_super_fc);
        mutex_unlock(&bdev->bd_fsfreeze_mutex);
        if (IS_ERR(s))
                return PTR_ERR(s);

        if (s->s_root) {
		snip ... (mount root already exists)
	} else {
                s->s_mode = mode;
                snprintf(s->s_id, sizeof(s->s_id), "%pg", bdev);
                sb_set_blocksize(s, block_size(bdev));
                error = fill_super(s, fc);
                if (error) {
                        deactivate_locked_super(s);
                        return error;
                }

                s->s_flags |= SB_ACTIVE;
                bdev->bd_super = s;
        }

        BUG_ON(fc->root);
        fc->root = dget(s->s_root);
	return 0;
}

This is to establish the order of what's done.

The thing we want to look at is the sget_fc() function.

struct super_block *sget_fc(struct fs_context *fc,
           int (*test)(struct super_block *, struct fs_context *),
           int (*set)(struct super_block *, struct fs_context *))
{
	snip ... (usual stuff that finds or allocates a super block)

        s->s_fs_info = fc->s_fs_info;
        err = set(s, fc);
        if (err) {
                s->s_fs_info = NULL;
                spin_unlock(&sb_lock);
                destroy_unused_super(s);
                return ERR_PTR(err);
        }
        fc->s_fs_info = NULL;

	snip ... (remaining setup stuff)

	return s;
}

This is where the s_fs_info changes from being the responsibility
of the fs context to being the responsibility of the super block.

And you can see that this is done before fill_super() is called.

Finally, fill_super() in xfs will do all the fs specific setup
and either return success or it will undo anything it has done
when returning an error, just as it did previously.

Specifically, it must set sb->s_fs_info == NULL on failure so
that when the VFS puts the super block xfs doesn't try and
release a whole bunch of stuff that wasn't setup. Consequently
mp must also be freed on error return from fill_super().

What I'm saying is that by the time fill_super() is called
everything is is a state where things need to be done pretty
much as though the mount api context isn't present, the only
thing that muddies the order of things is the calling of
.fc_free() that happens a bit later but isn't relevant to
this ordering of operations.

So I'm not sure there needs to be (or can be) changes to
the way s_fs_info is handled in the implementation.

We can go into more detail if it's needed, I am trying to
explain my motivations for doing what I've done, ;)

> 
> What I'm not clear on is whether something like xfs_fs_put_super()
> should still free mp as well. Once the filesystem successfully
> mounts,
> are we still going to see an xfs_fc_free() callback, or is this all
> just
> transient mount path stuff? If the former, perhaps put_super() should
> also not free mp and just reset its own ->s_fs_info reference. If the
> latter, then I guess we just need to understand at what point during
> a
> successful mount responsibility to free transfers from one place to
> the
> other. Thoughts?

put_super() should continue to function the same way it does
now provided fill_super() does the right thing, which I'm
pretty sure it (the mount api version) does now .

xfs_fs_put_super() won't need to free mp if an error occurs,
s_fs_info needs to be NULL in that case as has always been the
case.

And realize xfs_fc_free() (aka. .fc_free()) is largely irrelevant
from the POV of xfs once we get to fill_super().

> 
> Brian
> 
> > By the time fill_super() is called everything is set and you
> > should be able to proceed almost the same as before.
> > 
> > Ian
> > 
> > > Brian
> > > 
> > > > > So I'm not sure the allocation time and the place this is
> > > > > done
> > > > > can (or should) be done differently.
> > > > > 
> > > > > And that freeing on error exit from xfs_fill_super() is
> > > > > definitely
> > > > > wrong now! Ha, and I didn't see any crashes myself when I
> > > > > tested
> > > > > it ... maybe I need a reproducer ...
> > > > > 
> > > > > Ian
> > > > > 
> > > > > > Brian
> > > > > > 
> > > > > > > +	return error;
> > > > > > > +}
> > > > > > > +
> > > > > > > +STATIC int
> > > > > > > +xfs_get_tree(
> > > > > > > +	struct fs_context	*fc)
> > > > > > > +{
> > > > > > > +	return vfs_get_block_super(fc, xfs_fill_super);
> > > > > > > +}
> > > > > > > +
> > > > > > >  STATIC void
> > > > > > >  xfs_fs_put_super(
> > > > > > >  	struct super_block	*sb)
> > > > > > > @@ -2003,6 +2048,11 @@ static const struct
> > > > > > > super_operations
> > > > > > > xfs_super_operations = {
> > > > > > >  	.free_cached_objects	= xfs_fs_free_cached_objects,
> > > > > > >  };
> > > > > > >  
> > > > > > > +static const struct fs_context_operations
> > > > > > > xfs_context_ops =
> > > > > > > {
> > > > > > > +	.parse_param = xfs_parse_param,
> > > > > > > +	.get_tree    = xfs_get_tree,
> > > > > > > +};
> > > > > > > +
> > > > > > >  static struct file_system_type xfs_fs_type = {
> > > > > > >  	.owner			= THIS_MODULE,
> > > > > > >  	.name			= "xfs",
> > > > > > > 

Re: [REPOST PATCH v3 09/16] xfs: mount-api - add xfs_get_tree()

[Brian Foster]

On Fri, Sep 27, 2019 at 09:16:28AM +0800, Ian Kent wrote:
> On Thu, 2019-09-26 at 07:14 -0400, Brian Foster wrote:
> > On Thu, Sep 26, 2019 at 11:27:40AM +0800, Ian Kent wrote:
> > > On Wed, 2019-09-25 at 10:34 -0400, Brian Foster wrote:
> > > > On Wed, Sep 25, 2019 at 04:07:08PM +0800, Ian Kent wrote:
> > > > > On Wed, 2019-09-25 at 15:42 +0800, Ian Kent wrote:
> > > > > > On Tue, 2019-09-24 at 10:38 -0400, Brian Foster wrote:
> > > > > > > On Tue, Sep 24, 2019 at 09:22:49PM +0800, Ian Kent wrote:
> > > > > > > > Add the fs_context_operations method .get_tree that
> > > > > > > > validates
> > > > > > > > mount options and fills the super block as previously
> > > > > > > > done
> > > > > > > > by the file_system_type .mount method.
> > > > > > > > 
> > > > > > > > Signed-off-by: Ian Kent <raven@themaw.net>
> > > > > > > > ---
> > > > > > > >  fs/xfs/xfs_super.c |   50
> > > > > > > > ++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > > > > >  1 file changed, 50 insertions(+)
> > > > > > > > 
> > > > > > > > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > > > > > > > index ea3640ffd8f5..6f9fe92b4e21 100644
> > > > > > > > --- a/fs/xfs/xfs_super.c
> > > > > > > > +++ b/fs/xfs/xfs_super.c
> > > > > > > > @@ -1933,6 +1933,51 @@ xfs_fs_fill_super(
> > > > > > > >  	return error;
> > > > > > > >  }
> > > > > > > >  
> > > > > > > > +STATIC int
> > > > > > > > +xfs_fill_super(
> > > > > > > > +	struct super_block	*sb,
> > > > > > > > +	struct fs_context	*fc)
> > > > > > > > +{
> > > > > > > > +	struct xfs_fs_context	*ctx = fc->fs_private;
> > > > > > > > +	struct xfs_mount	*mp = sb->s_fs_info;
> > > > > > > > +	int			silent = fc->sb_flags &
> > > > > > > > SB_SILENT;
> > > > > > > > +	int			error = -ENOMEM;
> > > > > > > > +
> > > > > > > > +	mp->m_super = sb;
> > > > > > > > +
> > > > > > > > +	/*
> > > > > > > > +	 * set up the mount name first so all the errors will
> > > > > > > > refer to
> > > > > > > > the
> > > > > > > > +	 * correct device.
> > > > > > > > +	 */
> > > > > > > > +	mp->m_fsname = kstrndup(sb->s_id, MAXNAMELEN,
> > > > > > > > GFP_KERNEL);
> > > > > > > > +	if (!mp->m_fsname)
> > > > > > > > +		return -ENOMEM;
> > > > > > > > +	mp->m_fsname_len = strlen(mp->m_fsname) + 1;
> > > > > > > > +
> > > > > > > > +	error = xfs_validate_params(mp, ctx, false);
> > > > > > > > +	if (error)
> > > > > > > > +		goto out_free_fsname;
> > > > > > > > +
> > > > > > > > +	error = __xfs_fs_fill_super(mp, silent);
> > > > > > > > +	if (error)
> > > > > > > > +		goto out_free_fsname;
> > > > > > > > +
> > > > > > > > +	return 0;
> > > > > > > > +
> > > > > > > > + out_free_fsname:
> > > > > > > > +	sb->s_fs_info = NULL;
> > > > > > > > +	xfs_free_fsname(mp);
> > > > > > > > +
> > > > > > > 
> > > > > > > I'm still not following the (intended) lifecycle of mp
> > > > > > > here.
> > > > > > > Looking
> > > > > > > ahead in the series, we allocate mp in
> > > > > > > xfs_init_fs_context()
> > > > > > > and
> > > > > > > set
> > > > > > > some state. It looks like at some point we grow an
> > > > > > > xfs_fc_free()
> > > > > > > callback that frees mp, but that doesn't exist as of yet.
> > > > > > > So is
> > > > > > > that
> > > > > > > a
> > > > > > > memory leak as of this patch?
> > > > > > > 
> > > > > > > We also call xfs_free_fsname() here (which doesn't reset
> > > > > > > pointers
> > > > > > > to
> > > > > > > NULL) and open-code kfree()'s of a couple of the same
> > > > > > > fields in
> > > > > > > xfs_fc_free(). Those look like double frees to me.
> > > > > > > 
> > > > > > > Hmm.. I guess I'm kind of wondering why we lift the mp
> > > > > > > alloc
> > > > > > > out of
> > > > > > > the
> > > > > > > fill super call in the first place. At a glance, it doesn't
> > > > > > > look
> > > > > > > like
> > > > > > > we
> > > > > > > do anything in that xfs_init_fs_context() call that we
> > > > > > > couldn't
> > > > > > > do
> > > > > > > a
> > > > > > > bit
> > > > > > > later..
> > > > > > 
> > > > > > Umm ... yes ...
> > > > > > 
> > > > > > I think I've got the active code path right ...
> > > > > > 
> > > > > > At this point .mount == xfs_fs_mount() which will calls
> > > > > > xfs_fs_fill_super() to fill the super block.
> > > > > > 
> > > > > > xfs_fs_fill_super() allocates the super block info struct and
> > > > > > sets
> > > > > > it in the super block private info field, then calls
> > > > > > xfs_parseargs()
> > > > > > which still allocates mp->m_fsname at this point, to
> > > > > > accomodate a
> > > > > > similar free pattern in xfs_test_remount_options().
> > > > > > 
> > > > > > It then calls __xfs_fs_fill_super() which doesn't touch those
> > > > > > fsname
> > > > > > fields or mp to fit in with what will be done later.
> > > > > > 
> > > > > > If an error occurs both the fsname fields (xfs_free_fsname())
> > > > > > and
> > > > > > mp
> > > > > > are freed by the main caller, xfs_fs_fill_super().
> > > > > > 
> > > > > > I think that process is ok.
> > > > > > 
> > > > > > The mount api process that isn't active yet is a bit
> > > > > > different.
> > > > > > 
> > > > > > The context (ctx), a temporary working space, is allocated
> > > > > > then
> > > > > > saved
> > > > > > in the mount context (fc) and the super block info is also
> > > > > > allocated
> > > > > > and saved in the mount context in it's field of the same name
> > > > > > as
> > > > > > the
> > > > > > private super block info field, s_fs_info.
> > > > > > 
> > > > > > The function xfs_fill_super() is called as a result of the
> > > > > > .get_tree()
> > > > > > mount context operation to fill the super block.
> > > > > > 
> > > > > > During this process, when the VFS successfully allocates the
> > > > > > super
> > > > > > block s_fs_info is set in the super block and the mount
> > > > > > context
> > > > > > field set to NULL. From this point freeing the private super
> > > > > > block
> > > > > > info becomes part of usual freeing of the super block with
> > > > > > the
> > > > > > super
> > > > > > operation .kill_sb().
> > > > > > 
> > > > > > But if the super block allocation fails then the mount
> > > > > > context
> > > > > > s_fs_info field remains set and is the responsibility of the
> > > > > > mount context operations .fc_free() method to clean up.
> > > > > > 
> > > > > > Now the VFS calls to xfs_fill_super() after this.
> > > > > > 
> > > > > > I should have been able to leave xfs_fill_super() it as it
> > > > > > was with:
> > > > > >         sb->s_fs_info = NULL;
> > > > > >         xfs_free_fsname(mp);
> > > > > >         kfree(mp);
> > > > > > and that should have been ok but it wasn't, there was some
> > > > > > sort
> > > > > > of
> > > > > > allocation problem, possibly a double free, causing a crash.
> > > > > > 
> > > > > > Strictly speaking this cleanup process should be carried out
> > > > > > by
> > > > > > either the mount context .fc_free() or super operation
> > > > > > .kill_sb()
> > > > > > and that's what I want to do.
> > > > > 
> > > > > Umm ... but I can't actually do that ...
> > > > > 
> > > > > Looking back at xfs I realize that the filling of the super
> > > > > block is meant to leave nothing allocated and set
> > > > > sb->s_fs_info = NULL on error so that ->put_super() won't try
> > > > > and cleanup a whole bunch of stuff that hasn't been done.
> > > > > 
> > > > > Which brings me back to what I originally had above ... which
> > > > > we believe doesn't work ?
> > > > > 
> > > > 
> > > > It looks like perhaps the assignment of sb->s_fs_info was lost as
> > > > well?
> > > > Skipping to the end, I see xfs_init_fs_context() alloc mp and
> > > > assign
> > > > fc->s_fs_info. xfs_get_tree() leads to xfs_fill_super(), which
> > > > somehow
> > > > gets mp from sb->s_fs_info (not fc->...), but then resets sb-
> > > > > s_fs_info
> > > > on error and frees the names, leaving fs->s_fs_info so presumably
> > > > xfs_fc_free() can free mp along with a couple of the names
> > > > (again). I
> > > > can't really make heads or tails of what this is even attempting
> > > > to
> > > > do.
> > > 
> > > Ha, it seems a bit mysterious, but it's actually much simpler
> > > than it appears.
> > > 
> > 
> > Feel free to explain any of the above..? Where do you currently
> > assign
> > sb->s_fs_info, for example?
> 
> The VFS does the assignment, see below.
> 
> > 
> > > > That aside, it's not clear to me why the new code can't follow a
> > > > similar
> > > > pattern as the old code with regard to allocation. Allocate mp in
> > > > xfs_fill_super() and set up sb/fc pointers, reset pointers and
> > > > free
> > > > mp
> > > > on error return. Otherwise, xfs_fc_free() checks for fc-
> > > > >s_fs_info !=
> > > > NULL and frees mp from there. Is there some reason we can't
> > > > continue
> > > > to
> > > > do that?
> > > 
> > > I think not without a fairly significant re-design.
> > > 
> > > The main difference is the mount-api will allocate the super
> > > block later than the old mount code.
> > > 
> > > Basically, if file system parameter parsing fails the super
> > > block won't get allocated.
> > > 
> > > So the super block isn't available during parameter parsing
> > > but the file system private data structure may be needed for
> > > it, so it comes from the file system context at that point.
> > > 
> > > When the super block is successfully allocated the file system
> > > private data structure is set in the super block (and the field
> > > NULLed in the context) and things progress much the same as
> > > before from that point.
> > > 
> > > That's the essential difference in the process AFAICS.
> > > 
> > 
> > I see. This is probably something that should be noted in the commit
> > log (that the ordering changes from before such that we need to
> > allocate
> > mp a bit earlier). This is reasonable because even though the current
> > code allocs mp in the fill_super callback, we parse arguments
> > immediately after the mp allocation and don't otherwise rely on the
> > sb
> > in that code.
> > 
> > If I follow correctly, it sounds like perhaps we need to separate the
> > management of sb->s_fs_info from the "ownership" of mp. For example,
> > allocate mp, assign fc->s_fs_info and free via xfs_fc_free() as you
> > do
> > now. In the xfs_fill_super() callback, pull mp from fc->s_fs_info and
> > assign it to sb->s_fs_info. If we fail at this point, reset
> > sb->s_fs_info to NULL and let the fc infrastructure deal with freeing
> > mp
> > in its own callback.
> 
> The mount api callbacks are for use during setup and don't really
> apply once that's done by the mount api.
> 
> Lets describe the life cycle of s_fs_info by going through the call
> path from the xfs_get_tree() fs context operation.
> 
> At this point the context has been setup by .init_fs_context ( aka.
> xfs_init_fs_context()), and fc->s_fs_info == mp and parameter parsing
> has been done.
> 
> Then the .get_tree() method gets called (aka. fs_get_tree) and all
> this does (now) is call get_tree_bdev() as Al requested.
> 
> Even though that's changed it still ends up calling similar VFS
> functions to what it did before.
> 
> Now:
> int get_tree_bdev(struct fs_context *fc,
>                 int (*fill_super)(struct super_block *,
>                                   struct fs_context *))
> {
> 
> skip ... (the usual stuff until we get to)
> 
>         fc->sb_flags |= SB_NOSEC;
>         fc->sget_key = bdev;
>         s = sget_fc(fc, test_bdev_super_fc, set_bdev_super_fc);
>         mutex_unlock(&bdev->bd_fsfreeze_mutex);
>         if (IS_ERR(s))
>                 return PTR_ERR(s);
> 
>         if (s->s_root) {
> 		snip ... (mount root already exists)
> 	} else {
>                 s->s_mode = mode;
>                 snprintf(s->s_id, sizeof(s->s_id), "%pg", bdev);
>                 sb_set_blocksize(s, block_size(bdev));
>                 error = fill_super(s, fc);
>                 if (error) {
>                         deactivate_locked_super(s);
>                         return error;
>                 }
> 
>                 s->s_flags |= SB_ACTIVE;
>                 bdev->bd_super = s;
>         }
> 
>         BUG_ON(fc->root);
>         fc->root = dget(s->s_root);
> 	return 0;
> }
> 
> This is to establish the order of what's done.
> 
> The thing we want to look at is the sget_fc() function.
> 
> struct super_block *sget_fc(struct fs_context *fc,
>            int (*test)(struct super_block *, struct fs_context *),
>            int (*set)(struct super_block *, struct fs_context *))
> {
> 	snip ... (usual stuff that finds or allocates a super block)
> 
>         s->s_fs_info = fc->s_fs_info;
>         err = set(s, fc);
>         if (err) {
>                 s->s_fs_info = NULL;
>                 spin_unlock(&sb_lock);
>                 destroy_unused_super(s);
>                 return ERR_PTR(err);
>         }
>         fc->s_fs_info = NULL;
> 
> 	snip ... (remaining setup stuff)
> 
> 	return s;
> }
> 
> This is where the s_fs_info changes from being the responsibility
> of the fs context to being the responsibility of the super block.
> 

Ok, so basically we allocate the mp earlier so the mount api stuff can
do mount option parsing and such, the vfs allocs the sb and transfers
->s_fs_info from the context to the sb and thus at this point the
responsibility to free mp transfers from xfs_fc_free() to either
xfs_fill_super() (on error) or xfs_fs_put_super(). Makes sense I think,
thanks for the explanation (though again I think this is the kind of
thing that should be documented in the commit log).

> And you can see that this is done before fill_super() is called.
> 

So what is the expected behavior if a failure occurs after sget_fc()
does the ->s_fs_info transfer and before fill_super() is called?

> Finally, fill_super() in xfs will do all the fs specific setup
> and either return success or it will undo anything it has done
> when returning an error, just as it did previously.
> 
> Specifically, it must set sb->s_fs_info == NULL on failure so
> that when the VFS puts the super block xfs doesn't try and
> release a whole bunch of stuff that wasn't setup. Consequently
> mp must also be freed on error return from fill_super().
> 

Ok.

> What I'm saying is that by the time fill_super() is called
> everything is is a state where things need to be done pretty
> much as though the mount api context isn't present, the only
> thing that muddies the order of things is the calling of
> .fc_free() that happens a bit later but isn't relevant to
> this ordering of operations.
> 

So the intent is that in the event of a fill_super() failure as
described above, the ->s_fs_info transfer would have already happened
and fc->s_fs_info would be NULL, right?

> So I'm not sure there needs to be (or can be) changes to
> the way s_fs_info is handled in the implementation.
> 
> We can go into more detail if it's needed, I am trying to
> explain my motivations for doing what I've done, ;)
> 
> > 
> > What I'm not clear on is whether something like xfs_fs_put_super()
> > should still free mp as well. Once the filesystem successfully
> > mounts,
> > are we still going to see an xfs_fc_free() callback, or is this all
> > just
> > transient mount path stuff? If the former, perhaps put_super() should
> > also not free mp and just reset its own ->s_fs_info reference. If the
> > latter, then I guess we just need to understand at what point during
> > a
> > successful mount responsibility to free transfers from one place to
> > the
> > other. Thoughts?
> 
> put_super() should continue to function the same way it does
> now provided fill_super() does the right thing, which I'm
> pretty sure it (the mount api version) does now .
> 
> xfs_fs_put_super() won't need to free mp if an error occurs,
> s_fs_info needs to be NULL in that case as has always been the
> case.
> 
> And realize xfs_fc_free() (aka. .fc_free()) is largely irrelevant
> from the POV of xfs once we get to fill_super().
> 

Ok.

Brian

> > 
> > Brian
> > 
> > > By the time fill_super() is called everything is set and you
> > > should be able to proceed almost the same as before.
> > > 
> > > Ian
> > > 
> > > > Brian
> > > > 
> > > > > > So I'm not sure the allocation time and the place this is
> > > > > > done
> > > > > > can (or should) be done differently.
> > > > > > 
> > > > > > And that freeing on error exit from xfs_fill_super() is
> > > > > > definitely
> > > > > > wrong now! Ha, and I didn't see any crashes myself when I
> > > > > > tested
> > > > > > it ... maybe I need a reproducer ...
> > > > > > 
> > > > > > Ian
> > > > > > 
> > > > > > > Brian
> > > > > > > 
> > > > > > > > +	return error;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +STATIC int
> > > > > > > > +xfs_get_tree(
> > > > > > > > +	struct fs_context	*fc)
> > > > > > > > +{
> > > > > > > > +	return vfs_get_block_super(fc, xfs_fill_super);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > >  STATIC void
> > > > > > > >  xfs_fs_put_super(
> > > > > > > >  	struct super_block	*sb)
> > > > > > > > @@ -2003,6 +2048,11 @@ static const struct
> > > > > > > > super_operations
> > > > > > > > xfs_super_operations = {
> > > > > > > >  	.free_cached_objects	= xfs_fs_free_cached_objects,
> > > > > > > >  };
> > > > > > > >  
> > > > > > > > +static const struct fs_context_operations
> > > > > > > > xfs_context_ops =
> > > > > > > > {
> > > > > > > > +	.parse_param = xfs_parse_param,
> > > > > > > > +	.get_tree    = xfs_get_tree,
> > > > > > > > +};
> > > > > > > > +
> > > > > > > >  static struct file_system_type xfs_fs_type = {
> > > > > > > >  	.owner			= THIS_MODULE,
> > > > > > > >  	.name			= "xfs",
> > > > > > > > 
> 

[REPOST PATCH v3 10/16] xfs: mount-api - add xfs_remount_rw() helper

[Ian Kent]

Factor the remount read write code into a helper to simplify the
subsequent change from the super block method .remount_fs to the
mount-api fs_context_operations method .reconfigure.

Signed-off-by: Ian Kent <raven@themaw.net>
---
 fs/xfs/xfs_super.c |  115 +++++++++++++++++++++++++++++-----------------------
 1 file changed, 64 insertions(+), 51 deletions(-)

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 6f9fe92b4e21..aaee32162950 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1371,6 +1371,68 @@ xfs_test_remount_options(
 	return error;
 }
 
+STATIC int
+xfs_remount_rw(
+	struct xfs_mount	*mp)
+{
+	xfs_sb_t		*sbp = &mp->m_sb;
+	int error;
+
+	if (mp->m_flags & XFS_MOUNT_NORECOVERY) {
+		xfs_warn(mp,
+			"ro->rw transition prohibited on norecovery mount");
+		return -EINVAL;
+	}
+
+	if (XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5 &&
+	    xfs_sb_has_ro_compat_feature(sbp, XFS_SB_FEAT_RO_COMPAT_UNKNOWN)) {
+		xfs_warn(mp,
+	"ro->rw transition prohibited on unknown (0x%x) ro-compat filesystem",
+			(sbp->sb_features_ro_compat &
+				XFS_SB_FEAT_RO_COMPAT_UNKNOWN));
+		return -EINVAL;
+	}
+
+	mp->m_flags &= ~XFS_MOUNT_RDONLY;
+
+	/*
+	 * If this is the first remount to writeable state we
+	 * might have some superblock changes to update.
+	 */
+	if (mp->m_update_sb) {
+		error = xfs_sync_sb(mp, false);
+		if (error) {
+			xfs_warn(mp, "failed to write sb changes");
+			return error;
+		}
+		mp->m_update_sb = false;
+	}
+
+	/*
+	 * Fill out the reserve pool if it is empty. Use the stashed
+	 * value if it is non-zero, otherwise go with the default.
+	 */
+	xfs_restore_resvblks(mp);
+	xfs_log_work_queue(mp);
+
+	/* Recover any CoW blocks that never got remapped. */
+	error = xfs_reflink_recover_cow(mp);
+	if (error) {
+		xfs_err(mp,
+			"Error %d recovering leftover CoW allocations.", error);
+			xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
+		return error;
+	}
+	xfs_start_block_reaping(mp);
+
+	/* Create the per-AG metadata reservation pool .*/
+	error = xfs_fs_reserve_ag_blocks(mp);
+	if (error && error != -ENOSPC)
+		return error;
+
+	return 0;
+}
+
 STATIC int
 xfs_fs_remount(
 	struct super_block	*sb,
@@ -1434,57 +1496,8 @@ xfs_fs_remount(
 
 	/* ro -> rw */
 	if ((mp->m_flags & XFS_MOUNT_RDONLY) && !(*flags & SB_RDONLY)) {
-		if (mp->m_flags & XFS_MOUNT_NORECOVERY) {
-			xfs_warn(mp,
-		"ro->rw transition prohibited on norecovery mount");
-			return -EINVAL;
-		}
-
-		if (XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5 &&
-		    xfs_sb_has_ro_compat_feature(sbp,
-					XFS_SB_FEAT_RO_COMPAT_UNKNOWN)) {
-			xfs_warn(mp,
-"ro->rw transition prohibited on unknown (0x%x) ro-compat filesystem",
-				(sbp->sb_features_ro_compat &
-					XFS_SB_FEAT_RO_COMPAT_UNKNOWN));
-			return -EINVAL;
-		}
-
-		mp->m_flags &= ~XFS_MOUNT_RDONLY;
-
-		/*
-		 * If this is the first remount to writeable state we
-		 * might have some superblock changes to update.
-		 */
-		if (mp->m_update_sb) {
-			error = xfs_sync_sb(mp, false);
-			if (error) {
-				xfs_warn(mp, "failed to write sb changes");
-				return error;
-			}
-			mp->m_update_sb = false;
-		}
-
-		/*
-		 * Fill out the reserve pool if it is empty. Use the stashed
-		 * value if it is non-zero, otherwise go with the default.
-		 */
-		xfs_restore_resvblks(mp);
-		xfs_log_work_queue(mp);
-
-		/* Recover any CoW blocks that never got remapped. */
-		error = xfs_reflink_recover_cow(mp);
-		if (error) {
-			xfs_err(mp,
-	"Error %d recovering leftover CoW allocations.", error);
-			xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
-			return error;
-		}
-		xfs_start_block_reaping(mp);
-
-		/* Create the per-AG metadata reservation pool .*/
-		error = xfs_fs_reserve_ag_blocks(mp);
-		if (error && error != -ENOSPC)
+		error = xfs_remount_rw(mp);
+		if (error)
 			return error;
 	}
 

[REPOST PATCH v3 11/16] xfs: mount-api - add xfs_remount_ro() helper

[Ian Kent]

Factor the remount read only code into a helper to simplify the
subsequent change from the super block method .remount_fs to the
mount-api fs_context_operations method .reconfigure.

Signed-off-by: Ian Kent <raven@themaw.net>
---
 fs/xfs/xfs_super.c |   73 +++++++++++++++++++++++++++++++---------------------
 1 file changed, 43 insertions(+), 30 deletions(-)

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index aaee32162950..de75891c5551 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1433,6 +1433,47 @@ xfs_remount_rw(
 	return 0;
 }
 
+STATIC int
+xfs_remount_ro(
+	struct xfs_mount	*mp)
+{
+	int error;
+
+	/*
+	 * Cancel background eofb scanning so it cannot race with the
+	 * final log force+buftarg wait and deadlock the remount.
+	 */
+	xfs_stop_block_reaping(mp);
+
+	/* Get rid of any leftover CoW reservations... */
+	error = xfs_icache_free_cowblocks(mp, NULL);
+	if (error) {
+		xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
+		return error;
+	}
+
+	/* Free the per-AG metadata reservation pool. */
+	error = xfs_fs_unreserve_ag_blocks(mp);
+	if (error) {
+		xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
+		return error;
+	}
+
+	/*
+	 * Before we sync the metadata, we need to free up the reserve
+	 * block pool so that the used block count in the superblock on
+	 * disk is correct at the end of the remount. Stash the current
+	 * reserve pool size so that if we get remounted rw, we can
+	 * return it to the same size.
+	 */
+	xfs_save_resvblks(mp);
+
+	xfs_quiesce_attr(mp);
+	mp->m_flags |= XFS_MOUNT_RDONLY;
+
+	return 0;
+}
+
 STATIC int
 xfs_fs_remount(
 	struct super_block	*sb,
@@ -1503,37 +1544,9 @@ xfs_fs_remount(
 
 	/* rw -> ro */
 	if (!(mp->m_flags & XFS_MOUNT_RDONLY) && (*flags & SB_RDONLY)) {
-		/*
-		 * Cancel background eofb scanning so it cannot race with the
-		 * final log force+buftarg wait and deadlock the remount.
-		 */
-		xfs_stop_block_reaping(mp);
-
-		/* Get rid of any leftover CoW reservations... */
-		error = xfs_icache_free_cowblocks(mp, NULL);
-		if (error) {
-			xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
-			return error;
-		}
-
-		/* Free the per-AG metadata reservation pool. */
-		error = xfs_fs_unreserve_ag_blocks(mp);
-		if (error) {
-			xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
+		error = xfs_remount_ro(mp);
+		if (error)
 			return error;
-		}
-
-		/*
-		 * Before we sync the metadata, we need to free up the reserve
-		 * block pool so that the used block count in the superblock on
-		 * disk is correct at the end of the remount. Stash the current
-		 * reserve pool size so that if we get remounted rw, we can
-		 * return it to the same size.
-		 */
-		xfs_save_resvblks(mp);
-
-		xfs_quiesce_attr(mp);
-		mp->m_flags |= XFS_MOUNT_RDONLY;
 	}
 
 	return 0;

Re: [REPOST PATCH v3 11/16] xfs: mount-api - add xfs_remount_ro() helper

[Brian Foster]

On Tue, Sep 24, 2019 at 09:22:59PM +0800, Ian Kent wrote:
> Factor the remount read only code into a helper to simplify the
> subsequent change from the super block method .remount_fs to the
> mount-api fs_context_operations method .reconfigure.
> 
> Signed-off-by: Ian Kent <raven@themaw.net>
> ---

This (and the next patch) looks exactly like the previous version
(please retain review tags).

Brian

>  fs/xfs/xfs_super.c |   73 +++++++++++++++++++++++++++++++---------------------
>  1 file changed, 43 insertions(+), 30 deletions(-)
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index aaee32162950..de75891c5551 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1433,6 +1433,47 @@ xfs_remount_rw(
>  	return 0;
>  }
>  
> +STATIC int
> +xfs_remount_ro(
> +	struct xfs_mount	*mp)
> +{
> +	int error;
> +
> +	/*
> +	 * Cancel background eofb scanning so it cannot race with the
> +	 * final log force+buftarg wait and deadlock the remount.
> +	 */
> +	xfs_stop_block_reaping(mp);
> +
> +	/* Get rid of any leftover CoW reservations... */
> +	error = xfs_icache_free_cowblocks(mp, NULL);
> +	if (error) {
> +		xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
> +		return error;
> +	}
> +
> +	/* Free the per-AG metadata reservation pool. */
> +	error = xfs_fs_unreserve_ag_blocks(mp);
> +	if (error) {
> +		xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
> +		return error;
> +	}
> +
> +	/*
> +	 * Before we sync the metadata, we need to free up the reserve
> +	 * block pool so that the used block count in the superblock on
> +	 * disk is correct at the end of the remount. Stash the current
> +	 * reserve pool size so that if we get remounted rw, we can
> +	 * return it to the same size.
> +	 */
> +	xfs_save_resvblks(mp);
> +
> +	xfs_quiesce_attr(mp);
> +	mp->m_flags |= XFS_MOUNT_RDONLY;
> +
> +	return 0;
> +}
> +
>  STATIC int
>  xfs_fs_remount(
>  	struct super_block	*sb,
> @@ -1503,37 +1544,9 @@ xfs_fs_remount(
>  
>  	/* rw -> ro */
>  	if (!(mp->m_flags & XFS_MOUNT_RDONLY) && (*flags & SB_RDONLY)) {
> -		/*
> -		 * Cancel background eofb scanning so it cannot race with the
> -		 * final log force+buftarg wait and deadlock the remount.
> -		 */
> -		xfs_stop_block_reaping(mp);
> -
> -		/* Get rid of any leftover CoW reservations... */
> -		error = xfs_icache_free_cowblocks(mp, NULL);
> -		if (error) {
> -			xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
> -			return error;
> -		}
> -
> -		/* Free the per-AG metadata reservation pool. */
> -		error = xfs_fs_unreserve_ag_blocks(mp);
> -		if (error) {
> -			xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
> +		error = xfs_remount_ro(mp);
> +		if (error)
>  			return error;
> -		}
> -
> -		/*
> -		 * Before we sync the metadata, we need to free up the reserve
> -		 * block pool so that the used block count in the superblock on
> -		 * disk is correct at the end of the remount. Stash the current
> -		 * reserve pool size so that if we get remounted rw, we can
> -		 * return it to the same size.
> -		 */
> -		xfs_save_resvblks(mp);
> -
> -		xfs_quiesce_attr(mp);
> -		mp->m_flags |= XFS_MOUNT_RDONLY;
>  	}
>  
>  	return 0;
> 

Re: [REPOST PATCH v3 11/16] xfs: mount-api - add xfs_remount_ro() helper

[Ian Kent]

On Tue, 2019-09-24 at 10:38 -0400, Brian Foster wrote:
> On Tue, Sep 24, 2019 at 09:22:59PM +0800, Ian Kent wrote:
> > Factor the remount read only code into a helper to simplify the
> > subsequent change from the super block method .remount_fs to the
> > mount-api fs_context_operations method .reconfigure.
> > 
> > Signed-off-by: Ian Kent <raven@themaw.net>
> > ---
> 
> This (and the next patch) looks exactly like the previous version
> (please retain review tags).

Right, will do.

> 
> Brian
> 
> >  fs/xfs/xfs_super.c |   73 +++++++++++++++++++++++++++++++---------
> > ------------
> >  1 file changed, 43 insertions(+), 30 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > index aaee32162950..de75891c5551 100644
> > --- a/fs/xfs/xfs_super.c
> > +++ b/fs/xfs/xfs_super.c
> > @@ -1433,6 +1433,47 @@ xfs_remount_rw(
> >  	return 0;
> >  }
> >  
> > +STATIC int
> > +xfs_remount_ro(
> > +	struct xfs_mount	*mp)
> > +{
> > +	int error;
> > +
> > +	/*
> > +	 * Cancel background eofb scanning so it cannot race with the
> > +	 * final log force+buftarg wait and deadlock the remount.
> > +	 */
> > +	xfs_stop_block_reaping(mp);
> > +
> > +	/* Get rid of any leftover CoW reservations... */
> > +	error = xfs_icache_free_cowblocks(mp, NULL);
> > +	if (error) {
> > +		xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
> > +		return error;
> > +	}
> > +
> > +	/* Free the per-AG metadata reservation pool. */
> > +	error = xfs_fs_unreserve_ag_blocks(mp);
> > +	if (error) {
> > +		xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
> > +		return error;
> > +	}
> > +
> > +	/*
> > +	 * Before we sync the metadata, we need to free up the reserve
> > +	 * block pool so that the used block count in the superblock on
> > +	 * disk is correct at the end of the remount. Stash the current
> > +	 * reserve pool size so that if we get remounted rw, we can
> > +	 * return it to the same size.
> > +	 */
> > +	xfs_save_resvblks(mp);
> > +
> > +	xfs_quiesce_attr(mp);
> > +	mp->m_flags |= XFS_MOUNT_RDONLY;
> > +
> > +	return 0;
> > +}
> > +
> >  STATIC int
> >  xfs_fs_remount(
> >  	struct super_block	*sb,
> > @@ -1503,37 +1544,9 @@ xfs_fs_remount(
> >  
> >  	/* rw -> ro */
> >  	if (!(mp->m_flags & XFS_MOUNT_RDONLY) && (*flags & SB_RDONLY))
> > {
> > -		/*
> > -		 * Cancel background eofb scanning so it cannot race
> > with the
> > -		 * final log force+buftarg wait and deadlock the
> > remount.
> > -		 */
> > -		xfs_stop_block_reaping(mp);
> > -
> > -		/* Get rid of any leftover CoW reservations... */
> > -		error = xfs_icache_free_cowblocks(mp, NULL);
> > -		if (error) {
> > -			xfs_force_shutdown(mp,
> > SHUTDOWN_CORRUPT_INCORE);
> > -			return error;
> > -		}
> > -
> > -		/* Free the per-AG metadata reservation pool. */
> > -		error = xfs_fs_unreserve_ag_blocks(mp);
> > -		if (error) {
> > -			xfs_force_shutdown(mp,
> > SHUTDOWN_CORRUPT_INCORE);
> > +		error = xfs_remount_ro(mp);
> > +		if (error)
> >  			return error;
> > -		}
> > -
> > -		/*
> > -		 * Before we sync the metadata, we need to free up the
> > reserve
> > -		 * block pool so that the used block count in the
> > superblock on
> > -		 * disk is correct at the end of the remount. Stash the
> > current
> > -		 * reserve pool size so that if we get remounted rw, we
> > can
> > -		 * return it to the same size.
> > -		 */
> > -		xfs_save_resvblks(mp);
> > -
> > -		xfs_quiesce_attr(mp);
> > -		mp->m_flags |= XFS_MOUNT_RDONLY;
> >  	}
> >  
> >  	return 0;
> > 

[REPOST PATCH v3 12/16] xfs: mount api - add xfs_reconfigure()

[Ian Kent]

Add the fs_context_operations method .reconfigure that performs
remount validation as previously done by the super_operations
.remount_fs method.

An attempt has also been made to update the comment about options
handling problems with mount(8) to reflect the current situation.

Signed-off-by: Ian Kent <raven@themaw.net>
---
 fs/xfs/xfs_super.c |   84 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 84 insertions(+)

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index de75891c5551..e7627f7ca7f2 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1552,6 +1552,89 @@ xfs_fs_remount(
 	return 0;
 }
 
+/*
+ * There have been problems in the past with options passed from mount(8).
+ *
+ * The problem being that options passed by mount(8) in the case where only
+ * the the mount point path is given would consist of the existing fstab
+ * options with the options from mtab for the current mount merged in and
+ * the options given on the command line last. But the result couldn't be
+ * relied upon to accurately reflect the current mount options so that
+ * rejecting options that can't be changed on reconfigure could erronously
+ * cause mount failure.
+ *
+ * The mount-api uses a legacy mount options handler in the VFS to handle
+ * mount(8) so these options will continue to be passed. Even if mount(8)
+ * is updated to use fsopen()/fsconfig()/fsmount() it's likely to continue
+ * to set the existing options so options problems with reconfigure could
+ * continue.
+ *
+ * For the longest time mtab locking was a problem and this could have been
+ * one possible cause. It's also possible there could have been options
+ * order problems.
+ *
+ * That has changed now as mtab is a link to the proc file system mount
+ * table so mtab options should be always accurate.
+ *
+ * Consulting the util-linux maintainer (Karel Zak) he is confident that,
+ * in this case, the options passed by mount(8) will be those of the current
+ * mount and the options order should be a correct merge of fstab and mtab
+ * options, and new options given on the command line.
+ *
+ * So, in theory, it should be possible to compare incoming options and
+ * return an error for options that differ from the current mount and can't
+ * be changed on reconfigure to prevent users from believing they might have
+ * changed mount options using remount which can't be changed.
+ *
+ * But for now continue to return success for every reconfigure request, and
+ * silently ignore all options that can't actually be changed.
+ */
+STATIC int
+xfs_reconfigure(
+	struct fs_context *fc)
+{
+	struct xfs_fs_context	*ctx = fc->fs_private;
+	struct xfs_mount	*mp = XFS_M(fc->root->d_sb);
+	struct xfs_mount        *new_mp = fc->s_fs_info;
+	xfs_sb_t		*sbp = &mp->m_sb;
+	int			flags = fc->sb_flags;
+	int			error;
+
+	error = xfs_validate_params(new_mp, ctx, false);
+	if (error)
+		return error;
+
+	/* inode32 -> inode64 */
+	if ((mp->m_flags & XFS_MOUNT_SMALL_INUMS) &&
+	    !(new_mp->m_flags & XFS_MOUNT_SMALL_INUMS)) {
+		mp->m_flags &= ~XFS_MOUNT_SMALL_INUMS;
+		mp->m_maxagi = xfs_set_inode_alloc(mp, sbp->sb_agcount);
+	}
+
+	/* inode64 -> inode32 */
+	if (!(mp->m_flags & XFS_MOUNT_SMALL_INUMS) &&
+	    (new_mp->m_flags & XFS_MOUNT_SMALL_INUMS)) {
+		mp->m_flags |= XFS_MOUNT_SMALL_INUMS;
+		mp->m_maxagi = xfs_set_inode_alloc(mp, sbp->sb_agcount);
+	}
+
+	/* ro -> rw */
+	if ((mp->m_flags & XFS_MOUNT_RDONLY) && !(flags & SB_RDONLY)) {
+		error = xfs_remount_rw(mp);
+		if (error)
+			return error;
+	}
+
+	/* rw -> ro */
+	if (!(mp->m_flags & XFS_MOUNT_RDONLY) && (flags & SB_RDONLY)) {
+		error = xfs_remount_ro(mp);
+		if (error)
+			return error;
+	}
+
+	return 0;
+}
+
 /*
  * Second stage of a freeze. The data is already frozen so we only
  * need to take care of the metadata. Once that's done sync the superblock
@@ -2077,6 +2160,7 @@ static const struct super_operations xfs_super_operations = {
 static const struct fs_context_operations xfs_context_ops = {
 	.parse_param = xfs_parse_param,
 	.get_tree    = xfs_get_tree,
+	.reconfigure = xfs_reconfigure,
 };
 
 static struct file_system_type xfs_fs_type = {

Re: [REPOST PATCH v3 12/16] xfs: mount api - add xfs_reconfigure()

[Brian Foster]

On Tue, Sep 24, 2019 at 09:23:04PM +0800, Ian Kent wrote:
> Add the fs_context_operations method .reconfigure that performs
> remount validation as previously done by the super_operations
> .remount_fs method.
> 
> An attempt has also been made to update the comment about options
> handling problems with mount(8) to reflect the current situation.
> 
> Signed-off-by: Ian Kent <raven@themaw.net>
> ---

It doesn't look like this incorporated feedback from v2..

Brian

>  fs/xfs/xfs_super.c |   84 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 84 insertions(+)
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index de75891c5551..e7627f7ca7f2 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1552,6 +1552,89 @@ xfs_fs_remount(
>  	return 0;
>  }
>  
> +/*
> + * There have been problems in the past with options passed from mount(8).
> + *
> + * The problem being that options passed by mount(8) in the case where only
> + * the the mount point path is given would consist of the existing fstab
> + * options with the options from mtab for the current mount merged in and
> + * the options given on the command line last. But the result couldn't be
> + * relied upon to accurately reflect the current mount options so that
> + * rejecting options that can't be changed on reconfigure could erronously
> + * cause mount failure.
> + *
> + * The mount-api uses a legacy mount options handler in the VFS to handle
> + * mount(8) so these options will continue to be passed. Even if mount(8)
> + * is updated to use fsopen()/fsconfig()/fsmount() it's likely to continue
> + * to set the existing options so options problems with reconfigure could
> + * continue.
> + *
> + * For the longest time mtab locking was a problem and this could have been
> + * one possible cause. It's also possible there could have been options
> + * order problems.
> + *
> + * That has changed now as mtab is a link to the proc file system mount
> + * table so mtab options should be always accurate.
> + *
> + * Consulting the util-linux maintainer (Karel Zak) he is confident that,
> + * in this case, the options passed by mount(8) will be those of the current
> + * mount and the options order should be a correct merge of fstab and mtab
> + * options, and new options given on the command line.
> + *
> + * So, in theory, it should be possible to compare incoming options and
> + * return an error for options that differ from the current mount and can't
> + * be changed on reconfigure to prevent users from believing they might have
> + * changed mount options using remount which can't be changed.
> + *
> + * But for now continue to return success for every reconfigure request, and
> + * silently ignore all options that can't actually be changed.
> + */
> +STATIC int
> +xfs_reconfigure(
> +	struct fs_context *fc)
> +{
> +	struct xfs_fs_context	*ctx = fc->fs_private;
> +	struct xfs_mount	*mp = XFS_M(fc->root->d_sb);
> +	struct xfs_mount        *new_mp = fc->s_fs_info;
> +	xfs_sb_t		*sbp = &mp->m_sb;
> +	int			flags = fc->sb_flags;
> +	int			error;
> +
> +	error = xfs_validate_params(new_mp, ctx, false);
> +	if (error)
> +		return error;
> +
> +	/* inode32 -> inode64 */
> +	if ((mp->m_flags & XFS_MOUNT_SMALL_INUMS) &&
> +	    !(new_mp->m_flags & XFS_MOUNT_SMALL_INUMS)) {
> +		mp->m_flags &= ~XFS_MOUNT_SMALL_INUMS;
> +		mp->m_maxagi = xfs_set_inode_alloc(mp, sbp->sb_agcount);
> +	}
> +
> +	/* inode64 -> inode32 */
> +	if (!(mp->m_flags & XFS_MOUNT_SMALL_INUMS) &&
> +	    (new_mp->m_flags & XFS_MOUNT_SMALL_INUMS)) {
> +		mp->m_flags |= XFS_MOUNT_SMALL_INUMS;
> +		mp->m_maxagi = xfs_set_inode_alloc(mp, sbp->sb_agcount);
> +	}
> +
> +	/* ro -> rw */
> +	if ((mp->m_flags & XFS_MOUNT_RDONLY) && !(flags & SB_RDONLY)) {
> +		error = xfs_remount_rw(mp);
> +		if (error)
> +			return error;
> +	}
> +
> +	/* rw -> ro */
> +	if (!(mp->m_flags & XFS_MOUNT_RDONLY) && (flags & SB_RDONLY)) {
> +		error = xfs_remount_ro(mp);
> +		if (error)
> +			return error;
> +	}
> +
> +	return 0;
> +}
> +
>  /*
>   * Second stage of a freeze. The data is already frozen so we only
>   * need to take care of the metadata. Once that's done sync the superblock
> @@ -2077,6 +2160,7 @@ static const struct super_operations xfs_super_operations = {
>  static const struct fs_context_operations xfs_context_ops = {
>  	.parse_param = xfs_parse_param,
>  	.get_tree    = xfs_get_tree,
> +	.reconfigure = xfs_reconfigure,
>  };
>  
>  static struct file_system_type xfs_fs_type = {
> 

Re: [REPOST PATCH v3 12/16] xfs: mount api - add xfs_reconfigure()

[Ian Kent]

On Tue, 2019-09-24 at 10:38 -0400, Brian Foster wrote:
> On Tue, Sep 24, 2019 at 09:23:04PM +0800, Ian Kent wrote:
> > Add the fs_context_operations method .reconfigure that performs
> > remount validation as previously done by the super_operations
> > .remount_fs method.
> > 
> > An attempt has also been made to update the comment about options
> > handling problems with mount(8) to reflect the current situation.
> > 
> > Signed-off-by: Ian Kent <raven@themaw.net>
> > ---
> 
> It doesn't look like this incorporated feedback from v2..

That's right.

I spoke about that in the series cover letter, I'm not sure
where to put this and I think there's useful information that
probably should be kept, somewhere.

> 
> Brian
> 
> >  fs/xfs/xfs_super.c |   84
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 84 insertions(+)
> > 
> > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > index de75891c5551..e7627f7ca7f2 100644
> > --- a/fs/xfs/xfs_super.c
> > +++ b/fs/xfs/xfs_super.c
> > @@ -1552,6 +1552,89 @@ xfs_fs_remount(
> >  	return 0;
> >  }
> >  
> > +/*
> > + * There have been problems in the past with options passed from
> > mount(8).
> > + *
> > + * The problem being that options passed by mount(8) in the case
> > where only
> > + * the the mount point path is given would consist of the existing
> > fstab
> > + * options with the options from mtab for the current mount merged
> > in and
> > + * the options given on the command line last. But the result
> > couldn't be
> > + * relied upon to accurately reflect the current mount options so
> > that
> > + * rejecting options that can't be changed on reconfigure could
> > erronously
> > + * cause mount failure.
> > + *
> > + * The mount-api uses a legacy mount options handler in the VFS to
> > handle
> > + * mount(8) so these options will continue to be passed. Even if
> > mount(8)
> > + * is updated to use fsopen()/fsconfig()/fsmount() it's likely to
> > continue
> > + * to set the existing options so options problems with
> > reconfigure could
> > + * continue.
> > + *
> > + * For the longest time mtab locking was a problem and this could
> > have been
> > + * one possible cause. It's also possible there could have been
> > options
> > + * order problems.
> > + *
> > + * That has changed now as mtab is a link to the proc file system
> > mount
> > + * table so mtab options should be always accurate.
> > + *
> > + * Consulting the util-linux maintainer (Karel Zak) he is
> > confident that,
> > + * in this case, the options passed by mount(8) will be those of
> > the current
> > + * mount and the options order should be a correct merge of fstab
> > and mtab
> > + * options, and new options given on the command line.
> > + *
> > + * So, in theory, it should be possible to compare incoming
> > options and
> > + * return an error for options that differ from the current mount
> > and can't
> > + * be changed on reconfigure to prevent users from believing they
> > might have
> > + * changed mount options using remount which can't be changed.
> > + *
> > + * But for now continue to return success for every reconfigure
> > request, and
> > + * silently ignore all options that can't actually be changed.
> > + */
> > +STATIC int
> > +xfs_reconfigure(
> > +	struct fs_context *fc)
> > +{
> > +	struct xfs_fs_context	*ctx = fc->fs_private;
> > +	struct xfs_mount	*mp = XFS_M(fc->root->d_sb);
> > +	struct xfs_mount        *new_mp = fc->s_fs_info;
> > +	xfs_sb_t		*sbp = &mp->m_sb;
> > +	int			flags = fc->sb_flags;
> > +	int			error;
> > +
> > +	error = xfs_validate_params(new_mp, ctx, false);
> > +	if (error)
> > +		return error;
> > +
> > +	/* inode32 -> inode64 */
> > +	if ((mp->m_flags & XFS_MOUNT_SMALL_INUMS) &&
> > +	    !(new_mp->m_flags & XFS_MOUNT_SMALL_INUMS)) {
> > +		mp->m_flags &= ~XFS_MOUNT_SMALL_INUMS;
> > +		mp->m_maxagi = xfs_set_inode_alloc(mp, sbp-
> > >sb_agcount);
> > +	}
> > +
> > +	/* inode64 -> inode32 */
> > +	if (!(mp->m_flags & XFS_MOUNT_SMALL_INUMS) &&
> > +	    (new_mp->m_flags & XFS_MOUNT_SMALL_INUMS)) {
> > +		mp->m_flags |= XFS_MOUNT_SMALL_INUMS;
> > +		mp->m_maxagi = xfs_set_inode_alloc(mp, sbp-
> > >sb_agcount);
> > +	}
> > +
> > +	/* ro -> rw */
> > +	if ((mp->m_flags & XFS_MOUNT_RDONLY) && !(flags & SB_RDONLY)) {
> > +		error = xfs_remount_rw(mp);
> > +		if (error)
> > +			return error;
> > +	}
> > +
> > +	/* rw -> ro */
> > +	if (!(mp->m_flags & XFS_MOUNT_RDONLY) && (flags & SB_RDONLY)) {
> > +		error = xfs_remount_ro(mp);
> > +		if (error)
> > +			return error;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  /*
> >   * Second stage of a freeze. The data is already frozen so we only
> >   * need to take care of the metadata. Once that's done sync the
> > superblock
> > @@ -2077,6 +2160,7 @@ static const struct super_operations
> > xfs_super_operations = {
> >  static const struct fs_context_operations xfs_context_ops = {
> >  	.parse_param = xfs_parse_param,
> >  	.get_tree    = xfs_get_tree,
> > +	.reconfigure = xfs_reconfigure,
> >  };
> >  
> >  static struct file_system_type xfs_fs_type = {
> > 

Re: [REPOST PATCH v3 12/16] xfs: mount api - add xfs_reconfigure()

[Brian Foster]

On Wed, Sep 25, 2019 at 01:21:35PM +0800, Ian Kent wrote:
> On Tue, 2019-09-24 at 10:38 -0400, Brian Foster wrote:
> > On Tue, Sep 24, 2019 at 09:23:04PM +0800, Ian Kent wrote:
> > > Add the fs_context_operations method .reconfigure that performs
> > > remount validation as previously done by the super_operations
> > > .remount_fs method.
> > > 
> > > An attempt has also been made to update the comment about options
> > > handling problems with mount(8) to reflect the current situation.
> > > 
> > > Signed-off-by: Ian Kent <raven@themaw.net>
> > > ---
> > 
> > It doesn't look like this incorporated feedback from v2..
> 
> That's right.
> 
> I spoke about that in the series cover letter, I'm not sure
> where to put this and I think there's useful information that
> probably should be kept, somewhere.
> 

I'm sure you can find somewhere where it is remotely relevant. ;) The
only relevant bits I see here are the last few sentences or so that
describe how the function should behave. The rest of the background
doesn't belong here. ISTM that a more appropriate place would be either
the caller of ->reconfigure or perhaps the header where
fs_context_operations is defined. At minimum, you can always leave it in
the commit log.

Brian 

> > 
> > Brian
> > 
> > >  fs/xfs/xfs_super.c |   84
> > > ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 84 insertions(+)
> > > 
> > > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > > index de75891c5551..e7627f7ca7f2 100644
> > > --- a/fs/xfs/xfs_super.c
> > > +++ b/fs/xfs/xfs_super.c
> > > @@ -1552,6 +1552,89 @@ xfs_fs_remount(
> > >  	return 0;
> > >  }
> > >  
> > > +/*
> > > + * There have been problems in the past with options passed from
> > > mount(8).
> > > + *
> > > + * The problem being that options passed by mount(8) in the case
> > > where only
> > > + * the the mount point path is given would consist of the existing
> > > fstab
> > > + * options with the options from mtab for the current mount merged
> > > in and
> > > + * the options given on the command line last. But the result
> > > couldn't be
> > > + * relied upon to accurately reflect the current mount options so
> > > that
> > > + * rejecting options that can't be changed on reconfigure could
> > > erronously
> > > + * cause mount failure.
> > > + *
> > > + * The mount-api uses a legacy mount options handler in the VFS to
> > > handle
> > > + * mount(8) so these options will continue to be passed. Even if
> > > mount(8)
> > > + * is updated to use fsopen()/fsconfig()/fsmount() it's likely to
> > > continue
> > > + * to set the existing options so options problems with
> > > reconfigure could
> > > + * continue.
> > > + *
> > > + * For the longest time mtab locking was a problem and this could
> > > have been
> > > + * one possible cause. It's also possible there could have been
> > > options
> > > + * order problems.
> > > + *
> > > + * That has changed now as mtab is a link to the proc file system
> > > mount
> > > + * table so mtab options should be always accurate.
> > > + *
> > > + * Consulting the util-linux maintainer (Karel Zak) he is
> > > confident that,
> > > + * in this case, the options passed by mount(8) will be those of
> > > the current
> > > + * mount and the options order should be a correct merge of fstab
> > > and mtab
> > > + * options, and new options given on the command line.
> > > + *
> > > + * So, in theory, it should be possible to compare incoming
> > > options and
> > > + * return an error for options that differ from the current mount
> > > and can't
> > > + * be changed on reconfigure to prevent users from believing they
> > > might have
> > > + * changed mount options using remount which can't be changed.
> > > + *
> > > + * But for now continue to return success for every reconfigure
> > > request, and
> > > + * silently ignore all options that can't actually be changed.
> > > + */
> > > +STATIC int
> > > +xfs_reconfigure(
> > > +	struct fs_context *fc)
> > > +{
> > > +	struct xfs_fs_context	*ctx = fc->fs_private;
> > > +	struct xfs_mount	*mp = XFS_M(fc->root->d_sb);
> > > +	struct xfs_mount        *new_mp = fc->s_fs_info;
> > > +	xfs_sb_t		*sbp = &mp->m_sb;
> > > +	int			flags = fc->sb_flags;
> > > +	int			error;
> > > +
> > > +	error = xfs_validate_params(new_mp, ctx, false);
> > > +	if (error)
> > > +		return error;
> > > +
> > > +	/* inode32 -> inode64 */
> > > +	if ((mp->m_flags & XFS_MOUNT_SMALL_INUMS) &&
> > > +	    !(new_mp->m_flags & XFS_MOUNT_SMALL_INUMS)) {
> > > +		mp->m_flags &= ~XFS_MOUNT_SMALL_INUMS;
> > > +		mp->m_maxagi = xfs_set_inode_alloc(mp, sbp-
> > > >sb_agcount);
> > > +	}
> > > +
> > > +	/* inode64 -> inode32 */
> > > +	if (!(mp->m_flags & XFS_MOUNT_SMALL_INUMS) &&
> > > +	    (new_mp->m_flags & XFS_MOUNT_SMALL_INUMS)) {
> > > +		mp->m_flags |= XFS_MOUNT_SMALL_INUMS;
> > > +		mp->m_maxagi = xfs_set_inode_alloc(mp, sbp-
> > > >sb_agcount);
> > > +	}
> > > +
> > > +	/* ro -> rw */
> > > +	if ((mp->m_flags & XFS_MOUNT_RDONLY) && !(flags & SB_RDONLY)) {
> > > +		error = xfs_remount_rw(mp);
> > > +		if (error)
> > > +			return error;
> > > +	}
> > > +
> > > +	/* rw -> ro */
> > > +	if (!(mp->m_flags & XFS_MOUNT_RDONLY) && (flags & SB_RDONLY)) {
> > > +		error = xfs_remount_ro(mp);
> > > +		if (error)
> > > +			return error;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  /*
> > >   * Second stage of a freeze. The data is already frozen so we only
> > >   * need to take care of the metadata. Once that's done sync the
> > > superblock
> > > @@ -2077,6 +2160,7 @@ static const struct super_operations
> > > xfs_super_operations = {
> > >  static const struct fs_context_operations xfs_context_ops = {
> > >  	.parse_param = xfs_parse_param,
> > >  	.get_tree    = xfs_get_tree,
> > > +	.reconfigure = xfs_reconfigure,
> > >  };
> > >  
> > >  static struct file_system_type xfs_fs_type = {
> > > 
> 

[REPOST PATCH v3 13/16] xfs: mount-api - add xfs_fc_free()

[Ian Kent]

Add the fs_context_operations method .free that performs fs
context cleanup on context release.

Signed-off-by: Ian Kent <raven@themaw.net>
---
 fs/xfs/xfs_super.c |   27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index e7627f7ca7f2..5bc2363269a9 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -2157,10 +2157,37 @@ static const struct super_operations xfs_super_operations = {
 	.free_cached_objects	= xfs_fs_free_cached_objects,
 };
 
+static void xfs_fc_free(struct fs_context *fc)
+{
+	struct xfs_fs_context	*ctx = fc->fs_private;
+	struct xfs_mount	*mp = fc->s_fs_info;
+
+	/*
+	 * When the mount context is initialized the private
+	 * struct xfs_mount info (mp) is allocated and stored in
+	 * the fs context along with the struct xfs_fs_context
+	 * (ctx) mount context working working storage.
+	 *
+	 * On success the mount info struct, mp, is moved into
+	 * private info super block field ->s_fs_info of the
+	 * newly allocated super block. But if an error occurs
+	 * before this happens it's the responsibility of the fs
+	 * context to release the mount info struct in addition
+	 * to the mount context working storage.
+	 */
+	if (mp) {
+		kfree(mp->m_logname);
+		kfree(mp->m_rtname);
+		kfree(mp);
+	}
+	kfree(ctx);
+}
+
 static const struct fs_context_operations xfs_context_ops = {
 	.parse_param = xfs_parse_param,
 	.get_tree    = xfs_get_tree,
 	.reconfigure = xfs_reconfigure,
+	.free        = xfs_fc_free,
 };
 
 static struct file_system_type xfs_fs_type = {

[REPOST PATCH v3 14/16] xfs: mount-api - dont set sb in xfs_mount_alloc()

[Ian Kent]

When changing to use the new mount api the super block won't be
available when the xfs_mount info struct is allocated so move
setting the super block in xfs_mount to xfs_fs_fill_super().

Also change xfs_mount_alloc() decalaration static -> STATIC.

Signed-off-by: Ian Kent <raven@themaw.net>
---
 fs/xfs/xfs_super.c |    9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 5bc2363269a9..1c25d5dfd090 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1796,9 +1796,8 @@ xfs_destroy_percpu_counters(
 	percpu_counter_destroy(&mp->m_delalloc_blks);
 }
 
-static struct xfs_mount *
-xfs_mount_alloc(
-	struct super_block	*sb)
+STATIC struct xfs_mount *
+xfs_mount_alloc(void)
 {
 	struct xfs_mount	*mp;
 
@@ -1806,7 +1805,6 @@ xfs_mount_alloc(
 	if (!mp)
 		return NULL;
 
-	mp->m_super = sb;
 	spin_lock_init(&mp->m_sb_lock);
 	spin_lock_init(&mp->m_agirotor_lock);
 	INIT_RADIX_TREE(&mp->m_perag_tree, GFP_ATOMIC);
@@ -2020,9 +2018,10 @@ xfs_fs_fill_super(
 	 * allocate mp and do all low-level struct initializations before we
 	 * attach it to the super
 	 */
-	mp = xfs_mount_alloc(sb);
+	mp = xfs_mount_alloc();
 	if (!mp)
 		return -ENOMEM;
+	mp->m_super = sb;
 	sb->s_fs_info = mp;
 
 	error = xfs_parseargs(mp, (char *)data);

[REPOST PATCH v3 15/16] xfs: mount-api - switch to new mount-api

[Ian Kent]

The infrastructure needed to use the new mount api is now
in place, switch over to use it.

Signed-off-by: Ian Kent <raven@themaw.net>
---
 fs/xfs/xfs_super.c |  321 ++++++++--------------------------------------------
 1 file changed, 49 insertions(+), 272 deletions(-)

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 1c25d5dfd090..3d41bb141cab 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -447,132 +447,6 @@ xfs_validate_params(
 	return 0;
 }
 
-/*
- * This function fills in xfs_mount_t fields based on mount args.
- * Note: the superblock has _not_ yet been read in.
- *
- * Note that this function leaks the various device name allocations on
- * failure.  The caller takes care of them.
- *
- * *sb is const because this is also used to test options on the remount
- * path, and we don't want this to have any side effects at remount time.
- * Today this function does not change *sb, but just to future-proof...
- */
-STATIC int
-xfs_parseargs(
-	struct xfs_mount	*mp,
-	char			*options)
-{
-	const struct super_block *sb = mp->m_super;
-	char			*p;
-	struct fs_context	fc;
-	struct xfs_fs_context	context;
-	struct xfs_fs_context	*ctx;
-	int			ret;
-
-	memset(&fc, 0, sizeof(fc));
-	memset(&context, 0, sizeof(context));
-	fc.fs_private = &context;
-	ctx = &context;
-	fc.s_fs_info = mp;
-
-	/*
-	 * set up the mount name first so all the errors will refer to the
-	 * correct device.
-	 */
-	mp->m_fsname = kstrndup(sb->s_id, MAXNAMELEN, GFP_KERNEL);
-	if (!mp->m_fsname)
-		return -ENOMEM;
-	mp->m_fsname_len = strlen(mp->m_fsname) + 1;
-
-	/*
-	 * Copy binary VFS mount flags we are interested in.
-	 */
-	if (sb_rdonly(sb))
-		mp->m_flags |= XFS_MOUNT_RDONLY;
-	if (sb->s_flags & SB_DIRSYNC)
-		mp->m_flags |= XFS_MOUNT_DIRSYNC;
-	if (sb->s_flags & SB_SYNCHRONOUS)
-		mp->m_flags |= XFS_MOUNT_WSYNC;
-
-	/*
-	 * Set some default flags that could be cleared by the mount option
-	 * parsing.
-	 */
-	mp->m_flags |= XFS_MOUNT_COMPAT_IOSIZE;
-
-	/*
-	 * These can be overridden by the mount option parsing.
-	 */
-	mp->m_logbufs = -1;
-	mp->m_logbsize = -1;
-
-	if (!options)
-		goto done;
-
-	while ((p = strsep(&options, ",")) != NULL) {
-		struct fs_parameter	param;
-		char			*value;
-
-		if (!*p)
-			continue;
-
-		param.key = p;
-		param.type = fs_value_is_string;
-		param.string = NULL;
-		param.size = 0;
-
-		value = strchr(p, '=');
-		if (value) {
-			*value++ = 0;
-			param.size = strlen(value);
-			if (param.size > 0) {
-				param.string = kmemdup_nul(value,
-							   param.size,
-							   GFP_KERNEL);
-				if (!param.string)
-					return -ENOMEM;
-			}
-		}
-
-		ret = xfs_parse_param(&fc, &param);
-		kfree(param.string);
-		if (ret < 0)
-			goto done;
-	}
-
-	if ((mp->m_flags & XFS_MOUNT_NOALIGN) &&
-	    (ctx->dsunit || ctx->dswidth)) {
-		xfs_warn(mp,
-	"sunit and swidth options incompatible with the noalign option");
-		return -EINVAL;
-	}
-
-#ifndef CONFIG_XFS_QUOTA
-	if (XFS_IS_QUOTA_RUNNING(mp)) {
-		xfs_warn(mp, "quota support not available in this kernel.");
-		return -EINVAL;
-	}
-#endif
-
-	if ((ctx->dsunit && !ctx->dswidth) || (!ctx->dsunit && ctx->dswidth)) {
-		xfs_warn(mp, "sunit and swidth must be specified together");
-		return -EINVAL;
-	}
-
-	if (ctx->dsunit && (ctx->dswidth % ctx->dsunit != 0)) {
-		xfs_warn(mp,
-	"stripe width (%d) must be a multiple of the stripe unit (%d)",
-			ctx->dswidth, ctx->dsunit);
-		return -EINVAL;
-	}
-
-done:
-	ret = xfs_validate_params(mp, &context, false);
-
-	return ret;
-}
-
 struct proc_xfs_info {
 	uint64_t	flag;
 	char		*str;
@@ -1351,26 +1225,6 @@ xfs_quiesce_attr(
 	xfs_log_quiesce(mp);
 }
 
-STATIC int
-xfs_test_remount_options(
-	struct super_block	*sb,
-	char			*options)
-{
-	int			error = 0;
-	struct xfs_mount	*tmp_mp;
-
-	tmp_mp = kmem_zalloc(sizeof(*tmp_mp), KM_MAYFAIL);
-	if (!tmp_mp)
-		return -ENOMEM;
-
-	tmp_mp->m_super = sb;
-	error = xfs_parseargs(tmp_mp, options);
-	xfs_free_fsname(tmp_mp);
-	kmem_free(tmp_mp);
-
-	return error;
-}
-
 STATIC int
 xfs_remount_rw(
 	struct xfs_mount	*mp)
@@ -1474,84 +1328,6 @@ xfs_remount_ro(
 	return 0;
 }
 
-STATIC int
-xfs_fs_remount(
-	struct super_block	*sb,
-	int			*flags,
-	char			*options)
-{
-	struct xfs_mount	*mp = XFS_M(sb);
-	xfs_sb_t		*sbp = &mp->m_sb;
-	substring_t		args[MAX_OPT_ARGS];
-	char			*p;
-	int			error;
-
-	/* First, check for complete junk; i.e. invalid options */
-	error = xfs_test_remount_options(sb, options);
-	if (error)
-		return error;
-
-	sync_filesystem(sb);
-	while ((p = strsep(&options, ",")) != NULL) {
-		int token;
-
-		if (!*p)
-			continue;
-
-		token = match_token(p, tokens, args);
-		switch (token) {
-		case Opt_inode64:
-			mp->m_flags &= ~XFS_MOUNT_SMALL_INUMS;
-			mp->m_maxagi = xfs_set_inode_alloc(mp, sbp->sb_agcount);
-			break;
-		case Opt_inode32:
-			mp->m_flags |= XFS_MOUNT_SMALL_INUMS;
-			mp->m_maxagi = xfs_set_inode_alloc(mp, sbp->sb_agcount);
-			break;
-		default:
-			/*
-			 * Logically we would return an error here to prevent
-			 * users from believing they might have changed
-			 * mount options using remount which can't be changed.
-			 *
-			 * But unfortunately mount(8) adds all options from
-			 * mtab and fstab to the mount arguments in some cases
-			 * so we can't blindly reject options, but have to
-			 * check for each specified option if it actually
-			 * differs from the currently set option and only
-			 * reject it if that's the case.
-			 *
-			 * Until that is implemented we return success for
-			 * every remount request, and silently ignore all
-			 * options that we can't actually change.
-			 */
-#if 0
-			xfs_info(mp,
-		"mount option \"%s\" not supported for remount", p);
-			return -EINVAL;
-#else
-			break;
-#endif
-		}
-	}
-
-	/* ro -> rw */
-	if ((mp->m_flags & XFS_MOUNT_RDONLY) && !(*flags & SB_RDONLY)) {
-		error = xfs_remount_rw(mp);
-		if (error)
-			return error;
-	}
-
-	/* rw -> ro */
-	if (!(mp->m_flags & XFS_MOUNT_RDONLY) && (*flags & SB_RDONLY)) {
-		error = xfs_remount_ro(mp);
-		if (error)
-			return error;
-	}
-
-	return 0;
-}
-
 /*
  * There have been problems in the past with options passed from mount(8).
  *
@@ -2005,42 +1781,6 @@ __xfs_fs_fill_super(
 	goto out_free_sb;
 }
 
-STATIC int
-xfs_fs_fill_super(
-	struct super_block	*sb,
-	void			*data,
-	int			silent)
-{
-	struct xfs_mount	*mp = NULL;
-	int			error = -ENOMEM;
-
-	/*
-	 * allocate mp and do all low-level struct initializations before we
-	 * attach it to the super
-	 */
-	mp = xfs_mount_alloc();
-	if (!mp)
-		return -ENOMEM;
-	mp->m_super = sb;
-	sb->s_fs_info = mp;
-
-	error = xfs_parseargs(mp, (char *)data);
-	if (error)
-		goto out_free_fsname;
-
-	error = __xfs_fs_fill_super(mp, silent);
-	if (error)
-		goto out_free_fsname;
-
-	return 0;
-
- out_free_fsname:
-	sb->s_fs_info = NULL;
-	xfs_free_fsname(mp);
-	kfree(mp);
-	return error;
-}
-
 STATIC int
 xfs_fill_super(
 	struct super_block	*sb,
@@ -2111,16 +1851,6 @@ xfs_fs_put_super(
 	kfree(mp);
 }
 
-STATIC struct dentry *
-xfs_fs_mount(
-	struct file_system_type	*fs_type,
-	int			flags,
-	const char		*dev_name,
-	void			*data)
-{
-	return mount_bdev(fs_type, flags, dev_name, data, xfs_fs_fill_super);
-}
-
 static long
 xfs_fs_nr_cached_objects(
 	struct super_block	*sb,
@@ -2150,7 +1880,6 @@ static const struct super_operations xfs_super_operations = {
 	.freeze_fs		= xfs_fs_freeze,
 	.unfreeze_fs		= xfs_fs_unfreeze,
 	.statfs			= xfs_fs_statfs,
-	.remount_fs		= xfs_fs_remount,
 	.show_options		= xfs_fs_show_options,
 	.nr_cached_objects	= xfs_fs_nr_cached_objects,
 	.free_cached_objects	= xfs_fs_free_cached_objects,
@@ -2189,10 +1918,58 @@ static const struct fs_context_operations xfs_context_ops = {
 	.free        = xfs_fc_free,
 };
 
+/*
+ * Set up the filesystem mount context.
+ */
+int xfs_init_fs_context(struct fs_context *fc)
+{
+	struct xfs_fs_context	*ctx;
+	struct xfs_mount	*mp;
+
+	ctx = kzalloc(sizeof(struct xfs_fs_context), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+
+	mp = xfs_mount_alloc();
+	if (!mp) {
+		kfree(ctx);
+		return -ENOMEM;
+	}
+
+	/*
+	 * Set some default flags that could be cleared by the mount option
+	 * parsing.
+	 */
+	mp->m_flags |= XFS_MOUNT_COMPAT_IOSIZE;
+
+	/*
+	 * These can be overridden by the mount option parsing.
+	 */
+	mp->m_logbufs = -1;
+	mp->m_logbsize = -1;
+
+	/*
+	 * Copy binary VFS mount flags we are interested in.
+	 */
+	if (fc->sb_flags & SB_RDONLY)
+		mp->m_flags |= XFS_MOUNT_RDONLY;
+	if (fc->sb_flags & SB_DIRSYNC)
+		mp->m_flags |= XFS_MOUNT_DIRSYNC;
+	if (fc->sb_flags & SB_SYNCHRONOUS)
+		mp->m_flags |= XFS_MOUNT_WSYNC;
+
+	fc->fs_private = ctx;
+	fc->s_fs_info = mp;
+	fc->ops = &xfs_context_ops;
+
+	return 0;
+}
+
 static struct file_system_type xfs_fs_type = {
 	.owner			= THIS_MODULE,
 	.name			= "xfs",
-	.mount			= xfs_fs_mount,
+	.init_fs_context	= xfs_init_fs_context,
+	.parameters		= &xfs_fs_parameters,
 	.kill_sb		= kill_block_super,
 	.fs_flags		= FS_REQUIRES_DEV,
 };

[REPOST PATCH v3 16/16] xfs: mount-api - remove legacy mount functions

[Ian Kent]

Now that the new mount api is being used the old mount functions
and parsing table can be removed.

Signed-off-by: Ian Kent <raven@themaw.net>
---
 fs/xfs/xfs_super.c |   48 +-----------------------------------------------
 1 file changed, 1 insertion(+), 47 deletions(-)

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 3d41bb141cab..7820093bfca8 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -61,53 +61,7 @@ enum {
 	Opt_filestreams, Opt_quota, Opt_noquota, Opt_usrquota, Opt_grpquota,
 	Opt_prjquota, Opt_uquota, Opt_gquota, Opt_pquota,
 	Opt_uqnoenforce, Opt_gqnoenforce, Opt_pqnoenforce, Opt_qnoenforce,
-	Opt_discard, Opt_nodiscard, Opt_dax, Opt_err,
-};
-
-static const match_table_t tokens = {
-	{Opt_logbufs,	"logbufs=%u"},	/* number of XFS log buffers */
-	{Opt_logbsize,	"logbsize=%s"},	/* size of XFS log buffers */
-	{Opt_logdev,	"logdev=%s"},	/* log device */
-	{Opt_rtdev,	"rtdev=%s"},	/* realtime I/O device */
-	{Opt_wsync,	"wsync"},	/* safe-mode nfs compatible mount */
-	{Opt_noalign,	"noalign"},	/* turn off stripe alignment */
-	{Opt_swalloc,	"swalloc"},	/* turn on stripe width allocation */
-	{Opt_sunit,	"sunit=%u"},	/* data volume stripe unit */
-	{Opt_swidth,	"swidth=%u"},	/* data volume stripe width */
-	{Opt_nouuid,	"nouuid"},	/* ignore filesystem UUID */
-	{Opt_grpid,	"grpid"},	/* group-ID from parent directory */
-	{Opt_nogrpid,	"nogrpid"},	/* group-ID from current process */
-	{Opt_bsdgroups,	"bsdgroups"},	/* group-ID from parent directory */
-	{Opt_sysvgroups,"sysvgroups"},	/* group-ID from current process */
-	{Opt_allocsize,	"allocsize=%s"},/* preferred allocation size */
-	{Opt_norecovery,"norecovery"},	/* don't run XFS recovery */
-	{Opt_inode64,	"inode64"},	/* inodes can be allocated anywhere */
-	{Opt_inode32,   "inode32"},	/* inode allocation limited to
-					 * XFS_MAXINUMBER_32 */
-	{Opt_ikeep,	"ikeep"},	/* do not free empty inode clusters */
-	{Opt_noikeep,	"noikeep"},	/* free empty inode clusters */
-	{Opt_largeio,	"largeio"},	/* report large I/O sizes in stat() */
-	{Opt_nolargeio,	"nolargeio"},	/* do not report large I/O sizes
-					 * in stat(). */
-	{Opt_attr2,	"attr2"},	/* do use attr2 attribute format */
-	{Opt_noattr2,	"noattr2"},	/* do not use attr2 attribute format */
-	{Opt_filestreams,"filestreams"},/* use filestreams allocator */
-	{Opt_quota,	"quota"},	/* disk quotas (user) */
-	{Opt_noquota,	"noquota"},	/* no quotas */
-	{Opt_usrquota,	"usrquota"},	/* user quota enabled */
-	{Opt_grpquota,	"grpquota"},	/* group quota enabled */
-	{Opt_prjquota,	"prjquota"},	/* project quota enabled */
-	{Opt_uquota,	"uquota"},	/* user quota (IRIX variant) */
-	{Opt_gquota,	"gquota"},	/* group quota (IRIX variant) */
-	{Opt_pquota,	"pquota"},	/* project quota (IRIX variant) */
-	{Opt_uqnoenforce,"uqnoenforce"},/* user quota limit enforcement */
-	{Opt_gqnoenforce,"gqnoenforce"},/* group quota limit enforcement */
-	{Opt_pqnoenforce,"pqnoenforce"},/* project quota limit enforcement */
-	{Opt_qnoenforce, "qnoenforce"},	/* same as uqnoenforce */
-	{Opt_discard,	"discard"},	/* Discard unused blocks */
-	{Opt_nodiscard,	"nodiscard"},	/* Do not discard unused blocks */
-	{Opt_dax,	"dax"},		/* Enable direct access to bdev pages */
-	{Opt_err,	NULL},
+	Opt_discard, Opt_nodiscard, Opt_dax,
 };
 
 static const struct fs_parameter_spec xfs_param_specs[] = {