All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v6 00/12] xfs: mount API patch series
@ 2019-10-16  0:40 Ian Kent
  2019-10-16  0:40 ` [PATCH v6 01/12] vfs: add missing blkdev_put() in get_tree_bdev() Ian Kent
                   ` (11 more replies)
  0 siblings, 12 replies; 39+ messages in thread
From: Ian Kent @ 2019-10-16  0:40 UTC (permalink / raw)
  To: linux-xfs
  Cc: Brian Foster, Eric Sandeen, David Howells, Dave Chinner, Al Viro

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

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

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

Other things that continue to cause me concern:

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

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

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

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

The first patch in the series is needed only for testing. It is now
present in the current upstream rc kernel so it will need to be dropped
before merging.

Changes for v6:
- drop get_tree_bdev() patch since it's now present in the xfs-linux repo.
- eliminate unused mount info. field m_fsname_len.
- eliminate mount info field m_fsname, use super block field s_id instead.
- dont use XFS_IS_QUOTA_RUNNING() when checking if quota options have been
  specified.
- combine mount-api specific patches into a single patch to help with
  bi-sectability.

hanges for v5:
- give error exit label in xfs_fill_super() a sensible name.
- use original comment about options handling in xfs_fs_remount() for
  comment above xfs_reconfigure().
- use a much simpler comment in xfs_fc_free(), thanks to Brian Foster.
- move cover letter comment about the first two patches above the
  revision comentary so it isn't missed.

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

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

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

Ian Kent (12):
      vfs: add missing blkdev_put() in get_tree_bdev()
      xfs: remove very old mount option
      xfs: remove unused mount info field m_fsname_len
      xfs: use super s_id instead of mount info m_fsname
      xfs: dont use XFS_IS_QUOTA_RUNNING() for option check
      xfs: add xfs_remount_rw() helper
      xfs: add xfs_remount_ro() helper
      xfs: refactor suffix_kstrtoint()
      xfs: refactor xfs_parseags()
      xfs: move xfs_parseargs() validation to a helper
      xfs: dont set sb in xfs_mount_alloc()
      xfs: switch to use the new mount-api


 fs/super.c             |    5 
 fs/xfs/xfs_error.c     |    2 
 fs/xfs/xfs_log.c       |    2 
 fs/xfs/xfs_message.c   |    4 
 fs/xfs/xfs_mount.c     |    5 
 fs/xfs/xfs_mount.h     |    2 
 fs/xfs/xfs_pnfs.c      |    2 
 fs/xfs/xfs_super.c     |  903 ++++++++++++++++++++++++------------------------
 fs/xfs/xfs_trans_ail.c |    2 
 9 files changed, 469 insertions(+), 458 deletions(-)

--
Ian

^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH v6 01/12] vfs: add missing blkdev_put() in get_tree_bdev()
  2019-10-16  0:40 [PATCH v6 00/12] xfs: mount API patch series Ian Kent
@ 2019-10-16  0:40 ` Ian Kent
  2019-10-16  0:40 ` [PATCH v6 02/12] xfs: remove very old mount option Ian Kent
                   ` (10 subsequent siblings)
  11 siblings, 0 replies; 39+ messages in thread
From: Ian Kent @ 2019-10-16  0:40 UTC (permalink / raw)
  To: linux-xfs
  Cc: Brian Foster, Eric Sandeen, David Howells, Dave Chinner, Al Viro

There appear to be a couple of missing blkdev_put() in get_tree_bdev().
This change has been sent to Linus and has been included in the current
rc kernel but is not present in the xfs-linux tree.
---
 fs/super.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/super.c b/fs/super.c
index f627b7c53d2b..cfadab2cbf35 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -1300,6 +1300,7 @@ int get_tree_bdev(struct fs_context *fc,
 	mutex_lock(&bdev->bd_fsfreeze_mutex);
 	if (bdev->bd_fsfreeze_count > 0) {
 		mutex_unlock(&bdev->bd_fsfreeze_mutex);
+		blkdev_put(bdev, mode);
 		warnf(fc, "%pg: Can't mount, blockdev is frozen", bdev);
 		return -EBUSY;
 	}
@@ -1308,8 +1309,10 @@ int get_tree_bdev(struct fs_context *fc,
 	fc->sget_key = bdev;
 	s = sget_fc(fc, test_bdev_super_fc, set_bdev_super_fc);
 	mutex_unlock(&bdev->bd_fsfreeze_mutex);
-	if (IS_ERR(s))
+	if (IS_ERR(s)) {
+		blkdev_put(bdev, mode);
 		return PTR_ERR(s);
+	}
 
 	if (s->s_root) {
 		/* Don't summarily change the RO/RW state. */


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH v6 02/12] xfs: remove very old mount option
  2019-10-16  0:40 [PATCH v6 00/12] xfs: mount API patch series Ian Kent
  2019-10-16  0:40 ` [PATCH v6 01/12] vfs: add missing blkdev_put() in get_tree_bdev() Ian Kent
@ 2019-10-16  0:40 ` Ian Kent
  2019-10-16  8:26   ` Christoph Hellwig
  2019-10-16 18:25   ` Darrick J. Wong
  2019-10-16  0:40 ` [PATCH v6 03/12] xfs: remove unused mount info field m_fsname_len Ian Kent
                   ` (9 subsequent siblings)
  11 siblings, 2 replies; 39+ messages in thread
From: Ian Kent @ 2019-10-16  0:40 UTC (permalink / raw)
  To: linux-xfs
  Cc: Brian Foster, Eric Sandeen, David Howells, Dave Chinner, Al Viro

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

So remove it.

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

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


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH v6 03/12] xfs: remove unused mount info field m_fsname_len
  2019-10-16  0:40 [PATCH v6 00/12] xfs: mount API patch series Ian Kent
  2019-10-16  0:40 ` [PATCH v6 01/12] vfs: add missing blkdev_put() in get_tree_bdev() Ian Kent
  2019-10-16  0:40 ` [PATCH v6 02/12] xfs: remove very old mount option Ian Kent
@ 2019-10-16  0:40 ` Ian Kent
  2019-10-16  8:26   ` Christoph Hellwig
  2019-10-16 18:26   ` Darrick J. Wong
  2019-10-16  0:41 ` [PATCH v6 04/12] xfs: use super s_id instead of mount info m_fsname Ian Kent
                   ` (8 subsequent siblings)
  11 siblings, 2 replies; 39+ messages in thread
From: Ian Kent @ 2019-10-16  0:40 UTC (permalink / raw)
  To: linux-xfs
  Cc: Brian Foster, Eric Sandeen, David Howells, Dave Chinner, Al Viro

The mount info field m_fsname_len is not used anywhere, remove it.

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

diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
index fdb60e09a9c5..b3230f7ca2bf 100644
--- a/fs/xfs/xfs_mount.h
+++ b/fs/xfs/xfs_mount.h
@@ -90,7 +90,6 @@ typedef struct xfs_mount {
 
 	struct xfs_buf		*m_sb_bp;	/* buffer for superblock */
 	char			*m_fsname;	/* filesystem name */
-	int			m_fsname_len;	/* strlen of fs name */
 	char			*m_rtname;	/* realtime device name */
 	char			*m_logname;	/* external log device name */
 	int			m_bsize;	/* fs logical block size */
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 1bb7ede5d75b..cfa306f62bec 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -172,7 +172,6 @@ xfs_parseargs(
 	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.


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH v6 04/12] xfs: use super s_id instead of mount info m_fsname
  2019-10-16  0:40 [PATCH v6 00/12] xfs: mount API patch series Ian Kent
                   ` (2 preceding siblings ...)
  2019-10-16  0:40 ` [PATCH v6 03/12] xfs: remove unused mount info field m_fsname_len Ian Kent
@ 2019-10-16  0:41 ` Ian Kent
  2019-10-16  8:27   ` Christoph Hellwig
  2019-10-16 18:27   ` Darrick J. Wong
  2019-10-16  0:41 ` [PATCH v6 05/12] xfs: dont use XFS_IS_QUOTA_RUNNING() for option check Ian Kent
                   ` (7 subsequent siblings)
  11 siblings, 2 replies; 39+ messages in thread
From: Ian Kent @ 2019-10-16  0:41 UTC (permalink / raw)
  To: linux-xfs
  Cc: Brian Foster, Eric Sandeen, David Howells, Dave Chinner, Al Viro

Eliminate mount info field m_fsname by using the super block s_id
field directly.

Signed-off-by: Ian Kent <raven@themaw.net>
---
 fs/xfs/xfs_error.c     |    2 +-
 fs/xfs/xfs_log.c       |    2 +-
 fs/xfs/xfs_message.c   |    4 ++--
 fs/xfs/xfs_mount.c     |    5 +++--
 fs/xfs/xfs_mount.h     |    1 -
 fs/xfs/xfs_pnfs.c      |    2 +-
 fs/xfs/xfs_super.c     |   35 +++++++++++++----------------------
 fs/xfs/xfs_trans_ail.c |    2 +-
 8 files changed, 22 insertions(+), 31 deletions(-)

diff --git a/fs/xfs/xfs_error.c b/fs/xfs/xfs_error.c
index 849fd4476950..3ff3fc202522 100644
--- a/fs/xfs/xfs_error.c
+++ b/fs/xfs/xfs_error.c
@@ -257,7 +257,7 @@ xfs_errortag_test(
 
 	xfs_warn_ratelimited(mp,
 "Injecting error (%s) at file %s, line %d, on filesystem \"%s\"",
-			expression, file, line, mp->m_fsname);
+			expression, file, line, mp->m_super->s_id);
 	return true;
 }
 
diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index a2beee9f74da..ed99f4f063c3 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -1479,7 +1479,7 @@ xlog_alloc_log(
 
 	log->l_ioend_workqueue = alloc_workqueue("xfs-log/%s",
 			WQ_MEM_RECLAIM | WQ_FREEZABLE | WQ_HIGHPRI, 0,
-			mp->m_fsname);
+			mp->m_super->s_id);
 	if (!log->l_ioend_workqueue)
 		goto out_free_iclog;
 
diff --git a/fs/xfs/xfs_message.c b/fs/xfs/xfs_message.c
index 9804efe525a9..aaea6faf5acc 100644
--- a/fs/xfs/xfs_message.c
+++ b/fs/xfs/xfs_message.c
@@ -20,8 +20,8 @@ __xfs_printk(
 	const struct xfs_mount	*mp,
 	struct va_format	*vaf)
 {
-	if (mp && mp->m_fsname) {
-		printk("%sXFS (%s): %pV\n", level, mp->m_fsname, vaf);
+	if (mp && mp->m_super && mp->m_super->s_id) {
+		printk("%sXFS (%s): %pV\n", level, mp->m_super->s_id, vaf);
 		return;
 	}
 	printk("%sXFS: %pV\n", level, vaf);
diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index ba5b6f3b2b88..b6145a70a593 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -706,7 +706,8 @@ xfs_mountfs(
 	/* enable fail_at_unmount as default */
 	mp->m_fail_unmount = true;
 
-	error = xfs_sysfs_init(&mp->m_kobj, &xfs_mp_ktype, NULL, mp->m_fsname);
+	error = xfs_sysfs_init(&mp->m_kobj, &xfs_mp_ktype,
+			       NULL, mp->m_super->s_id);
 	if (error)
 		goto out;
 
@@ -1277,7 +1278,7 @@ xfs_mod_fdblocks(
 	printk_once(KERN_WARNING
 		"Filesystem \"%s\": reserve blocks depleted! "
 		"Consider increasing reserve pool size.",
-		mp->m_fsname);
+		mp->m_super->s_id);
 fdblocks_enospc:
 	spin_unlock(&mp->m_sb_lock);
 	return -ENOSPC;
diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
index b3230f7ca2bf..f2a9f316fa6c 100644
--- a/fs/xfs/xfs_mount.h
+++ b/fs/xfs/xfs_mount.h
@@ -89,7 +89,6 @@ typedef struct xfs_mount {
 	struct percpu_counter	m_delalloc_blks;
 
 	struct xfs_buf		*m_sb_bp;	/* buffer for superblock */
-	char			*m_fsname;	/* filesystem name */
 	char			*m_rtname;	/* realtime device name */
 	char			*m_logname;	/* external log device name */
 	int			m_bsize;	/* fs logical block size */
diff --git a/fs/xfs/xfs_pnfs.c b/fs/xfs/xfs_pnfs.c
index a339bd5fa260..81df10f69ade 100644
--- a/fs/xfs/xfs_pnfs.c
+++ b/fs/xfs/xfs_pnfs.c
@@ -59,7 +59,7 @@ xfs_fs_get_uuid(
 
 	printk_once(KERN_NOTICE
 "XFS (%s): using experimental pNFS feature, use at your own risk!\n",
-		mp->m_fsname);
+		mp->m_super->s_id);
 
 	if (*len < sizeof(uuid_t))
 		return -EINVAL;
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index cfa306f62bec..5876c2b551b5 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -165,14 +165,6 @@ xfs_parseargs(
 	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;
-
 	/*
 	 * Copy binary VFS mount flags we are interested in.
 	 */
@@ -805,33 +797,33 @@ xfs_init_mount_workqueues(
 	struct xfs_mount	*mp)
 {
 	mp->m_buf_workqueue = alloc_workqueue("xfs-buf/%s",
-			WQ_MEM_RECLAIM|WQ_FREEZABLE, 1, mp->m_fsname);
+			WQ_MEM_RECLAIM|WQ_FREEZABLE, 1, mp->m_super->s_id);
 	if (!mp->m_buf_workqueue)
 		goto out;
 
 	mp->m_unwritten_workqueue = alloc_workqueue("xfs-conv/%s",
-			WQ_MEM_RECLAIM|WQ_FREEZABLE, 0, mp->m_fsname);
+			WQ_MEM_RECLAIM|WQ_FREEZABLE, 0, mp->m_super->s_id);
 	if (!mp->m_unwritten_workqueue)
 		goto out_destroy_buf;
 
 	mp->m_cil_workqueue = alloc_workqueue("xfs-cil/%s",
 			WQ_MEM_RECLAIM | WQ_FREEZABLE | WQ_UNBOUND,
-			0, mp->m_fsname);
+			0, mp->m_super->s_id);
 	if (!mp->m_cil_workqueue)
 		goto out_destroy_unwritten;
 
 	mp->m_reclaim_workqueue = alloc_workqueue("xfs-reclaim/%s",
-			WQ_MEM_RECLAIM|WQ_FREEZABLE, 0, mp->m_fsname);
+			WQ_MEM_RECLAIM|WQ_FREEZABLE, 0, mp->m_super->s_id);
 	if (!mp->m_reclaim_workqueue)
 		goto out_destroy_cil;
 
 	mp->m_eofblocks_workqueue = alloc_workqueue("xfs-eofblocks/%s",
-			WQ_MEM_RECLAIM|WQ_FREEZABLE, 0, mp->m_fsname);
+			WQ_MEM_RECLAIM|WQ_FREEZABLE, 0, mp->m_super->s_id);
 	if (!mp->m_eofblocks_workqueue)
 		goto out_destroy_reclaim;
 
 	mp->m_sync_workqueue = alloc_workqueue("xfs-sync/%s", WQ_FREEZABLE, 0,
-					       mp->m_fsname);
+					       mp->m_super->s_id);
 	if (!mp->m_sync_workqueue)
 		goto out_destroy_eofb;
 
@@ -1036,10 +1028,9 @@ xfs_fs_drop_inode(
 }
 
 STATIC void
-xfs_free_fsname(
+xfs_free_names(
 	struct xfs_mount	*mp)
 {
-	kfree(mp->m_fsname);
 	kfree(mp->m_rtname);
 	kfree(mp->m_logname);
 }
@@ -1216,7 +1207,7 @@ xfs_test_remount_options(
 
 	tmp_mp->m_super = sb;
 	error = xfs_parseargs(tmp_mp, options);
-	xfs_free_fsname(tmp_mp);
+	xfs_free_names(tmp_mp);
 	kmem_free(tmp_mp);
 
 	return error;
@@ -1591,7 +1582,7 @@ xfs_fs_fill_super(
 
 	error = xfs_parseargs(mp, (char *)data);
 	if (error)
-		goto out_free_fsname;
+		goto out_free_names;
 
 	sb_min_blocksize(sb, BBSIZE);
 	sb->s_xattr = xfs_xattr_handlers;
@@ -1618,7 +1609,7 @@ xfs_fs_fill_super(
 
 	error = xfs_open_devices(mp);
 	if (error)
-		goto out_free_fsname;
+		goto out_free_names;
 
 	error = xfs_init_mount_workqueues(mp);
 	if (error)
@@ -1755,9 +1746,9 @@ xfs_fs_fill_super(
 	xfs_destroy_mount_workqueues(mp);
  out_close_devices:
 	xfs_close_devices(mp);
- out_free_fsname:
+ out_free_names:
 	sb->s_fs_info = NULL;
-	xfs_free_fsname(mp);
+	xfs_free_names(mp);
 	kfree(mp);
  out:
 	return error;
@@ -1789,7 +1780,7 @@ xfs_fs_put_super(
 	xfs_close_devices(mp);
 
 	sb->s_fs_info = NULL;
-	xfs_free_fsname(mp);
+	xfs_free_names(mp);
 	kfree(mp);
 }
 
diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
index 6ccfd75d3c24..aea71ee189f5 100644
--- a/fs/xfs/xfs_trans_ail.c
+++ b/fs/xfs/xfs_trans_ail.c
@@ -836,7 +836,7 @@ xfs_trans_ail_init(
 	init_waitqueue_head(&ailp->ail_empty);
 
 	ailp->ail_task = kthread_run(xfsaild, ailp, "xfsaild/%s",
-			ailp->ail_mount->m_fsname);
+			ailp->ail_mount->m_super->s_id);
 	if (IS_ERR(ailp->ail_task))
 		goto out_free_ailp;
 


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH v6 05/12] xfs: dont use XFS_IS_QUOTA_RUNNING() for option check
  2019-10-16  0:40 [PATCH v6 00/12] xfs: mount API patch series Ian Kent
                   ` (3 preceding siblings ...)
  2019-10-16  0:41 ` [PATCH v6 04/12] xfs: use super s_id instead of mount info m_fsname Ian Kent
@ 2019-10-16  0:41 ` Ian Kent
  2019-10-16  8:28   ` Christoph Hellwig
  2019-10-16 18:28   ` Darrick J. Wong
  2019-10-16  0:41 ` [PATCH v6 06/12] xfs: add xfs_remount_rw() helper Ian Kent
                   ` (6 subsequent siblings)
  11 siblings, 2 replies; 39+ messages in thread
From: Ian Kent @ 2019-10-16  0:41 UTC (permalink / raw)
  To: linux-xfs
  Cc: Brian Foster, Eric Sandeen, David Howells, Dave Chinner, Al Viro

When CONFIG_XFS_QUOTA is not defined any quota option is invalid.

Using the macro XFS_IS_QUOTA_RUNNING() as a check if any quota option
has been given is a little misleading so use a simple m_qflags != 0
check to make the intended use more explicit.

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

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 5876c2b551b5..f8770206b66e 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -349,7 +349,7 @@ xfs_parseargs(
 	}
 
 #ifndef CONFIG_XFS_QUOTA
-	if (XFS_IS_QUOTA_RUNNING(mp)) {
+	if (mp->m_qflags != 0) {
 		xfs_warn(mp, "quota support not available in this kernel.");
 		return -EINVAL;
 	}


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH v6 06/12] xfs: add xfs_remount_rw() helper
  2019-10-16  0:40 [PATCH v6 00/12] xfs: mount API patch series Ian Kent
                   ` (4 preceding siblings ...)
  2019-10-16  0:41 ` [PATCH v6 05/12] xfs: dont use XFS_IS_QUOTA_RUNNING() for option check Ian Kent
@ 2019-10-16  0:41 ` Ian Kent
  2019-10-16  8:30   ` Christoph Hellwig
  2019-10-16 18:29   ` Darrick J. Wong
  2019-10-16  0:41 ` [PATCH v6 07/12] xfs: add xfs_remount_ro() helper Ian Kent
                   ` (5 subsequent siblings)
  11 siblings, 2 replies; 39+ messages in thread
From: Ian Kent @ 2019-10-16  0:41 UTC (permalink / raw)
  To: linux-xfs
  Cc: Brian Foster, Eric Sandeen, David Howells, Dave Chinner, Al Viro

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

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

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


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH v6 07/12] xfs: add xfs_remount_ro() helper
  2019-10-16  0:40 [PATCH v6 00/12] xfs: mount API patch series Ian Kent
                   ` (5 preceding siblings ...)
  2019-10-16  0:41 ` [PATCH v6 06/12] xfs: add xfs_remount_rw() helper Ian Kent
@ 2019-10-16  0:41 ` Ian Kent
  2019-10-16  8:31   ` Christoph Hellwig
  2019-10-16 18:29   ` Darrick J. Wong
  2019-10-16  0:41 ` [PATCH v6 08/12] xfs: refactor suffix_kstrtoint() Ian Kent
                   ` (4 subsequent siblings)
  11 siblings, 2 replies; 39+ messages in thread
From: Ian Kent @ 2019-10-16  0:41 UTC (permalink / raw)
  To: linux-xfs
  Cc: Brian Foster, Eric Sandeen, David Howells, Dave Chinner, Al Viro

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

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

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


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH v6 08/12] xfs: refactor suffix_kstrtoint()
  2019-10-16  0:40 [PATCH v6 00/12] xfs: mount API patch series Ian Kent
                   ` (6 preceding siblings ...)
  2019-10-16  0:41 ` [PATCH v6 07/12] xfs: add xfs_remount_ro() helper Ian Kent
@ 2019-10-16  0:41 ` Ian Kent
  2019-10-16  8:34   ` Christoph Hellwig
  2019-10-16  0:41 ` [PATCH v6 09/12] xfs: refactor xfs_parseags() Ian Kent
                   ` (3 subsequent siblings)
  11 siblings, 1 reply; 39+ messages in thread
From: Ian Kent @ 2019-10-16  0:41 UTC (permalink / raw)
  To: linux-xfs
  Cc: Brian Foster, Eric Sandeen, David Howells, Dave Chinner, Al Viro

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.

When xfs is switched to use the new mount-api match_kstrtoint()
will no longer be used and will be removed.

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

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 1f706cbf9624..e54348be2a17 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -110,13 +110,13 @@ static const match_table_t tokens = {
 
 
 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;
 
@@ -141,6 +141,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.
@@ -203,7 +217,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:
@@ -219,7 +233,7 @@ xfs_parseargs(
 				return -ENOMEM;
 			break;
 		case Opt_allocsize:
-			if (suffix_kstrtoint(args, 10, &iosize))
+			if (match_kstrtoint(args, 10, &iosize))
 				return -EINVAL;
 			iosizelog = ffs(iosize) - 1;
 			break;


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH v6 09/12] xfs: refactor xfs_parseags()
  2019-10-16  0:40 [PATCH v6 00/12] xfs: mount API patch series Ian Kent
                   ` (7 preceding siblings ...)
  2019-10-16  0:41 ` [PATCH v6 08/12] xfs: refactor suffix_kstrtoint() Ian Kent
@ 2019-10-16  0:41 ` Ian Kent
  2019-10-16  8:36   ` Christoph Hellwig
  2019-10-16  0:41 ` [PATCH v6 10/12] xfs: move xfs_parseargs() validation to a helper Ian Kent
                   ` (2 subsequent siblings)
  11 siblings, 1 reply; 39+ messages in thread
From: Ian Kent @ 2019-10-16  0:41 UTC (permalink / raw)
  To: linux-xfs
  Cc: Brian Foster, Eric Sandeen, David Howells, Dave Chinner, Al Viro

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

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

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

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


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH v6 10/12] xfs: move xfs_parseargs() validation to a helper
  2019-10-16  0:40 [PATCH v6 00/12] xfs: mount API patch series Ian Kent
                   ` (8 preceding siblings ...)
  2019-10-16  0:41 ` [PATCH v6 09/12] xfs: refactor xfs_parseags() Ian Kent
@ 2019-10-16  0:41 ` Ian Kent
  2019-10-16  8:40   ` Christoph Hellwig
  2019-10-16  0:41 ` [PATCH v6 11/12] xfs: dont set sb in xfs_mount_alloc() Ian Kent
  2019-10-16  0:41 ` [PATCH v6 12/12] xfs: switch to use the new mount-api Ian Kent
  11 siblings, 1 reply; 39+ messages in thread
From: Ian Kent @ 2019-10-16  0:41 UTC (permalink / raw)
  To: linux-xfs
  Cc: Brian Foster, Eric Sandeen, David Howells, Dave Chinner, Al Viro

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

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

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index cd17e4e6b922..f5ea96073d11 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -305,67 +305,16 @@ xfs_parse_param(
 	return 0;
 }
 
-/*
- * This function fills in xfs_mount_t fields based on mount args.
- * Note: the superblock has _not_ yet been read in.
- *
- * Note that this function leaks the various device name allocations on
- * failure.  The caller takes care of them.
- *
- * *sb is const because this is also used to test options on the remount
- * path, and we don't want this to have any side effects at remount time.
- * Today this function does not change *sb, but just to future-proof...
- */
-STATIC int
-xfs_parseargs(
-	struct xfs_mount	*mp,
-	char			*options)
+static int
+xfs_validate_params(
+	struct xfs_mount        *mp,
+	int			dsunit,
+	int			dswidth,
+	uint8_t			iosizelog,
+	bool			nooptions)
 {
-	const struct super_block *sb = mp->m_super;
-	char			*p;
-	substring_t		args[MAX_OPT_ARGS];
-	int			dsunit = 0;
-	int			dswidth = 0;
-	uint8_t			iosizelog = 0;
-
-	/*
-	 * 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;
-		int		ret;
-
-		if (!*p)
-			continue;
-
-		token = match_token(p, tokens, args);
-		ret = xfs_parse_param(token, p, args, mp,
-				      &dsunit, &dswidth, &iosizelog);
-		if (ret)
-			return ret;
-	}
+	if (nooptions)
+		goto noopts;
 
 	/*
 	 * no recovery flag requires a read-only mount
@@ -401,7 +350,7 @@ xfs_parseargs(
 		return -EINVAL;
 	}
 
-done:
+noopts:
 	if (dsunit && !(mp->m_flags & XFS_MOUNT_NOALIGN)) {
 		/*
 		 * At this point the superblock has not been read
@@ -449,6 +398,71 @@ xfs_parseargs(
 	return 0;
 }
 
+/*
+ * This function fills in xfs_mount_t fields based on mount args.
+ * Note: the superblock has _not_ yet been read in.
+ *
+ * Note that this function leaks the various device name allocations on
+ * failure.  The caller takes care of them.
+ *
+ * *sb is const because this is also used to test options on the remount
+ * path, and we don't want this to have any side effects at remount time.
+ * Today this function does not change *sb, but just to future-proof...
+ */
+static int
+xfs_parseargs(
+	struct xfs_mount	*mp,
+	char			*options)
+{
+	const struct super_block *sb = mp->m_super;
+	char			*p;
+	substring_t		args[MAX_OPT_ARGS];
+	int			dsunit = 0;
+	int			dswidth = 0;
+	uint8_t			iosizelog = 0;
+
+	/*
+	 * 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;
+		int		ret;
+
+		if (!*p)
+			continue;
+
+		token = match_token(p, tokens, args);
+		ret = xfs_parse_param(token, p, args, mp,
+				      &dsunit, &dswidth, &iosizelog);
+		if (ret)
+			return ret;
+	}
+done:
+	return xfs_validate_params(mp, dsunit, dswidth, iosizelog, false);
+}
+
 struct proc_xfs_info {
 	uint64_t	flag;
 	char		*str;


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH v6 11/12] xfs: dont set sb in xfs_mount_alloc()
  2019-10-16  0:40 [PATCH v6 00/12] xfs: mount API patch series Ian Kent
                   ` (9 preceding siblings ...)
  2019-10-16  0:41 ` [PATCH v6 10/12] xfs: move xfs_parseargs() validation to a helper Ian Kent
@ 2019-10-16  0:41 ` Ian Kent
  2019-10-16  8:36   ` Christoph Hellwig
  2019-10-16  0:41 ` [PATCH v6 12/12] xfs: switch to use the new mount-api Ian Kent
  11 siblings, 1 reply; 39+ messages in thread
From: Ian Kent @ 2019-10-16  0:41 UTC (permalink / raw)
  To: linux-xfs
  Cc: Brian Foster, Eric Sandeen, David Howells, Dave Chinner, Al Viro

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

Also change xfs_mount_alloc() decalaration static -> STATIC.

Signed-off-by: Ian Kent <raven@themaw.net>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_super.c |    7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index f5ea96073d11..13848465303a 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1604,8 +1604,7 @@ xfs_destroy_percpu_counters(
 }
 
 static struct xfs_mount *
-xfs_mount_alloc(
-	struct super_block	*sb)
+xfs_mount_alloc(void)
 {
 	struct xfs_mount	*mp;
 
@@ -1613,7 +1612,6 @@ xfs_mount_alloc(
 	if (!mp)
 		return NULL;
 
-	mp->m_super = sb;
 	spin_lock_init(&mp->m_sb_lock);
 	spin_lock_init(&mp->m_agirotor_lock);
 	INIT_RADIX_TREE(&mp->m_perag_tree, GFP_ATOMIC);
@@ -1649,9 +1647,10 @@ xfs_fs_fill_super(
 	 * allocate mp and do all low-level struct initializations before we
 	 * attach it to the super
 	 */
-	mp = xfs_mount_alloc(sb);
+	mp = xfs_mount_alloc();
 	if (!mp)
 		goto out;
+	mp->m_super = sb;
 	sb->s_fs_info = mp;
 
 	error = xfs_parseargs(mp, (char *)data);


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH v6 12/12] xfs: switch to use the new mount-api
  2019-10-16  0:40 [PATCH v6 00/12] xfs: mount API patch series Ian Kent
                   ` (10 preceding siblings ...)
  2019-10-16  0:41 ` [PATCH v6 11/12] xfs: dont set sb in xfs_mount_alloc() Ian Kent
@ 2019-10-16  0:41 ` Ian Kent
  2019-10-16 18:18   ` Christoph Hellwig
  11 siblings, 1 reply; 39+ messages in thread
From: Ian Kent @ 2019-10-16  0:41 UTC (permalink / raw)
  To: linux-xfs
  Cc: Brian Foster, Eric Sandeen, David Howells, Dave Chinner, Al Viro

Define the struct fs_parameter_spec table that's used by the new
mount-api for options parsing.

Create the various fs context operations methods and define the
fs_context_operations struct.

Create the fs context initialization method and update the struct
file_system_type to utilize it. The initialization function is
responsible for working storage initialization, allocation and
initialization of file system private information storage and for
setting the operations in the fs context.

Also set struct file_system_type .parameters to the newly defined
struct fs_parameter_spec options parsing table for use by the fs
context methods and remove unused code.

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

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 13848465303a..e2a75dfd6119 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -38,6 +38,8 @@
 
 #include <linux/magic.h>
 #include <linux/parser.h>
+#include <linux/fs_context.h>
+#include <linux/fs_parser.h>
 
 static const struct super_operations xfs_super_operations;
 struct bio_set xfs_ioend_bioset;
@@ -59,55 +61,57 @@ 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 = {
-	{Opt_logbufs,	"logbufs=%u"},	/* number of XFS log buffers */
-	{Opt_logbsize,	"logbsize=%s"},	/* size of XFS log buffers */
-	{Opt_logdev,	"logdev=%s"},	/* log device */
-	{Opt_rtdev,	"rtdev=%s"},	/* realtime I/O device */
-	{Opt_wsync,	"wsync"},	/* safe-mode nfs compatible mount */
-	{Opt_noalign,	"noalign"},	/* turn off stripe alignment */
-	{Opt_swalloc,	"swalloc"},	/* turn on stripe width allocation */
-	{Opt_sunit,	"sunit=%u"},	/* data volume stripe unit */
-	{Opt_swidth,	"swidth=%u"},	/* data volume stripe width */
-	{Opt_nouuid,	"nouuid"},	/* ignore filesystem UUID */
-	{Opt_grpid,	"grpid"},	/* group-ID from parent directory */
-	{Opt_nogrpid,	"nogrpid"},	/* group-ID from current process */
-	{Opt_bsdgroups,	"bsdgroups"},	/* group-ID from parent directory */
-	{Opt_sysvgroups,"sysvgroups"},	/* group-ID from current process */
-	{Opt_allocsize,	"allocsize=%s"},/* preferred allocation size */
-	{Opt_norecovery,"norecovery"},	/* don't run XFS recovery */
-	{Opt_inode64,	"inode64"},	/* inodes can be allocated anywhere */
-	{Opt_inode32,   "inode32"},	/* inode allocation limited to
-					 * XFS_MAXINUMBER_32 */
-	{Opt_ikeep,	"ikeep"},	/* do not free empty inode clusters */
-	{Opt_noikeep,	"noikeep"},	/* free empty inode clusters */
-	{Opt_largeio,	"largeio"},	/* report large I/O sizes in stat() */
-	{Opt_nolargeio,	"nolargeio"},	/* do not report large I/O sizes
-					 * in stat(). */
-	{Opt_attr2,	"attr2"},	/* do use attr2 attribute format */
-	{Opt_noattr2,	"noattr2"},	/* do not use attr2 attribute format */
-	{Opt_filestreams,"filestreams"},/* use filestreams allocator */
-	{Opt_quota,	"quota"},	/* disk quotas (user) */
-	{Opt_noquota,	"noquota"},	/* no quotas */
-	{Opt_usrquota,	"usrquota"},	/* user quota enabled */
-	{Opt_grpquota,	"grpquota"},	/* group quota enabled */
-	{Opt_prjquota,	"prjquota"},	/* project quota enabled */
-	{Opt_uquota,	"uquota"},	/* user quota (IRIX variant) */
-	{Opt_gquota,	"gquota"},	/* group quota (IRIX variant) */
-	{Opt_pquota,	"pquota"},	/* project quota (IRIX variant) */
-	{Opt_uqnoenforce,"uqnoenforce"},/* user quota limit enforcement */
-	{Opt_gqnoenforce,"gqnoenforce"},/* group quota limit enforcement */
-	{Opt_pqnoenforce,"pqnoenforce"},/* project quota limit enforcement */
-	{Opt_qnoenforce, "qnoenforce"},	/* same as uqnoenforce */
-	{Opt_discard,	"discard"},	/* Discard unused blocks */
-	{Opt_nodiscard,	"nodiscard"},	/* Do not discard unused blocks */
-	{Opt_dax,	"dax"},		/* Enable direct access to bdev pages */
-	{Opt_err,	NULL},
+static const struct fs_parameter_spec xfs_param_specs[] = {
+	fsparam_u32("logbufs",		Opt_logbufs),
+	fsparam_string("logbsize",	Opt_logbsize),
+	fsparam_string("logdev",	Opt_logdev),
+	fsparam_string("rtdev",		Opt_rtdev),
+	fsparam_flag("wsync",		Opt_wsync),
+	fsparam_flag("noalign",		Opt_noalign),
+	fsparam_flag("swalloc",		Opt_swalloc),
+	fsparam_u32("sunit",		Opt_sunit),
+	fsparam_u32("swidth",		Opt_swidth),
+	fsparam_flag("nouuid",		Opt_nouuid),
+	fsparam_flag("grpid",		Opt_grpid),
+	fsparam_flag("nogrpid",		Opt_nogrpid),
+	fsparam_flag("bsdgroups",	Opt_bsdgroups),
+	fsparam_flag("sysvgroups",	Opt_sysvgroups),
+	fsparam_string("allocsize",	Opt_allocsize),
+	fsparam_flag("norecovery",	Opt_norecovery),
+	fsparam_flag("inode64",		Opt_inode64),
+	fsparam_flag("inode32",		Opt_inode32),
+	fsparam_flag("ikeep",		Opt_ikeep),
+	fsparam_flag("noikeep",		Opt_noikeep),
+	fsparam_flag("largeio",		Opt_largeio),
+	fsparam_flag("nolargeio",	Opt_nolargeio),
+	fsparam_flag("attr2",		Opt_attr2),
+	fsparam_flag("noattr2",		Opt_noattr2),
+	fsparam_flag("filestreams",	Opt_filestreams),
+	fsparam_flag("quota",		Opt_quota),
+	fsparam_flag("noquota",		Opt_noquota),
+	fsparam_flag("usrquota",	Opt_usrquota),
+	fsparam_flag("grpquota",	Opt_grpquota),
+	fsparam_flag("prjquota",	Opt_prjquota),
+	fsparam_flag("uquota",		Opt_uquota),
+	fsparam_flag("gquota",		Opt_gquota),
+	fsparam_flag("pquota",		Opt_pquota),
+	fsparam_flag("uqnoenforce",	Opt_uqnoenforce),
+	fsparam_flag("gqnoenforce",	Opt_gqnoenforce),
+	fsparam_flag("pqnoenforce",	Opt_pqnoenforce),
+	fsparam_flag("qnoenforce",	Opt_qnoenforce),
+	fsparam_flag("discard",		Opt_discard),
+	fsparam_flag("nodiscard",	Opt_nodiscard),
+	fsparam_flag("dax",		Opt_dax),
+	{}
 };
 
+static const struct fs_parameter_description xfs_fs_parameters = {
+	.name		= "XFS",
+	.specs		= xfs_param_specs,
+};
 
 STATIC int
 suffix_kstrtoint(const char *s, unsigned int base, int *res)
@@ -141,57 +145,51 @@ suffix_kstrtoint(const char *s, unsigned int base, int *res)
 	return ret;
 }
 
-STATIC int
-match_kstrtoint(const substring_t *s, unsigned int base, int *res)
-{
-	const char	*value;
-	int ret;
-
-	value = match_strdup(s);
-	if (!value)
-		return -ENOMEM;
-	ret = suffix_kstrtoint(value, base, res);
-	kfree(value);
-	return ret;
-}
+struct xfs_fs_context {
+	int     dsunit;
+	int     dswidth;
+	uint8_t iosizelog;
+};
 
 static int
 xfs_parse_param(
-	int			token,
-	char			*p,
-	substring_t		*args,
-	struct xfs_mount	*mp,
-	int			*dsunit,
-	int			*dswidth,
-	uint8_t			*iosizelog)
+	struct fs_context	*fc,
+	struct fs_parameter	*param)
 {
+	struct xfs_fs_context	*ctx = fc->fs_private;
+	struct xfs_mount	*mp = fc->s_fs_info;
+	struct fs_parse_result	result;
 	int			iosize = 0;
+	int			opt;
 
-	switch (token) {
+	opt = fs_parse(fc, &xfs_fs_parameters, param, &result);
+	if (opt < 0)
+		return opt;
+
+	switch (opt) {
 	case Opt_logbufs:
-		if (match_int(args, &mp->m_logbufs))
-			return -EINVAL;
+		mp->m_logbufs = result.uint_32;
 		break;
 	case Opt_logbsize:
-		if (match_kstrtoint(args, 10, &mp->m_logbsize))
+		if (suffix_kstrtoint(param->string, 10, &mp->m_logbsize))
 			return -EINVAL;
 		break;
 	case Opt_logdev:
 		kfree(mp->m_logname);
-		mp->m_logname = match_strdup(args);
+		mp->m_logname = kstrdup(param->string, GFP_KERNEL);
 		if (!mp->m_logname)
 			return -ENOMEM;
 		break;
 	case Opt_rtdev:
 		kfree(mp->m_rtname);
-		mp->m_rtname = match_strdup(args);
+		mp->m_rtname = kstrdup(param->string, GFP_KERNEL);
 		if (!mp->m_rtname)
 			return -ENOMEM;
 		break;
 	case Opt_allocsize:
-		if (match_kstrtoint(args, 10, &iosize))
+		if (suffix_kstrtoint(param->string, 10, &iosize))
 			return -EINVAL;
-		*iosizelog = ffs(iosize) - 1;
+		ctx->iosizelog = ffs(iosize) - 1;
 		break;
 	case Opt_grpid:
 	case Opt_bsdgroups:
@@ -214,12 +212,10 @@ xfs_parse_param(
 		mp->m_flags |= XFS_MOUNT_SWALLOC;
 		break;
 	case Opt_sunit:
-		if (match_int(args, dsunit))
-			return -EINVAL;
+		ctx->dsunit = result.uint_32;
 		break;
 	case Opt_swidth:
-		if (match_int(args, dswidth))
-			return -EINVAL;
+		ctx->dswidth = result.uint_32;
 		break;
 	case Opt_inode32:
 		mp->m_flags |= XFS_MOUNT_SMALL_INUMS;
@@ -298,7 +294,7 @@ xfs_parse_param(
 		break;
 #endif
 	default:
-		xfs_warn(mp, "unknown mount option [%s].", p);
+		xfs_warn(mp, "unknown mount option [%s].", param->key);
 		return -EINVAL;
 	}
 
@@ -308,9 +304,7 @@ xfs_parse_param(
 static int
 xfs_validate_params(
 	struct xfs_mount        *mp,
-	int			dsunit,
-	int			dswidth,
-	uint8_t			iosizelog,
+	struct xfs_fs_context	*ctx,
 	bool			nooptions)
 {
 	if (nooptions)
@@ -325,7 +319,8 @@ xfs_validate_params(
 		return -EINVAL;
 	}
 
-	if ((mp->m_flags & XFS_MOUNT_NOALIGN) && (dsunit || dswidth)) {
+	if ((mp->m_flags & XFS_MOUNT_NOALIGN) &&
+	    (ctx->dsunit || ctx->dswidth)) {
 		xfs_warn(mp,
 	"sunit and swidth options incompatible with the noalign option");
 		return -EINVAL;
@@ -338,28 +333,28 @@ xfs_validate_params(
 	}
 #endif
 
-	if ((dsunit && !dswidth) || (!dsunit && dswidth)) {
+	if ((ctx->dsunit && !ctx->dswidth) || (!ctx->dsunit && ctx->dswidth)) {
 		xfs_warn(mp, "sunit and swidth must be specified together");
 		return -EINVAL;
 	}
 
-	if (dsunit && (dswidth % dsunit != 0)) {
+	if (ctx->dsunit && (ctx->dswidth % ctx->dsunit != 0)) {
 		xfs_warn(mp,
 	"stripe width (%d) must be a multiple of the stripe unit (%d)",
-			dswidth, dsunit);
+			ctx->dswidth, ctx->dsunit);
 		return -EINVAL;
 	}
 
 noopts:
-	if (dsunit && !(mp->m_flags & XFS_MOUNT_NOALIGN)) {
+	if (ctx->dsunit && !(mp->m_flags & XFS_MOUNT_NOALIGN)) {
 		/*
 		 * At this point the superblock has not been read
 		 * in, therefore we do not know the block size.
 		 * Before the mount call ends we will convert
 		 * these to FSBs.
 		 */
-		mp->m_dalign = dsunit;
-		mp->m_swidth = dswidth;
+		mp->m_dalign = ctx->dsunit;
+		mp->m_swidth = ctx->dswidth;
 	}
 
 	if (mp->m_logbufs != -1 &&
@@ -381,88 +376,23 @@ xfs_validate_params(
 		return -EINVAL;
 	}
 
-	if (iosizelog) {
-		if (iosizelog > XFS_MAX_IO_LOG ||
-		    iosizelog < XFS_MIN_IO_LOG) {
+	if (ctx->iosizelog) {
+		if (ctx->iosizelog > XFS_MAX_IO_LOG ||
+		    ctx->iosizelog < XFS_MIN_IO_LOG) {
 			xfs_warn(mp, "invalid log iosize: %d [not %d-%d]",
-				iosizelog, XFS_MIN_IO_LOG,
+				ctx->iosizelog, XFS_MIN_IO_LOG,
 				XFS_MAX_IO_LOG);
 			return -EINVAL;
 		}
 
 		mp->m_flags |= XFS_MOUNT_DFLT_IOSIZE;
-		mp->m_readio_log = iosizelog;
-		mp->m_writeio_log = iosizelog;
+		mp->m_readio_log = ctx->iosizelog;
+		mp->m_writeio_log = ctx->iosizelog;
 	}
 
 	return 0;
 }
 
-/*
- * 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;
-	uint8_t			iosizelog = 0;
-
-	/*
-	 * 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;
-		int		ret;
-
-		if (!*p)
-			continue;
-
-		token = match_token(p, tokens, args);
-		ret = xfs_parse_param(token, p, args, mp,
-				      &dsunit, &dswidth, &iosizelog);
-		if (ret)
-			return ret;
-	}
-done:
-	return xfs_validate_params(mp, dsunit, dswidth, iosizelog, false);
-}
-
 struct proc_xfs_info {
 	uint64_t	flag;
 	char		*str;
@@ -1241,26 +1171,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_names(tmp_mp);
-	kmem_free(tmp_mp);
-
-	return error;
-}
-
 static int
 xfs_remount_rw(
 	struct xfs_mount	*mp)
@@ -1364,76 +1274,56 @@ xfs_remount_ro(
 	return 0;
 }
 
-STATIC int
-xfs_fs_remount(
-	struct super_block	*sb,
-	int			*flags,
-	char			*options)
+/*
+ * 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.
+ */
+static int
+xfs_reconfigure(
+	struct fs_context *fc)
 {
-	struct xfs_mount	*mp = XFS_M(sb);
+	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;
-	substring_t		args[MAX_OPT_ARGS];
-	char			*p;
+	int			flags = fc->sb_flags;
 	int			error;
 
-	/* First, check for complete junk; i.e. invalid options */
-	error = xfs_test_remount_options(sb, options);
+	error = xfs_validate_params(new_mp, ctx, false);
 	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
-		}
+	/* 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_RDONLY) && !(flags & SB_RDONLY)) {
 		error = xfs_remount_rw(mp);
 		if (error)
 			return error;
 	}
 
 	/* rw -> ro */
-	if (!(mp->m_flags & XFS_MOUNT_RDONLY) && (*flags & SB_RDONLY)) {
+	if (!(mp->m_flags & XFS_MOUNT_RDONLY) && (flags & SB_RDONLY)) {
 		error = xfs_remount_ro(mp);
 		if (error)
 			return error;
@@ -1636,24 +1526,17 @@ xfs_mount_alloc(void)
 STATIC int
 xfs_fs_fill_super(
 	struct super_block	*sb,
-	void			*data,
-	int			silent)
+	struct fs_context	*fc)
 {
+	struct xfs_fs_context	*ctx = fc->fs_private;
+	struct xfs_mount	*mp = sb->s_fs_info;
 	struct inode		*root;
-	struct xfs_mount	*mp = NULL;
+	int			silent = fc->sb_flags & SB_SILENT;
 	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();
-	if (!mp)
-		goto out;
 	mp->m_super = sb;
-	sb->s_fs_info = mp;
 
-	error = xfs_parseargs(mp, (char *)data);
+	error = xfs_validate_params(mp, ctx, false);
 	if (error)
 		goto out_free_names;
 
@@ -1823,7 +1706,6 @@ xfs_fs_fill_super(
 	sb->s_fs_info = NULL;
 	xfs_free_names(mp);
 	kfree(mp);
- out:
 	return error;
 
  out_unmount:
@@ -1832,6 +1714,13 @@ xfs_fs_fill_super(
 	goto out_free_sb;
 }
 
+static int
+xfs_get_tree(
+	struct fs_context	*fc)
+{
+	return get_tree_bdev(fc, xfs_fs_fill_super);
+}
+
 STATIC void
 xfs_fs_put_super(
 	struct super_block	*sb)
@@ -1857,16 +1746,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,
@@ -1896,16 +1775,85 @@ 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,
 };
 
+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;
+
+	/*
+	 * mp and ctx are stored in the fs_context when it is
+	 * initialized. mp is transferred to the superblock on
+	 * a successful mount, but if an error occurs before the
+	 * transfer we have to free it here.
+	 */
+	if (mp) {
+		xfs_free_names(mp);
+		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 int xfs_init_fs_context(struct fs_context *fc)
+{
+	struct xfs_fs_context	*ctx;
+	struct xfs_mount	*mp;
+
+	ctx = kzalloc(sizeof(struct xfs_fs_context), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+
+	mp = xfs_mount_alloc();
+	if (!mp) {
+		kfree(ctx);
+		return -ENOMEM;
+	}
+
+	/*
+	 * Set some default flags that could be cleared by the mount option
+	 * parsing.
+	 */
+	mp->m_flags |= XFS_MOUNT_COMPAT_IOSIZE;
+
+	/*
+	 * These can be overridden by the mount option parsing.
+	 */
+	mp->m_logbufs = -1;
+	mp->m_logbsize = -1;
+
+	/*
+	 * Copy binary VFS mount flags we are interested in.
+	 */
+	if (fc->sb_flags & SB_RDONLY)
+		mp->m_flags |= XFS_MOUNT_RDONLY;
+	if (fc->sb_flags & SB_DIRSYNC)
+		mp->m_flags |= XFS_MOUNT_DIRSYNC;
+	if (fc->sb_flags & SB_SYNCHRONOUS)
+		mp->m_flags |= XFS_MOUNT_WSYNC;
+
+	fc->fs_private = ctx;
+	fc->s_fs_info = mp;
+	fc->ops = &xfs_context_ops;
+
+	return 0;
+}
+
 static struct file_system_type xfs_fs_type = {
 	.owner			= THIS_MODULE,
 	.name			= "xfs",
-	.mount			= xfs_fs_mount,
+	.init_fs_context	= xfs_init_fs_context,
+	.parameters		= &xfs_fs_parameters,
 	.kill_sb		= kill_block_super,
 	.fs_flags		= FS_REQUIRES_DEV,
 };


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* Re: [PATCH v6 02/12] xfs: remove very old mount option
  2019-10-16  0:40 ` [PATCH v6 02/12] xfs: remove very old mount option Ian Kent
@ 2019-10-16  8:26   ` Christoph Hellwig
  2019-10-16 18:25   ` Darrick J. Wong
  1 sibling, 0 replies; 39+ messages in thread
From: Christoph Hellwig @ 2019-10-16  8:26 UTC (permalink / raw)
  To: Ian Kent
  Cc: linux-xfs, Brian Foster, Eric Sandeen, David Howells,
	Dave Chinner, Al Viro

Looks good:

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

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v6 03/12] xfs: remove unused mount info field m_fsname_len
  2019-10-16  0:40 ` [PATCH v6 03/12] xfs: remove unused mount info field m_fsname_len Ian Kent
@ 2019-10-16  8:26   ` Christoph Hellwig
  2019-10-16 18:26   ` Darrick J. Wong
  1 sibling, 0 replies; 39+ messages in thread
From: Christoph Hellwig @ 2019-10-16  8:26 UTC (permalink / raw)
  To: Ian Kent
  Cc: linux-xfs, Brian Foster, Eric Sandeen, David Howells,
	Dave Chinner, Al Viro

s/mount info/struct xfs_mount/g

in the subject and commit log (and various follow on patches).

Otherwise looks good:

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

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v6 04/12] xfs: use super s_id instead of mount info m_fsname
  2019-10-16  0:41 ` [PATCH v6 04/12] xfs: use super s_id instead of mount info m_fsname Ian Kent
@ 2019-10-16  8:27   ` Christoph Hellwig
  2019-10-16 18:27   ` Darrick J. Wong
  1 sibling, 0 replies; 39+ messages in thread
From: Christoph Hellwig @ 2019-10-16  8:27 UTC (permalink / raw)
  To: Ian Kent
  Cc: linux-xfs, Brian Foster, Eric Sandeen, David Howells,
	Dave Chinner, Al Viro

Except for the mount info thing mentioned earlier:

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

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v6 05/12] xfs: dont use XFS_IS_QUOTA_RUNNING() for option check
  2019-10-16  0:41 ` [PATCH v6 05/12] xfs: dont use XFS_IS_QUOTA_RUNNING() for option check Ian Kent
@ 2019-10-16  8:28   ` Christoph Hellwig
  2019-10-16 18:28   ` Darrick J. Wong
  1 sibling, 0 replies; 39+ messages in thread
From: Christoph Hellwig @ 2019-10-16  8:28 UTC (permalink / raw)
  To: Ian Kent
  Cc: linux-xfs, Brian Foster, Eric Sandeen, David Howells,
	Dave Chinner, Al Viro

On Wed, Oct 16, 2019 at 08:41:08AM +0800, Ian Kent wrote:
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 5876c2b551b5..f8770206b66e 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -349,7 +349,7 @@ xfs_parseargs(
>  	}
>  
>  #ifndef CONFIG_XFS_QUOTA
> -	if (XFS_IS_QUOTA_RUNNING(mp)) {
> +	if (mp->m_qflags != 0) {
>  		xfs_warn(mp, "quota support not available in this kernel.");
>  		return -EINVAL;
>  	}

Please also convert to use !IS_ENABLED while you are at it.

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v6 06/12] xfs: add xfs_remount_rw() helper
  2019-10-16  0:41 ` [PATCH v6 06/12] xfs: add xfs_remount_rw() helper Ian Kent
@ 2019-10-16  8:30   ` Christoph Hellwig
  2019-10-16 18:29   ` Darrick J. Wong
  1 sibling, 0 replies; 39+ messages in thread
From: Christoph Hellwig @ 2019-10-16  8:30 UTC (permalink / raw)
  To: Ian Kent
  Cc: linux-xfs, Brian Foster, Eric Sandeen, David Howells,
	Dave Chinner, Al Viro

> +	xfs_sb_t		*sbp = &mp->m_sb;

Please use the underlying struct types instead of the typedefs for all
new/modified code.

Otherwise looks good:

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

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v6 07/12] xfs: add xfs_remount_ro() helper
  2019-10-16  0:41 ` [PATCH v6 07/12] xfs: add xfs_remount_ro() helper Ian Kent
@ 2019-10-16  8:31   ` Christoph Hellwig
  2019-10-16 18:29   ` Darrick J. Wong
  1 sibling, 0 replies; 39+ messages in thread
From: Christoph Hellwig @ 2019-10-16  8:31 UTC (permalink / raw)
  To: Ian Kent
  Cc: linux-xfs, Brian Foster, Eric Sandeen, David Howells,
	Dave Chinner, Al Viro

> +	/*
> +	 * Cancel background eofb scanning so it cannot race with the
> +	 * final log force+buftarg wait and deadlock the remount.
> +	 */

> +	/*
> +	 * 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.
> +	 */

Please use up the full 80 chars available for each line for comments.

Otherwise looks fine:


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

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v6 08/12] xfs: refactor suffix_kstrtoint()
  2019-10-16  0:41 ` [PATCH v6 08/12] xfs: refactor suffix_kstrtoint() Ian Kent
@ 2019-10-16  8:34   ` Christoph Hellwig
  2019-10-16 15:37     ` Darrick J. Wong
  0 siblings, 1 reply; 39+ messages in thread
From: Christoph Hellwig @ 2019-10-16  8:34 UTC (permalink / raw)
  To: Ian Kent
  Cc: linux-xfs, Brian Foster, Eric Sandeen, David Howells,
	Dave Chinner, Al Viro

On Wed, Oct 16, 2019 at 08:41:27AM +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.
> 
> 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.
> 
> When xfs is switched to use the new mount-api match_kstrtoint()
> will no longer be used and will be removed.

Please use up the full 72 chars available for the commit log.

> +STATIC int
> +match_kstrtoint(const substring_t *s, unsigned int base, int *res)

No need for static on new/heavily modified functions, just use static.

Note that both this and suffix_kstrtoint don't really follow the
normal XFS prototype formatting style either.

> +	const char	*value;
> +	int ret;

Similarly here - either you follow the XFS style of tab alignining
the variable names for all variables, or for none, but a mix is very
odd.

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v6 09/12] xfs: refactor xfs_parseags()
  2019-10-16  0:41 ` [PATCH v6 09/12] xfs: refactor xfs_parseags() Ian Kent
@ 2019-10-16  8:36   ` Christoph Hellwig
  0 siblings, 0 replies; 39+ messages in thread
From: Christoph Hellwig @ 2019-10-16  8:36 UTC (permalink / raw)
  To: Ian Kent
  Cc: linux-xfs, Brian Foster, Eric Sandeen, David Howells,
	Dave Chinner, Al Viro

On Wed, Oct 16, 2019 at 08:41:32AM +0800, Ian Kent wrote:
> Refactor xfs_parseags(), move the entire token case block to a
> separate function in an attempt to highlight the code that
> actually changes in converting to use the new mount api.
> 
> The only changes are what's needed to communicate the variables
> dsunit, dswidth and iosizelog back to xfs_parseags().

Use up the full space avaiable for commit log, please.

> +	case Opt_gqnoenforce:
> +		mp->m_qflags |= (XFS_GQUOTA_ACCT | XFS_GQUOTA_ACTIVE);
> +		mp->m_qflags &= ~XFS_GQUOTA_ENFD;
> +		break;
> +	case Opt_discard:
> +		mp->m_flags |= XFS_MOUNT_DISCARD;
> +		break;
> +	case Opt_nodiscard:
> +		mp->m_flags &= ~XFS_MOUNT_DISCARD;
> +		break;
> +#ifdef CONFIG_FS_DAX
> +	case Opt_dax:
> +		mp->m_flags |= XFS_MOUNT_DAX;
> +		break;
> +#endif
> +	default:
> +		xfs_warn(mp, "unknown mount option [%s].", p);
> +		return -EINVAL;
> +	}
> +
> +	return 0;

It seems all these breaks could simply directly do a return 0, make
the function a tidbit easier to read.

> +		ret = xfs_parse_param(token, p, args, mp,
> +				      &dsunit, &dswidth, &iosizelog);

Please use up the full 80 chars on the first line

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v6 11/12] xfs: dont set sb in xfs_mount_alloc()
  2019-10-16  0:41 ` [PATCH v6 11/12] xfs: dont set sb in xfs_mount_alloc() Ian Kent
@ 2019-10-16  8:36   ` Christoph Hellwig
  0 siblings, 0 replies; 39+ messages in thread
From: Christoph Hellwig @ 2019-10-16  8:36 UTC (permalink / raw)
  To: Ian Kent
  Cc: linux-xfs, Brian Foster, Eric Sandeen, David Howells,
	Dave Chinner, Al Viro

On Wed, Oct 16, 2019 at 08:41:43AM +0800, Ian Kent wrote:
> When changing to use the new mount api the super block won't be
> available when the xfs_mount info struct is allocated so move
> setting the super block in xfs_mount to xfs_fs_fill_super().
> 
> Also change xfs_mount_alloc() decalaration static -> STATIC.

Looks fine:

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

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v6 10/12] xfs: move xfs_parseargs() validation to a helper
  2019-10-16  0:41 ` [PATCH v6 10/12] xfs: move xfs_parseargs() validation to a helper Ian Kent
@ 2019-10-16  8:40   ` Christoph Hellwig
  2019-10-17  0:58     ` Ian Kent
  2019-10-23  2:59     ` Ian Kent
  0 siblings, 2 replies; 39+ messages in thread
From: Christoph Hellwig @ 2019-10-16  8:40 UTC (permalink / raw)
  To: Ian Kent
  Cc: linux-xfs, Brian Foster, Eric Sandeen, David Howells,
	Dave Chinner, Al Viro

On Wed, Oct 16, 2019 at 08:41:38AM +0800, Ian Kent wrote:
> +static int
> +xfs_validate_params(
> +	struct xfs_mount        *mp,
> +	int			dsunit,
> +	int			dswidth,
> +	uint8_t			iosizelog,
> +	bool			nooptions)

Please add a refactor patch before this one to always set the stripe
unit / width / log I/O size values in the mount structure at parsing
time as suggested before.  The will also remove the need for the
xfs_fs_context structure in the last patch.

> +	if (nooptions)
> +		goto noopts;

This option is always false in this patch.  I'd suggest you only add
it once we actually add a user.

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v6 08/12] xfs: refactor suffix_kstrtoint()
  2019-10-16  8:34   ` Christoph Hellwig
@ 2019-10-16 15:37     ` Darrick J. Wong
  0 siblings, 0 replies; 39+ messages in thread
From: Darrick J. Wong @ 2019-10-16 15:37 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Ian Kent, linux-xfs, Brian Foster, Eric Sandeen, David Howells,
	Dave Chinner, Al Viro

On Wed, Oct 16, 2019 at 01:34:20AM -0700, Christoph Hellwig wrote:
> On Wed, Oct 16, 2019 at 08:41:27AM +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.
> > 
> > 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.
> > 
> > When xfs is switched to use the new mount-api match_kstrtoint()
> > will no longer be used and will be removed.
> 
> Please use up the full 72 chars available for the commit log.
> 
> > +STATIC int
> > +match_kstrtoint(const substring_t *s, unsigned int base, int *res)
> 
> No need for static on new/heavily modified functions, just use static.
> 
> Note that both this and suffix_kstrtoint don't really follow the
> normal XFS prototype formatting style either.
> 
> > +	const char	*value;
> > +	int ret;
> 
> Similarly here - either you follow the XFS style of tab alignining
> the variable names for all variables, or for none, but a mix is very
> odd.

Please follow the xfs style of tab aligning variables inside xfs.

--D

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v6 12/12] xfs: switch to use the new mount-api
  2019-10-16  0:41 ` [PATCH v6 12/12] xfs: switch to use the new mount-api Ian Kent
@ 2019-10-16 18:18   ` Christoph Hellwig
  2019-10-17  1:13     ` Ian Kent
  2019-10-23  3:17     ` Ian Kent
  0 siblings, 2 replies; 39+ messages in thread
From: Christoph Hellwig @ 2019-10-16 18:18 UTC (permalink / raw)
  To: Ian Kent
  Cc: linux-xfs, Brian Foster, Eric Sandeen, David Howells,
	Dave Chinner, Al Viro

On Wed, Oct 16, 2019 at 08:41:48AM +0800, Ian Kent wrote:
> +static const struct fs_parameter_description xfs_fs_parameters = {
> +	.name		= "XFS",
> +	.specs		= xfs_param_specs,
> +};

Well spell xfs in lower case in the file system type, so I think we should
spell it the same here.

Btw, can we keep all the mount code together where most of it already
is at the top of the file?  I know the existing version has some remount
stuff at the bottom, but as that get entirely rewritten we might as well
move it all up.

> +	int			silent = fc->sb_flags & SB_SILENT;

The silent variable is only used once, so we might as well remove it.

> +	struct xfs_mount	*mp = fc->s_fs_info;
> +
> +	/*
> +	 * mp and ctx are stored in the fs_context when it is
> +	 * initialized. mp is transferred to the superblock on
> +	 * a successful mount, but if an error occurs before the
> +	 * transfer we have to free it here.
> +	 */
> +	if (mp) {
> +		xfs_free_names(mp);
> +		kfree(mp);
> +	}

We always pair xfs_free_names with freeing the mount structure.
I think it would be nice to add an xfs_free_mount that does both
as a refactoring at the beginning of the series. 

> +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,
> +};

Should these all get a prefix like xfs_fc_free?  Maybe xfs_fsctx
to be a little bit more descriptive?

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v6 02/12] xfs: remove very old mount option
  2019-10-16  0:40 ` [PATCH v6 02/12] xfs: remove very old mount option Ian Kent
  2019-10-16  8:26   ` Christoph Hellwig
@ 2019-10-16 18:25   ` Darrick J. Wong
  1 sibling, 0 replies; 39+ messages in thread
From: Darrick J. Wong @ 2019-10-16 18:25 UTC (permalink / raw)
  To: Ian Kent
  Cc: linux-xfs, Brian Foster, Eric Sandeen, David Howells,
	Dave Chinner, Al Viro

On Wed, Oct 16, 2019 at 08:40:52AM +0800, Ian Kent wrote:
> It appears the biosize mount option hasn't been documented as
> a vilid option since 2005.

    ^^^^^ "valid"

With that fixed,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> 
> So remove it.
> 
> Signed-off-by: Ian Kent <raven@themaw.net>
> Reviewed-by: Brian Foster <bfoster@redhat.com>
> ---
>  fs/xfs/xfs_super.c |    4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 8d1df9f8be07..1bb7ede5d75b 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -51,7 +51,7 @@ static struct xfs_kobj xfs_dbg_kobj;	/* global debug sysfs attrs */
>   * Table driven mount option parser.
>   */
>  enum {
> -	Opt_logbufs, Opt_logbsize, Opt_logdev, Opt_rtdev, Opt_biosize,
> +	Opt_logbufs, Opt_logbsize, Opt_logdev, Opt_rtdev,
>  	Opt_wsync, Opt_noalign, Opt_swalloc, Opt_sunit, Opt_swidth, Opt_nouuid,
>  	Opt_grpid, Opt_nogrpid, Opt_bsdgroups, Opt_sysvgroups,
>  	Opt_allocsize, Opt_norecovery, Opt_inode64, Opt_inode32, Opt_ikeep,
> @@ -67,7 +67,6 @@ static const match_table_t tokens = {
>  	{Opt_logbsize,	"logbsize=%s"},	/* size of XFS log buffers */
>  	{Opt_logdev,	"logdev=%s"},	/* log device */
>  	{Opt_rtdev,	"rtdev=%s"},	/* realtime I/O device */
> -	{Opt_biosize,	"biosize=%u"},	/* log2 of preferred buffered io size */
>  	{Opt_wsync,	"wsync"},	/* safe-mode nfs compatible mount */
>  	{Opt_noalign,	"noalign"},	/* turn off stripe alignment */
>  	{Opt_swalloc,	"swalloc"},	/* turn on stripe width allocation */
> @@ -229,7 +228,6 @@ xfs_parseargs(
>  				return -ENOMEM;
>  			break;
>  		case Opt_allocsize:
> -		case Opt_biosize:
>  			if (suffix_kstrtoint(args, 10, &iosize))
>  				return -EINVAL;
>  			iosizelog = ffs(iosize) - 1;
> 

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v6 03/12] xfs: remove unused mount info field m_fsname_len
  2019-10-16  0:40 ` [PATCH v6 03/12] xfs: remove unused mount info field m_fsname_len Ian Kent
  2019-10-16  8:26   ` Christoph Hellwig
@ 2019-10-16 18:26   ` Darrick J. Wong
  1 sibling, 0 replies; 39+ messages in thread
From: Darrick J. Wong @ 2019-10-16 18:26 UTC (permalink / raw)
  To: Ian Kent
  Cc: linux-xfs, Brian Foster, Eric Sandeen, David Howells,
	Dave Chinner, Al Viro

On Wed, Oct 16, 2019 at 08:40:58AM +0800, Ian Kent wrote:
> The mount info field m_fsname_len is not used anywhere, remove it.
> 
> Signed-off-by: Ian Kent <raven@themaw.net>

With hch's comments applied,

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

--D

> ---
>  fs/xfs/xfs_mount.h |    1 -
>  fs/xfs/xfs_super.c |    1 -
>  2 files changed, 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
> index fdb60e09a9c5..b3230f7ca2bf 100644
> --- a/fs/xfs/xfs_mount.h
> +++ b/fs/xfs/xfs_mount.h
> @@ -90,7 +90,6 @@ typedef struct xfs_mount {
>  
>  	struct xfs_buf		*m_sb_bp;	/* buffer for superblock */
>  	char			*m_fsname;	/* filesystem name */
> -	int			m_fsname_len;	/* strlen of fs name */
>  	char			*m_rtname;	/* realtime device name */
>  	char			*m_logname;	/* external log device name */
>  	int			m_bsize;	/* fs logical block size */
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 1bb7ede5d75b..cfa306f62bec 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -172,7 +172,6 @@ xfs_parseargs(
>  	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.
> 

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v6 04/12] xfs: use super s_id instead of mount info m_fsname
  2019-10-16  0:41 ` [PATCH v6 04/12] xfs: use super s_id instead of mount info m_fsname Ian Kent
  2019-10-16  8:27   ` Christoph Hellwig
@ 2019-10-16 18:27   ` Darrick J. Wong
  1 sibling, 0 replies; 39+ messages in thread
From: Darrick J. Wong @ 2019-10-16 18:27 UTC (permalink / raw)
  To: Ian Kent
  Cc: linux-xfs, Brian Foster, Eric Sandeen, David Howells,
	Dave Chinner, Al Viro

On Wed, Oct 16, 2019 at 08:41:03AM +0800, Ian Kent wrote:
> Eliminate mount info field m_fsname by using the super block s_id
> field directly.
> 
> Signed-off-by: Ian Kent <raven@themaw.net>

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

--D

> ---
>  fs/xfs/xfs_error.c     |    2 +-
>  fs/xfs/xfs_log.c       |    2 +-
>  fs/xfs/xfs_message.c   |    4 ++--
>  fs/xfs/xfs_mount.c     |    5 +++--
>  fs/xfs/xfs_mount.h     |    1 -
>  fs/xfs/xfs_pnfs.c      |    2 +-
>  fs/xfs/xfs_super.c     |   35 +++++++++++++----------------------
>  fs/xfs/xfs_trans_ail.c |    2 +-
>  8 files changed, 22 insertions(+), 31 deletions(-)
> 
> diff --git a/fs/xfs/xfs_error.c b/fs/xfs/xfs_error.c
> index 849fd4476950..3ff3fc202522 100644
> --- a/fs/xfs/xfs_error.c
> +++ b/fs/xfs/xfs_error.c
> @@ -257,7 +257,7 @@ xfs_errortag_test(
>  
>  	xfs_warn_ratelimited(mp,
>  "Injecting error (%s) at file %s, line %d, on filesystem \"%s\"",
> -			expression, file, line, mp->m_fsname);
> +			expression, file, line, mp->m_super->s_id);
>  	return true;
>  }
>  
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index a2beee9f74da..ed99f4f063c3 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -1479,7 +1479,7 @@ xlog_alloc_log(
>  
>  	log->l_ioend_workqueue = alloc_workqueue("xfs-log/%s",
>  			WQ_MEM_RECLAIM | WQ_FREEZABLE | WQ_HIGHPRI, 0,
> -			mp->m_fsname);
> +			mp->m_super->s_id);
>  	if (!log->l_ioend_workqueue)
>  		goto out_free_iclog;
>  
> diff --git a/fs/xfs/xfs_message.c b/fs/xfs/xfs_message.c
> index 9804efe525a9..aaea6faf5acc 100644
> --- a/fs/xfs/xfs_message.c
> +++ b/fs/xfs/xfs_message.c
> @@ -20,8 +20,8 @@ __xfs_printk(
>  	const struct xfs_mount	*mp,
>  	struct va_format	*vaf)
>  {
> -	if (mp && mp->m_fsname) {
> -		printk("%sXFS (%s): %pV\n", level, mp->m_fsname, vaf);
> +	if (mp && mp->m_super && mp->m_super->s_id) {
> +		printk("%sXFS (%s): %pV\n", level, mp->m_super->s_id, vaf);
>  		return;
>  	}
>  	printk("%sXFS: %pV\n", level, vaf);
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index ba5b6f3b2b88..b6145a70a593 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -706,7 +706,8 @@ xfs_mountfs(
>  	/* enable fail_at_unmount as default */
>  	mp->m_fail_unmount = true;
>  
> -	error = xfs_sysfs_init(&mp->m_kobj, &xfs_mp_ktype, NULL, mp->m_fsname);
> +	error = xfs_sysfs_init(&mp->m_kobj, &xfs_mp_ktype,
> +			       NULL, mp->m_super->s_id);
>  	if (error)
>  		goto out;
>  
> @@ -1277,7 +1278,7 @@ xfs_mod_fdblocks(
>  	printk_once(KERN_WARNING
>  		"Filesystem \"%s\": reserve blocks depleted! "
>  		"Consider increasing reserve pool size.",
> -		mp->m_fsname);
> +		mp->m_super->s_id);
>  fdblocks_enospc:
>  	spin_unlock(&mp->m_sb_lock);
>  	return -ENOSPC;
> diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
> index b3230f7ca2bf..f2a9f316fa6c 100644
> --- a/fs/xfs/xfs_mount.h
> +++ b/fs/xfs/xfs_mount.h
> @@ -89,7 +89,6 @@ typedef struct xfs_mount {
>  	struct percpu_counter	m_delalloc_blks;
>  
>  	struct xfs_buf		*m_sb_bp;	/* buffer for superblock */
> -	char			*m_fsname;	/* filesystem name */
>  	char			*m_rtname;	/* realtime device name */
>  	char			*m_logname;	/* external log device name */
>  	int			m_bsize;	/* fs logical block size */
> diff --git a/fs/xfs/xfs_pnfs.c b/fs/xfs/xfs_pnfs.c
> index a339bd5fa260..81df10f69ade 100644
> --- a/fs/xfs/xfs_pnfs.c
> +++ b/fs/xfs/xfs_pnfs.c
> @@ -59,7 +59,7 @@ xfs_fs_get_uuid(
>  
>  	printk_once(KERN_NOTICE
>  "XFS (%s): using experimental pNFS feature, use at your own risk!\n",
> -		mp->m_fsname);
> +		mp->m_super->s_id);
>  
>  	if (*len < sizeof(uuid_t))
>  		return -EINVAL;
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index cfa306f62bec..5876c2b551b5 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -165,14 +165,6 @@ xfs_parseargs(
>  	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;
> -
>  	/*
>  	 * Copy binary VFS mount flags we are interested in.
>  	 */
> @@ -805,33 +797,33 @@ xfs_init_mount_workqueues(
>  	struct xfs_mount	*mp)
>  {
>  	mp->m_buf_workqueue = alloc_workqueue("xfs-buf/%s",
> -			WQ_MEM_RECLAIM|WQ_FREEZABLE, 1, mp->m_fsname);
> +			WQ_MEM_RECLAIM|WQ_FREEZABLE, 1, mp->m_super->s_id);
>  	if (!mp->m_buf_workqueue)
>  		goto out;
>  
>  	mp->m_unwritten_workqueue = alloc_workqueue("xfs-conv/%s",
> -			WQ_MEM_RECLAIM|WQ_FREEZABLE, 0, mp->m_fsname);
> +			WQ_MEM_RECLAIM|WQ_FREEZABLE, 0, mp->m_super->s_id);
>  	if (!mp->m_unwritten_workqueue)
>  		goto out_destroy_buf;
>  
>  	mp->m_cil_workqueue = alloc_workqueue("xfs-cil/%s",
>  			WQ_MEM_RECLAIM | WQ_FREEZABLE | WQ_UNBOUND,
> -			0, mp->m_fsname);
> +			0, mp->m_super->s_id);
>  	if (!mp->m_cil_workqueue)
>  		goto out_destroy_unwritten;
>  
>  	mp->m_reclaim_workqueue = alloc_workqueue("xfs-reclaim/%s",
> -			WQ_MEM_RECLAIM|WQ_FREEZABLE, 0, mp->m_fsname);
> +			WQ_MEM_RECLAIM|WQ_FREEZABLE, 0, mp->m_super->s_id);
>  	if (!mp->m_reclaim_workqueue)
>  		goto out_destroy_cil;
>  
>  	mp->m_eofblocks_workqueue = alloc_workqueue("xfs-eofblocks/%s",
> -			WQ_MEM_RECLAIM|WQ_FREEZABLE, 0, mp->m_fsname);
> +			WQ_MEM_RECLAIM|WQ_FREEZABLE, 0, mp->m_super->s_id);
>  	if (!mp->m_eofblocks_workqueue)
>  		goto out_destroy_reclaim;
>  
>  	mp->m_sync_workqueue = alloc_workqueue("xfs-sync/%s", WQ_FREEZABLE, 0,
> -					       mp->m_fsname);
> +					       mp->m_super->s_id);
>  	if (!mp->m_sync_workqueue)
>  		goto out_destroy_eofb;
>  
> @@ -1036,10 +1028,9 @@ xfs_fs_drop_inode(
>  }
>  
>  STATIC void
> -xfs_free_fsname(
> +xfs_free_names(
>  	struct xfs_mount	*mp)
>  {
> -	kfree(mp->m_fsname);
>  	kfree(mp->m_rtname);
>  	kfree(mp->m_logname);
>  }
> @@ -1216,7 +1207,7 @@ xfs_test_remount_options(
>  
>  	tmp_mp->m_super = sb;
>  	error = xfs_parseargs(tmp_mp, options);
> -	xfs_free_fsname(tmp_mp);
> +	xfs_free_names(tmp_mp);
>  	kmem_free(tmp_mp);
>  
>  	return error;
> @@ -1591,7 +1582,7 @@ xfs_fs_fill_super(
>  
>  	error = xfs_parseargs(mp, (char *)data);
>  	if (error)
> -		goto out_free_fsname;
> +		goto out_free_names;
>  
>  	sb_min_blocksize(sb, BBSIZE);
>  	sb->s_xattr = xfs_xattr_handlers;
> @@ -1618,7 +1609,7 @@ xfs_fs_fill_super(
>  
>  	error = xfs_open_devices(mp);
>  	if (error)
> -		goto out_free_fsname;
> +		goto out_free_names;
>  
>  	error = xfs_init_mount_workqueues(mp);
>  	if (error)
> @@ -1755,9 +1746,9 @@ xfs_fs_fill_super(
>  	xfs_destroy_mount_workqueues(mp);
>   out_close_devices:
>  	xfs_close_devices(mp);
> - out_free_fsname:
> + out_free_names:
>  	sb->s_fs_info = NULL;
> -	xfs_free_fsname(mp);
> +	xfs_free_names(mp);
>  	kfree(mp);
>   out:
>  	return error;
> @@ -1789,7 +1780,7 @@ xfs_fs_put_super(
>  	xfs_close_devices(mp);
>  
>  	sb->s_fs_info = NULL;
> -	xfs_free_fsname(mp);
> +	xfs_free_names(mp);
>  	kfree(mp);
>  }
>  
> diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
> index 6ccfd75d3c24..aea71ee189f5 100644
> --- a/fs/xfs/xfs_trans_ail.c
> +++ b/fs/xfs/xfs_trans_ail.c
> @@ -836,7 +836,7 @@ xfs_trans_ail_init(
>  	init_waitqueue_head(&ailp->ail_empty);
>  
>  	ailp->ail_task = kthread_run(xfsaild, ailp, "xfsaild/%s",
> -			ailp->ail_mount->m_fsname);
> +			ailp->ail_mount->m_super->s_id);
>  	if (IS_ERR(ailp->ail_task))
>  		goto out_free_ailp;
>  
> 

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v6 05/12] xfs: dont use XFS_IS_QUOTA_RUNNING() for option check
  2019-10-16  0:41 ` [PATCH v6 05/12] xfs: dont use XFS_IS_QUOTA_RUNNING() for option check Ian Kent
  2019-10-16  8:28   ` Christoph Hellwig
@ 2019-10-16 18:28   ` Darrick J. Wong
  1 sibling, 0 replies; 39+ messages in thread
From: Darrick J. Wong @ 2019-10-16 18:28 UTC (permalink / raw)
  To: Ian Kent
  Cc: linux-xfs, Brian Foster, Eric Sandeen, David Howells,
	Dave Chinner, Al Viro

On Wed, Oct 16, 2019 at 08:41:08AM +0800, Ian Kent wrote:
> When CONFIG_XFS_QUOTA is not defined any quota option is invalid.
> 
> Using the macro XFS_IS_QUOTA_RUNNING() as a check if any quota option
> has been given is a little misleading so use a simple m_qflags != 0
> check to make the intended use more explicit.
> 
> Signed-off-by: Ian Kent <raven@themaw.net>

With hch's comments addressed,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/xfs_super.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 5876c2b551b5..f8770206b66e 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -349,7 +349,7 @@ xfs_parseargs(
>  	}
>  
>  #ifndef CONFIG_XFS_QUOTA
> -	if (XFS_IS_QUOTA_RUNNING(mp)) {
> +	if (mp->m_qflags != 0) {
>  		xfs_warn(mp, "quota support not available in this kernel.");
>  		return -EINVAL;
>  	}
> 

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v6 06/12] xfs: add xfs_remount_rw() helper
  2019-10-16  0:41 ` [PATCH v6 06/12] xfs: add xfs_remount_rw() helper Ian Kent
  2019-10-16  8:30   ` Christoph Hellwig
@ 2019-10-16 18:29   ` Darrick J. Wong
  1 sibling, 0 replies; 39+ messages in thread
From: Darrick J. Wong @ 2019-10-16 18:29 UTC (permalink / raw)
  To: Ian Kent
  Cc: linux-xfs, Brian Foster, Eric Sandeen, David Howells,
	Dave Chinner, Al Viro

On Wed, Oct 16, 2019 at 08:41:13AM +0800, Ian Kent wrote:
> Factor the remount read write code into a helper to simplify the
> subsequent change from the super block method .remount_fs to the
> mount-api fs_context_operations method .reconfigure.
> 
> Signed-off-by: Ian Kent <raven@themaw.net>
> Reviewed-by: Brian Foster <bfoster@redhat.com>
> ---
>  fs/xfs/xfs_super.c |  115 +++++++++++++++++++++++++++++-----------------------
>  1 file changed, 64 insertions(+), 51 deletions(-)
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index f8770206b66e..cdc19c2af50f 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1213,6 +1213,68 @@ xfs_test_remount_options(
>  	return error;
>  }
>  
> +static int
> +xfs_remount_rw(
> +	struct xfs_mount	*mp)
> +{
> +	xfs_sb_t		*sbp = &mp->m_sb;
> +	int error;

Don't use typedefs (as hch said) and please indent all the variables,
not just some of them.

--D

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

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v6 07/12] xfs: add xfs_remount_ro() helper
  2019-10-16  0:41 ` [PATCH v6 07/12] xfs: add xfs_remount_ro() helper Ian Kent
  2019-10-16  8:31   ` Christoph Hellwig
@ 2019-10-16 18:29   ` Darrick J. Wong
  1 sibling, 0 replies; 39+ messages in thread
From: Darrick J. Wong @ 2019-10-16 18:29 UTC (permalink / raw)
  To: Ian Kent
  Cc: linux-xfs, Brian Foster, Eric Sandeen, David Howells,
	Dave Chinner, Al Viro

On Wed, Oct 16, 2019 at 08:41:22AM +0800, Ian Kent wrote:
> Factor the remount read only code into a helper to simplify the
> subsequent change from the super block method .remount_fs to the
> mount-api fs_context_operations method .reconfigure.
> 
> Signed-off-by: Ian Kent <raven@themaw.net>
> Reviewed-by: Brian Foster <bfoster@redhat.com>
> ---
>  fs/xfs/xfs_super.c |   73 +++++++++++++++++++++++++++++++---------------------
>  1 file changed, 43 insertions(+), 30 deletions(-)
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index cdc19c2af50f..1f706cbf9624 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1275,6 +1275,47 @@ xfs_remount_rw(
>  	return 0;
>  }
>  
> +static int
> +xfs_remount_ro(
> +	struct xfs_mount	*mp)
> +{
> +	int error;

Please indent the variable consistently with the parameter list.
Otherwise looks ok.

--D

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

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v6 10/12] xfs: move xfs_parseargs() validation to a helper
  2019-10-16  8:40   ` Christoph Hellwig
@ 2019-10-17  0:58     ` Ian Kent
  2019-10-17  6:48       ` Christoph Hellwig
  2019-10-23  2:59     ` Ian Kent
  1 sibling, 1 reply; 39+ messages in thread
From: Ian Kent @ 2019-10-17  0:58 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: linux-xfs, Brian Foster, Eric Sandeen, David Howells,
	Dave Chinner, Al Viro

On Wed, 2019-10-16 at 01:40 -0700, Christoph Hellwig wrote:
> On Wed, Oct 16, 2019 at 08:41:38AM +0800, Ian Kent wrote:
> > +static int
> > +xfs_validate_params(
> > +	struct xfs_mount        *mp,
> > +	int			dsunit,
> > +	int			dswidth,
> > +	uint8_t			iosizelog,
> > +	bool			nooptions)
> 
> Please add a refactor patch before this one to always set the stripe
> unit / width / log I/O size values in the mount structure at parsing
> time as suggested before.  The will also remove the need for the
> xfs_fs_context structure in the last patch.
> 
> > +	if (nooptions)
> > +		goto noopts;
> 
> This option is always false in this patch.  I'd suggest you only add
> it once we actually add a user.

Thanks for looking at the series and for the comments.
I'll get on with the recommended changes and post an update.

Ian


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v6 12/12] xfs: switch to use the new mount-api
  2019-10-16 18:18   ` Christoph Hellwig
@ 2019-10-17  1:13     ` Ian Kent
  2019-10-17  4:53       ` Darrick J. Wong
  2019-10-23  3:17     ` Ian Kent
  1 sibling, 1 reply; 39+ messages in thread
From: Ian Kent @ 2019-10-17  1:13 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: linux-xfs, Brian Foster, Eric Sandeen, David Howells,
	Dave Chinner, Al Viro

On Wed, 2019-10-16 at 11:18 -0700, Christoph Hellwig wrote:
> On Wed, Oct 16, 2019 at 08:41:48AM +0800, Ian Kent wrote:
> > +static const struct fs_parameter_description xfs_fs_parameters = {
> > +	.name		= "XFS",
> > +	.specs		= xfs_param_specs,
> > +};
> 
> Well spell xfs in lower case in the file system type, so I think we
> should
> spell it the same here.

The problem is that this will probably be used in logging later and
there's a lot of logging that uses the upper case variant.

OTOH if all the log messages were changed to use lower case "xfs" then
one of the problems I see with logging (that name inconsistency) would
go away.

So I'm not sure what I should do here.

> 
> Btw, can we keep all the mount code together where most of it already
> is at the top of the file?  I know the existing version has some
> remount
> stuff at the bottom, but as that get entirely rewritten we might as
> well
> move it all up.

Yep, sounds good.

> 
> > +	int			silent = fc->sb_flags & SB_SILENT;
> 
> The silent variable is only used once, so we might as well remove it.

And again.

> 
> > +	struct xfs_mount	*mp = fc->s_fs_info;
> > +
> > +	/*
> > +	 * mp and ctx are stored in the fs_context when it is
> > +	 * initialized. mp is transferred to the superblock on
> > +	 * a successful mount, but if an error occurs before the
> > +	 * transfer we have to free it here.
> > +	 */
> > +	if (mp) {
> > +		xfs_free_names(mp);
> > +		kfree(mp);
> > +	}
> 
> We always pair xfs_free_names with freeing the mount structure.
> I think it would be nice to add an xfs_free_mount that does both
> as a refactoring at the beginning of the series. 

Ditto.

> 
> > +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,
> > +};
> 
> Should these all get a prefix like xfs_fc_free?  Maybe xfs_fsctx
> to be a little bit more descriptive?

Good point, since it's struct fs_context* I think an "xfs_fc_"
prefix on the context related structures and variables would make
the most sense.

I'll do that too.
Ian


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v6 12/12] xfs: switch to use the new mount-api
  2019-10-17  1:13     ` Ian Kent
@ 2019-10-17  4:53       ` Darrick J. Wong
  2019-10-17  6:51         ` Christoph Hellwig
  0 siblings, 1 reply; 39+ messages in thread
From: Darrick J. Wong @ 2019-10-17  4:53 UTC (permalink / raw)
  To: Ian Kent
  Cc: Christoph Hellwig, linux-xfs, Brian Foster, Eric Sandeen,
	David Howells, Dave Chinner, Al Viro

On Thu, Oct 17, 2019 at 09:13:29AM +0800, Ian Kent wrote:
> On Wed, 2019-10-16 at 11:18 -0700, Christoph Hellwig wrote:
> > On Wed, Oct 16, 2019 at 08:41:48AM +0800, Ian Kent wrote:
> > > +static const struct fs_parameter_description xfs_fs_parameters = {
> > > +	.name		= "XFS",
> > > +	.specs		= xfs_param_specs,
> > > +};
> > 
> > Well spell xfs in lower case in the file system type, so I think we
> > should
> > spell it the same here.
> 
> The problem is that this will probably be used in logging later and
> there's a lot of logging that uses the upper case variant.
> 
> OTOH if all the log messages were changed to use lower case "xfs" then
> one of the problems I see with logging (that name inconsistency) would
> go away.
> 
> So I'm not sure what I should do here.

I would just leave it 'XFS' for consistency, but I might be in the back
pocket of Big Letter. ;)

--D

> > 
> > Btw, can we keep all the mount code together where most of it already
> > is at the top of the file?  I know the existing version has some
> > remount
> > stuff at the bottom, but as that get entirely rewritten we might as
> > well
> > move it all up.
> 
> Yep, sounds good.
> 
> > 
> > > +	int			silent = fc->sb_flags & SB_SILENT;
> > 
> > The silent variable is only used once, so we might as well remove it.
> 
> And again.
> 
> > 
> > > +	struct xfs_mount	*mp = fc->s_fs_info;
> > > +
> > > +	/*
> > > +	 * mp and ctx are stored in the fs_context when it is
> > > +	 * initialized. mp is transferred to the superblock on
> > > +	 * a successful mount, but if an error occurs before the
> > > +	 * transfer we have to free it here.
> > > +	 */
> > > +	if (mp) {
> > > +		xfs_free_names(mp);
> > > +		kfree(mp);
> > > +	}
> > 
> > We always pair xfs_free_names with freeing the mount structure.
> > I think it would be nice to add an xfs_free_mount that does both
> > as a refactoring at the beginning of the series. 
> 
> Ditto.
> 
> > 
> > > +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,
> > > +};
> > 
> > Should these all get a prefix like xfs_fc_free?  Maybe xfs_fsctx
> > to be a little bit more descriptive?
> 
> Good point, since it's struct fs_context* I think an "xfs_fc_"
> prefix on the context related structures and variables would make
> the most sense.
> 
> I'll do that too.
> Ian
> 

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v6 10/12] xfs: move xfs_parseargs() validation to a helper
  2019-10-17  0:58     ` Ian Kent
@ 2019-10-17  6:48       ` Christoph Hellwig
  0 siblings, 0 replies; 39+ messages in thread
From: Christoph Hellwig @ 2019-10-17  6:48 UTC (permalink / raw)
  To: Ian Kent
  Cc: Christoph Hellwig, linux-xfs, Brian Foster, Eric Sandeen,
	David Howells, Dave Chinner, Al Viro

On Thu, Oct 17, 2019 at 08:58:26AM +0800, Ian Kent wrote:
> > > +	if (nooptions)
> > > +		goto noopts;
> > 
> > This option is always false in this patch.  I'd suggest you only add
> > it once we actually add a user.
> 
> Thanks for looking at the series and for the comments.
> I'll get on with the recommended changes and post an update.

And for this one I'm not sure the goto option actually ever makes sense,
even with the final patch applied.  To me it looks like splitting the
function into two might be much cleaner.

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v6 12/12] xfs: switch to use the new mount-api
  2019-10-17  4:53       ` Darrick J. Wong
@ 2019-10-17  6:51         ` Christoph Hellwig
  2019-10-18  0:38           ` Ian Kent
  0 siblings, 1 reply; 39+ messages in thread
From: Christoph Hellwig @ 2019-10-17  6:51 UTC (permalink / raw)
  To: Darrick J. Wong
  Cc: Ian Kent, Christoph Hellwig, linux-xfs, Brian Foster,
	Eric Sandeen, David Howells, Dave Chinner, Al Viro

On Wed, Oct 16, 2019 at 09:53:30PM -0700, Darrick J. Wong wrote:
> > The problem is that this will probably be used in logging later and
> > there's a lot of logging that uses the upper case variant.
> > 
> > OTOH if all the log messages were changed to use lower case "xfs" then
> > one of the problems I see with logging (that name inconsistency) would
> > go away.
> > 
> > So I'm not sure what I should do here.
> 
> I would just leave it 'XFS' for consistency, but I might be in the back
> pocket of Big Letter. ;)

I isn't really used for much, and Al already has a patch from Eric in on
of his trees that kills the field in favor of the file_system_type name
field:

https://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git/commit/?h=work.mount-parser-later&id=543fdf1d617edd6d681fcc50e16478e832a7a2ac

So we should not spell them differently if we can.

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v6 12/12] xfs: switch to use the new mount-api
  2019-10-17  6:51         ` Christoph Hellwig
@ 2019-10-18  0:38           ` Ian Kent
  0 siblings, 0 replies; 39+ messages in thread
From: Ian Kent @ 2019-10-18  0:38 UTC (permalink / raw)
  To: Christoph Hellwig, Darrick J. Wong
  Cc: linux-xfs, Brian Foster, Eric Sandeen, David Howells,
	Dave Chinner, Al Viro

On Wed, 2019-10-16 at 23:51 -0700, Christoph Hellwig wrote:
> On Wed, Oct 16, 2019 at 09:53:30PM -0700, Darrick J. Wong wrote:
> > > The problem is that this will probably be used in logging later
> > > and
> > > there's a lot of logging that uses the upper case variant.
> > > 
> > > OTOH if all the log messages were changed to use lower case "xfs"
> > > then
> > > one of the problems I see with logging (that name inconsistency)
> > > would
> > > go away.
> > > 
> > > So I'm not sure what I should do here.
> > 
> > I would just leave it 'XFS' for consistency, but I might be in the
> > back
> > pocket of Big Letter. ;)
> 
> I isn't really used for much, and Al already has a patch from Eric in
> on
> of his trees that kills the field in favor of the file_system_type
> name
> field:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git/commit/?h=work.mount-parser-later&id=543fdf1d617edd6d681fcc50e16478e832a7a2ac

Yes, I was thinking about that when I replied.

> 
> So we should not spell them differently if we can.

I don't think it will make much difference in the long run, I'll just
change it to lower case for the time being.

I think the implications of the mount api on logging need to be
discussed further (see my thoughts in the series cover letter), there's
not just a log entry case issue. I plan on making some RFC patches for
this later.

Since I'm doing the mount api change for xfs I'll base my RFC patches
on xfs and they will need to go to a wider audience (fsdevel as well
I expect).

Ian


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v6 10/12] xfs: move xfs_parseargs() validation to a helper
  2019-10-16  8:40   ` Christoph Hellwig
  2019-10-17  0:58     ` Ian Kent
@ 2019-10-23  2:59     ` Ian Kent
  1 sibling, 0 replies; 39+ messages in thread
From: Ian Kent @ 2019-10-23  2:59 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: linux-xfs, Brian Foster, Eric Sandeen, David Howells,
	Dave Chinner, Al Viro

On Wed, 2019-10-16 at 01:40 -0700, Christoph Hellwig wrote:
> On Wed, Oct 16, 2019 at 08:41:38AM +0800, Ian Kent wrote:
> > +static int
> > +xfs_validate_params(
> > +	struct xfs_mount        *mp,
> > +	int			dsunit,
> > +	int			dswidth,
> > +	uint8_t			iosizelog,
> > +	bool			nooptions)
> 
> Please add a refactor patch before this one to always set the stripe
> unit / width / log I/O size values in the mount structure at parsing
> time as suggested before.  The will also remove the need for the
> xfs_fs_context structure in the last patch.

With the mount api it looks like this means adding the validation
checks to xfs_parse_param().

That could be done for iosizelog without problem other than the check
being redone for each instance of the "allocsize" option which is
probably ok.

But the dsunit and dswidth checks depend on one another so that check
needs to be done after all the options have been handled.

After the change to use the mount api the options string separation
is done in the VFS and xfs_parse_param() called separately for each
option.

This is done so that the new system call fsconfig() can be implemented
which will call the .parse_param() method separately for each option
too.

So it isn't known if both "sunit" and "swidth" have been given until
aft
er all the options have been looked at.

Ian


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH v6 12/12] xfs: switch to use the new mount-api
  2019-10-16 18:18   ` Christoph Hellwig
  2019-10-17  1:13     ` Ian Kent
@ 2019-10-23  3:17     ` Ian Kent
  1 sibling, 0 replies; 39+ messages in thread
From: Ian Kent @ 2019-10-23  3:17 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: linux-xfs, Brian Foster, Eric Sandeen, David Howells,
	Dave Chinner, Al Viro

On Wed, 2019-10-16 at 11:18 -0700, Christoph Hellwig wrote:
> 
> Btw, can we keep all the mount code together where most of it already
> is at the top of the file?  I know the existing version has some
> remount
> stuff at the bottom, but as that get entirely rewritten we might as
> well
> move it all up.

I like the recommendation to bring all the mounting code together.

I think that would mean also moving xfs_fs_fill_super() and that
would mean adding several forward declarations because the several
functions called by xfs_fs_fill_super() appear to have reasonably
sensible ordering already.

Personally I think adding the forward declarations is worthwhile
to get improved code order.

So I'll do that but thought I should mention it in case anyone
has a different POV.

Ian


^ permalink raw reply	[flat|nested] 39+ messages in thread

end of thread, other threads:[~2019-10-23  3:17 UTC | newest]

Thread overview: 39+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-16  0:40 [PATCH v6 00/12] xfs: mount API patch series Ian Kent
2019-10-16  0:40 ` [PATCH v6 01/12] vfs: add missing blkdev_put() in get_tree_bdev() Ian Kent
2019-10-16  0:40 ` [PATCH v6 02/12] xfs: remove very old mount option Ian Kent
2019-10-16  8:26   ` Christoph Hellwig
2019-10-16 18:25   ` Darrick J. Wong
2019-10-16  0:40 ` [PATCH v6 03/12] xfs: remove unused mount info field m_fsname_len Ian Kent
2019-10-16  8:26   ` Christoph Hellwig
2019-10-16 18:26   ` Darrick J. Wong
2019-10-16  0:41 ` [PATCH v6 04/12] xfs: use super s_id instead of mount info m_fsname Ian Kent
2019-10-16  8:27   ` Christoph Hellwig
2019-10-16 18:27   ` Darrick J. Wong
2019-10-16  0:41 ` [PATCH v6 05/12] xfs: dont use XFS_IS_QUOTA_RUNNING() for option check Ian Kent
2019-10-16  8:28   ` Christoph Hellwig
2019-10-16 18:28   ` Darrick J. Wong
2019-10-16  0:41 ` [PATCH v6 06/12] xfs: add xfs_remount_rw() helper Ian Kent
2019-10-16  8:30   ` Christoph Hellwig
2019-10-16 18:29   ` Darrick J. Wong
2019-10-16  0:41 ` [PATCH v6 07/12] xfs: add xfs_remount_ro() helper Ian Kent
2019-10-16  8:31   ` Christoph Hellwig
2019-10-16 18:29   ` Darrick J. Wong
2019-10-16  0:41 ` [PATCH v6 08/12] xfs: refactor suffix_kstrtoint() Ian Kent
2019-10-16  8:34   ` Christoph Hellwig
2019-10-16 15:37     ` Darrick J. Wong
2019-10-16  0:41 ` [PATCH v6 09/12] xfs: refactor xfs_parseags() Ian Kent
2019-10-16  8:36   ` Christoph Hellwig
2019-10-16  0:41 ` [PATCH v6 10/12] xfs: move xfs_parseargs() validation to a helper Ian Kent
2019-10-16  8:40   ` Christoph Hellwig
2019-10-17  0:58     ` Ian Kent
2019-10-17  6:48       ` Christoph Hellwig
2019-10-23  2:59     ` Ian Kent
2019-10-16  0:41 ` [PATCH v6 11/12] xfs: dont set sb in xfs_mount_alloc() Ian Kent
2019-10-16  8:36   ` Christoph Hellwig
2019-10-16  0:41 ` [PATCH v6 12/12] xfs: switch to use the new mount-api Ian Kent
2019-10-16 18:18   ` Christoph Hellwig
2019-10-17  1:13     ` Ian Kent
2019-10-17  4:53       ` Darrick J. Wong
2019-10-17  6:51         ` Christoph Hellwig
2019-10-18  0:38           ` Ian Kent
2019-10-23  3:17     ` Ian Kent

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.