[PATCH 01/10] xfs: mount-api - add fs parameter description

[PATCH 01/10] xfs: mount-api - add fs parameter description

[Ian Kent]

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

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

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

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index a14d11d78bd8..ab8145bf6fff 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -51,6 +51,8 @@
 #include <linux/kthread.h>
 #include <linux/freezer.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;
@@ -60,9 +62,6 @@ static struct kset *xfs_kset;		/* top-level xfs sysfs dir */
 static struct xfs_kobj xfs_dbg_kobj;	/* global debug sysfs attrs */
 #endif
 
-/*
- * Table driven mount option parser.
- */
 enum {
 	Opt_logbufs, Opt_logbsize, Opt_logdev, Opt_rtdev, Opt_biosize,
 	Opt_wsync, Opt_noalign, Opt_swalloc, Opt_sunit, Opt_swidth, Opt_nouuid,
@@ -122,6 +121,49 @@ 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_u32	("biosize",    Opt_biosize),   /* log2 of preferred buffered io size */
+ 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_no("grpid",      Opt_grpid),     /* group-ID from parent directory (or not) */
+ 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_no("ikeep",      Opt_ikeep),     /* do not free (or keep) empty inode clusters */
+ fsparam_flag_no("largeio",    Opt_largeio),   /* report (or do not report) large I/O sizes in stat() */
+ fsparam_flag_no("attr2",      Opt_attr2),     /* do (or do not) use attr2 attribute format */
+ fsparam_flag	("filestreams",Opt_filestreams), /* use filestreams allocator */
+ fsparam_flag_no("quota",      Opt_quota),     /* disk quotas (user) */
+ 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_no("discard",    Opt_discard),   /* Do (or do not) 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 02/10] 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 switching to use the new mount-api match_kstrtoint()
will no longer be called and will be removed.

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

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index ab8145bf6fff..14c2a775786c 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -166,13 +166,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;
 
@@ -197,6 +197,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.
@@ -268,7 +282,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:
@@ -285,7 +299,7 @@ xfs_parseargs(
 			break;
 		case Opt_allocsize:
 		case Opt_biosize:
-			if (suffix_kstrtoint(args, 10, &iosize))
+			if (match_kstrtoint(args, 10, &iosize))
 				return -EINVAL;
 			iosizelog = ffs(iosize) - 1;
 			break;

Re: [PATCH 02/10] xfs: mount-api - refactor suffix_kstrtoint()

[Darrick J. Wong]

On Mon, Jun 24, 2019 at 10:58:30AM +0800, Ian Kent wrote:
> 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.

/me wonders if that ought to be lifted to fs_parser.c, or is xfs the
only filesystem that has mount options with unit suffixes?

--D

> 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 switching to use the new mount-api match_kstrtoint()
> will no longer be called and will be removed.
> 
> Signed-off-by: Ian Kent <raven@themaw.net>
> ---
>  fs/xfs/xfs_super.c |   22 ++++++++++++++++++----
>  1 file changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index ab8145bf6fff..14c2a775786c 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -166,13 +166,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;
>  
> @@ -197,6 +197,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.
> @@ -268,7 +282,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:
> @@ -285,7 +299,7 @@ xfs_parseargs(
>  			break;
>  		case Opt_allocsize:
>  		case Opt_biosize:
> -			if (suffix_kstrtoint(args, 10, &iosize))
> +			if (match_kstrtoint(args, 10, &iosize))
>  				return -EINVAL;
>  			iosizelog = ffs(iosize) - 1;
>  			break;
> 

Re: [PATCH 02/10] xfs: mount-api - refactor suffix_kstrtoint()

[Dave Chinner]

On Mon, Jun 24, 2019 at 10:29:43AM -0700, Darrick J. Wong wrote:
> On Mon, Jun 24, 2019 at 10:58:30AM +0800, Ian Kent wrote:
> > 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.
> 
> /me wonders if that ought to be lifted to fs_parser.c, or is xfs the
> only filesystem that has mount options with unit suffixes?

I've suggested the same thing (I've seen this patchset before :)
and ISTR it makes everything easier if we just keep it here for this
patchset and then lift it once everything is merged...

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 02/10] xfs: mount-api - refactor suffix_kstrtoint()

[Darrick J. Wong]

On Tue, Jun 25, 2019 at 08:35:54AM +1000, Dave Chinner wrote:
> On Mon, Jun 24, 2019 at 10:29:43AM -0700, Darrick J. Wong wrote:
> > On Mon, Jun 24, 2019 at 10:58:30AM +0800, Ian Kent wrote:
> > > 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.
> > 
> > /me wonders if that ought to be lifted to fs_parser.c, or is xfs the
> > only filesystem that has mount options with unit suffixes?
> 
> I've suggested the same thing (I've seen this patchset before :)
> and ISTR it makes everything easier if we just keep it here for this
> patchset and then lift it once everything is merged...

Ok, fair enough. :)

--D

> -Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

Re: [PATCH 02/10] xfs: mount-api - refactor suffix_kstrtoint()

[Ian Kent]

On Mon, 2019-06-24 at 16:06 -0700, Darrick J. Wong wrote:
> On Tue, Jun 25, 2019 at 08:35:54AM +1000, Dave Chinner wrote:
> > On Mon, Jun 24, 2019 at 10:29:43AM -0700, Darrick J. Wong wrote:
> > > On Mon, Jun 24, 2019 at 10:58:30AM +0800, Ian Kent wrote:
> > > > 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.
> > > 
> > > /me wonders if that ought to be lifted to fs_parser.c, or is xfs the
> > > only filesystem that has mount options with unit suffixes?
> > 
> > I've suggested the same thing (I've seen this patchset before :)
> > and ISTR it makes everything easier if we just keep it here for this
> > patchset and then lift it once everything is merged...
> 
> Ok, fair enough. :)

I mentioned it to David after Dave said much the same thing, David said
he had done some work on it but I haven't heard more yet.

> 
> --D
> 
> > -Dave.
> > -- 
> > Dave Chinner
> > david@fromorbit.com

[PATCH 03/10] xfs: mount-api - add xfs_parse_param()

[Ian Kent]

Add the fs_context_operations method .parse_param that's called
by the mount-api ito parse each file system mount option.

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

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 14c2a775786c..e78fea14d598 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -495,6 +495,178 @@ xfs_parseargs(
 	return 0;
 }
 
+struct xfs_fs_context {
+	int	dsunit;
+	int	dswidth;
+	uint8_t	iosizelog;
+};
+
+/*
+ * 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.
+ */
+STATIC int
+xfs_parse_param(
+	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;
+
+	opt = fs_parse(fc, &xfs_fs_parameters, param, &result);
+	if (opt < 0)
+		return opt;
+
+	switch (opt) {
+	case Opt_logbufs:
+		mp->m_logbufs = result.uint_32;
+		break;
+	case Opt_logbsize:
+		if (suffix_kstrtoint(param->string, 10, &mp->m_logbsize))
+			return -EINVAL;
+		break;
+	case Opt_logdev:
+		kfree(mp->m_logname);
+		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 = kstrdup(param->string, GFP_KERNEL);
+		if (!mp->m_rtname)
+			return -ENOMEM;
+		break;
+	case Opt_allocsize:
+	case Opt_biosize:
+		if (suffix_kstrtoint(param->string, 10, &iosize))
+			return -EINVAL;
+		ctx->iosizelog = ffs(iosize) - 1;
+		break;
+	case Opt_bsdgroups:
+		mp->m_flags |= XFS_MOUNT_GRPID;
+		break;
+	case Opt_grpid:
+		if (result.negated)
+			mp->m_flags &= ~XFS_MOUNT_GRPID;
+		else
+			mp->m_flags |= XFS_MOUNT_GRPID;
+		break;
+	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:
+		ctx->dsunit = result.uint_32;
+		break;
+	case Opt_swidth:
+		ctx->dswidth = result.uint_32;
+		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:
+		if (result.negated)
+			mp->m_flags &= ~XFS_MOUNT_IKEEP;
+		else
+			mp->m_flags |= XFS_MOUNT_IKEEP;
+		break;
+	case Opt_largeio:
+		if (result.negated)
+			mp->m_flags |= XFS_MOUNT_COMPAT_IOSIZE;
+		else
+			mp->m_flags &= ~XFS_MOUNT_COMPAT_IOSIZE;
+		break;
+	case Opt_attr2:
+		if (!result.negated) {
+			mp->m_flags |= XFS_MOUNT_ATTR2;
+		} else {
+			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_quota:
+		if (!result.negated) {
+			mp->m_qflags |= (XFS_UQUOTA_ACCT | XFS_UQUOTA_ACTIVE |
+					 XFS_UQUOTA_ENFD);
+		} else {
+			mp->m_qflags &= ~XFS_ALL_QUOTA_ACCT;
+			mp->m_qflags &= ~XFS_ALL_QUOTA_ENFD;
+			mp->m_qflags &= ~XFS_ALL_QUOTA_ACTIVE;
+		}
+		break;
+	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:
+		if (result.negated)
+			mp->m_flags &= ~XFS_MOUNT_DISCARD;
+		else
+			mp->m_flags |= XFS_MOUNT_DISCARD;
+		break;
+#ifdef CONFIG_FS_DAX
+	case Opt_dax:
+		mp->m_flags |= XFS_MOUNT_DAX;
+		break;
+#endif
+	default:
+		return invalf(fc, "XFS: unknown mount option [%s].", param->key);
+	}
+
+	return 0;
+}
+
 struct proc_xfs_info {
 	uint64_t	flag;
 	char		*str;
@@ -1914,6 +2086,10 @@ 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,
+};
+
 static struct file_system_type xfs_fs_type = {
 	.owner			= THIS_MODULE,
 	.name			= "xfs",

Re: [PATCH 03/10] xfs: mount-api - add xfs_parse_param()

[Darrick J. Wong]

On Mon, Jun 24, 2019 at 10:58:36AM +0800, Ian Kent wrote:
> Add the fs_context_operations method .parse_param that's called
> by the mount-api ito parse each file system mount option.
> 
> Signed-off-by: Ian Kent <raven@themaw.net>
> ---
>  fs/xfs/xfs_super.c |  176 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 176 insertions(+)
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 14c2a775786c..e78fea14d598 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -495,6 +495,178 @@ xfs_parseargs(
>  	return 0;
>  }
>  
> +struct xfs_fs_context {
> +	int	dsunit;
> +	int	dswidth;
> +	uint8_t	iosizelog;
> +};
> +
> +/*
> + * 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.

Wait, what?  Do you mean "The caller is responsible for freeing the
device name allocations if option parsing ends in failure"?

> + */
> +STATIC int
> +xfs_parse_param(
> +	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;
> +
> +	opt = fs_parse(fc, &xfs_fs_parameters, param, &result);
> +	if (opt < 0)
> +		return opt;
> +
> +	switch (opt) {
> +	case Opt_logbufs:
> +		mp->m_logbufs = result.uint_32;
> +		break;
> +	case Opt_logbsize:
> +		if (suffix_kstrtoint(param->string, 10, &mp->m_logbsize))
> +			return -EINVAL;
> +		break;
> +	case Opt_logdev:
> +		kfree(mp->m_logname);
> +		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 = kstrdup(param->string, GFP_KERNEL);
> +		if (!mp->m_rtname)
> +			return -ENOMEM;
> +		break;
> +	case Opt_allocsize:
> +	case Opt_biosize:
> +		if (suffix_kstrtoint(param->string, 10, &iosize))
> +			return -EINVAL;
> +		ctx->iosizelog = ffs(iosize) - 1;
> +		break;
> +	case Opt_bsdgroups:
> +		mp->m_flags |= XFS_MOUNT_GRPID;
> +		break;
> +	case Opt_grpid:
> +		if (result.negated)
> +			mp->m_flags &= ~XFS_MOUNT_GRPID;
> +		else
> +			mp->m_flags |= XFS_MOUNT_GRPID;
> +		break;
> +	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:
> +		ctx->dsunit = result.uint_32;
> +		break;
> +	case Opt_swidth:
> +		ctx->dswidth = result.uint_32;
> +		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:
> +		if (result.negated)
> +			mp->m_flags &= ~XFS_MOUNT_IKEEP;
> +		else
> +			mp->m_flags |= XFS_MOUNT_IKEEP;
> +		break;
> +	case Opt_largeio:
> +		if (result.negated)
> +			mp->m_flags |= XFS_MOUNT_COMPAT_IOSIZE;
> +		else
> +			mp->m_flags &= ~XFS_MOUNT_COMPAT_IOSIZE;
> +		break;
> +	case Opt_attr2:
> +		if (!result.negated) {
> +			mp->m_flags |= XFS_MOUNT_ATTR2;
> +		} else {
> +			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_quota:
> +		if (!result.negated) {
> +			mp->m_qflags |= (XFS_UQUOTA_ACCT | XFS_UQUOTA_ACTIVE |
> +					 XFS_UQUOTA_ENFD);
> +		} else {
> +			mp->m_qflags &= ~XFS_ALL_QUOTA_ACCT;
> +			mp->m_qflags &= ~XFS_ALL_QUOTA_ENFD;
> +			mp->m_qflags &= ~XFS_ALL_QUOTA_ACTIVE;
> +		}
> +		break;
> +	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:
> +		if (result.negated)
> +			mp->m_flags &= ~XFS_MOUNT_DISCARD;
> +		else
> +			mp->m_flags |= XFS_MOUNT_DISCARD;
> +		break;
> +#ifdef CONFIG_FS_DAX
> +	case Opt_dax:
> +		mp->m_flags |= XFS_MOUNT_DAX;
> +		break;
> +#endif
> +	default:
> +		return invalf(fc, "XFS: unknown mount option [%s].", param->key);

What do these messages end up looking like in dmesg?

The reason I ask is that today when mount option processing fails we log
the device name in the error message:

# mount /dev/sda1 /mnt -o gribblegronk
[64010.878477] XFS (sda1): unknown mount option [gribblegronk].

AFAICT using invalf (instead of xfs_warn) means that now we don't report
the device name, and all you'd get is:

"[64010.878477] XFS: unknown mount option [gribblegronk]."

which is not as helpful...

--D

> +	}
> +
> +	return 0;
> +}
> +
>  struct proc_xfs_info {
>  	uint64_t	flag;
>  	char		*str;
> @@ -1914,6 +2086,10 @@ 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,
> +};
> +
>  static struct file_system_type xfs_fs_type = {
>  	.owner			= THIS_MODULE,
>  	.name			= "xfs",
> 

Re: [PATCH 03/10] xfs: mount-api - add xfs_parse_param()

[Ian Kent]

On Mon, 2019-06-24 at 10:26 -0700, Darrick J. Wong wrote:
> On Mon, Jun 24, 2019 at 10:58:36AM +0800, Ian Kent wrote:
> > Add the fs_context_operations method .parse_param that's called
> > by the mount-api ito parse each file system mount option.
> > 
> > Signed-off-by: Ian Kent <raven@themaw.net>
> > ---
> >  fs/xfs/xfs_super.c |  176
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 176 insertions(+)
> > 
> > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > index 14c2a775786c..e78fea14d598 100644
> > --- a/fs/xfs/xfs_super.c
> > +++ b/fs/xfs/xfs_super.c
> > @@ -495,6 +495,178 @@ xfs_parseargs(
> >  	return 0;
> >  }
> >  
> > +struct xfs_fs_context {
> > +	int	dsunit;
> > +	int	dswidth;
> > +	uint8_t	iosizelog;
> > +};
> > +
> > +/*
> > + * 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.
> 
> Wait, what?  Do you mean "The caller is responsible for freeing the
> device name allocations if option parsing ends in failure"?

Mmm ... yes, that needs to be re-worded.

It sounded sensible at the time and was copied from the initial
code, but it isn't clear, I'll have another go at that one in
v2.

> 
> > + */
> > +STATIC int
> > +xfs_parse_param(
> > +	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;
> > +
> > +	opt = fs_parse(fc, &xfs_fs_parameters, param, &result);
> > +	if (opt < 0)
> > +		return opt;
> > +
> > +	switch (opt) {
> > +	case Opt_logbufs:
> > +		mp->m_logbufs = result.uint_32;
> > +		break;
> > +	case Opt_logbsize:
> > +		if (suffix_kstrtoint(param->string, 10, &mp->m_logbsize))
> > +			return -EINVAL;
> > +		break;
> > +	case Opt_logdev:
> > +		kfree(mp->m_logname);
> > +		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 = kstrdup(param->string, GFP_KERNEL);
> > +		if (!mp->m_rtname)
> > +			return -ENOMEM;
> > +		break;
> > +	case Opt_allocsize:
> > +	case Opt_biosize:
> > +		if (suffix_kstrtoint(param->string, 10, &iosize))
> > +			return -EINVAL;
> > +		ctx->iosizelog = ffs(iosize) - 1;
> > +		break;
> > +	case Opt_bsdgroups:
> > +		mp->m_flags |= XFS_MOUNT_GRPID;
> > +		break;
> > +	case Opt_grpid:
> > +		if (result.negated)
> > +			mp->m_flags &= ~XFS_MOUNT_GRPID;
> > +		else
> > +			mp->m_flags |= XFS_MOUNT_GRPID;
> > +		break;
> > +	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:
> > +		ctx->dsunit = result.uint_32;
> > +		break;
> > +	case Opt_swidth:
> > +		ctx->dswidth = result.uint_32;
> > +		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:
> > +		if (result.negated)
> > +			mp->m_flags &= ~XFS_MOUNT_IKEEP;
> > +		else
> > +			mp->m_flags |= XFS_MOUNT_IKEEP;
> > +		break;
> > +	case Opt_largeio:
> > +		if (result.negated)
> > +			mp->m_flags |= XFS_MOUNT_COMPAT_IOSIZE;
> > +		else
> > +			mp->m_flags &= ~XFS_MOUNT_COMPAT_IOSIZE;
> > +		break;
> > +	case Opt_attr2:
> > +		if (!result.negated) {
> > +			mp->m_flags |= XFS_MOUNT_ATTR2;
> > +		} else {
> > +			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_quota:
> > +		if (!result.negated) {
> > +			mp->m_qflags |= (XFS_UQUOTA_ACCT | XFS_UQUOTA_ACTIVE |
> > +					 XFS_UQUOTA_ENFD);
> > +		} else {
> > +			mp->m_qflags &= ~XFS_ALL_QUOTA_ACCT;
> > +			mp->m_qflags &= ~XFS_ALL_QUOTA_ENFD;
> > +			mp->m_qflags &= ~XFS_ALL_QUOTA_ACTIVE;
> > +		}
> > +		break;
> > +	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:
> > +		if (result.negated)
> > +			mp->m_flags &= ~XFS_MOUNT_DISCARD;
> > +		else
> > +			mp->m_flags |= XFS_MOUNT_DISCARD;
> > +		break;
> > +#ifdef CONFIG_FS_DAX
> > +	case Opt_dax:
> > +		mp->m_flags |= XFS_MOUNT_DAX;
> > +		break;
> > +#endif
> > +	default:
> > +		return invalf(fc, "XFS: unknown mount option [%s].", param-
> > >key);
> 
> What do these messages end up looking like in dmesg?
> 
> The reason I ask is that today when mount option processing fails we log
> the device name in the error message:
> 
> # mount /dev/sda1 /mnt -o gribblegronk
> [64010.878477] XFS (sda1): unknown mount option [gribblegronk].
> 
> AFAICT using invalf (instead of xfs_warn) means that now we don't report
> the device name, and all you'd get is:
> 
> "[64010.878477] XFS: unknown mount option [gribblegronk]."
> 
> which is not as helpful...

Yes, I thought that might be seen as a problem.

The device name was obtained from the super block in the the
original code and the super block isn't available at this
point.

Not sure what to do about it but I'll have a look.

> 
> --D
> 
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  struct proc_xfs_info {
> >  	uint64_t	flag;
> >  	char		*str;
> > @@ -1914,6 +2086,10 @@ 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,
> > +};
> > +
> >  static struct file_system_type xfs_fs_type = {
> >  	.owner			= THIS_MODULE,
> >  	.name			= "xfs",
> > 

Re: [PATCH 03/10] xfs: mount-api - add xfs_parse_param()

[Ian Kent]

On Tue, 2019-06-25 at 07:47 +0800, Ian Kent wrote:
> +	default:
> > > +		return invalf(fc, "XFS: unknown mount option [%s].", param-
> > > > key);
> > 
> > What do these messages end up looking like in dmesg?
> > 
> > The reason I ask is that today when mount option processing fails we log
> > the device name in the error message:
> > 
> > # mount /dev/sda1 /mnt -o gribblegronk
> > [64010.878477] XFS (sda1): unknown mount option [gribblegronk].
> > 
> > AFAICT using invalf (instead of xfs_warn) means that now we don't report
> > the device name, and all you'd get is:
> > 
> > "[64010.878477] XFS: unknown mount option [gribblegronk]."
> > 
> > which is not as helpful...
> 
> Yes, I thought that might be seen as a problem.
> 
> The device name was obtained from the super block in the the
> original code and the super block isn't available at this
> point.
> 
> Not sure what to do about it but I'll have a look.
> 

It turns out that I also made a mistake with the error string.

The above would actually print (something like):
[64010.878477] xfs: XFS: unknown mount option [gribblegronk]."

I can change the case of the struct fs_parameter_description
.name and leave out the "XFS" in the error string to fix that.

But, because the parameter parsing is done before super block
creation, there's no bdev to get a device name from.

There should be a source (block device path) filed set in
the fs context and I could dup the last component of that
or walk the path to resolve symlinks and dup the path
dentry d_name which should give (something like) the
device name.

But the printk %pg format is a bit different in the way
it constructs the name from the bdev so it could well be
different.

That could lead to confusion too.

Also, if an error is occurs in the VFS the message won't
have the device name either.

So I'm not sure what to do about this one, suggestions?

Ian

[PATCH 04/10] 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 used by the new mount-api.

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

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

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index e78fea14d598..cf8efb465969 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1823,27 +1823,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;
@@ -1870,7 +1857,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)
@@ -2003,10 +1990,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;
 
@@ -2016,6 +1999,42 @@ xfs_fs_fill_super(
 	goto out_free_sb;
 }
 
+STATIC int
+xfs_fs_fill_super(
+	struct super_block	*sb,
+	void			*data,
+	int			silent)
+{
+	struct xfs_mount	*mp = NULL;
+	int			error = -ENOMEM;
+
+	/*
+	 * allocate mp and do all low-level struct initializations before we
+	 * attach it to the super
+	 */
+	mp = xfs_mount_alloc(sb);
+	if (!mp)
+		goto out;
+	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);
+out:
+	return error;
+}
+
 STATIC void
 xfs_fs_put_super(
 	struct super_block	*sb)

[PATCH 05/10] 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 |  139 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 139 insertions(+)

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index cf8efb465969..0ec0142b94e1 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1629,6 +1629,98 @@ xfs_fs_remount(
 	return 0;
 }
 
+STATIC int
+xfs_validate_params(
+	struct xfs_mount *mp,
+	struct xfs_fs_context *ctx,
+	enum fs_context_purpose purpose)
+{
+	/*
+	 * 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;
+	}
+
+	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.
+		 */
+		if (purpose == FS_CONTEXT_FOR_MOUNT) {
+			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;
+		}
+
+		if (purpose == FS_CONTEXT_FOR_MOUNT) {
+			mp->m_flags |= XFS_MOUNT_DFLT_IOSIZE;
+			mp->m_readio_log = ctx->iosizelog;
+			mp->m_writeio_log = ctx->iosizelog;
+		}
+	}
+
+	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
@@ -2035,6 +2127,52 @@ xfs_fs_fill_super(
 	return error;
 }
 
+STATIC int
+xfs_fill_super(
+	struct super_block	*sb,
+	struct fs_context	*fc)
+{
+	struct xfs_fs_context	*ctx = fc->fs_private;
+	struct xfs_mount	*mp = sb->s_fs_info;
+	int			silent = fc->sb_flags & SB_SILENT;
+	int			error = -ENOMEM;
+
+	mp->m_super = sb;
+
+	/*
+	 * set up the mount name first so all the errors will refer to the
+	 * correct device.
+	 */
+	mp->m_fsname = kstrndup(sb->s_id, MAXNAMELEN, GFP_KERNEL);
+	if (!mp->m_fsname)
+		return -ENOMEM;
+	mp->m_fsname_len = strlen(mp->m_fsname) + 1;
+
+	error = xfs_validate_params(mp, ctx, fc->purpose);
+	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 vfs_get_block_super(fc, xfs_fill_super);
+}
+
 STATIC void
 xfs_fs_put_super(
 	struct super_block	*sb)
@@ -2107,6 +2245,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,
 };
 
 static struct file_system_type xfs_fs_type = {

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

[Darrick J. Wong]

On Mon, Jun 24, 2019 at 10:58:50AM +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 |  139 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 139 insertions(+)
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index cf8efb465969..0ec0142b94e1 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1629,6 +1629,98 @@ xfs_fs_remount(
>  	return 0;
>  }
>  
> +STATIC int
> +xfs_validate_params(
> +	struct xfs_mount *mp,
> +	struct xfs_fs_context *ctx,
> +	enum fs_context_purpose purpose)

Tabs here, please:

	struct xfs_mount	*mp,
	struct xfs_fs_context	*ctx,
	enum fs_context_purpose	purpose)

> +{
> +	/*
> +	 * 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;
> +	}
> +
> +	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.
> +		 */
> +		if (purpose == FS_CONTEXT_FOR_MOUNT) {
> +			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;
> +		}
> +
> +		if (purpose == FS_CONTEXT_FOR_MOUNT) {
> +			mp->m_flags |= XFS_MOUNT_DFLT_IOSIZE;
> +			mp->m_readio_log = ctx->iosizelog;
> +			mp->m_writeio_log = ctx->iosizelog;
> +		}
> +	}

Ugggh, I don't wanna have to compare the old xfs_parseargs code with
this new xfs_validate_params code to make sure nothing got screwed up.
:)

Can this code be broken out of the existing parseargs (instead of added
further down in the file) to minimize the diff?  You're getting rid of
the old option processing code at the end of the series anyway so I
don't mind creating temporary struct xfs_fs_context structures in
xfs_parseargs if that makes it much more obvious that the validation
code itself isn't changing.

--D

> +
> +	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
> @@ -2035,6 +2127,52 @@ xfs_fs_fill_super(
>  	return error;
>  }
>  
> +STATIC int
> +xfs_fill_super(
> +	struct super_block	*sb,
> +	struct fs_context	*fc)
> +{
> +	struct xfs_fs_context	*ctx = fc->fs_private;
> +	struct xfs_mount	*mp = sb->s_fs_info;
> +	int			silent = fc->sb_flags & SB_SILENT;
> +	int			error = -ENOMEM;
> +
> +	mp->m_super = sb;
> +
> +	/*
> +	 * set up the mount name first so all the errors will refer to the
> +	 * correct device.
> +	 */
> +	mp->m_fsname = kstrndup(sb->s_id, MAXNAMELEN, GFP_KERNEL);
> +	if (!mp->m_fsname)
> +		return -ENOMEM;
> +	mp->m_fsname_len = strlen(mp->m_fsname) + 1;
> +
> +	error = xfs_validate_params(mp, ctx, fc->purpose);
> +	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 vfs_get_block_super(fc, xfs_fill_super);
> +}
> +
>  STATIC void
>  xfs_fs_put_super(
>  	struct super_block	*sb)
> @@ -2107,6 +2245,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,
>  };
>  
>  static struct file_system_type xfs_fs_type = {
> 

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

[Ian Kent]

On Mon, 2019-06-24 at 10:44 -0700, Darrick J. Wong wrote:
> On Mon, Jun 24, 2019 at 10:58:50AM +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 |  139
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 139 insertions(+)
> > 
> > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > index cf8efb465969..0ec0142b94e1 100644
> > --- a/fs/xfs/xfs_super.c
> > +++ b/fs/xfs/xfs_super.c
> > @@ -1629,6 +1629,98 @@ xfs_fs_remount(
> >  	return 0;
> >  }
> >  
> > +STATIC int
> > +xfs_validate_params(
> > +	struct xfs_mount *mp,
> > +	struct xfs_fs_context *ctx,
> > +	enum fs_context_purpose purpose)
> 
> Tabs here, please:
> 
> 	struct xfs_mount	*mp,
> 	struct xfs_fs_context	*ctx,
> 	enum fs_context_purpose	purpose)

Oops, my bad, will fix.

> 
> > +{
> > +	/*
> > +	 * 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;
> > +	}
> > +
> > +	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.
> > +		 */
> > +		if (purpose == FS_CONTEXT_FOR_MOUNT) {
> > +			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;
> > +		}
> > +
> > +		if (purpose == FS_CONTEXT_FOR_MOUNT) {
> > +			mp->m_flags |= XFS_MOUNT_DFLT_IOSIZE;
> > +			mp->m_readio_log = ctx->iosizelog;
> > +			mp->m_writeio_log = ctx->iosizelog;
> > +		}
> > +	}
> 
> Ugggh, I don't wanna have to compare the old xfs_parseargs code with
> this new xfs_validate_params code to make sure nothing got screwed up.
> :)
> 
> Can this code be broken out of the existing parseargs (instead of added
> further down in the file) to minimize the diff?  You're getting rid of
> the old option processing code at the end of the series anyway so I
> don't mind creating temporary struct xfs_fs_context structures in
> xfs_parseargs if that makes it much more obvious that the validation
> code itself isn't changing.

Both you, Dave, and myself agree but ...

The indentation is different between the two functions resulting
in an even harder to understand patch.

I will have another go at it and see if I can come up with a
re-factoring that helps.

> 
> --D
> 
> > +
> > +	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
> > @@ -2035,6 +2127,52 @@ xfs_fs_fill_super(
> >  	return error;
> >  }
> >  
> > +STATIC int
> > +xfs_fill_super(
> > +	struct super_block	*sb,
> > +	struct fs_context	*fc)
> > +{
> > +	struct xfs_fs_context	*ctx = fc->fs_private;
> > +	struct xfs_mount	*mp = sb->s_fs_info;
> > +	int			silent = fc->sb_flags & SB_SILENT;
> > +	int			error = -ENOMEM;
> > +
> > +	mp->m_super = sb;
> > +
> > +	/*
> > +	 * set up the mount name first so all the errors will refer to the
> > +	 * correct device.
> > +	 */
> > +	mp->m_fsname = kstrndup(sb->s_id, MAXNAMELEN, GFP_KERNEL);
> > +	if (!mp->m_fsname)
> > +		return -ENOMEM;
> > +	mp->m_fsname_len = strlen(mp->m_fsname) + 1;
> > +
> > +	error = xfs_validate_params(mp, ctx, fc->purpose);
> > +	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 vfs_get_block_super(fc, xfs_fill_super);
> > +}
> > +
> >  STATIC void
> >  xfs_fs_put_super(
> >  	struct super_block	*sb)
> > @@ -2107,6 +2245,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,
> >  };
> >  
> >  static struct file_system_type xfs_fs_type = {
> > 

[PATCH 06/10] 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 |  171 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 171 insertions(+)

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 0ec0142b94e1..7326b21b32d1 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1721,6 +1721,176 @@ xfs_validate_params(
 	return 0;
 }
 
+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, fc->purpose);
+	if (error)
+		return error;
+
+	/*
+	 * There have been problems in the past with options
+	 * passed from mount(8).
+	 *
+	 * The problem being that options passed by mount(8) in
+	 * the case where only the the mount point path is given
+	 * would consist of the existing fstab options with the
+	 * options from mtab for the current mount merged in and
+	 * the options given on the command line last. But the
+	 * result couldn't be relied upon to accurately reflect the
+	 * current mount options so that rejecting options that
+	 * can't be changed on reconfigure could erronously cause
+	 * mount failure.
+	 *
+	 * The mount-api uses a legacy mount options handler
+	 * in the VFS to accomodate mount(8) so these options
+	 * will continue to be passed. Even if mount(8) is
+	 * updated to use fsopen()/fsconfig()/fsmount() it's
+	 * likely to continue to set the existing options so
+	 * options problems with reconfigure could continue.
+	 *
+	 * For the longest time mtab locking was a problem and
+	 * this could have been one possible cause. It's also
+	 * possible there could have been options order problems.
+	 *
+	 * That has changed now as mtab is a link to the proc
+	 * file system mount table so mtab options should be
+	 * always accurate.
+	 *
+	 * Consulting the util-linux maintainer (Karel Zak) he
+	 * is confident that, in this case, the options passed
+	 * by mount(8) will be those of the current mount and
+	 * the options order should be a correct merge of fstab
+	 * and mtab options, and new options given on the command
+	 * line.
+	 *
+	 * So, in theory, it should be possible to compare incoming
+	 * options and return an error for options that differ from
+	 * the current mount and can't be changed on reconfigure to
+	 * prevent users from believing they might have changed mount
+	 * options using remount which can't be changed.
+	 *
+	 * But for now continue to return success for every reconfigure
+	 * request, and silently ignore all options that can't actually
+	 * be changed.
+	 */
+
+	/* 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)) {
+		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;
+	}
+
+	/* 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);
+			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;
+}
+
 /*
  * 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
@@ -2246,6 +2416,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 06/10] xfs: mount api - add xfs_reconfigure()

[Darrick J. Wong]

On Mon, Jun 24, 2019 at 10:58:56AM +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 |  171 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 171 insertions(+)
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 0ec0142b94e1..7326b21b32d1 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1721,6 +1721,176 @@ xfs_validate_params(
>  	return 0;
>  }
>  
> +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, fc->purpose);
> +	if (error)
> +		return error;
> +
> +	/*
> +	 * There have been problems in the past with options
> +	 * passed from mount(8).
> +	 *
> +	 * The problem being that options passed by mount(8) in
> +	 * the case where only the the mount point path is given
> +	 * would consist of the existing fstab options with the
> +	 * options from mtab for the current mount merged in and
> +	 * the options given on the command line last. But the
> +	 * result couldn't be relied upon to accurately reflect the
> +	 * current mount options so that rejecting options that
> +	 * can't be changed on reconfigure could erronously cause
> +	 * mount failure.
> +	 *
> +	 * The mount-api uses a legacy mount options handler
> +	 * in the VFS to accomodate mount(8) so these options
> +	 * will continue to be passed. Even if mount(8) is
> +	 * updated to use fsopen()/fsconfig()/fsmount() it's
> +	 * likely to continue to set the existing options so
> +	 * options problems with reconfigure could continue.
> +	 *
> +	 * For the longest time mtab locking was a problem and
> +	 * this could have been one possible cause. It's also
> +	 * possible there could have been options order problems.
> +	 *
> +	 * That has changed now as mtab is a link to the proc
> +	 * file system mount table so mtab options should be
> +	 * always accurate.
> +	 *
> +	 * Consulting the util-linux maintainer (Karel Zak) he
> +	 * is confident that, in this case, the options passed
> +	 * by mount(8) will be those of the current mount and
> +	 * the options order should be a correct merge of fstab
> +	 * and mtab options, and new options given on the command
> +	 * line.

I don't know if it's too late to do this, but could we mandate this
behavior for the new mount api that dhowells has been working on, and
then pass a flag to all the fs parsers so that they know when it's safe
to complain about attempts to remount with changes to options that can't
be changed?

> +	 *
> +	 * So, in theory, it should be possible to compare incoming
> +	 * options and return an error for options that differ from
> +	 * the current mount and can't be changed on reconfigure to
> +	 * prevent users from believing they might have changed mount
> +	 * options using remount which can't be changed.
> +	 *
> +	 * But for now continue to return success for every reconfigure
> +	 * request, and silently ignore all options that can't actually
> +	 * be changed.

The comment lines could be longer (i.e. wrapped at column 80 instead of
72 or wherever they are now) and moved to be part of the comment for the
function instead of inside the body.

> +	 */
> +
> +	/* 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)) {
> +		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;

Ugh, could you please refactor everything from the "ro -> rw" case in
xfs_fs_remount into a separate function and then call it from here?
Then both functions can shrink to:

	if ((mp->m_flags & XFS_MOUNT_RDONLY) && !(flags & SB_RDONLY)) {
		error = xfs_remount_rw(mp);
		if (error)
			return error;
	} else if (!(mp->m_flags & XFS_MOUNT_RDONLY) && (flags & SB_RDONLY)) {
		error = xfs_remount_ro(mp);
		if (error)
			return error;
	}

> +	}
> +
> +	/* 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);
> +			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;

...and here?

> +	}
> +
> +	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
> @@ -2246,6 +2416,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 06/10] xfs: mount api - add xfs_reconfigure()

[Ian Kent]

On Mon, 2019-06-24 at 10:55 -0700, Darrick J. Wong wrote:
> On Mon, Jun 24, 2019 at 10:58:56AM +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 |  171
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 171 insertions(+)
> > 
> > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > index 0ec0142b94e1..7326b21b32d1 100644
> > --- a/fs/xfs/xfs_super.c
> > +++ b/fs/xfs/xfs_super.c
> > @@ -1721,6 +1721,176 @@ xfs_validate_params(
> >  	return 0;
> >  }
> >  
> > +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, fc->purpose);
> > +	if (error)
> > +		return error;
> > +
> > +	/*
> > +	 * There have been problems in the past with options
> > +	 * passed from mount(8).
> > +	 *
> > +	 * The problem being that options passed by mount(8) in
> > +	 * the case where only the the mount point path is given
> > +	 * would consist of the existing fstab options with the
> > +	 * options from mtab for the current mount merged in and
> > +	 * the options given on the command line last. But the
> > +	 * result couldn't be relied upon to accurately reflect the
> > +	 * current mount options so that rejecting options that
> > +	 * can't be changed on reconfigure could erronously cause
> > +	 * mount failure.
> > +	 *
> > +	 * The mount-api uses a legacy mount options handler
> > +	 * in the VFS to accomodate mount(8) so these options
> > +	 * will continue to be passed. Even if mount(8) is
> > +	 * updated to use fsopen()/fsconfig()/fsmount() it's
> > +	 * likely to continue to set the existing options so
> > +	 * options problems with reconfigure could continue.
> > +	 *
> > +	 * For the longest time mtab locking was a problem and
> > +	 * this could have been one possible cause. It's also
> > +	 * possible there could have been options order problems.
> > +	 *
> > +	 * That has changed now as mtab is a link to the proc
> > +	 * file system mount table so mtab options should be
> > +	 * always accurate.
> > +	 *
> > +	 * Consulting the util-linux maintainer (Karel Zak) he
> > +	 * is confident that, in this case, the options passed
> > +	 * by mount(8) will be those of the current mount and
> > +	 * the options order should be a correct merge of fstab
> > +	 * and mtab options, and new options given on the command
> > +	 * line.
> 
> I don't know if it's too late to do this, but could we mandate this
> behavior for the new mount api that dhowells has been working on, and
> then pass a flag to all the fs parsers so that they know when it's safe
> to complain about attempts to remount with changes to options that can't
> be changed?

Sounds like a good idea but I'm not sure if it can be done.
David?

> 
> > +	 *
> > +	 * So, in theory, it should be possible to compare incoming
> > +	 * options and return an error for options that differ from
> > +	 * the current mount and can't be changed on reconfigure to
> > +	 * prevent users from believing they might have changed mount
> > +	 * options using remount which can't be changed.
> > +	 *
> > +	 * But for now continue to return success for every reconfigure
> > +	 * request, and silently ignore all options that can't actually
> > +	 * be changed.
> 
> The comment lines could be longer (i.e. wrapped at column 80 instead of
> 72 or wherever they are now) and moved to be part of the comment for the
> function instead of inside the body.

Ok, will do.

TBH I wish it was less verbose but this is what I ended up with
after trying to understand what the original comment meant.

> 
> > +	 */
> > +
> > +	/* 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)) {
> > +		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;
> 
> Ugh, could you please refactor everything from the "ro -> rw" case in
> xfs_fs_remount into a separate function and then call it from here?

Yes, I admit I thought about doing that myself pretty much immediately
after posting the series, ;)

> Then both functions can shrink to:
> 
> 	if ((mp->m_flags & XFS_MOUNT_RDONLY) && !(flags & SB_RDONLY)) {
> 		error = xfs_remount_rw(mp);
> 		if (error)
> 			return error;
> 	} else if (!(mp->m_flags & XFS_MOUNT_RDONLY) && (flags & SB_RDONLY)) {
> 		error = xfs_remount_ro(mp);
> 		if (error)
> 			return error;
> 	}
> 
> > +	}
> > +
> > +	/* 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);
> > +			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;
> 
> ...and here?
> 
> > +	}
> > +
> > +	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
> > @@ -2246,6 +2416,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 07/10] 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 |   23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 7326b21b32d1..0a771e1390e7 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -2413,10 +2413,33 @@ 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;
+
+	if (mp) {
+		/*
+		 * If an error occurs before ownership the xfs_mount
+		 * info struct is passed to xfs by the VFS (by assigning
+		 * it to sb->s_fs_info and clearing the corresponding
+		 * fs_context field, done before calling fill super via
+		 * .get_tree()) there may be some strings to cleanup.
+		 */
+		if (mp->m_logname)
+			kfree(mp->m_logname);
+		if (mp->m_rtname)
+			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 = {

Re: [PATCH 07/10] xfs: mount-api - add xfs_fc_free()

[Darrick J. Wong]

On Mon, Jun 24, 2019 at 10:59:02AM +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 |   23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 7326b21b32d1..0a771e1390e7 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -2413,10 +2413,33 @@ 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;
> +
> +	if (mp) {
> +		/*
> +		 * If an error occurs before ownership the xfs_mount
> +		 * info struct is passed to xfs by the VFS (by assigning
> +		 * it to sb->s_fs_info and clearing the corresponding
> +		 * fs_context field, done before calling fill super via
> +		 * .get_tree()) there may be some strings to cleanup.
> +		 */
> +		if (mp->m_logname)
> +			kfree(mp->m_logname);

Doesn't kfree handle null parameters gracefully?

--D

> +		if (mp->m_rtname)
> +			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 = {
> 

Re: [PATCH 07/10] xfs: mount-api - add xfs_fc_free()

[Ian Kent]

On Mon, 2019-06-24 at 10:56 -0700, Darrick J. Wong wrote:
> On Mon, Jun 24, 2019 at 10:59:02AM +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 |   23 +++++++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> > 
> > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > index 7326b21b32d1..0a771e1390e7 100644
> > --- a/fs/xfs/xfs_super.c
> > +++ b/fs/xfs/xfs_super.c
> > @@ -2413,10 +2413,33 @@ 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;
> > +
> > +	if (mp) {
> > +		/*
> > +		 * If an error occurs before ownership the xfs_mount
> > +		 * info struct is passed to xfs by the VFS (by assigning
> > +		 * it to sb->s_fs_info and clearing the corresponding
> > +		 * fs_context field, done before calling fill super via
> > +		 * .get_tree()) there may be some strings to cleanup.
> > +		 */
> > +		if (mp->m_logname)
> > +			kfree(mp->m_logname);
> 
> Doesn't kfree handle null parameters gracefully?

It does, old habits die hard, will fix, ;)

> 
> --D
> 
> > +		if (mp->m_rtname)
> > +			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 08/10] 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.

Also remove Opt_err enum as it's no longer used.

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

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 0a771e1390e7..6e6eace31e07 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -71,7 +71,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,
+	Opt_discard, Opt_nodiscard, Opt_dax,
 };
 
 static const match_table_t tokens = {
@@ -118,7 +118,6 @@ static const match_table_t tokens = {
 	{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},
 };
 
 static const struct fs_parameter_spec xfs_param_specs[] = {
@@ -2407,7 +2406,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,
@@ -2442,10 +2440,76 @@ 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 = kzalloc(sizeof(struct xfs_mount), GFP_KERNEL);
+	if (!mp) {
+		kfree(ctx);
+		return -ENOMEM;
+	}
+
+	spin_lock_init(&mp->m_sb_lock);
+	spin_lock_init(&mp->m_agirotor_lock);
+	INIT_RADIX_TREE(&mp->m_perag_tree, GFP_ATOMIC);
+	spin_lock_init(&mp->m_perag_lock);
+	mutex_init(&mp->m_growlock);
+	atomic_set(&mp->m_active_trans, 0);
+	INIT_DELAYED_WORK(&mp->m_reclaim_work, xfs_reclaim_worker);
+	INIT_DELAYED_WORK(&mp->m_eofblocks_work, xfs_eofblocks_worker);
+	INIT_DELAYED_WORK(&mp->m_cowblocks_work, xfs_cowblocks_worker);
+	mp->m_kobj.kobject.kset = xfs_kset;
+	/*
+	 * We don't create the finobt per-ag space reservation until after log
+	 * recovery, so we must set this to true so that an ifree transaction
+	 * started during log recovery will not depend on space reservations
+	 * for finobt expansion.
+	 */
+	mp->m_finobt_nores = true;
+
+	/*
+	 * 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 09/10] xfs: mount-api - remove legacy mount functions

[Ian Kent]

Now that the new mount-api is being used the old mount functions
can be removed.

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

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 6e6eace31e07..643b40e8a328 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -196,304 +196,6 @@ 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;
-}
-
-/*
- * 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;
-	substring_t		args[MAX_OPT_ARGS];
-	int			dsunit = 0;
-	int			dswidth = 0;
-	int			iosize = 0;
-	uint8_t			iosizelog = 0;
-
-	/*
-	 * 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) {
-		int		token;
-
-		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:
-		case Opt_biosize:
-			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;
-		}
-	}
-
-	/*
-	 * 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) && (dsunit || 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 ((dsunit && !dswidth) || (!dsunit && dswidth)) {
-		xfs_warn(mp, "sunit and swidth must be specified together");
-		return -EINVAL;
-	}
-
-	if (dsunit && (dswidth % dsunit != 0)) {
-		xfs_warn(mp,
-	"stripe width (%d) must be a multiple of the stripe unit (%d)",
-			dswidth, dsunit);
-		return -EINVAL;
-	}
-
-done:
-	if (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;
-	}
-
-	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 (iosizelog) {
-		if (iosizelog > XFS_MAX_IO_LOG ||
-		    iosizelog < XFS_MIN_IO_LOG) {
-			xfs_warn(mp, "invalid log iosize: %d [not %d-%d]",
-				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;
-	}
-
-	return 0;
-}
-
 struct xfs_fs_context {
 	int	dsunit;
 	int	dswidth;
@@ -1453,181 +1155,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_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)) {
-		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;
-	}
-
-	/* 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);
-			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_validate_params(
 	struct xfs_mount *mp,
@@ -2051,38 +1578,6 @@ xfs_destroy_percpu_counters(
 	percpu_counter_destroy(&mp->m_delalloc_blks);
 }
 
-static struct xfs_mount *
-xfs_mount_alloc(
-	struct super_block	*sb)
-{
-	struct xfs_mount	*mp;
-
-	mp = kzalloc(sizeof(struct xfs_mount), GFP_KERNEL);
-	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);
-	spin_lock_init(&mp->m_perag_lock);
-	mutex_init(&mp->m_growlock);
-	atomic_set(&mp->m_active_trans, 0);
-	INIT_DELAYED_WORK(&mp->m_reclaim_work, xfs_reclaim_worker);
-	INIT_DELAYED_WORK(&mp->m_eofblocks_work, xfs_eofblocks_worker);
-	INIT_DELAYED_WORK(&mp->m_cowblocks_work, xfs_cowblocks_worker);
-	mp->m_kobj.kobject.kset = xfs_kset;
-	/*
-	 * We don't create the finobt per-ag space reservation until after log
-	 * recovery, so we must set this to true so that an ifree transaction
-	 * started during log recovery will not depend on space reservations
-	 * for finobt expansion.
-	 */
-	mp->m_finobt_nores = true;
-	return mp;
-}
-
-
 STATIC int
 __xfs_fs_fill_super(
 	struct xfs_mount	*mp,
@@ -2260,42 +1755,6 @@ __xfs_fs_fill_super(
 	goto out_free_sb;
 }
 
-STATIC int
-xfs_fs_fill_super(
-	struct super_block	*sb,
-	void			*data,
-	int			silent)
-{
-	struct xfs_mount	*mp = NULL;
-	int			error = -ENOMEM;
-
-	/*
-	 * allocate mp and do all low-level struct initializations before we
-	 * attach it to the super
-	 */
-	mp = xfs_mount_alloc(sb);
-	if (!mp)
-		goto out;
-	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);
-out:
-	return error;
-}
-
 STATIC int
 xfs_fill_super(
 	struct super_block	*sb,
@@ -2367,16 +1826,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,

[PATCH 10/10] xfs: mount-api - rename xfs_fill_super()

[Ian Kent]

Now the legacy mount functions have been removed xfs_fill_super()
can be renamed to xfs_fs_fill_super() in keeping with the previous
xfs naming convention.

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

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 643b40e8a328..38f3af44fbbf 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1756,7 +1756,7 @@ __xfs_fs_fill_super(
 }
 
 STATIC int
-xfs_fill_super(
+xfs_fs_fill_super(
 	struct super_block	*sb,
 	struct fs_context	*fc)
 {
@@ -1798,7 +1798,7 @@ STATIC int
 xfs_get_tree(
 	struct fs_context	*fc)
 {
-	return vfs_get_block_super(fc, xfs_fill_super);
+	return vfs_get_block_super(fc, xfs_fs_fill_super);
 }
 
 STATIC void

Re: [PATCH 10/10] xfs: mount-api - rename xfs_fill_super()

[Darrick J. Wong]

On Mon, Jun 24, 2019 at 10:59:21AM +0800, Ian Kent wrote:
> Now the legacy mount functions have been removed xfs_fill_super()
> can be renamed to xfs_fs_fill_super() in keeping with the previous
> xfs naming convention.
> 
> Signed-off-by: Ian Kent <raven@themaw.net>

Looks ok I guess.
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/xfs_super.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 643b40e8a328..38f3af44fbbf 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1756,7 +1756,7 @@ __xfs_fs_fill_super(
>  }
>  
>  STATIC int
> -xfs_fill_super(
> +xfs_fs_fill_super(
>  	struct super_block	*sb,
>  	struct fs_context	*fc)
>  {
> @@ -1798,7 +1798,7 @@ STATIC int
>  xfs_get_tree(
>  	struct fs_context	*fc)
>  {
> -	return vfs_get_block_super(fc, xfs_fill_super);
> +	return vfs_get_block_super(fc, xfs_fs_fill_super);
>  }
>  
>  STATIC void
> 

Re: [PATCH 01/10] xfs: mount-api - add fs parameter description

[Darrick J. Wong]

On Mon, Jun 24, 2019 at 10:58:22AM +0800, Ian Kent wrote:
> 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>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/xfs_super.c |   48 +++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 45 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index a14d11d78bd8..ab8145bf6fff 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -51,6 +51,8 @@
>  #include <linux/kthread.h>
>  #include <linux/freezer.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;
> @@ -60,9 +62,6 @@ static struct kset *xfs_kset;		/* top-level xfs sysfs dir */
>  static struct xfs_kobj xfs_dbg_kobj;	/* global debug sysfs attrs */
>  #endif
>  
> -/*
> - * Table driven mount option parser.
> - */
>  enum {
>  	Opt_logbufs, Opt_logbsize, Opt_logdev, Opt_rtdev, Opt_biosize,
>  	Opt_wsync, Opt_noalign, Opt_swalloc, Opt_sunit, Opt_swidth, Opt_nouuid,
> @@ -122,6 +121,49 @@ 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_u32	("biosize",    Opt_biosize),   /* log2 of preferred buffered io size */
> + 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_no("grpid",      Opt_grpid),     /* group-ID from parent directory (or not) */
> + 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_no("ikeep",      Opt_ikeep),     /* do not free (or keep) empty inode clusters */
> + fsparam_flag_no("largeio",    Opt_largeio),   /* report (or do not report) large I/O sizes in stat() */
> + fsparam_flag_no("attr2",      Opt_attr2),     /* do (or do not) use attr2 attribute format */
> + fsparam_flag	("filestreams",Opt_filestreams), /* use filestreams allocator */
> + fsparam_flag_no("quota",      Opt_quota),     /* disk quotas (user) */
> + 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_no("discard",    Opt_discard),   /* Do (or do not) 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)
> 

Re: [PATCH 01/10] xfs: mount-api - add fs parameter description

[Christoph Hellwig]

This is missing a cover letter to explain the point of this
series.

It adds giant amount of code for not user visible benefit.  Why would
we want this series?

Re: [PATCH 01/10] xfs: mount-api - add fs parameter description

[Ian Kent]

On Tue, 2019-06-25 at 03:34 -0700, Christoph Hellwig wrote:
> This is missing a cover letter to explain the point of this
> series.
> 
> It adds giant amount of code for not user visible benefit.  Why would
> we want this series?

A fair comment.

I'll add a cover letter when I post a v2 of the series.

Ian

Re: [PATCH 01/10] xfs: mount-api - add fs parameter description

[Eric Sandeen]

On 6/23/19 9:58 PM, Ian Kent wrote:
> 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>

Ian, I saw hints about a V2 in replies, is that still in the works?

Thanks,
-Eric

> ---
>  fs/xfs/xfs_super.c |   48 +++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 45 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index a14d11d78bd8..ab8145bf6fff 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -51,6 +51,8 @@
>  #include <linux/kthread.h>
>  #include <linux/freezer.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;
> @@ -60,9 +62,6 @@ static struct kset *xfs_kset;		/* top-level xfs sysfs dir */
>  static struct xfs_kobj xfs_dbg_kobj;	/* global debug sysfs attrs */
>  #endif
>  
> -/*
> - * Table driven mount option parser.
> - */
>  enum {
>  	Opt_logbufs, Opt_logbsize, Opt_logdev, Opt_rtdev, Opt_biosize,
>  	Opt_wsync, Opt_noalign, Opt_swalloc, Opt_sunit, Opt_swidth, Opt_nouuid,
> @@ -122,6 +121,49 @@ 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_u32	("biosize",    Opt_biosize),   /* log2 of preferred buffered io size */
> + 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_no("grpid",      Opt_grpid),     /* group-ID from parent directory (or not) */
> + 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_no("ikeep",      Opt_ikeep),     /* do not free (or keep) empty inode clusters */
> + fsparam_flag_no("largeio",    Opt_largeio),   /* report (or do not report) large I/O sizes in stat() */
> + fsparam_flag_no("attr2",      Opt_attr2),     /* do (or do not) use attr2 attribute format */
> + fsparam_flag	("filestreams",Opt_filestreams), /* use filestreams allocator */
> + fsparam_flag_no("quota",      Opt_quota),     /* disk quotas (user) */
> + 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_no("discard",    Opt_discard),   /* Do (or do not) 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)
> 

Re: [PATCH 01/10] xfs: mount-api - add fs parameter description

[Ian Kent]

On Wed, 2019-08-21 at 09:52 -0500, Eric Sandeen wrote:
> On 6/23/19 9:58 PM, Ian Kent wrote:
> > 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>
> 
> Ian, I saw hints about a V2 in replies, is that still in the works?

I have a v2 that should address the comments made on the initial
post.

My reluctance to post the v2 is because the series uses a patch
that's currently sitting in linux-next and that will cause a
conflict if the series is accepted.

I'm thinking about doing an RFC post with that patch included
and a caution about the possible conflict.

Not sure yet, I've only just yesterday been able to successfully
run xfstests on a kernel with the changes so deciding what to do
next is upon me now, ;)

> 
> Thanks,
> -Eric
> 
> > ---
> >  fs/xfs/xfs_super.c |   48
> > +++++++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 45 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > index a14d11d78bd8..ab8145bf6fff 100644
> > --- a/fs/xfs/xfs_super.c
> > +++ b/fs/xfs/xfs_super.c
> > @@ -51,6 +51,8 @@
> >  #include <linux/kthread.h>
> >  #include <linux/freezer.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;
> > @@ -60,9 +62,6 @@ static struct kset *xfs_kset;		/* top-
> > level xfs sysfs dir */
> >  static struct xfs_kobj xfs_dbg_kobj;	/* global debug sysfs
> > attrs */
> >  #endif
> >  
> > -/*
> > - * Table driven mount option parser.
> > - */
> >  enum {
> >  	Opt_logbufs, Opt_logbsize, Opt_logdev, Opt_rtdev, Opt_biosize,
> >  	Opt_wsync, Opt_noalign, Opt_swalloc, Opt_sunit, Opt_swidth,
> > Opt_nouuid,
> > @@ -122,6 +121,49 @@ 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_u32	("biosize",    Opt_biosize),   /* log2 of
> > preferred buffered io size */
> > + 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_no("grpid",      Opt_grpid),     /* group-ID from
> > parent directory (or not) */
> > + 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_no("ikeep",      Opt_ikeep),     /* do not free (or
> > keep) empty inode clusters */
> > + fsparam_flag_no("largeio",    Opt_largeio),   /* report (or do
> > not report) large I/O sizes in stat() */
> > + fsparam_flag_no("attr2",      Opt_attr2),     /* do (or do not)
> > use attr2 attribute format */
> > + fsparam_flag	("filestreams",Opt_filestreams), /* use
> > filestreams allocator */
> > + fsparam_flag_no("quota",      Opt_quota),     /* disk quotas
> > (user) */
> > + 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_no("discard",    Opt_discard),   /* Do (or do not)
> > 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)
> > 

Re: [PATCH 01/10] xfs: mount-api - add fs parameter description

[Eric Sandeen]

On 8/22/19 4:05 AM, Ian Kent wrote:
> On Wed, 2019-08-21 at 09:52 -0500, Eric Sandeen wrote:
>> On 6/23/19 9:58 PM, Ian Kent wrote:
>>> 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>
>>
>> Ian, I saw hints about a V2 in replies, is that still in the works?
> 
> I have a v2 that should address the comments made on the initial
> post.
> 
> My reluctance to post the v2 is because the series uses a patch
> that's currently sitting in linux-next and that will cause a
> conflict if the series is accepted.
> 
> I'm thinking about doing an RFC post with that patch included
> and a caution about the possible conflict.

That should be fine, just let everyone know what the patchset is based
on and we can do another round of review & sort out the dependencies
as needed, I think.

Thanks
-Eric
 
> Not sure yet, I've only just yesterday been able to successfully
> run xfstests on a kernel with the changes so deciding what to do
> next is upon me now, ;)