[PATCH v4 00/17] xfs: mount API patch series

[PATCH v4 00/17] xfs: mount API patch series

[Ian Kent]

This patch series add 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 along with the generic xfstests.

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 sometimes inconsistent logging.

  The mount-api feature of saving error message text to the mount
  context for later retrieval 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 so it which can lead to messages,
  possibly needed for debug purposes, not being sent to the kernel
  log. There's also the pr_xxx() log functions (not a problem for
  xfs AFAICS) that aren't aware of the mount context at all.

  In the xfs patches I've used the same method that is used in
  gfs2 and was suggested by Al Viro (essentially return the error
  if fs_parse() returns one) except that I've also not used the
  mount api log macros to minimise the possibility of lost log
  messages.

  This isn't the best so follow up patches for RFC (with a
  slightly wider audience) will be needed to try and improve
  this aspect of the mount api.

Changes for v4:
- changed xfs_fill_super() cleanup back to what it was in v2, until
  I can work out what's causing the problem had previously seen (I can't
  reproduce it myself), since it looks like it was right from the start.
- use get_tree_bdev() instead of vfs_get_block_super() in xfs_get_tree()
  as requested by Al Viro.
- removed redundant initialisation in xfs_fs_fill_super().
- fix long line in xfs_validate_params().
- no need to validate if parameter parsing fails, just return the error.
- summarise reconfigure comment about option handling, transfer bulk
  of comment to commit log message.
- use minimal change in xfs_parse_param(), deffer discussion of mount
  api logging improvements until later and with a wider audience.

Changes for v3:
- fix struct xfs_fs_context initialisation 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 obsolete 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.
- clarify 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 manageable.

Al Viro has sent a pull request to Linus for the patch containing
get_tree_bdev() recently and I think there's a small problem with
that patch too so there will be conflicts with merging this series
without dropping the first two patches of the series.

---

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

Ian Kent (16):
      vfs: add missing blkdev_put() in get_tree_bdev()
      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 remaining legacy mount code


 fs/super.c                 |   97 +++++
 fs/xfs/xfs_super.c         |  939 +++++++++++++++++++++++---------------------
 include/linux/fs_context.h |    5 
 3 files changed, 600 insertions(+), 441 deletions(-)

--
Ian

[PATCH v4 01/17] vfs: Create fs_context-aware mount_bdev() replacement

[Ian Kent]

From: David Howells <dhowells@redhat.com>

Create a function, get_tree_bdev(), that is fs_context-aware and a
->get_tree() counterpart of mount_bdev().

It caches the block device pointer 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/super.c                 |   94 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/fs_context.h |    5 ++
 2 files changed, 99 insertions(+)

diff --git a/fs/super.c b/fs/super.c
index 113c58f19425..a7f62c964e58 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -1215,6 +1215,7 @@ int get_tree_single(struct fs_context *fc,
 EXPORT_SYMBOL(get_tree_single);
 
 #ifdef CONFIG_BLOCK
+
 static int set_bdev_super(struct super_block *s, void *data)
 {
 	s->s_bdev = data;
@@ -1224,6 +1225,99 @@ static int set_bdev_super(struct super_block *s, void *data)
 	return 0;
 }
 
+static int set_bdev_super_fc(struct super_block *s, struct fs_context *fc)
+{
+	return set_bdev_super(s, fc->sget_key);
+}
+
+static int test_bdev_super_fc(struct super_block *s, struct fs_context *fc)
+{
+	return s->s_bdev == fc->sget_key;
+}
+
+/**
+ * get_tree_bdev - Get a superblock based on a single block device
+ * @fc: The filesystem context holding the parameters
+ * @fill_super: Helper to initialise a new superblock
+ */
+int get_tree_bdev(struct fs_context *fc,
+		int (*fill_super)(struct super_block *,
+				  struct fs_context *))
+{
+	struct block_device *bdev;
+	struct super_block *s;
+	fmode_t mode = FMODE_READ | FMODE_EXCL;
+	int error = 0;
+
+	if (!(fc->sb_flags & SB_RDONLY))
+		mode |= FMODE_WRITE;
+
+	if (!fc->source)
+		return invalf(fc, "No source specified");
+
+	bdev = blkdev_get_by_path(fc->source, mode, fc->fs_type);
+	if (IS_ERR(bdev)) {
+		errorf(fc, "%s: Can't open blockdev", fc->source);
+		return PTR_ERR(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;
+	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) {
+		/* 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);
+			deactivate_locked_super(s);
+			blkdev_put(bdev, mode);
+			return -EBUSY;
+		}
+
+		/*
+		 * s_umount nests inside bd_mutex during
+		 * __invalidate_device().  blkdev_put() acquires
+		 * bd_mutex and can't be called under s_umount.  Drop
+		 * s_umount temporarily.  This is safe as we're
+		 * holding an active reference.
+		 */
+		up_write(&s->s_umount);
+		blkdev_put(bdev, mode);
+		down_write(&s->s_umount);
+	} 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;
+}
+EXPORT_SYMBOL(get_tree_bdev);
+
 static int test_bdev_super(struct super_block *s, void *data)
 {
 	return (void *)s->s_bdev == data;
diff --git a/include/linux/fs_context.h b/include/linux/fs_context.h
index 7c6fe3d47fa6..7bf6179a83fd 100644
--- a/include/linux/fs_context.h
+++ b/include/linux/fs_context.h
@@ -88,6 +88,7 @@ struct fs_context {
 	struct mutex		uapi_mutex;	/* Userspace access mutex */
 	struct file_system_type	*fs_type;
 	void			*fs_private;	/* The filesystem's context */
+	void			*sget_key;
 	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 */
@@ -154,6 +155,10 @@ extern int get_tree_single(struct fs_context *fc,
 			 int (*fill_super)(struct super_block *sb,
 					   struct fs_context *fc));
 
+extern int get_tree_bdev(struct fs_context *fc,
+			       int (*fill_super)(struct super_block *sb,
+						 struct fs_context *fc));
+
 extern const struct file_operations fscontext_fops;
 
 /*

[PATCH v4 02/17] vfs: add missing blkdev_put() in get_tree_bdev()

[Ian Kent]

There appear to be a couple of missing blkdev_put() in get_tree_bdev().
---
 fs/super.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/super.c b/fs/super.c
index a7f62c964e58..fd816014bd7d 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -1268,6 +1268,7 @@ int get_tree_bdev(struct fs_context *fc,
 	mutex_lock(&bdev->bd_fsfreeze_mutex);
 	if (bdev->bd_fsfreeze_count > 0) {
 		mutex_unlock(&bdev->bd_fsfreeze_mutex);
+		blkdev_put(bdev, mode);
 		warnf(fc, "%pg: Can't mount, blockdev is frozen", bdev);
 		return -EBUSY;
 	}
@@ -1276,8 +1277,10 @@ int get_tree_bdev(struct fs_context *fc,
 	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))
+	if (IS_ERR(s)) {
+		blkdev_put(bdev, mode);
 		return PTR_ERR(s);
+	}
 
 	if (s->s_root) {
 		/* Don't summarily change the RO/RW state. */

[PATCH v4 03/17] 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>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 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;

[PATCH v4 04/17] 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>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 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)

[PATCH v4 05/17] 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>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 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;

[PATCH v4 06/17] 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>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 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;
 	}
 
 	/*

[PATCH v4 07/17] 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().

In xfs_parse_param() immediately returning an error if fs_parse()
returns one will lead to an inconsistent log entry for unknown
parameters.

But there's also a need to support saving error messages to the
mount context when the fsxxx() system calls are used for passing
options and performing the mount which needs to be done without
possibly losing log entries. This isn't the way that the VFS mount
api log macros work now so follow up patches will be needed later
and they will need to be discussed to work out how this should
acheived for xfs.

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

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

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index b04aebab69ab..7fd3975d5523 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -191,57 +191,51 @@ 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)
+		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 +258,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 +340,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 +365,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 +411,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 +450,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 +464,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 +507,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;

[PATCH v4 08/17] 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 |  147 +++++++++++++++++++++++++++++++++-------------------
 1 file changed, 94 insertions(+), 53 deletions(-)

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 7fd3975d5523..7008355df065 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -347,6 +347,98 @@ 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.
@@ -441,15 +533,6 @@ xfs_parseargs(
 			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;
-	}
-
 	if ((mp->m_flags & XFS_MOUNT_NOALIGN) &&
 	    (ctx->dsunit || ctx->dswidth)) {
 		xfs_warn(mp,
@@ -477,51 +560,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;
-	}
+	ret = xfs_validate_params(mp, &context, false);
 
-	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;
+	return ret;
 }
 
 struct proc_xfs_info {

[PATCH v4 09/17] 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>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 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 7008355df065..cc2da9093e34 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1712,27 +1712,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;
@@ -1759,7 +1746,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)
@@ -1894,10 +1881,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;
 
@@ -1907,6 +1890,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;
+	int			error;
+
+	/*
+	 * 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)

[PATCH v4 10/17] 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 cc2da9093e34..b984120667da 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1925,6 +1925,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)
+		goto out_free_fsname;
+	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);
+	kfree(mp);
+	return error;
+}
+
+STATIC int
+xfs_get_tree(
+	struct fs_context	*fc)
+{
+	return get_tree_bdev(fc, xfs_fill_super);
+}
+
 STATIC void
 xfs_fs_put_super(
 	struct super_block	*sb)
@@ -1995,6 +2040,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",

[PATCH v4 11/17] 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>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 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 b984120667da..d4a84bee0254 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1363,6 +1363,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,
@@ -1426,57 +1488,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;
 	}
 

[PATCH v4 12/17] 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>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 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 d4a84bee0254..ddcf030cca7c 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1425,6 +1425,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,
@@ -1495,37 +1536,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;

[PATCH v4 13/17] 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 |   68 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index ddcf030cca7c..06f650fb3a8c 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1544,6 +1544,73 @@ xfs_fs_remount(
 	return 0;
 }
 
+/*
+ * There can be problems with options passed from mount(8) when
+ * only the mount point path is given. The options are a merge
+ * of options from the fstab, mtab of the current mount and options
+ * given on the command line.
+ *
+ * But this can't be relied upon to accurately reflect the current
+ * mount options. Consequently rejecting options that can't be
+ * changed on reconfigure could erronously cause a mount failure.
+ *
+ * Nowadays 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.
+ *
+ * But this still might not always be the case so for now continue
+ * to return success for every reconfigure request, and silently
+ * ignore all options that can't actually be changed.
+ *
+ * See the commit log entry of this change for a more detailed
+ * desription of the problem.
+ */
+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
@@ -2069,6 +2136,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 = {

[PATCH v4 14/17] 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 |   26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 06f650fb3a8c..4f2963ff9e06 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -2133,10 +2133,36 @@ 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 super block allocation the mount info struct, mp,
+	 * is moved into private super block info 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.
+	 */
+	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 = {

[PATCH v4 15/17] 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 4f2963ff9e06..919afd7082b7 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1772,9 +1772,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;
 
@@ -1782,7 +1781,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);
@@ -1996,9 +1994,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);

[PATCH v4 16/17] 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 919afd7082b7..93f42160aa6f 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -439,132 +439,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)
-			return ret;
-	}
-
-	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;
@@ -1343,26 +1217,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)
@@ -1466,84 +1320,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 can be problems with options passed from mount(8) when
  * only the mount point path is given. The options are a merge
@@ -1981,42 +1757,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;
-	int			error;
-
-	/*
-	 * 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,
@@ -2087,16 +1827,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,
@@ -2126,7 +1856,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,
@@ -2164,10 +1893,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,
 };

[PATCH v4 17/17] xfs: mount-api - remove remaining legacy mount code

[Ian Kent]

Now that the new mount api is being used the remaining old mount
code 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 93f42160aa6f..4d65e6c7cfb2 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[] = {

Re: [PATCH v4 02/17] vfs: add missing blkdev_put() in get_tree_bdev()

[Darrick J. Wong]

On Thu, Oct 03, 2019 at 06:25:28PM +0800, Ian Kent wrote:
> There appear to be a couple of missing blkdev_put() in get_tree_bdev().

No SOB, not reviewable......

--D

> ---
>  fs/super.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/super.c b/fs/super.c
> index a7f62c964e58..fd816014bd7d 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -1268,6 +1268,7 @@ int get_tree_bdev(struct fs_context *fc,
>  	mutex_lock(&bdev->bd_fsfreeze_mutex);
>  	if (bdev->bd_fsfreeze_count > 0) {
>  		mutex_unlock(&bdev->bd_fsfreeze_mutex);
> +		blkdev_put(bdev, mode);
>  		warnf(fc, "%pg: Can't mount, blockdev is frozen", bdev);
>  		return -EBUSY;
>  	}
> @@ -1276,8 +1277,10 @@ int get_tree_bdev(struct fs_context *fc,
>  	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))
> +	if (IS_ERR(s)) {
> +		blkdev_put(bdev, mode);
>  		return PTR_ERR(s);
> +	}
>  
>  	if (s->s_root) {
>  		/* Don't summarily change the RO/RW state. */
> 

Re: [PATCH v4 00/17] xfs: mount API patch series

[Eric Sandeen]

On 10/3/19 5:25 AM, Ian Kent wrote:
> This patch series add 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 along with the generic xfstests.
> 
> Other things that continue to cause me concern:
> 
> - Message logging.

...

Haven't actually reviewed yet, but just playing with it, I noticed an oddity;

# mount -o loop,allocsize=abc fsfile mnt

fails as expected, but with no dmesg to be found.  Is that a known behavior?

-Eric

Re: [PATCH v4 02/17] vfs: add missing blkdev_put() in get_tree_bdev()

[Ian Kent]

On Thu, 2019-10-03 at 07:56 -0700, Darrick J. Wong wrote:
> On Thu, Oct 03, 2019 at 06:25:28PM +0800, Ian Kent wrote:
> > There appear to be a couple of missing blkdev_put() in
> > get_tree_bdev().
> 
> No SOB, not reviewable......

It's not expected to be but is needed if anyone wants to test
the series.

I sent this to Al asking if these are in fact missing.
If they are I expect he will push an update to Linus pretty
quickly.

> 
> --D
> 
> > ---
> >  fs/super.c |    5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/super.c b/fs/super.c
> > index a7f62c964e58..fd816014bd7d 100644
> > --- a/fs/super.c
> > +++ b/fs/super.c
> > @@ -1268,6 +1268,7 @@ int get_tree_bdev(struct fs_context *fc,
> >  	mutex_lock(&bdev->bd_fsfreeze_mutex);
> >  	if (bdev->bd_fsfreeze_count > 0) {
> >  		mutex_unlock(&bdev->bd_fsfreeze_mutex);
> > +		blkdev_put(bdev, mode);
> >  		warnf(fc, "%pg: Can't mount, blockdev is frozen",
> > bdev);
> >  		return -EBUSY;
> >  	}
> > @@ -1276,8 +1277,10 @@ int get_tree_bdev(struct fs_context *fc,
> >  	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))
> > +	if (IS_ERR(s)) {
> > +		blkdev_put(bdev, mode);
> >  		return PTR_ERR(s);
> > +	}
> >  
> >  	if (s->s_root) {
> >  		/* Don't summarily change the RO/RW state. */
> > 

Re: [PATCH v4 00/17] xfs: mount API patch series

[Ian Kent]

On Thu, 2019-10-03 at 18:30 -0500, Eric Sandeen wrote:
> On 10/3/19 5:25 AM, Ian Kent wrote:
> > This patch series add 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 along with the generic xfstests.
> > 
> > Other things that continue to cause me concern:
> > 
> > - Message logging.
> 
> ...
> 
> Haven't actually reviewed yet, but just playing with it, I noticed an
> oddity;
> 
> # mount -o loop,allocsize=abc fsfile mnt
> 
> fails as expected, but with no dmesg to be found.  Is that a known
> behavior?

That's interesting.

I'll see if I can work out what path that is taking though the
kernel, don't think it's getting to the xfs options handling.

> 
> -Eric

Re: [PATCH v4 02/17] vfs: add missing blkdev_put() in get_tree_bdev()

[Ian Kent]

On Fri, 2019-10-04 at 14:49 +0800, Ian Kent wrote:
> On Thu, 2019-10-03 at 07:56 -0700, Darrick J. Wong wrote:
> > On Thu, Oct 03, 2019 at 06:25:28PM +0800, Ian Kent wrote:
> > > There appear to be a couple of missing blkdev_put() in
> > > get_tree_bdev().
> > 
> > No SOB, not reviewable......
> 
> It's not expected to be but is needed if anyone wants to test
> the series.
> 
> I sent this to Al asking if these are in fact missing.
> If they are I expect he will push an update to Linus pretty
> quickly.

But he hasn't responded so perhaps I should have annotated
it, just in case ...

> 
> > --D
> > 
> > > ---
> > >  fs/super.c |    5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/super.c b/fs/super.c
> > > index a7f62c964e58..fd816014bd7d 100644
> > > --- a/fs/super.c
> > > +++ b/fs/super.c
> > > @@ -1268,6 +1268,7 @@ int get_tree_bdev(struct fs_context *fc,
> > >  	mutex_lock(&bdev->bd_fsfreeze_mutex);
> > >  	if (bdev->bd_fsfreeze_count > 0) {
> > >  		mutex_unlock(&bdev->bd_fsfreeze_mutex);
> > > +		blkdev_put(bdev, mode);
> > >  		warnf(fc, "%pg: Can't mount, blockdev is frozen",
> > > bdev);
> > >  		return -EBUSY;
> > >  	}
> > > @@ -1276,8 +1277,10 @@ int get_tree_bdev(struct fs_context *fc,
> > >  	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))
> > > +	if (IS_ERR(s)) {
> > > +		blkdev_put(bdev, mode);
> > >  		return PTR_ERR(s);
> > > +	}
> > >  
> > >  	if (s->s_root) {
> > >  		/* Don't summarily change the RO/RW state. */
> > > 

Re: [PATCH v4 00/17] xfs: mount API patch series

[Ian Kent]

On Fri, 2019-10-04 at 14:57 +0800, Ian Kent wrote:
> On Thu, 2019-10-03 at 18:30 -0500, Eric Sandeen wrote:
> > On 10/3/19 5:25 AM, Ian Kent wrote:
> > > This patch series add 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 along with the generic xfstests.
> > > 
> > > Other things that continue to cause me concern:
> > > 
> > > - Message logging.
> > 
> > ...
> > 
> > Haven't actually reviewed yet, but just playing with it, I noticed
> > an
> > oddity;
> > 
> > # mount -o loop,allocsize=abc fsfile mnt
> > 
> > fails as expected, but with no dmesg to be found.  Is that a known
> > behavior?
> 
> That's interesting.

But it's not actually.

> 
> I'll see if I can work out what path that is taking though the
> kernel, don't think it's getting to the xfs options handling.

In the original xfs code, if there's a failure in xfs_parseargs()
such as in this case when suffix_kstrtoint() fails, probably at
kstrtoint(), the -EINVAL is returned to xfs_fs_fill_super() which
subsequently returns that to the VFS.

The VFS itself doesn't log a failure message, it just returns the
error to user space.

With the patch series applied, xfs_parse_param() does essentially
the same thing and because it returns other than -ENOPARAM to the
VFS the error is returned, without the VFS logging an error, to
user space.

There are a few cases in xfs options handling where this happens.
The series hasn't tried to change this.

> 
> > -Eric

Re: [PATCH v4 00/17] xfs: mount API patch series

[Ian Kent]

On Fri, 2019-10-04 at 16:25 +0800, Ian Kent wrote:
> On Fri, 2019-10-04 at 14:57 +0800, Ian Kent wrote:
> > On Thu, 2019-10-03 at 18:30 -0500, Eric Sandeen wrote:
> > > On 10/3/19 5:25 AM, Ian Kent wrote:
> > > > This patch series add 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 along with the generic xfstests.
> > > > 
> > > > Other things that continue to cause me concern:
> > > > 
> > > > - Message logging.
> > > 
> > > ...
> > > 
> > > Haven't actually reviewed yet, but just playing with it, I
> > > noticed
> > > an
> > > oddity;
> > > 
> > > # mount -o loop,allocsize=abc fsfile mnt
> > > 
> > > fails as expected, but with no dmesg to be found.  Is that a
> > > known
> > > behavior?
> > 
> > That's interesting.
> 
> But it's not actually.
> 
> > I'll see if I can work out what path that is taking though the
> > kernel, don't think it's getting to the xfs options handling.
> 
> In the original xfs code, if there's a failure in xfs_parseargs()
> such as in this case when suffix_kstrtoint() fails, probably at
> kstrtoint(), the -EINVAL is returned to xfs_fs_fill_super() which
> subsequently returns that to the VFS.
> 
> The VFS itself doesn't log a failure message, it just returns the
> error to user space.
> 
> With the patch series applied, xfs_parse_param() does essentially
> the same thing and because it returns other than -ENOPARAM to the
> VFS the error is returned, without the VFS logging an error, to
> user space.
> 
> There are a few cases in xfs options handling where this happens.
> The series hasn't tried to change this.

While the description above is accurate in terms of xfs options
handling behaviour, in this case it's not the invalid allocsize
that's causing the failure, it fails with a valid size as well.

Certainly mount(8) is calling mount(2) trying a bunch of alternate
fs types, xfs being one, and is getting -EINVAL back from them all.

Not sure of the value of tracking down an explanation of this since
it's fairly well outside the scope of the series but I will if you
want me to.

> 
> > > -Eric

Re: [PATCH v4 02/17] vfs: add missing blkdev_put() in get_tree_bdev()

[Al Viro]

On Fri, Oct 04, 2019 at 02:59:57PM +0800, Ian Kent wrote:
> On Fri, 2019-10-04 at 14:49 +0800, Ian Kent wrote:
> > On Thu, 2019-10-03 at 07:56 -0700, Darrick J. Wong wrote:
> > > On Thu, Oct 03, 2019 at 06:25:28PM +0800, Ian Kent wrote:
> > > > There appear to be a couple of missing blkdev_put() in
> > > > get_tree_bdev().
> > > 
> > > No SOB, not reviewable......
> > 
> > It's not expected to be but is needed if anyone wants to test
> > the series.
> > 
> > I sent this to Al asking if these are in fact missing.
> > If they are I expect he will push an update to Linus pretty
> > quickly.
> 
> But he hasn't responded so perhaps I should have annotated
> it, just in case ...

Sorry, just getting out of flu ;-/  I'll apply that fix and push out.

Re: [PATCH v4 00/17] xfs: mount API patch series

[Eric Sandeen]

On 10/4/19 3:25 AM, Ian Kent wrote:
> On Fri, 2019-10-04 at 14:57 +0800, Ian Kent wrote:
>> On Thu, 2019-10-03 at 18:30 -0500, Eric Sandeen wrote:
>>> On 10/3/19 5:25 AM, Ian Kent wrote:
>>>> This patch series add 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 along with the generic xfstests.
>>>>
>>>> Other things that continue to cause me concern:
>>>>
>>>> - Message logging.
>>>
>>> ...
>>>
>>> Haven't actually reviewed yet, but just playing with it, I noticed
>>> an
>>> oddity;
>>>
>>> # mount -o loop,allocsize=abc fsfile mnt
>>>
>>> fails as expected, but with no dmesg to be found.  Is that a known
>>> behavior?
>>
>> That's interesting.
> 
> But it's not actually.
> 
>>
>> I'll see if I can work out what path that is taking though the
>> kernel, don't think it's getting to the xfs options handling.
> 
> In the original xfs code, if there's a failure in xfs_parseargs()
> such as in this case when suffix_kstrtoint() fails, probably at
> kstrtoint(), the -EINVAL is returned to xfs_fs_fill_super() which
> subsequently returns that to the VFS.
> 
> The VFS itself doesn't log a failure message, it just returns the
> error to user space.
> 

Oh, crud, I see now that it's the same upstream.  :(  Sorry for the noise.

-Eric

Re: [PATCH v4 07/17] xfs: mount-api - make xfs_parse_param() take context .parse_param() args

[Brian Foster]

On Thu, Oct 03, 2019 at 06:25:56PM +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().
> 
> In xfs_parse_param() immediately returning an error if fs_parse()
> returns one will lead to an inconsistent log entry for unknown
> parameters.
> 
> But there's also a need to support saving error messages to the
> mount context when the fsxxx() system calls are used for passing
> options and performing the mount which needs to be done without
> possibly losing log entries. This isn't the way that the VFS mount
> api log macros work now so follow up patches will be needed later
> and they will need to be discussed to work out how this should
> acheived for xfs.
> 
> Also the temporary function match_kstrtoint() is now unused, remove it.
> 
> Signed-off-by: Ian Kent <raven@themaw.net>
> ---

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

>  fs/xfs/xfs_super.c |  128 +++++++++++++++++++++++++++++-----------------------
>  1 file changed, 72 insertions(+), 56 deletions(-)
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index b04aebab69ab..7fd3975d5523 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -191,57 +191,51 @@ 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)
> +		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 +258,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 +340,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 +365,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 +411,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 +450,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 +464,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 +507,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: [PATCH v4 08/17] xfs: mount-api - move xfs_parseargs() validation to a helper

[Brian Foster]

On Thu, Oct 03, 2019 at 06:26:01PM +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 |  147 +++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 94 insertions(+), 53 deletions(-)
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 7fd3975d5523..7008355df065 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
...
> @@ -441,15 +533,6 @@ xfs_parseargs(
>  			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;
> -	}
> -

Is there a reason that various checks above this one are replicated in
the helper and not removed from xfs_parseargs()? Either way, it looks
like this code goes away anyways:

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

>  	if ((mp->m_flags & XFS_MOUNT_NOALIGN) &&
>  	    (ctx->dsunit || ctx->dswidth)) {
>  		xfs_warn(mp,
> @@ -477,51 +560,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;
> -	}
> +	ret = xfs_validate_params(mp, &context, false);
>  
> -	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;
> +	return ret;
>  }
>  
>  struct proc_xfs_info {
> 

Re: [PATCH v4 10/17] xfs: mount-api - add xfs_get_tree()

[Brian Foster]

On Thu, Oct 03, 2019 at 06:26:12PM +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 cc2da9093e34..b984120667da 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1925,6 +1925,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)
> +		goto out_free_fsname;
> +	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);
> +	kfree(mp);
> +	return error;

Ok, I think I have a better understanding of how this is supposed to
work with the background context. mp starts off in fc->s_fs_info, ends
up transferred to sb->s_fs_info and passed into here. We allocate
->m_fsname and carry on from here with ownership of mp.

The only thing I'll note is that the out_free_fsname label is misnamed
and probably could be out_free or out_free_mp or something. With that
nit addressed:

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

> +}
> +
> +STATIC int
> +xfs_get_tree(
> +	struct fs_context	*fc)
> +{
> +	return get_tree_bdev(fc, xfs_fill_super);
> +}
> +
>  STATIC void
>  xfs_fs_put_super(
>  	struct super_block	*sb)
> @@ -1995,6 +2040,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: [PATCH v4 13/17] xfs: mount api - add xfs_reconfigure()

[Brian Foster]

On Thu, Oct 03, 2019 at 06:26:27PM +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>
> ---
>  fs/xfs/xfs_super.c |   68 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 68 insertions(+)
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index ddcf030cca7c..06f650fb3a8c 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1544,6 +1544,73 @@ xfs_fs_remount(
>  	return 0;
>  }
>  
> +/*
> + * There can be problems with options passed from mount(8) when
> + * only the mount point path is given. The options are a merge
> + * of options from the fstab, mtab of the current mount and options
> + * given on the command line.
> + *
> + * But this can't be relied upon to accurately reflect the current
> + * mount options. Consequently rejecting options that can't be
> + * changed on reconfigure could erronously cause a mount failure.
> + *
> + * Nowadays 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.
> + *
> + * But this still might not always be the case so for now continue
> + * to return success for every reconfigure request, and silently
> + * ignore all options that can't actually be changed.
> + *
> + * See the commit log entry of this change for a more detailed
> + * desription of the problem.
> + */

But the commit log entry doesn't include any new info..?

How about this.. we already have a similar comment in xfs_fs_remount()
that I happen to find a bit more clear. It also obviously has precedent.
How about we copy that one to the top of this function verbatim, and
whatever extra you want to get across can be added to the commit log
description. Hm?

Brian

> +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
> @@ -2069,6 +2136,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: [PATCH v4 10/17] xfs: mount-api - add xfs_get_tree()

[Ian Kent]

On Fri, 2019-10-04 at 11:52 -0400, Brian Foster wrote:
> On Thu, Oct 03, 2019 at 06:26:12PM +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 cc2da9093e34..b984120667da 100644
> > --- a/fs/xfs/xfs_super.c
> > +++ b/fs/xfs/xfs_super.c
> > @@ -1925,6 +1925,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)
> > +		goto out_free_fsname;
> > +	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);
> > +	kfree(mp);
> > +	return error;
> 
> Ok, I think I have a better understanding of how this is supposed to
> work with the background context. mp starts off in fc->s_fs_info,
> ends
> up transferred to sb->s_fs_info and passed into here. We allocate
> ->m_fsname and carry on from here with ownership of mp.
> 
> The only thing I'll note is that the out_free_fsname label is
> misnamed
> and probably could be out_free or out_free_mp or something. With that
> nit addressed:

Or out_error perhaps, IIRC I remember thinking I needed to do that but
it slipped my mind?

> 
> Reviewed-by: Brian Foster <bfoster@redhat.com>
> 
> > +}
> > +
> > +STATIC int
> > +xfs_get_tree(
> > +	struct fs_context	*fc)
> > +{
> > +	return get_tree_bdev(fc, xfs_fill_super);
> > +}
> > +
> >  STATIC void
> >  xfs_fs_put_super(
> >  	struct super_block	*sb)
> > @@ -1995,6 +2040,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: [PATCH v4 13/17] xfs: mount api - add xfs_reconfigure()

[Ian Kent]

On Fri, 2019-10-04 at 11:53 -0400, Brian Foster wrote:
> On Thu, Oct 03, 2019 at 06:26:27PM +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>
> > ---
> >  fs/xfs/xfs_super.c |   68
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 68 insertions(+)
> > 
> > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > index ddcf030cca7c..06f650fb3a8c 100644
> > --- a/fs/xfs/xfs_super.c
> > +++ b/fs/xfs/xfs_super.c
> > @@ -1544,6 +1544,73 @@ xfs_fs_remount(
> >  	return 0;
> >  }
> >  
> > +/*
> > + * There can be problems with options passed from mount(8) when
> > + * only the mount point path is given. The options are a merge
> > + * of options from the fstab, mtab of the current mount and
> > options
> > + * given on the command line.
> > + *
> > + * But this can't be relied upon to accurately reflect the current
> > + * mount options. Consequently rejecting options that can't be
> > + * changed on reconfigure could erronously cause a mount failure.
> > + *
> > + * Nowadays 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.
> > + *
> > + * But this still might not always be the case so for now continue
> > + * to return success for every reconfigure request, and silently
> > + * ignore all options that can't actually be changed.
> > + *
> > + * See the commit log entry of this change for a more detailed
> > + * desription of the problem.
> > + */
> 
> But the commit log entry doesn't include any new info..?

I thought I did, honest, ;)

> 
> How about this.. we already have a similar comment in
> xfs_fs_remount()
> that I happen to find a bit more clear. It also obviously has
> precedent.
> How about we copy that one to the top of this function verbatim, and
> whatever extra you want to get across can be added to the commit log
> description. Hm?

Trying to understand that comment and whether it was still needed
is what lead to this.

It is still relevant and IIRC the only extra info. needed is that
the mount-api implementation can't help with the problem because
it's what's given to the kernel via. mount(8) and that must continue
to be supported.

I'll re-read the original message, maybe retaining it is sufficient
to imply the above.

> 
> Brian
> 
> > +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
> > @@ -2069,6 +2136,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: [PATCH v4 13/17] xfs: mount api - add xfs_reconfigure()

[Brian Foster]

On Sat, Oct 05, 2019 at 07:16:17AM +0800, Ian Kent wrote:
> On Fri, 2019-10-04 at 11:53 -0400, Brian Foster wrote:
> > On Thu, Oct 03, 2019 at 06:26:27PM +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>
> > > ---
> > >  fs/xfs/xfs_super.c |   68
> > > ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 68 insertions(+)
> > > 
> > > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > > index ddcf030cca7c..06f650fb3a8c 100644
> > > --- a/fs/xfs/xfs_super.c
> > > +++ b/fs/xfs/xfs_super.c
> > > @@ -1544,6 +1544,73 @@ xfs_fs_remount(
> > >  	return 0;
> > >  }
> > >  
> > > +/*
> > > + * There can be problems with options passed from mount(8) when
> > > + * only the mount point path is given. The options are a merge
> > > + * of options from the fstab, mtab of the current mount and
> > > options
> > > + * given on the command line.
> > > + *
> > > + * But this can't be relied upon to accurately reflect the current
> > > + * mount options. Consequently rejecting options that can't be
> > > + * changed on reconfigure could erronously cause a mount failure.
> > > + *
> > > + * Nowadays 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.
> > > + *
> > > + * But this still might not always be the case so for now continue
> > > + * to return success for every reconfigure request, and silently
> > > + * ignore all options that can't actually be changed.
> > > + *
> > > + * See the commit log entry of this change for a more detailed
> > > + * desription of the problem.
> > > + */
> > 
> > But the commit log entry doesn't include any new info..?
> 
> I thought I did, honest, ;)
> 
> > 
> > How about this.. we already have a similar comment in
> > xfs_fs_remount()
> > that I happen to find a bit more clear. It also obviously has
> > precedent.
> > How about we copy that one to the top of this function verbatim, and
> > whatever extra you want to get across can be added to the commit log
> > description. Hm?
> 
> Trying to understand that comment and whether it was still needed
> is what lead to this.
> 
> It is still relevant and IIRC the only extra info. needed is that
> the mount-api implementation can't help with the problem because
> it's what's given to the kernel via. mount(8) and that must continue
> to be supported.
> 
> I'll re-read the original message, maybe retaining it is sufficient
> to imply the above.
> 

I think it's sufficient. There's no need to comment on the previous
implementation in the code because that code is being removed. If
necessary, please do that in the commit log.

Brian

> > 
> > Brian
> > 
> > > +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
> > > @@ -2069,6 +2136,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: [PATCH v4 14/17] xfs: mount-api - add xfs_fc_free()

[Brian Foster]

On Thu, Oct 03, 2019 at 06:26:32PM +0800, Ian Kent wrote:
> 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 |   26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 06f650fb3a8c..4f2963ff9e06 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -2133,10 +2133,36 @@ 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.
> +	 *

"working working storage" ?

> +	 * On super block allocation the mount info struct, mp,
> +	 * is moved into private super block info 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.
> +	 */

I like the comment here, but it seems it could be simplified. E.g:

	/*
	 * mp and ctx are stored in the fs_context when it is initialized. mp is
	 * transferred to the superblock on a successful mount, but if an error
	 * occurs before the transfer we have to free it here.
	 */

> +	if (mp) {
> +		kfree(mp->m_logname);
> +		kfree(mp->m_rtname);

Also, can we just call xfs_free_fsname() here to be safe/consisent? With
those nits fixed up, this seems fine to me.

Brian

> +		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 = {
> 

Re: [PATCH v4 15/17] xfs: mount-api - dont set sb in xfs_mount_alloc()

[Brian Foster]

On Thu, Oct 03, 2019 at 06:26:38PM +0800, Ian Kent wrote:
> 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>
> ---

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

>  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 4f2963ff9e06..919afd7082b7 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1772,9 +1772,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;
>  
> @@ -1782,7 +1781,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);
> @@ -1996,9 +1994,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);
> 

Re: [PATCH v4 16/17] xfs: mount-api - switch to new mount-api

[Brian Foster]

On Thu, Oct 03, 2019 at 06:26:43PM +0800, Ian Kent wrote:
> 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>
> ---

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

>  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 919afd7082b7..93f42160aa6f 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -439,132 +439,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)
> -			return ret;
> -	}
> -
> -	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;
> @@ -1343,26 +1217,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)
> @@ -1466,84 +1320,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 can be problems with options passed from mount(8) when
>   * only the mount point path is given. The options are a merge
> @@ -1981,42 +1757,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;
> -	int			error;
> -
> -	/*
> -	 * 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,
> @@ -2087,16 +1827,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,
> @@ -2126,7 +1856,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,
> @@ -2164,10 +1893,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,
>  };
> 

Re: [PATCH v4 17/17] xfs: mount-api - remove remaining legacy mount code

[Brian Foster]

On Thu, Oct 03, 2019 at 06:26:48PM +0800, Ian Kent wrote:
> Now that the new mount api is being used the remaining old mount
> code can be removed.
> 
> Signed-off-by: Ian Kent <raven@themaw.net>
> ---

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

>  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 93f42160aa6f..4d65e6c7cfb2 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[] = {
> 

Re: [PATCH v4 00/17] xfs: mount API patch series

[Brian Foster]

On Thu, Oct 03, 2019 at 06:25:18PM +0800, Ian Kent wrote:
> This patch series add 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 along with the generic xfstests.
> 

I'm not sure that we have any focused mount option testing in fstests.
It looks like we have various remount tests and such to cover corner
cases and/or specific bugs, but nothing to make sure various options
continue to work or otherwise fail gracefully. Do you have any plans to
add such a test to help verify this work?

Brian

> 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 sometimes inconsistent logging.
> 
>   The mount-api feature of saving error message text to the mount
>   context for later retrieval 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 so it which can lead to messages,
>   possibly needed for debug purposes, not being sent to the kernel
>   log. There's also the pr_xxx() log functions (not a problem for
>   xfs AFAICS) that aren't aware of the mount context at all.
> 
>   In the xfs patches I've used the same method that is used in
>   gfs2 and was suggested by Al Viro (essentially return the error
>   if fs_parse() returns one) except that I've also not used the
>   mount api log macros to minimise the possibility of lost log
>   messages.
> 
>   This isn't the best so follow up patches for RFC (with a
>   slightly wider audience) will be needed to try and improve
>   this aspect of the mount api.
> 
> Changes for v4:
> - changed xfs_fill_super() cleanup back to what it was in v2, until
>   I can work out what's causing the problem had previously seen (I can't
>   reproduce it myself), since it looks like it was right from the start.
> - use get_tree_bdev() instead of vfs_get_block_super() in xfs_get_tree()
>   as requested by Al Viro.
> - removed redundant initialisation in xfs_fs_fill_super().
> - fix long line in xfs_validate_params().
> - no need to validate if parameter parsing fails, just return the error.
> - summarise reconfigure comment about option handling, transfer bulk
>   of comment to commit log message.
> - use minimal change in xfs_parse_param(), deffer discussion of mount
>   api logging improvements until later and with a wider audience.
> 
> Changes for v3:
> - fix struct xfs_fs_context initialisation 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 obsolete 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.
> - clarify 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 manageable.
> 
> Al Viro has sent a pull request to Linus for the patch containing
> get_tree_bdev() recently and I think there's a small problem with
> that patch too so there will be conflicts with merging this series
> without dropping the first two patches of the series.
> 
> ---
> 
> David Howells (1):
>       vfs: Create fs_context-aware mount_bdev() replacement
> 
> Ian Kent (16):
>       vfs: add missing blkdev_put() in get_tree_bdev()
>       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 remaining legacy mount code
> 
> 
>  fs/super.c                 |   97 +++++
>  fs/xfs/xfs_super.c         |  939 +++++++++++++++++++++++---------------------
>  include/linux/fs_context.h |    5 
>  3 files changed, 600 insertions(+), 441 deletions(-)
> 
> --
> Ian

Re: [PATCH v4 00/17] xfs: mount API patch series

[Ian Kent]

On Mon, 2019-10-07 at 07:52 -0400, Brian Foster wrote:
> On Thu, Oct 03, 2019 at 06:25:18PM +0800, Ian Kent wrote:
> > This patch series add 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 along with the generic xfstests.
> > 
> 
> I'm not sure that we have any focused mount option testing in
> fstests.
> It looks like we have various remount tests and such to cover corner
> cases and/or specific bugs, but nothing to make sure various options
> continue to work or otherwise fail gracefully. Do you have any plans
> to
> add such a test to help verify this work?

Darrick was concerned about that.

Some sort of xfstest is needed in order to be able to merge this
so he has some confidence that it won't break things.

I volunteered to do have a go at writing a test.

I've given that some thought and done an initial survey of xfstests
but it's still new to me so I'm not sure how this will end up.

Darrick thought it would need a generic test to test VFS options
and one in xfs for the xfs specific options.

At this point I'm thinking I'll have a go at adding an xfs specific
options test but, while I can find out what the optionsare and what
validation they use, there's a lot about some of the xfs options
I don't fully understand so I don't know what a sensible test might
be.

> 
> Brian
> 
> > 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 sometimes inconsistent logging.
> > 
> >   The mount-api feature of saving error message text to the mount
> >   context for later retrieval 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 so it which can lead to messages,
> >   possibly needed for debug purposes, not being sent to the kernel
> >   log. There's also the pr_xxx() log functions (not a problem for
> >   xfs AFAICS) that aren't aware of the mount context at all.
> > 
> >   In the xfs patches I've used the same method that is used in
> >   gfs2 and was suggested by Al Viro (essentially return the error
> >   if fs_parse() returns one) except that I've also not used the
> >   mount api log macros to minimise the possibility of lost log
> >   messages.
> > 
> >   This isn't the best so follow up patches for RFC (with a
> >   slightly wider audience) will be needed to try and improve
> >   this aspect of the mount api.
> > 
> > Changes for v4:
> > - changed xfs_fill_super() cleanup back to what it was in v2, until
> >   I can work out what's causing the problem had previously seen (I
> > can't
> >   reproduce it myself), since it looks like it was right from the
> > start.
> > - use get_tree_bdev() instead of vfs_get_block_super() in
> > xfs_get_tree()
> >   as requested by Al Viro.
> > - removed redundant initialisation in xfs_fs_fill_super().
> > - fix long line in xfs_validate_params().
> > - no need to validate if parameter parsing fails, just return the
> > error.
> > - summarise reconfigure comment about option handling, transfer
> > bulk
> >   of comment to commit log message.
> > - use minimal change in xfs_parse_param(), deffer discussion of
> > mount
> >   api logging improvements until later and with a wider audience.
> > 
> > Changes for v3:
> > - fix struct xfs_fs_context initialisation 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 obsolete 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.
> > - clarify 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 manageable.
> > 
> > Al Viro has sent a pull request to Linus for the patch containing
> > get_tree_bdev() recently and I think there's a small problem with
> > that patch too so there will be conflicts with merging this series
> > without dropping the first two patches of the series.
> > 
> > ---
> > 
> > David Howells (1):
> >       vfs: Create fs_context-aware mount_bdev() replacement
> > 
> > Ian Kent (16):
> >       vfs: add missing blkdev_put() in get_tree_bdev()
> >       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 remaining legacy mount code
> > 
> > 
> >  fs/super.c                 |   97 +++++
> >  fs/xfs/xfs_super.c         |  939 +++++++++++++++++++++++---------
> > ------------
> >  include/linux/fs_context.h |    5 
> >  3 files changed, 600 insertions(+), 441 deletions(-)
> > 
> > --
> > Ian

Re: [PATCH v4 13/17] xfs: mount api - add xfs_reconfigure()

[Ian Kent]

On Mon, 2019-10-07 at 07:51 -0400, Brian Foster wrote:
> On Sat, Oct 05, 2019 at 07:16:17AM +0800, Ian Kent wrote:
> > On Fri, 2019-10-04 at 11:53 -0400, Brian Foster wrote:
> > > On Thu, Oct 03, 2019 at 06:26:27PM +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>
> > > > ---
> > > >  fs/xfs/xfs_super.c |   68
> > > > ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 68 insertions(+)
> > > > 
> > > > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > > > index ddcf030cca7c..06f650fb3a8c 100644
> > > > --- a/fs/xfs/xfs_super.c
> > > > +++ b/fs/xfs/xfs_super.c
> > > > @@ -1544,6 +1544,73 @@ xfs_fs_remount(
> > > >  	return 0;
> > > >  }
> > > >  
> > > > +/*
> > > > + * There can be problems with options passed from mount(8)
> > > > when
> > > > + * only the mount point path is given. The options are a merge
> > > > + * of options from the fstab, mtab of the current mount and
> > > > options
> > > > + * given on the command line.
> > > > + *
> > > > + * But this can't be relied upon to accurately reflect the
> > > > current
> > > > + * mount options. Consequently rejecting options that can't be
> > > > + * changed on reconfigure could erronously cause a mount
> > > > failure.
> > > > + *
> > > > + * Nowadays 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.
> > > > + *
> > > > + * But this still might not always be the case so for now
> > > > continue
> > > > + * to return success for every reconfigure request, and
> > > > silently
> > > > + * ignore all options that can't actually be changed.
> > > > + *
> > > > + * See the commit log entry of this change for a more detailed
> > > > + * desription of the problem.
> > > > + */
> > > 
> > > But the commit log entry doesn't include any new info..?
> > 
> > I thought I did, honest, ;)
> > 
> > > How about this.. we already have a similar comment in
> > > xfs_fs_remount()
> > > that I happen to find a bit more clear. It also obviously has
> > > precedent.
> > > How about we copy that one to the top of this function verbatim,
> > > and
> > > whatever extra you want to get across can be added to the commit
> > > log
> > > description. Hm?
> > 
> > Trying to understand that comment and whether it was still needed
> > is what lead to this.
> > 
> > It is still relevant and IIRC the only extra info. needed is that
> > the mount-api implementation can't help with the problem because
> > it's what's given to the kernel via. mount(8) and that must
> > continue
> > to be supported.
> > 
> > I'll re-read the original message, maybe retaining it is sufficient
> > to imply the above.
> > 
> 
> I think it's sufficient. There's no need to comment on the previous
> implementation in the code because that code is being removed. If
> necessary, please do that in the commit log.

I re-read the original comment and I think I may have miss-interpreted
it a little because I was reading it in the context of "why can't the
mount-api handle this problem".

But it actually reads like it's saying xfs needs to needs to improve
its options validation for this.

Given that the mount-api has no control over what comes from user
space in the way of options I don't see how it can make any difference
to this problem.

So, bottom line is I'm thinking of discarding that comment and using
the original.

> 
> Brian
> 
> > > Brian
> > > 
> > > > +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
> > > > @@ -2069,6 +2136,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: [PATCH v4 14/17] xfs: mount-api - add xfs_fc_free()

[Ian Kent]

On Mon, 2019-10-07 at 07:51 -0400, Brian Foster wrote:
> On Thu, Oct 03, 2019 at 06:26:32PM +0800, Ian Kent wrote:
> > 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 |   26 ++++++++++++++++++++++++++
> >  1 file changed, 26 insertions(+)
> > 
> > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > index 06f650fb3a8c..4f2963ff9e06 100644
> > --- a/fs/xfs/xfs_super.c
> > +++ b/fs/xfs/xfs_super.c
> > @@ -2133,10 +2133,36 @@ 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.
> > +	 *
> 
> "working working storage" ?

Oops!

> 
> > +	 * On super block allocation the mount info struct, mp,
> > +	 * is moved into private super block info 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.
> > +	 */
> 
> I like the comment here, but it seems it could be simplified. E.g:
> 
> 	/*
> 	 * mp and ctx are stored in the fs_context when it is
> initialized. mp is
> 	 * transferred to the superblock on a successful mount, but if
> an error
> 	 * occurs before the transfer we have to free it here.
> 	 */

I like this, I'll use it thanks.

> 
> > +	if (mp) {
> > +		kfree(mp->m_logname);
> > +		kfree(mp->m_rtname);
> 
> Also, can we just call xfs_free_fsname() here to be safe/consisent?
> With
> those nits fixed up, this seems fine to me.

I think so, I'll check.

> 
> Brian
> 
> > +		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 = {
> > 

Re: [PATCH v4 00/17] xfs: mount API patch series

[Darrick J. Wong]

On Tue, Oct 08, 2019 at 08:13:57AM +0800, Ian Kent wrote:
> On Mon, 2019-10-07 at 07:52 -0400, Brian Foster wrote:
> > On Thu, Oct 03, 2019 at 06:25:18PM +0800, Ian Kent wrote:
> > > This patch series add 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 along with the generic xfstests.
> > > 
> > 
> > I'm not sure that we have any focused mount option testing in
> > fstests.
> > It looks like we have various remount tests and such to cover corner
> > cases and/or specific bugs, but nothing to make sure various options
> > continue to work or otherwise fail gracefully. Do you have any plans
> > to
> > add such a test to help verify this work?
> 
> Darrick was concerned about that.
> 
> Some sort of xfstest is needed in order to be able to merge this
> so he has some confidence that it won't break things.
> 
> I volunteered to do have a go at writing a test.
> 
> I've given that some thought and done an initial survey of xfstests
> but it's still new to me so I'm not sure how this will end up.
> 
> Darrick thought it would need a generic test to test VFS options
> and one in xfs for the xfs specific options.
> 
> At this point I'm thinking I'll have a go at adding an xfs specific
> options test but, while I can find out what the optionsare and what
> validation they use, there's a lot about some of the xfs options
> I don't fully understand so I don't know what a sensible test might
> be.

Hm, that's evidence of inadequate documentation.  If you can't figure
out what would be sensible tests for a particular mount option from
xfs(5) then we have work to do. :)

So if you can't come up with something that seems 'reasonable' to test,
I suggest random gibberish(!) and send the outcome of those iterations
to the list to see what kinds of arguments you can stir up.  Since we're
only interested in testing the mounting code here, you can declare
victory if the fs mounts, never mind if the option actually has any
effect on fs operations.  That kind of functional testing should be in
separate tests anyway.

One advantage that you probably have over us is that our understanding
of the mount options and associated behavior is based on a lot of
experience working in the code base, whereas most everyone else's is
based entirely on whatever's in the manpage.  It's helpful to have
someone hold us to our words every now and then.

(This is going to get interesting when we get to mount options whose
validity changes depending on mkfs parameters, etc.)

--D

> > 
> > Brian
> > 
> > > 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 sometimes inconsistent logging.
> > > 
> > >   The mount-api feature of saving error message text to the mount
> > >   context for later retrieval 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 so it which can lead to messages,
> > >   possibly needed for debug purposes, not being sent to the kernel
> > >   log. There's also the pr_xxx() log functions (not a problem for
> > >   xfs AFAICS) that aren't aware of the mount context at all.
> > > 
> > >   In the xfs patches I've used the same method that is used in
> > >   gfs2 and was suggested by Al Viro (essentially return the error
> > >   if fs_parse() returns one) except that I've also not used the
> > >   mount api log macros to minimise the possibility of lost log
> > >   messages.
> > > 
> > >   This isn't the best so follow up patches for RFC (with a
> > >   slightly wider audience) will be needed to try and improve
> > >   this aspect of the mount api.
> > > 
> > > Changes for v4:
> > > - changed xfs_fill_super() cleanup back to what it was in v2, until
> > >   I can work out what's causing the problem had previously seen (I
> > > can't
> > >   reproduce it myself), since it looks like it was right from the
> > > start.
> > > - use get_tree_bdev() instead of vfs_get_block_super() in
> > > xfs_get_tree()
> > >   as requested by Al Viro.
> > > - removed redundant initialisation in xfs_fs_fill_super().
> > > - fix long line in xfs_validate_params().
> > > - no need to validate if parameter parsing fails, just return the
> > > error.
> > > - summarise reconfigure comment about option handling, transfer
> > > bulk
> > >   of comment to commit log message.
> > > - use minimal change in xfs_parse_param(), deffer discussion of
> > > mount
> > >   api logging improvements until later and with a wider audience.
> > > 
> > > Changes for v3:
> > > - fix struct xfs_fs_context initialisation 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 obsolete 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.
> > > - clarify 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 manageable.
> > > 
> > > Al Viro has sent a pull request to Linus for the patch containing
> > > get_tree_bdev() recently and I think there's a small problem with
> > > that patch too so there will be conflicts with merging this series
> > > without dropping the first two patches of the series.
> > > 
> > > ---
> > > 
> > > David Howells (1):
> > >       vfs: Create fs_context-aware mount_bdev() replacement
> > > 
> > > Ian Kent (16):
> > >       vfs: add missing blkdev_put() in get_tree_bdev()
> > >       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 remaining legacy mount code
> > > 
> > > 
> > >  fs/super.c                 |   97 +++++
> > >  fs/xfs/xfs_super.c         |  939 +++++++++++++++++++++++---------
> > > ------------
> > >  include/linux/fs_context.h |    5 
> > >  3 files changed, 600 insertions(+), 441 deletions(-)
> > > 
> > > --
> > > Ian
> 

Re: [PATCH v4 00/17] xfs: mount API patch series

[Ian Kent]

On Mon, 2019-10-07 at 17:35 -0700, Darrick J. Wong wrote:
> On Tue, Oct 08, 2019 at 08:13:57AM +0800, Ian Kent wrote:
> > On Mon, 2019-10-07 at 07:52 -0400, Brian Foster wrote:
> > > On Thu, Oct 03, 2019 at 06:25:18PM +0800, Ian Kent wrote:
> > > > This patch series add 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 along with the generic xfstests.
> > > > 
> > > 
> > > I'm not sure that we have any focused mount option testing in
> > > fstests.
> > > It looks like we have various remount tests and such to cover
> > > corner
> > > cases and/or specific bugs, but nothing to make sure various
> > > options
> > > continue to work or otherwise fail gracefully. Do you have any
> > > plans
> > > to
> > > add such a test to help verify this work?
> > 
> > Darrick was concerned about that.
> > 
> > Some sort of xfstest is needed in order to be able to merge this
> > so he has some confidence that it won't break things.
> > 
> > I volunteered to do have a go at writing a test.
> > 
> > I've given that some thought and done an initial survey of xfstests
> > but it's still new to me so I'm not sure how this will end up.
> > 
> > Darrick thought it would need a generic test to test VFS options
> > and one in xfs for the xfs specific options.
> > 
> > At this point I'm thinking I'll have a go at adding an xfs specific
> > options test but, while I can find out what the optionsare and what
> > validation they use, there's a lot about some of the xfs options
> > I don't fully understand so I don't know what a sensible test might
> > be.
> 
> Hm, that's evidence of inadequate documentation.  If you can't figure
> out what would be sensible tests for a particular mount option from
> xfs(5) then we have work to do. :)

Maybe, I have looked at xfs(5) but haven't yet started trying to
work out what I need to do so we will see how that goes.

> 
> So if you can't come up with something that seems 'reasonable' to
> test,
> I suggest random gibberish(!) and send the outcome of those
> iterations
> to the list to see what kinds of arguments you can stir up.  Since
> we're
> only interested in testing the mounting code here, you can declare
> victory if the fs mounts, never mind if the option actually has any
> effect on fs operations.  That kind of functional testing should be
> in
> separate tests anyway.

I thought your suggestion of minimum, maximum and out of range for
options that have a range is good. There's also the individual
options which should be straight forward.

But there's a range of other options that sound like they aren't
straight forward.

For example, IIRC, I can give inode64 or inode32 on mount regardless
of (presumably) the on-disk inode size which seemed odd to me.

But of course the file system isn't mounted yet so the options
parsing won't know this at the time. I supposed that would be
handled later, probably with some sort of warning to the log.

> 
> One advantage that you probably have over us is that our
> understanding
> of the mount options and associated behavior is based on a lot of
> experience working in the code base, whereas most everyone else's is
> based entirely on whatever's in the manpage.  It's helpful to have
> someone hold us to our words every now and then.

Indeed, I think this will be a useful exercise for xfs and myself.

> 
> (This is going to get interesting when we get to mount options whose
> validity changes depending on mkfs parameters, etc.)

Second pass of writing the test will need input on that.

Perhaps (but probably not yet so I don't make implicit assumptions)
someone could come up with a list of common mkfs vs needed mount
options for the more sophisticated tests once I get to them.

Ian
> 
> --D
> 
> > > Brian
> > > 
> > > > 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 sometimes inconsistent logging.
> > > > 
> > > >   The mount-api feature of saving error message text to the
> > > > mount
> > > >   context for later retrieval 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 so it which can lead to
> > > > messages,
> > > >   possibly needed for debug purposes, not being sent to the
> > > > kernel
> > > >   log. There's also the pr_xxx() log functions (not a problem
> > > > for
> > > >   xfs AFAICS) that aren't aware of the mount context at all.
> > > > 
> > > >   In the xfs patches I've used the same method that is used in
> > > >   gfs2 and was suggested by Al Viro (essentially return the
> > > > error
> > > >   if fs_parse() returns one) except that I've also not used the
> > > >   mount api log macros to minimise the possibility of lost log
> > > >   messages.
> > > > 
> > > >   This isn't the best so follow up patches for RFC (with a
> > > >   slightly wider audience) will be needed to try and improve
> > > >   this aspect of the mount api.
> > > > 
> > > > Changes for v4:
> > > > - changed xfs_fill_super() cleanup back to what it was in v2,
> > > > until
> > > >   I can work out what's causing the problem had previously seen
> > > > (I
> > > > can't
> > > >   reproduce it myself), since it looks like it was right from
> > > > the
> > > > start.
> > > > - use get_tree_bdev() instead of vfs_get_block_super() in
> > > > xfs_get_tree()
> > > >   as requested by Al Viro.
> > > > - removed redundant initialisation in xfs_fs_fill_super().
> > > > - fix long line in xfs_validate_params().
> > > > - no need to validate if parameter parsing fails, just return
> > > > the
> > > > error.
> > > > - summarise reconfigure comment about option handling, transfer
> > > > bulk
> > > >   of comment to commit log message.
> > > > - use minimal change in xfs_parse_param(), deffer discussion of
> > > > mount
> > > >   api logging improvements until later and with a wider
> > > > audience.
> > > > 
> > > > Changes for v3:
> > > > - fix struct xfs_fs_context initialisation 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 obsolete 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.
> > > > - clarify 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
> > > > manageable.
> > > > 
> > > > Al Viro has sent a pull request to Linus for the patch
> > > > containing
> > > > get_tree_bdev() recently and I think there's a small problem
> > > > with
> > > > that patch too so there will be conflicts with merging this
> > > > series
> > > > without dropping the first two patches of the series.
> > > > 
> > > > ---
> > > > 
> > > > David Howells (1):
> > > >       vfs: Create fs_context-aware mount_bdev() replacement
> > > > 
> > > > Ian Kent (16):
> > > >       vfs: add missing blkdev_put() in get_tree_bdev()
> > > >       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 remaining legacy mount code
> > > > 
> > > > 
> > > >  fs/super.c                 |   97 +++++
> > > >  fs/xfs/xfs_super.c         |  939 +++++++++++++++++++++++-----
> > > > ----
> > > > ------------
> > > >  include/linux/fs_context.h |    5 
> > > >  3 files changed, 600 insertions(+), 441 deletions(-)
> > > > 
> > > > --
> > > > Ian

Re: [PATCH v4 00/17] xfs: mount API patch series

[Darrick J. Wong]

On Tue, Oct 08, 2019 at 09:20:19AM +0800, Ian Kent wrote:
> On Mon, 2019-10-07 at 17:35 -0700, Darrick J. Wong wrote:
> > On Tue, Oct 08, 2019 at 08:13:57AM +0800, Ian Kent wrote:
> > > On Mon, 2019-10-07 at 07:52 -0400, Brian Foster wrote:
> > > > On Thu, Oct 03, 2019 at 06:25:18PM +0800, Ian Kent wrote:
> > > > > This patch series add 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 along with the generic xfstests.
> > > > > 
> > > > 
> > > > I'm not sure that we have any focused mount option testing in
> > > > fstests.
> > > > It looks like we have various remount tests and such to cover
> > > > corner
> > > > cases and/or specific bugs, but nothing to make sure various
> > > > options
> > > > continue to work or otherwise fail gracefully. Do you have any
> > > > plans
> > > > to
> > > > add such a test to help verify this work?
> > > 
> > > Darrick was concerned about that.
> > > 
> > > Some sort of xfstest is needed in order to be able to merge this
> > > so he has some confidence that it won't break things.
> > > 
> > > I volunteered to do have a go at writing a test.
> > > 
> > > I've given that some thought and done an initial survey of xfstests
> > > but it's still new to me so I'm not sure how this will end up.
> > > 
> > > Darrick thought it would need a generic test to test VFS options
> > > and one in xfs for the xfs specific options.
> > > 
> > > At this point I'm thinking I'll have a go at adding an xfs specific
> > > options test but, while I can find out what the optionsare and what
> > > validation they use, there's a lot about some of the xfs options
> > > I don't fully understand so I don't know what a sensible test might
> > > be.
> > 
> > Hm, that's evidence of inadequate documentation.  If you can't figure
> > out what would be sensible tests for a particular mount option from
> > xfs(5) then we have work to do. :)
> 
> Maybe, I have looked at xfs(5) but haven't yet started trying to
> work out what I need to do so we will see how that goes.
> 
> > 
> > So if you can't come up with something that seems 'reasonable' to
> > test,
> > I suggest random gibberish(!) and send the outcome of those
> > iterations
> > to the list to see what kinds of arguments you can stir up.  Since
> > we're
> > only interested in testing the mounting code here, you can declare
> > victory if the fs mounts, never mind if the option actually has any
> > effect on fs operations.  That kind of functional testing should be
> > in
> > separate tests anyway.
> 
> I thought your suggestion of minimum, maximum and out of range for
> options that have a range is good. There's also the individual
> options which should be straight forward.
> 
> But there's a range of other options that sound like they aren't
> straight forward.

<nod> If you use the fstests scratch device for the crash-dummy
filesystem, the worst that happens is we screw up and (so long as the
kernel doesn't crash) the device gets wiped between tests.

> For example, IIRC, I can give inode64 or inode32 on mount regardless
> of (presumably) the on-disk inode size which seemed odd to me.
> 
> But of course the file system isn't mounted yet so the options
> parsing won't know this at the time. I supposed that would be
> handled later, probably with some sort of warning to the log.

inode32 has nothing to do with inode size, just (new) inode location.

Specifically, it prevents allocation of an inode that would have a
64-bit inode number.

> > 
> > One advantage that you probably have over us is that our
> > understanding
> > of the mount options and associated behavior is based on a lot of
> > experience working in the code base, whereas most everyone else's is
> > based entirely on whatever's in the manpage.  It's helpful to have
> > someone hold us to our words every now and then.
> 
> Indeed, I think this will be a useful exercise for xfs and myself.
> 
> > 
> > (This is going to get interesting when we get to mount options whose
> > validity changes depending on mkfs parameters, etc.)
> 
> Second pass of writing the test will need input on that.
> 
> Perhaps (but probably not yet so I don't make implicit assumptions)
> someone could come up with a list of common mkfs vs needed mount
> options for the more sophisticated tests once I get to them.

Looking forward to it. :)

--D

> Ian
> > 
> > --D
> > 
> > > > Brian
> > > > 
> > > > > 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 sometimes inconsistent logging.
> > > > > 
> > > > >   The mount-api feature of saving error message text to the
> > > > > mount
> > > > >   context for later retrieval 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 so it which can lead to
> > > > > messages,
> > > > >   possibly needed for debug purposes, not being sent to the
> > > > > kernel
> > > > >   log. There's also the pr_xxx() log functions (not a problem
> > > > > for
> > > > >   xfs AFAICS) that aren't aware of the mount context at all.
> > > > > 
> > > > >   In the xfs patches I've used the same method that is used in
> > > > >   gfs2 and was suggested by Al Viro (essentially return the
> > > > > error
> > > > >   if fs_parse() returns one) except that I've also not used the
> > > > >   mount api log macros to minimise the possibility of lost log
> > > > >   messages.
> > > > > 
> > > > >   This isn't the best so follow up patches for RFC (with a
> > > > >   slightly wider audience) will be needed to try and improve
> > > > >   this aspect of the mount api.
> > > > > 
> > > > > Changes for v4:
> > > > > - changed xfs_fill_super() cleanup back to what it was in v2,
> > > > > until
> > > > >   I can work out what's causing the problem had previously seen
> > > > > (I
> > > > > can't
> > > > >   reproduce it myself), since it looks like it was right from
> > > > > the
> > > > > start.
> > > > > - use get_tree_bdev() instead of vfs_get_block_super() in
> > > > > xfs_get_tree()
> > > > >   as requested by Al Viro.
> > > > > - removed redundant initialisation in xfs_fs_fill_super().
> > > > > - fix long line in xfs_validate_params().
> > > > > - no need to validate if parameter parsing fails, just return
> > > > > the
> > > > > error.
> > > > > - summarise reconfigure comment about option handling, transfer
> > > > > bulk
> > > > >   of comment to commit log message.
> > > > > - use minimal change in xfs_parse_param(), deffer discussion of
> > > > > mount
> > > > >   api logging improvements until later and with a wider
> > > > > audience.
> > > > > 
> > > > > Changes for v3:
> > > > > - fix struct xfs_fs_context initialisation 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 obsolete 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.
> > > > > - clarify 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
> > > > > manageable.
> > > > > 
> > > > > Al Viro has sent a pull request to Linus for the patch
> > > > > containing
> > > > > get_tree_bdev() recently and I think there's a small problem
> > > > > with
> > > > > that patch too so there will be conflicts with merging this
> > > > > series
> > > > > without dropping the first two patches of the series.
> > > > > 
> > > > > ---
> > > > > 
> > > > > David Howells (1):
> > > > >       vfs: Create fs_context-aware mount_bdev() replacement
> > > > > 
> > > > > Ian Kent (16):
> > > > >       vfs: add missing blkdev_put() in get_tree_bdev()
> > > > >       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 remaining legacy mount code
> > > > > 
> > > > > 
> > > > >  fs/super.c                 |   97 +++++
> > > > >  fs/xfs/xfs_super.c         |  939 +++++++++++++++++++++++-----
> > > > > ----
> > > > > ------------
> > > > >  include/linux/fs_context.h |    5 
> > > > >  3 files changed, 600 insertions(+), 441 deletions(-)
> > > > > 
> > > > > --
> > > > > Ian
>