s_fs_info and ->kill_sb revisited

s_fs_info and ->kill_sb revisited

[Christoph Hellwig]

Hi all,

this series is against the VFS vfs.super branch does two slightly
related things:

 - move closing of the external devices in ext4 and xfs from ->put_super
   into ->kill_sb so that this isn't done under s_umount which creates
   lock ordere reversal
 - move freeing the private dta in s_fs_info into ->kill_sb for file systems
   that pass it in through the fs_context, as otherwise we could leak it
   before fill_super is called (this is something new on the vfs.super
   branch because of the changed place where blkdev_get is called)

Diffstat:
 exfat/exfat_fs.h |    2 -
 exfat/super.c    |   39 +++++++++++++-------------
 ext4/super.c     |   50 +++++++++++++++++-----------------
 ntfs3/super.c    |   33 ++++++++++------------
 xfs/xfs_buf.c    |    7 +++-
 xfs/xfs_super.c  |   80 +++++++++++++++++++++++++------------------------------
 6 files changed, 102 insertions(+), 109 deletions(-)

[PATCH 01/13] xfs: reformat the xfs_fs_free prototype

[Christoph Hellwig]

The xfs_fs_free prototype formatting is a weird mix of the classic XFS
style and the Linux style.  Fix it up to be consistent.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_super.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index bf6e0a261a49e6..0a294659c18972 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1933,7 +1933,8 @@ xfs_fs_reconfigure(
 	return 0;
 }
 
-static void xfs_fs_free(
+static void
+xfs_fs_free(
 	struct fs_context	*fc)
 {
 	struct xfs_mount	*mp = fc->s_fs_info;
-- 
2.39.2

[PATCH 02/13] xfs: remove a superflous s_fs_info NULL check in xfs_fs_put_super

[Christoph Hellwig]

->put_super is only called when sb->s_root is set, and thus when
fill_super succeeds.  Thus drop the NULL check that can't happen in
xfs_fs_put_super.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_super.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 0a294659c18972..128f4a2924d49c 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1132,10 +1132,6 @@ xfs_fs_put_super(
 {
 	struct xfs_mount	*mp = XFS_M(sb);
 
-	/* if ->fill_super failed, we have no mount to tear down */
-	if (!sb->s_fs_info)
-		return;
-
 	xfs_notice(mp, "Unmounting Filesystem %pU", &mp->m_sb.sb_uuid);
 	xfs_filestream_unmount(mp);
 	xfs_unmountfs(mp);
-- 
2.39.2

[PATCH 03/13] xfs: free the mount in ->kill_sb

[Christoph Hellwig]

As a rule of thumb everything allocated to the fs_context and moved into
the super_block should be freed by ->kill_sb so that the teardown
handling doesn't need to be duplicated between the fill_super error
path and put_super.  Implement a XFS-specific kill_sb method to do that.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_super.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 128f4a2924d49c..d2f3ae6ba8938b 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1143,9 +1143,6 @@ xfs_fs_put_super(
 	xfs_destroy_percpu_counters(mp);
 	xfs_destroy_mount_workqueues(mp);
 	xfs_close_devices(mp);
-
-	sb->s_fs_info = NULL;
-	xfs_mount_free(mp);
 }
 
 static long
@@ -1487,7 +1484,7 @@ xfs_fs_fill_super(
 
 	error = xfs_fs_validate_params(mp);
 	if (error)
-		goto out_free_names;
+		return error;
 
 	sb_min_blocksize(sb, BBSIZE);
 	sb->s_xattr = xfs_xattr_handlers;
@@ -1514,7 +1511,7 @@ xfs_fs_fill_super(
 
 	error = xfs_open_devices(mp);
 	if (error)
-		goto out_free_names;
+		return error;
 
 	error = xfs_init_mount_workqueues(mp);
 	if (error)
@@ -1734,9 +1731,6 @@ xfs_fs_fill_super(
 	xfs_destroy_mount_workqueues(mp);
  out_close_devices:
 	xfs_close_devices(mp);
- out_free_names:
-	sb->s_fs_info = NULL;
-	xfs_mount_free(mp);
 	return error;
 
  out_unmount:
@@ -1999,12 +1993,20 @@ static int xfs_init_fs_context(
 	return 0;
 }
 
+static void
+xfs_kill_sb(
+	struct super_block		*sb)
+{
+	kill_block_super(sb);
+	xfs_mount_free(XFS_M(sb));
+}
+
 static struct file_system_type xfs_fs_type = {
 	.owner			= THIS_MODULE,
 	.name			= "xfs",
 	.init_fs_context	= xfs_init_fs_context,
 	.parameters		= xfs_fs_parameters,
-	.kill_sb		= kill_block_super,
+	.kill_sb		= xfs_kill_sb,
 	.fs_flags		= FS_REQUIRES_DEV | FS_ALLOW_IDMAP,
 };
 MODULE_ALIAS_FS("xfs");
-- 
2.39.2

[PATCH 04/13] xfs: remove xfs_blkdev_put

[Christoph Hellwig]

There isn't much use for this trivial wrapper, especially as the NULL
check is only needed in a single call site.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_super.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index d2f3ae6ba8938b..f00d1162815d19 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -395,15 +395,6 @@ xfs_blkdev_get(
 	return error;
 }
 
-STATIC void
-xfs_blkdev_put(
-	struct xfs_mount	*mp,
-	struct block_device	*bdev)
-{
-	if (bdev)
-		blkdev_put(bdev, mp->m_super);
-}
-
 STATIC void
 xfs_close_devices(
 	struct xfs_mount	*mp)
@@ -412,13 +403,13 @@ xfs_close_devices(
 		struct block_device *logdev = mp->m_logdev_targp->bt_bdev;
 
 		xfs_free_buftarg(mp->m_logdev_targp);
-		xfs_blkdev_put(mp, logdev);
+		blkdev_put(logdev, mp->m_super);
 	}
 	if (mp->m_rtdev_targp) {
 		struct block_device *rtdev = mp->m_rtdev_targp->bt_bdev;
 
 		xfs_free_buftarg(mp->m_rtdev_targp);
-		xfs_blkdev_put(mp, rtdev);
+		blkdev_put(rtdev, mp->m_super);
 	}
 	xfs_free_buftarg(mp->m_ddev_targp);
 }
@@ -503,10 +494,11 @@ xfs_open_devices(
  out_free_ddev_targ:
 	xfs_free_buftarg(mp->m_ddev_targp);
  out_close_rtdev:
-	xfs_blkdev_put(mp, rtdev);
+ 	if (rtdev)
+		blkdev_put(rtdev, sb);
  out_close_logdev:
 	if (logdev && logdev != ddev)
-		xfs_blkdev_put(mp, logdev);
+		blkdev_put(logdev, sb);
 	goto out_relock;
 }
 
-- 
2.39.2

[PATCH 05/13] xfs: don't call invalidate_bdev in xfs_free_buftarg

[Christoph Hellwig]

XFS never uses the block device mapping, so there is no point in calling
invalidate_bdev which invalidates said mapping.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_buf.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index 15d1e5a7c2d340..83b8702030f71d 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -1944,7 +1944,6 @@ xfs_free_buftarg(
 	list_lru_destroy(&btp->bt_lru);
 
 	blkdev_issue_flush(btp->bt_bdev);
-	invalidate_bdev(btp->bt_bdev);
 	fs_put_dax(btp->bt_daxdev, btp->bt_mount);
 
 	kmem_free(btp);
-- 
2.39.2

[PATCH 06/13] xfs: close the RT and log block devices in xfs_free_buftarg

[Christoph Hellwig]

Closing the block devices logically belongs into xfs_free_buftarg,  So instead
of open coding it in the caller move it there and add a check for the s_bdev
so that the main device isn't close as that's done by the VFS helper.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_buf.c   |  5 +++++
 fs/xfs/xfs_super.c | 12 ++----------
 2 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index 83b8702030f71d..c57e6e03dfa80c 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -1938,6 +1938,8 @@ void
 xfs_free_buftarg(
 	struct xfs_buftarg	*btp)
 {
+	struct block_device	*bdev = btp->bt_bdev;
+
 	unregister_shrinker(&btp->bt_shrinker);
 	ASSERT(percpu_counter_sum(&btp->bt_io_count) == 0);
 	percpu_counter_destroy(&btp->bt_io_count);
@@ -1945,6 +1947,9 @@ xfs_free_buftarg(
 
 	blkdev_issue_flush(btp->bt_bdev);
 	fs_put_dax(btp->bt_daxdev, btp->bt_mount);
+	/* the main block device is closed by kill_block_super */
+	if (bdev != btp->bt_mount->m_super->s_bdev)
+		blkdev_put(bdev, btp->bt_mount->m_super);
 
 	kmem_free(btp);
 }
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index f00d1162815d19..37b1b763a0bef0 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -399,18 +399,10 @@ STATIC void
 xfs_close_devices(
 	struct xfs_mount	*mp)
 {
-	if (mp->m_logdev_targp && mp->m_logdev_targp != mp->m_ddev_targp) {
-		struct block_device *logdev = mp->m_logdev_targp->bt_bdev;
-
+	if (mp->m_logdev_targp && mp->m_logdev_targp != mp->m_ddev_targp)
 		xfs_free_buftarg(mp->m_logdev_targp);
-		blkdev_put(logdev, mp->m_super);
-	}
-	if (mp->m_rtdev_targp) {
-		struct block_device *rtdev = mp->m_rtdev_targp->bt_bdev;
-
+	if (mp->m_rtdev_targp)
 		xfs_free_buftarg(mp->m_rtdev_targp);
-		blkdev_put(rtdev, mp->m_super);
-	}
 	xfs_free_buftarg(mp->m_ddev_targp);
 }
 
-- 
2.39.2

[PATCH 07/13] xfs: close the external block devices in xfs_mount_free

[Christoph Hellwig]

blkdev_put must not be called under sb->s_umount to avoid a lock order
reversal with disk->open_mutex.  Move closing the buftargs into ->kill_sb
to archive that.  Note that the flushing of the disk caches needs to be
kept in ->put_super as the main block device is closed in
kill_block_super already.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_buf.c   |  1 -
 fs/xfs/xfs_super.c | 27 +++++++++++++++++++--------
 2 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index c57e6e03dfa80c..3b903f6bce98d8 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -1945,7 +1945,6 @@ xfs_free_buftarg(
 	percpu_counter_destroy(&btp->bt_io_count);
 	list_lru_destroy(&btp->bt_lru);
 
-	blkdev_issue_flush(btp->bt_bdev);
 	fs_put_dax(btp->bt_daxdev, btp->bt_mount);
 	/* the main block device is closed by kill_block_super */
 	if (bdev != btp->bt_mount->m_super->s_bdev)
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 37b1b763a0bef0..67343364ac66e9 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -396,14 +396,14 @@ xfs_blkdev_get(
 }
 
 STATIC void
-xfs_close_devices(
+xfs_flush_disk_caches(
 	struct xfs_mount	*mp)
 {
 	if (mp->m_logdev_targp && mp->m_logdev_targp != mp->m_ddev_targp)
-		xfs_free_buftarg(mp->m_logdev_targp);
+		blkdev_issue_flush(mp->m_logdev_targp->bt_bdev);
 	if (mp->m_rtdev_targp)
-		xfs_free_buftarg(mp->m_rtdev_targp);
-	xfs_free_buftarg(mp->m_ddev_targp);
+		blkdev_issue_flush(mp->m_rtdev_targp->bt_bdev);
+	blkdev_issue_flush(mp->m_ddev_targp->bt_bdev);
 }
 
 /*
@@ -741,6 +741,17 @@ static void
 xfs_mount_free(
 	struct xfs_mount	*mp)
 {
+	/*
+	 * Free the buftargs here because blkdev_put needs to be called outside
+	 * of sb->s_umount, which is held around the call to ->put_super.
+	 */
+	if (mp->m_logdev_targp && mp->m_logdev_targp != mp->m_ddev_targp)
+		xfs_free_buftarg(mp->m_logdev_targp);
+	if (mp->m_rtdev_targp)
+		xfs_free_buftarg(mp->m_rtdev_targp);
+	if (mp->m_ddev_targp)
+		xfs_free_buftarg(mp->m_ddev_targp);
+
 	kfree(mp->m_rtname);
 	kfree(mp->m_logname);
 	kmem_free(mp);
@@ -1126,7 +1137,7 @@ xfs_fs_put_super(
 	xfs_inodegc_free_percpu(mp);
 	xfs_destroy_percpu_counters(mp);
 	xfs_destroy_mount_workqueues(mp);
-	xfs_close_devices(mp);
+	xfs_flush_disk_caches(mp);
 }
 
 static long
@@ -1499,7 +1510,7 @@ xfs_fs_fill_super(
 
 	error = xfs_init_mount_workqueues(mp);
 	if (error)
-		goto out_close_devices;
+		goto out_flush_caches;
 
 	error = xfs_init_percpu_counters(mp);
 	if (error)
@@ -1713,8 +1724,8 @@ xfs_fs_fill_super(
 	xfs_destroy_percpu_counters(mp);
  out_destroy_workqueues:
 	xfs_destroy_mount_workqueues(mp);
- out_close_devices:
-	xfs_close_devices(mp);
+ out_flush_caches:
+	xfs_flush_disk_caches(mp);
 	return error;
 
  out_unmount:
-- 
2.39.2

[PATCH 08/13] ext4: close the external journal device in ->kill_sb

[Christoph Hellwig]

blkdev_put must not be called under sb->s_umount to avoid a lock order
reversal with disk->open_mutex.  Move closing the external journal device
into ->kill_sb to archive that.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/ext4/super.c | 50 ++++++++++++++++++++++++-------------------------
 1 file changed, 25 insertions(+), 25 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 063832e2d12a8e..0511fffb59b40d 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -93,6 +93,7 @@ static int ext4_get_tree(struct fs_context *fc);
 static int ext4_reconfigure(struct fs_context *fc);
 static void ext4_fc_free(struct fs_context *fc);
 static int ext4_init_fs_context(struct fs_context *fc);
+static void ext4_kill_sb(struct super_block *sb);
 static const struct fs_parameter_spec ext4_param_specs[];
 
 /*
@@ -135,7 +136,7 @@ static struct file_system_type ext2_fs_type = {
 	.name			= "ext2",
 	.init_fs_context	= ext4_init_fs_context,
 	.parameters		= ext4_param_specs,
-	.kill_sb		= kill_block_super,
+	.kill_sb		= ext4_kill_sb,
 	.fs_flags		= FS_REQUIRES_DEV,
 };
 MODULE_ALIAS_FS("ext2");
@@ -151,7 +152,7 @@ static struct file_system_type ext3_fs_type = {
 	.name			= "ext3",
 	.init_fs_context	= ext4_init_fs_context,
 	.parameters		= ext4_param_specs,
-	.kill_sb		= kill_block_super,
+	.kill_sb		= ext4_kill_sb,
 	.fs_flags		= FS_REQUIRES_DEV,
 };
 MODULE_ALIAS_FS("ext3");
@@ -1116,25 +1117,6 @@ static struct block_device *ext4_blkdev_get(dev_t dev, struct super_block *sb)
 	return NULL;
 }
 
-/*
- * Release the journal device
- */
-static void ext4_blkdev_remove(struct ext4_sb_info *sbi)
-{
-	struct block_device *bdev;
-	bdev = sbi->s_journal_bdev;
-	if (bdev) {
-		/*
-		 * Invalidate the journal device's buffers.  We don't want them
-		 * floating about in memory - the physical journal device may
-		 * hotswapped, and it breaks the `ro-after' testing code.
-		 */
-		invalidate_bdev(bdev);
-		blkdev_put(bdev, sbi->s_sb);
-		sbi->s_journal_bdev = NULL;
-	}
-}
-
 static inline struct inode *orphan_list_entry(struct list_head *l)
 {
 	return &list_entry(l, struct ext4_inode_info, i_orphan)->vfs_inode;
@@ -1330,8 +1312,13 @@ static void ext4_put_super(struct super_block *sb)
 	sync_blockdev(sb->s_bdev);
 	invalidate_bdev(sb->s_bdev);
 	if (sbi->s_journal_bdev) {
+		/*
+		 * Invalidate the journal device's buffers.  We don't want them
+		 * floating about in memory - the physical journal device may
+		 * hotswapped, and it breaks the `ro-after' testing code.
+		 */
 		sync_blockdev(sbi->s_journal_bdev);
-		ext4_blkdev_remove(sbi);
+		invalidate_bdev(sbi->s_journal_bdev);
 	}
 
 	ext4_xattr_destroy_cache(sbi->s_ea_inode_cache);
@@ -5655,9 +5642,11 @@ failed_mount9: __maybe_unused
 		kfree(get_qf_name(sb, sbi, i));
 #endif
 	fscrypt_free_dummy_policy(&sbi->s_dummy_enc_policy);
-	/* ext4_blkdev_remove() calls kill_bdev(), release bh before it. */
 	brelse(sbi->s_sbh);
-	ext4_blkdev_remove(sbi);
+	if (sbi->s_journal_bdev) {
+		invalidate_bdev(sbi->s_journal_bdev);
+		blkdev_put(sbi->s_journal_bdev, sb);
+	}
 out_fail:
 	invalidate_bdev(sb->s_bdev);
 	sb->s_fs_info = NULL;
@@ -7267,12 +7256,23 @@ static inline int ext3_feature_set_ok(struct super_block *sb)
 	return 1;
 }
 
+static void ext4_kill_sb(struct super_block *sb)
+{
+	struct ext4_sb_info *sbi = EXT4_SB(sb);
+	struct block_device *journal_bdev = sbi ? sbi->s_journal_bdev : NULL;
+
+	kill_block_super(sb);
+
+	if (journal_bdev)
+		blkdev_put(journal_bdev, sb);
+}
+
 static struct file_system_type ext4_fs_type = {
 	.owner			= THIS_MODULE,
 	.name			= "ext4",
 	.init_fs_context	= ext4_init_fs_context,
 	.parameters		= ext4_param_specs,
-	.kill_sb		= kill_block_super,
+	.kill_sb		= ext4_kill_sb,
 	.fs_flags		= FS_REQUIRES_DEV | FS_ALLOW_IDMAP,
 };
 MODULE_ALIAS_FS("ext4");
-- 
2.39.2

[PATCH 09/13] exfat: don't RCU-free the sbi

[Christoph Hellwig]

There are no RCU critical sections for accessing any information in the
sbi, so drop the call_rcu indirection for freeing the sbi.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/exfat/exfat_fs.h |  2 --
 fs/exfat/super.c    | 15 ++++-----------
 2 files changed, 4 insertions(+), 13 deletions(-)

diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h
index 729ada9e26e82e..f55498e5c23d46 100644
--- a/fs/exfat/exfat_fs.h
+++ b/fs/exfat/exfat_fs.h
@@ -273,8 +273,6 @@ struct exfat_sb_info {
 
 	spinlock_t inode_hash_lock;
 	struct hlist_head inode_hashtable[EXFAT_HASH_SIZE];
-
-	struct rcu_head rcu;
 };
 
 #define EXFAT_CACHE_VALID	0
diff --git a/fs/exfat/super.c b/fs/exfat/super.c
index 8c32460e031e80..3c6aec96d0dc85 100644
--- a/fs/exfat/super.c
+++ b/fs/exfat/super.c
@@ -31,16 +31,6 @@ static void exfat_free_iocharset(struct exfat_sb_info *sbi)
 		kfree(sbi->options.iocharset);
 }
 
-static void exfat_delayed_free(struct rcu_head *p)
-{
-	struct exfat_sb_info *sbi = container_of(p, struct exfat_sb_info, rcu);
-
-	unload_nls(sbi->nls_io);
-	exfat_free_iocharset(sbi);
-	exfat_free_upcase_table(sbi);
-	kfree(sbi);
-}
-
 static void exfat_put_super(struct super_block *sb)
 {
 	struct exfat_sb_info *sbi = EXFAT_SB(sb);
@@ -50,7 +40,10 @@ static void exfat_put_super(struct super_block *sb)
 	brelse(sbi->boot_bh);
 	mutex_unlock(&sbi->s_lock);
 
-	call_rcu(&sbi->rcu, exfat_delayed_free);
+	unload_nls(sbi->nls_io);
+	exfat_free_iocharset(sbi);
+	exfat_free_upcase_table(sbi);
+	kfree(sbi);
 }
 
 static int exfat_sync_fs(struct super_block *sb, int wait)
-- 
2.39.2

[PATCH 10/13] exfat: free the sbi and iocharset in ->kill_sb

[Christoph Hellwig]

As a rule of thumb everything allocated to the fs_context and moved into
the super_block should be freed by ->kill_sb so that the teardown
handling doesn't need to be duplicated between the fill_super error
path and put_super.  Implement an exfat-specific kill_sb method to do
that and share the code with the mount contex free helper for the
mount error handling case.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/exfat/super.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/fs/exfat/super.c b/fs/exfat/super.c
index 3c6aec96d0dc85..85b04a4064af1e 100644
--- a/fs/exfat/super.c
+++ b/fs/exfat/super.c
@@ -41,9 +41,7 @@ static void exfat_put_super(struct super_block *sb)
 	mutex_unlock(&sbi->s_lock);
 
 	unload_nls(sbi->nls_io);
-	exfat_free_iocharset(sbi);
 	exfat_free_upcase_table(sbi);
-	kfree(sbi);
 }
 
 static int exfat_sync_fs(struct super_block *sb, int wait)
@@ -703,9 +701,6 @@ static int exfat_fill_super(struct super_block *sb, struct fs_context *fc)
 
 check_nls_io:
 	unload_nls(sbi->nls_io);
-	exfat_free_iocharset(sbi);
-	sb->s_fs_info = NULL;
-	kfree(sbi);
 	return err;
 }
 
@@ -714,14 +709,18 @@ static int exfat_get_tree(struct fs_context *fc)
 	return get_tree_bdev(fc, exfat_fill_super);
 }
 
+static void exfat_free_sbi(struct exfat_sb_info *sbi)
+{
+	exfat_free_iocharset(sbi);
+	kfree(sbi);
+}
+
 static void exfat_free(struct fs_context *fc)
 {
 	struct exfat_sb_info *sbi = fc->s_fs_info;
 
-	if (sbi) {
-		exfat_free_iocharset(sbi);
-		kfree(sbi);
-	}
+	if (sbi)
+		exfat_free_sbi(sbi);
 }
 
 static int exfat_reconfigure(struct fs_context *fc)
@@ -766,12 +765,21 @@ static int exfat_init_fs_context(struct fs_context *fc)
 	return 0;
 }
 
+static void exfat_kill_sb(struct super_block *sb)
+{
+	struct exfat_sb_info *sbi = sb->s_fs_info;
+
+	kill_block_super(sb);
+	if (sbi)
+		exfat_free_sbi(sbi);
+}
+
 static struct file_system_type exfat_fs_type = {
 	.owner			= THIS_MODULE,
 	.name			= "exfat",
 	.init_fs_context	= exfat_init_fs_context,
 	.parameters		= exfat_parameters,
-	.kill_sb		= kill_block_super,
+	.kill_sb		= exfat_kill_sb,
 	.fs_flags		= FS_REQUIRES_DEV,
 };
 
-- 
2.39.2

[PATCH 11/13] ntfs3: rename put_ntfs ntfs3_free_sbi

[Christoph Hellwig]

put_ntfs is a rather unconventional name for a function that frees the
sbi and associated resources.  Give it a more descriptive name and drop
the duplicate name in the top of the function comment.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/ntfs3/super.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c
index 1a02072b6b0e16..bb985d3756d949 100644
--- a/fs/ntfs3/super.c
+++ b/fs/ntfs3/super.c
@@ -569,9 +569,9 @@ static void init_once(void *foo)
 }
 
 /*
- * put_ntfs - Noinline to reduce binary size.
+ * Noinline to reduce binary size.
  */
-static noinline void put_ntfs(struct ntfs_sb_info *sbi)
+static noinline void ntfs3_free_sbi(struct ntfs_sb_info *sbi)
 {
 	kfree(sbi->new_rec);
 	kvfree(ntfs_put_shared(sbi->upcase));
@@ -627,7 +627,7 @@ static void ntfs_put_super(struct super_block *sb)
 	ntfs_set_state(sbi, NTFS_DIRTY_CLEAR);
 
 	put_mount_options(sbi->options);
-	put_ntfs(sbi);
+	ntfs3_free_sbi(sbi);
 	sb->s_fs_info = NULL;
 
 	sync_blockdev(sb->s_bdev);
@@ -1569,7 +1569,7 @@ static int ntfs_fill_super(struct super_block *sb, struct fs_context *fc)
 	 * ntfs_fs_free will be called with fc->s_fs_info = NULL
 	 */
 	put_mount_options(sbi->options);
-	put_ntfs(sbi);
+	ntfs3_free_sbi(sbi);
 	sb->s_fs_info = NULL;
 	kfree(boot2);
 
@@ -1659,7 +1659,7 @@ static void ntfs_fs_free(struct fs_context *fc)
 	struct ntfs_sb_info *sbi = fc->s_fs_info;
 
 	if (sbi)
-		put_ntfs(sbi);
+		ntfs3_free_sbi(sbi);
 
 	if (opts)
 		put_mount_options(opts);
-- 
2.39.2

[PATCH 12/13] ntfs3: don't call sync_blockdev in ntfs_put_super

[Christoph Hellwig]

kill_block_super will call sync_blockdev just a tad later already.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/ntfs3/super.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c
index bb985d3756d949..727138933a9324 100644
--- a/fs/ntfs3/super.c
+++ b/fs/ntfs3/super.c
@@ -629,8 +629,6 @@ static void ntfs_put_super(struct super_block *sb)
 	put_mount_options(sbi->options);
 	ntfs3_free_sbi(sbi);
 	sb->s_fs_info = NULL;
-
-	sync_blockdev(sb->s_bdev);
 }
 
 static int ntfs_statfs(struct dentry *dentry, struct kstatfs *buf)
-- 
2.39.2

[PATCH 13/13] ntfs3: free the sbi in ->kill_sb

[Christoph Hellwig]

As a rule of thumb everything allocated to the fs_context and moved into
the super_block should be freed by ->kill_sb so that the teardown
handling doesn't need to be duplicated between the fill_super error
path and put_super.  Implement an ntfs3-specific kill_sb method to do
that.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/ntfs3/super.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c
index 727138933a9324..5fffddea554f18 100644
--- a/fs/ntfs3/super.c
+++ b/fs/ntfs3/super.c
@@ -625,10 +625,6 @@ static void ntfs_put_super(struct super_block *sb)
 
 	/* Mark rw ntfs as clear, if possible. */
 	ntfs_set_state(sbi, NTFS_DIRTY_CLEAR);
-
-	put_mount_options(sbi->options);
-	ntfs3_free_sbi(sbi);
-	sb->s_fs_info = NULL;
 }
 
 static int ntfs_statfs(struct dentry *dentry, struct kstatfs *buf)
@@ -1562,15 +1558,7 @@ static int ntfs_fill_super(struct super_block *sb, struct fs_context *fc)
 put_inode_out:
 	iput(inode);
 out:
-	/*
-	 * Free resources here.
-	 * ntfs_fs_free will be called with fc->s_fs_info = NULL
-	 */
-	put_mount_options(sbi->options);
-	ntfs3_free_sbi(sbi);
-	sb->s_fs_info = NULL;
 	kfree(boot2);
-
 	return err;
 }
 
@@ -1726,13 +1714,24 @@ static int ntfs_init_fs_context(struct fs_context *fc)
 	return -ENOMEM;
 }
 
+static void ntfs3_kill_sb(struct super_block *sb)
+{
+	struct ntfs_sb_info *sbi = sb->s_fs_info;
+
+	kill_block_super(sb);
+
+	if (sbi->options)
+		put_mount_options(sbi->options);
+	ntfs3_free_sbi(sbi);
+}
+
 // clang-format off
 static struct file_system_type ntfs_fs_type = {
 	.owner			= THIS_MODULE,
 	.name			= "ntfs3",
 	.init_fs_context	= ntfs_init_fs_context,
 	.parameters		= ntfs_fs_parameters,
-	.kill_sb		= kill_block_super,
+	.kill_sb		= ntfs3_kill_sb,
 	.fs_flags		= FS_REQUIRES_DEV | FS_ALLOW_IDMAP,
 };
 // clang-format on
-- 
2.39.2

Re: [PATCH 01/13] xfs: reformat the xfs_fs_free prototype

[Darrick J. Wong]

On Tue, Aug 08, 2023 at 09:15:48AM -0700, Christoph Hellwig wrote:
> The xfs_fs_free prototype formatting is a weird mix of the classic XFS
> style and the Linux style.  Fix it up to be consistent.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_super.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index bf6e0a261a49e6..0a294659c18972 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1933,7 +1933,8 @@ xfs_fs_reconfigure(
>  	return 0;
>  }
>  
> -static void xfs_fs_free(
> +static void
> +xfs_fs_free(
>  	struct fs_context	*fc)
>  {
>  	struct xfs_mount	*mp = fc->s_fs_info;
> -- 
> 2.39.2
> 

Re: [PATCH 05/13] xfs: don't call invalidate_bdev in xfs_free_buftarg

[Darrick J. Wong]

On Tue, Aug 08, 2023 at 09:15:52AM -0700, Christoph Hellwig wrote:
> XFS never uses the block device mapping, so there is no point in calling
> invalidate_bdev which invalidates said mapping.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

This is a revert of commit 032e160305f68 ("xfs: invalidate block device
page cache during unmount").  How do you propose dealing with the block
device mapping being incoherent due to unmount write races so you don't
re-break my test system?

--D

> ---
>  fs/xfs/xfs_buf.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index 15d1e5a7c2d340..83b8702030f71d 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -1944,7 +1944,6 @@ xfs_free_buftarg(
>  	list_lru_destroy(&btp->bt_lru);
>  
>  	blkdev_issue_flush(btp->bt_bdev);
> -	invalidate_bdev(btp->bt_bdev);
>  	fs_put_dax(btp->bt_daxdev, btp->bt_mount);
>  
>  	kmem_free(btp);
> -- 
> 2.39.2
> 

Re: [PATCH 02/13] xfs: remove a superflous s_fs_info NULL check in xfs_fs_put_super

[Christian Brauner]

On Tue, Aug 08, 2023 at 09:15:49AM -0700, Christoph Hellwig wrote:
> ->put_super is only called when sb->s_root is set, and thus when
> fill_super succeeds.  Thus drop the NULL check that can't happen in
> xfs_fs_put_super.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Looks good to me,
Reviewed-by: Christian Brauner <brauner@kernel.org>

Re: [PATCH 03/13] xfs: free the mount in ->kill_sb

[Christian Brauner]

On Tue, Aug 08, 2023 at 09:15:50AM -0700, Christoph Hellwig wrote:
> As a rule of thumb everything allocated to the fs_context and moved into
> the super_block should be freed by ->kill_sb so that the teardown
> handling doesn't need to be duplicated between the fill_super error
> path and put_super.  Implement a XFS-specific kill_sb method to do that.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Looks good to me,
Reviewed-by: Christian Brauner <brauner@kernel.org>

For filesystems on the new mount api it's always nice if the rules
can simply be made:

* prior to sget_fc() succeeding fc->s_fs_info and freed in fs_context_operations->free()
* after    sget_fc() succeeding fc->s_fs_info transfered to sb->s_fs_info and freed in fs_type->kill_sb()

Re: [PATCH 04/13] xfs: remove xfs_blkdev_put

[Christian Brauner]

On Tue, Aug 08, 2023 at 09:15:51AM -0700, Christoph Hellwig wrote:
> There isn't much use for this trivial wrapper, especially as the NULL
> check is only needed in a single call site.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_super.c | 18 +++++-------------
>  1 file changed, 5 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index d2f3ae6ba8938b..f00d1162815d19 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -395,15 +395,6 @@ xfs_blkdev_get(
>  	return error;
>  }
>  
> -STATIC void
> -xfs_blkdev_put(
> -	struct xfs_mount	*mp,
> -	struct block_device	*bdev)
> -{
> -	if (bdev)
> -		blkdev_put(bdev, mp->m_super);
> -}
> -
>  STATIC void
>  xfs_close_devices(
>  	struct xfs_mount	*mp)
> @@ -412,13 +403,13 @@ xfs_close_devices(
>  		struct block_device *logdev = mp->m_logdev_targp->bt_bdev;
>  
>  		xfs_free_buftarg(mp->m_logdev_targp);
> -		xfs_blkdev_put(mp, logdev);
> +		blkdev_put(logdev, mp->m_super);
>  	}
>  	if (mp->m_rtdev_targp) {
>  		struct block_device *rtdev = mp->m_rtdev_targp->bt_bdev;
>  
>  		xfs_free_buftarg(mp->m_rtdev_targp);
> -		xfs_blkdev_put(mp, rtdev);
> +		blkdev_put(rtdev, mp->m_super);
>  	}
>  	xfs_free_buftarg(mp->m_ddev_targp);
>  }
> @@ -503,10 +494,11 @@ xfs_open_devices(
>   out_free_ddev_targ:
>  	xfs_free_buftarg(mp->m_ddev_targp);
>   out_close_rtdev:
> -	xfs_blkdev_put(mp, rtdev);
> + 	if (rtdev)

nit: I think there's a stray space here.

Otherwise,
Reviewed-by: Christian Brauner <brauner@kernel.org>

Re: [PATCH 13/13] ntfs3: free the sbi in ->kill_sb

[Christian Brauner]

On Tue, Aug 08, 2023 at 09:16:00AM -0700, Christoph Hellwig wrote:
> As a rule of thumb everything allocated to the fs_context and moved into
> the super_block should be freed by ->kill_sb so that the teardown
> handling doesn't need to be duplicated between the fill_super error
> path and put_super.  Implement an ntfs3-specific kill_sb method to do
> that.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Looks good to me,
Reviewed-by: Christian Brauner <brauner@kernel.org>

Re: [PATCH 11/13] ntfs3: rename put_ntfs ntfs3_free_sbi

[Christian Brauner]

On Tue, Aug 08, 2023 at 09:15:58AM -0700, Christoph Hellwig wrote:
> put_ntfs is a rather unconventional name for a function that frees the
> sbi and associated resources.  Give it a more descriptive name and drop
> the duplicate name in the top of the function comment.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Looks good to me,
Reviewed-by: Christian Brauner <brauner@kernel.org>

Re: [PATCH 12/13] ntfs3: don't call sync_blockdev in ntfs_put_super

[Christian Brauner]

On Tue, Aug 08, 2023 at 09:15:59AM -0700, Christoph Hellwig wrote:
> kill_block_super will call sync_blockdev just a tad later already.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Looks good to me,
Reviewed-by: Christian Brauner <brauner@kernel.org>

Re: [PATCH 02/13] xfs: remove a superflous s_fs_info NULL check in xfs_fs_put_super

[Darrick J. Wong]

On Tue, Aug 08, 2023 at 09:15:49AM -0700, Christoph Hellwig wrote:
> ->put_super is only called when sb->s_root is set, and thus when
> fill_super succeeds.  Thus drop the NULL check that can't happen in
> xfs_fs_put_super.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Ahahaha, here's the /rest/ of the patchset.  I wondered if vger was
busted yesterday.  Carrying on...

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_super.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 0a294659c18972..128f4a2924d49c 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1132,10 +1132,6 @@ xfs_fs_put_super(
>  {
>  	struct xfs_mount	*mp = XFS_M(sb);
>  
> -	/* if ->fill_super failed, we have no mount to tear down */
> -	if (!sb->s_fs_info)
> -		return;
> -
>  	xfs_notice(mp, "Unmounting Filesystem %pU", &mp->m_sb.sb_uuid);
>  	xfs_filestream_unmount(mp);
>  	xfs_unmountfs(mp);
> -- 
> 2.39.2
> 

Re: [PATCH 03/13] xfs: free the mount in ->kill_sb

[Darrick J. Wong]

On Tue, Aug 08, 2023 at 09:15:50AM -0700, Christoph Hellwig wrote:
> As a rule of thumb everything allocated to the fs_context and moved into
> the super_block should be freed by ->kill_sb so that the teardown
> handling doesn't need to be duplicated between the fill_super error
> path and put_super.  Implement a XFS-specific kill_sb method to do that.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_super.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 128f4a2924d49c..d2f3ae6ba8938b 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1143,9 +1143,6 @@ xfs_fs_put_super(
>  	xfs_destroy_percpu_counters(mp);
>  	xfs_destroy_mount_workqueues(mp);
>  	xfs_close_devices(mp);
> -
> -	sb->s_fs_info = NULL;
> -	xfs_mount_free(mp);
>  }
>  
>  static long
> @@ -1487,7 +1484,7 @@ xfs_fs_fill_super(
>  
>  	error = xfs_fs_validate_params(mp);
>  	if (error)
> -		goto out_free_names;
> +		return error;
>  
>  	sb_min_blocksize(sb, BBSIZE);
>  	sb->s_xattr = xfs_xattr_handlers;
> @@ -1514,7 +1511,7 @@ xfs_fs_fill_super(
>  
>  	error = xfs_open_devices(mp);
>  	if (error)
> -		goto out_free_names;
> +		return error;
>  
>  	error = xfs_init_mount_workqueues(mp);
>  	if (error)
> @@ -1734,9 +1731,6 @@ xfs_fs_fill_super(
>  	xfs_destroy_mount_workqueues(mp);
>   out_close_devices:
>  	xfs_close_devices(mp);
> - out_free_names:
> -	sb->s_fs_info = NULL;
> -	xfs_mount_free(mp);
>  	return error;
>  
>   out_unmount:
> @@ -1999,12 +1993,20 @@ static int xfs_init_fs_context(
>  	return 0;
>  }
>  
> +static void
> +xfs_kill_sb(
> +	struct super_block		*sb)
> +{
> +	kill_block_super(sb);
> +	xfs_mount_free(XFS_M(sb));
> +}
> +
>  static struct file_system_type xfs_fs_type = {
>  	.owner			= THIS_MODULE,
>  	.name			= "xfs",
>  	.init_fs_context	= xfs_init_fs_context,
>  	.parameters		= xfs_fs_parameters,
> -	.kill_sb		= kill_block_super,
> +	.kill_sb		= xfs_kill_sb,
>  	.fs_flags		= FS_REQUIRES_DEV | FS_ALLOW_IDMAP,
>  };
>  MODULE_ALIAS_FS("xfs");
> -- 
> 2.39.2
> 

Re: [PATCH 04/13] xfs: remove xfs_blkdev_put

[Darrick J. Wong]

On Tue, Aug 08, 2023 at 09:15:51AM -0700, Christoph Hellwig wrote:
> There isn't much use for this trivial wrapper, especially as the NULL
> check is only needed in a single call site.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_super.c | 18 +++++-------------
>  1 file changed, 5 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index d2f3ae6ba8938b..f00d1162815d19 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -395,15 +395,6 @@ xfs_blkdev_get(
>  	return error;
>  }
>  
> -STATIC void
> -xfs_blkdev_put(
> -	struct xfs_mount	*mp,
> -	struct block_device	*bdev)
> -{
> -	if (bdev)
> -		blkdev_put(bdev, mp->m_super);
> -}
> -
>  STATIC void
>  xfs_close_devices(
>  	struct xfs_mount	*mp)
> @@ -412,13 +403,13 @@ xfs_close_devices(
>  		struct block_device *logdev = mp->m_logdev_targp->bt_bdev;
>  
>  		xfs_free_buftarg(mp->m_logdev_targp);
> -		xfs_blkdev_put(mp, logdev);
> +		blkdev_put(logdev, mp->m_super);
>  	}
>  	if (mp->m_rtdev_targp) {
>  		struct block_device *rtdev = mp->m_rtdev_targp->bt_bdev;
>  
>  		xfs_free_buftarg(mp->m_rtdev_targp);
> -		xfs_blkdev_put(mp, rtdev);
> +		blkdev_put(rtdev, mp->m_super);
>  	}
>  	xfs_free_buftarg(mp->m_ddev_targp);
>  }
> @@ -503,10 +494,11 @@ xfs_open_devices(
>   out_free_ddev_targ:
>  	xfs_free_buftarg(mp->m_ddev_targp);
>   out_close_rtdev:
> -	xfs_blkdev_put(mp, rtdev);
> + 	if (rtdev)

   ^ extra space here

With that removed,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D


> +		blkdev_put(rtdev, sb);
>   out_close_logdev:
>  	if (logdev && logdev != ddev)
> -		xfs_blkdev_put(mp, logdev);
> +		blkdev_put(logdev, sb);
>  	goto out_relock;
>  }
>  
> -- 
> 2.39.2
> 

Re: [PATCH 06/13] xfs: close the RT and log block devices in xfs_free_buftarg

[Darrick J. Wong]

On Tue, Aug 08, 2023 at 09:15:53AM -0700, Christoph Hellwig wrote:
> Closing the block devices logically belongs into xfs_free_buftarg,  So instead
> of open coding it in the caller move it there and add a check for the s_bdev
> so that the main device isn't close as that's done by the VFS helper.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_buf.c   |  5 +++++
>  fs/xfs/xfs_super.c | 12 ++----------
>  2 files changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index 83b8702030f71d..c57e6e03dfa80c 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -1938,6 +1938,8 @@ void
>  xfs_free_buftarg(
>  	struct xfs_buftarg	*btp)
>  {
> +	struct block_device	*bdev = btp->bt_bdev;
> +
>  	unregister_shrinker(&btp->bt_shrinker);
>  	ASSERT(percpu_counter_sum(&btp->bt_io_count) == 0);
>  	percpu_counter_destroy(&btp->bt_io_count);
> @@ -1945,6 +1947,9 @@ xfs_free_buftarg(
>  
>  	blkdev_issue_flush(btp->bt_bdev);
>  	fs_put_dax(btp->bt_daxdev, btp->bt_mount);
> +	/* the main block device is closed by kill_block_super */
> +	if (bdev != btp->bt_mount->m_super->s_bdev)
> +		blkdev_put(bdev, btp->bt_mount->m_super);

Hmm... I feel like this would be cleaner if the data dev buftarg could
get its own refcount separate from super_block.s_bdev, but I looked
through the code and couldn't identify a simple way to do that. Soo...

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D


>  
>  	kmem_free(btp);
>  }
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index f00d1162815d19..37b1b763a0bef0 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -399,18 +399,10 @@ STATIC void
>  xfs_close_devices(
>  	struct xfs_mount	*mp)
>  {
> -	if (mp->m_logdev_targp && mp->m_logdev_targp != mp->m_ddev_targp) {
> -		struct block_device *logdev = mp->m_logdev_targp->bt_bdev;
> -
> +	if (mp->m_logdev_targp && mp->m_logdev_targp != mp->m_ddev_targp)
>  		xfs_free_buftarg(mp->m_logdev_targp);
> -		blkdev_put(logdev, mp->m_super);
> -	}
> -	if (mp->m_rtdev_targp) {
> -		struct block_device *rtdev = mp->m_rtdev_targp->bt_bdev;
> -
> +	if (mp->m_rtdev_targp)
>  		xfs_free_buftarg(mp->m_rtdev_targp);
> -		blkdev_put(rtdev, mp->m_super);
> -	}
>  	xfs_free_buftarg(mp->m_ddev_targp);
>  }
>  
> -- 
> 2.39.2
> 

Re: [PATCH 07/13] xfs: close the external block devices in xfs_mount_free

[Darrick J. Wong]

On Tue, Aug 08, 2023 at 09:15:54AM -0700, Christoph Hellwig wrote:
> blkdev_put must not be called under sb->s_umount to avoid a lock order
> reversal with disk->open_mutex.  Move closing the buftargs into ->kill_sb
> to archive that.  Note that the flushing of the disk caches needs to be
> kept in ->put_super as the main block device is closed in
> kill_block_super already.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_buf.c   |  1 -
>  fs/xfs/xfs_super.c | 27 +++++++++++++++++++--------
>  2 files changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index c57e6e03dfa80c..3b903f6bce98d8 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -1945,7 +1945,6 @@ xfs_free_buftarg(
>  	percpu_counter_destroy(&btp->bt_io_count);
>  	list_lru_destroy(&btp->bt_lru);
>  
> -	blkdev_issue_flush(btp->bt_bdev);
>  	fs_put_dax(btp->bt_daxdev, btp->bt_mount);
>  	/* the main block device is closed by kill_block_super */
>  	if (bdev != btp->bt_mount->m_super->s_bdev)
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 37b1b763a0bef0..67343364ac66e9 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -396,14 +396,14 @@ xfs_blkdev_get(
>  }
>  
>  STATIC void
> -xfs_close_devices(
> +xfs_flush_disk_caches(
>  	struct xfs_mount	*mp)
>  {
>  	if (mp->m_logdev_targp && mp->m_logdev_targp != mp->m_ddev_targp)
> -		xfs_free_buftarg(mp->m_logdev_targp);
> +		blkdev_issue_flush(mp->m_logdev_targp->bt_bdev);
>  	if (mp->m_rtdev_targp)
> -		xfs_free_buftarg(mp->m_rtdev_targp);
> -	xfs_free_buftarg(mp->m_ddev_targp);
> +		blkdev_issue_flush(mp->m_rtdev_targp->bt_bdev);
> +	blkdev_issue_flush(mp->m_ddev_targp->bt_bdev);

If I'm following this correctly, putting the superblock flushes the
bdevs (though it doesn't invalidate the bdev mapping!) and only later
when we free the xfs_mount do we actually put the buftargs?

That works, though I still think we need to invalidate the bdev
pagecache for the log and data bdevs.

--D

>  }
>  
>  /*
> @@ -741,6 +741,17 @@ static void
>  xfs_mount_free(
>  	struct xfs_mount	*mp)
>  {
> +	/*
> +	 * Free the buftargs here because blkdev_put needs to be called outside
> +	 * of sb->s_umount, which is held around the call to ->put_super.
> +	 */
> +	if (mp->m_logdev_targp && mp->m_logdev_targp != mp->m_ddev_targp)
> +		xfs_free_buftarg(mp->m_logdev_targp);
> +	if (mp->m_rtdev_targp)
> +		xfs_free_buftarg(mp->m_rtdev_targp);
> +	if (mp->m_ddev_targp)
> +		xfs_free_buftarg(mp->m_ddev_targp);
> +
>  	kfree(mp->m_rtname);
>  	kfree(mp->m_logname);
>  	kmem_free(mp);
> @@ -1126,7 +1137,7 @@ xfs_fs_put_super(
>  	xfs_inodegc_free_percpu(mp);
>  	xfs_destroy_percpu_counters(mp);
>  	xfs_destroy_mount_workqueues(mp);
> -	xfs_close_devices(mp);
> +	xfs_flush_disk_caches(mp);
>  }
>  
>  static long
> @@ -1499,7 +1510,7 @@ xfs_fs_fill_super(
>  
>  	error = xfs_init_mount_workqueues(mp);
>  	if (error)
> -		goto out_close_devices;
> +		goto out_flush_caches;
>  
>  	error = xfs_init_percpu_counters(mp);
>  	if (error)
> @@ -1713,8 +1724,8 @@ xfs_fs_fill_super(
>  	xfs_destroy_percpu_counters(mp);
>   out_destroy_workqueues:
>  	xfs_destroy_mount_workqueues(mp);
> - out_close_devices:
> -	xfs_close_devices(mp);
> + out_flush_caches:
> +	xfs_flush_disk_caches(mp);
>  	return error;
>  
>   out_unmount:
> -- 
> 2.39.2
> 

Re: [PATCH 07/13] xfs: close the external block devices in xfs_mount_free

[Christoph Hellwig]

On Wed, Aug 09, 2023 at 08:55:24AM -0700, Darrick J. Wong wrote:
> If I'm following this correctly, putting the superblock flushes the
> bdevs (though it doesn't invalidate the bdev mapping!) and only later
> when we free the xfs_mount do we actually put the buftargs?
> 
> That works, though I still think we need to invalidate the bdev
> pagecache for the log and data bdevs.

Yes, I'll respin it with that.  And I'll also add a comment to the
invalidate_bdev calls because it's completely non-obvious as-is.

Re: [PATCH 06/13] xfs: close the RT and log block devices in xfs_free_buftarg

[Christoph Hellwig]

On Wed, Aug 09, 2023 at 08:45:32AM -0700, Darrick J. Wong wrote:
> >  	blkdev_issue_flush(btp->bt_bdev);
> >  	fs_put_dax(btp->bt_daxdev, btp->bt_mount);
> > +	/* the main block device is closed by kill_block_super */
> > +	if (bdev != btp->bt_mount->m_super->s_bdev)
> > +		blkdev_put(bdev, btp->bt_mount->m_super);
> 
> Hmm... I feel like this would be cleaner if the data dev buftarg could
> get its own refcount separate from super_block.s_bdev, but I looked
> through the code and couldn't identify a simple way to do that. Soo...

blkdev_put doesn't really drop a refcount, it closes the device.
It just happens to be misnamed, but Jan is looking into a series that
will as a side effect end up with a better name for this functionality.

Re: [PATCH 03/13] xfs: free the mount in ->kill_sb

[Christoph Hellwig]

On Wed, Aug 09, 2023 at 09:38:13AM +0200, Christian Brauner wrote:
> For filesystems on the new mount api it's always nice if the rules
> can simply be made:
> 
> * prior to sget_fc() succeeding fc->s_fs_info and freed in fs_context_operations->free()
> * after    sget_fc() succeeding fc->s_fs_info transfered to sb->s_fs_info and freed in fs_type->kill_sb()

Yes.  I've been wanting to write this down somewhere, but I can't really
think of a good place.

Re: [PATCH 07/13] xfs: close the external block devices in xfs_mount_free

[Darrick J. Wong]

On Wed, Aug 09, 2023 at 06:14:11PM +0200, Christoph Hellwig wrote:
> On Wed, Aug 09, 2023 at 08:55:24AM -0700, Darrick J. Wong wrote:
> > If I'm following this correctly, putting the superblock flushes the
> > bdevs (though it doesn't invalidate the bdev mapping!) and only later
> > when we free the xfs_mount do we actually put the buftargs?
> > 
> > That works, though I still think we need to invalidate the bdev
> > pagecache for the log and data bdevs.
> 
> Yes, I'll respin it with that.  And I'll also add a comment to the
> invalidate_bdev calls because it's completely non-obvious as-is.

Ok, thanks!  You can reuse the commit message if you want. :)

--D

[PATCH 01/13] xfs: reformat the xfs_fs_free prototype

[Christoph Hellwig]

The xfs_fs_free prototype formatting is a weird mix of the classic XFS
style and the Linux style.  Fix it up to be consistent.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_super.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index bf6e0a261a49e6..0a294659c18972 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1933,7 +1933,8 @@ xfs_fs_reconfigure(
 	return 0;
 }
 
-static void xfs_fs_free(
+static void
+xfs_fs_free(
 	struct fs_context	*fc)
 {
 	struct xfs_mount	*mp = fc->s_fs_info;
-- 
2.39.2

Re: [PATCH 01/13] xfs: reformat the xfs_fs_free prototype

[Christian Brauner]

On Wed, 09 Aug 2023 15:05:33 -0700, Christoph Hellwig wrote:
> The xfs_fs_free prototype formatting is a weird mix of the classic XFS
> style and the Linux style.  Fix it up to be consistent.
> 
> 

I've completely reshuffled that branch now:

(1) bd_super removal
(2) ->kill_sb() fixes
(3) setup_bdev_super() work (open devices after sb creation)
(4) fs_holder_ops rework
(5) exclusive sb creation

I think that's the most natural order. That involved quite a bit of
massaging, so please make sure that everything's in proper order. The
most sensitive and error prone part of the reordering is that before the
switch from fs_type to sb as holder took place prior to this series.
Whereas due to the reordering the switch now takes place after this
series. This is good though because we can't use setup_bdev_super()
correctly anyway before we have the ->kill_sb() changes done.

git rebase -i v6.5-rc1 -x "make fs/"

builds cleanly. Please double check!

---

Applied to the vfs.super branch of the vfs/vfs.git tree.
Patches in the vfs.super branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.super

[01/13] xfs: reformat the xfs_fs_free prototype
        https://git.kernel.org/vfs/vfs/c/183f60c58f51
[02/13] xfs: remove a superfluous s_fs_info NULL check in xfs_fs_put_super
        https://git.kernel.org/vfs/vfs/c/94d1bb71d395
[03/13] xfs: free the xfs_mount in ->kill_sb
        https://git.kernel.org/vfs/vfs/c/b92fea73ce6a
[04/13] xfs: remove xfs_blkdev_put
        https://git.kernel.org/vfs/vfs/c/3b6c117834c2
[05/13] xfs: close the RT and log block devices in xfs_free_buftarg
        https://git.kernel.org/vfs/vfs/c/6d4e81f94e80
[06/13] xfs: close the external block devices in xfs_mount_free
        https://git.kernel.org/vfs/vfs/c/bfeb8750e6fe
[07/13] xfs: document the invalidate_bdev call in invalidate_bdev
        https://git.kernel.org/vfs/vfs/c/e4676171bad6
[08/13] ext4: close the external journal device in ->kill_sb
        https://git.kernel.org/vfs/vfs/c/e1a1b0fba97b
[09/13] exfat: don't RCU-free the sbi
        https://git.kernel.org/vfs/vfs/c/6a3f5dee46f6
[10/13] exfat: free the sbi and iocharset in ->kill_sb
        https://git.kernel.org/vfs/vfs/c/cc0313e8135e
[11/13] ntfs3: rename put_ntfs ntfs3_free_sbi
        https://git.kernel.org/vfs/vfs/c/34371bb44afc
[12/13] ntfs3: don't call sync_blockdev in ntfs_put_super
        https://git.kernel.org/vfs/vfs/c/91952e99005b
[13/13] ntfs3: free the sbi in ->kill_sb
        https://git.kernel.org/vfs/vfs/c/5fb25fde7dee

Re: [PATCH 01/13] xfs: reformat the xfs_fs_free prototype

[Christoph Hellwig]

I think the btrfs hunk (below) in "fs: use the super_block as holder when
mounting file systems" needs to be dropped, as we dropped the prep patch
that allows to use the sb as a holder for now.  I'll add it to my resend
of the btrfs conversion.

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index f1dd172d8d5bd7..d58ace4c1d2962 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -69,8 +69,6 @@ static const struct super_operations btrfs_super_ops;
  * requested by subvol=/path. That way the callchain is straightforward and we
  * don't have to play tricks with the mount options and recursive calls to
  * btrfs_mount.
- *
- * The new btrfs_root_fs_type also servers as a tag for the bdev_holder.
  */
 static struct file_system_type btrfs_fs_type;
 static struct file_system_type btrfs_root_fs_type;
@@ -1515,7 +1513,7 @@ static struct dentry *btrfs_mount_root(struct file_system_type *fs_type,
 		snprintf(s->s_id, sizeof(s->s_id), "%pg", bdev);
 		shrinker_debugfs_rename(&s->s_shrink, "sb-%s:%s", fs_type->name,
 					s->s_id);
-		btrfs_sb(s)->bdev_holder = fs_type;
+		fs_info->bdev_holder = s;
 		error = btrfs_fill_super(s, fs_devices, data);
 	}
 	if (!error)

Re: [PATCH 01/13] xfs: reformat the xfs_fs_free prototype

[Christian Brauner]

On Thu, Aug 10, 2023 at 05:51:16PM +0200, Christoph Hellwig wrote:
> I think the btrfs hunk (below) in "fs: use the super_block as holder when
> mounting file systems" needs to be dropped, as we dropped the prep patch

Ah yes, thanks!

> that allows to use the sb as a holder for now.  I'll add it to my resend
> of the btrfs conversion.

Dropped.