[PATCH 0/3] Move fscrypt keyring destruction to after ->put_super

[PATCH 0/3] Move fscrypt keyring destruction to after ->put_super

[Eric Biggers]

This series moves the fscrypt keyring destruction to after ->put_super,
as this will be needed by the btrfs fscrypt support.  To make this
possible, it also changes btrfs and f2fs to release their block devices
after generic_shutdown_super() rather than before.

This supersedes "[PATCH] fscrypt: move the call to
fscrypt_destroy_keyring() into ->put_super()"
(https://lore.kernel.org/linux-fscrypt/20231206001325.13676-1-ebiggers@kernel.org/T/#u)

This applies to v6.7-rc5.

Christoph Hellwig (1):
  btrfs: call btrfs_close_devices from ->kill_sb

Eric Biggers (1):
  f2fs: move release of block devices to after kill_block_super()

Josef Bacik (1):
  fs: move fscrypt keyring destruction to after ->put_super

 fs/btrfs/disk-io.c |  4 ++--
 fs/btrfs/super.c   |  7 ++-----
 fs/f2fs/super.c    | 12 +++++++-----
 fs/super.c         | 12 ++++++------
 4 files changed, 17 insertions(+), 18 deletions(-)


base-commit: a39b6ac3781d46ba18193c9dbb2110f31e9bffe9
-- 
2.43.0

[PATCH 1/3] btrfs: call btrfs_close_devices from ->kill_sb

[Eric Biggers]

From: Christoph Hellwig <hch@lst.de>

blkdev_put must not be called under sb->s_umount to avoid a lock order
reversal with disk->open_mutex once call backs from block devices to
the file system using the holder ops are supported.  Move the call
to btrfs_close_devices into btrfs_free_fs_info so that it is closed
from ->kill_sb (which is also called from the mount failure handling
path unlike ->put_super) as well as when an fs_info is freed because
an existing superblock already exists.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Christian Brauner <brauner@kernel.org>
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/btrfs/disk-io.c | 4 ++--
 fs/btrfs/super.c   | 7 ++-----
 2 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index bbcc3df776461..fe98e6b1d9c61 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1237,20 +1237,22 @@ static void free_global_roots(struct btrfs_fs_info *fs_info)
 
 	while ((node = rb_first_postorder(&fs_info->global_root_tree)) != NULL) {
 		root = rb_entry(node, struct btrfs_root, rb_node);
 		rb_erase(&root->rb_node, &fs_info->global_root_tree);
 		btrfs_put_root(root);
 	}
 }
 
 void btrfs_free_fs_info(struct btrfs_fs_info *fs_info)
 {
+	if (fs_info->fs_devices)
+		btrfs_close_devices(fs_info->fs_devices);
 	percpu_counter_destroy(&fs_info->dirty_metadata_bytes);
 	percpu_counter_destroy(&fs_info->delalloc_bytes);
 	percpu_counter_destroy(&fs_info->ordered_bytes);
 	percpu_counter_destroy(&fs_info->dev_replace.bio_counter);
 	btrfs_free_csum_hash(fs_info);
 	btrfs_free_stripe_hash_table(fs_info);
 	btrfs_free_ref_cache(fs_info);
 	kfree(fs_info->balance_ctl);
 	kfree(fs_info->delayed_root);
 	free_global_roots(fs_info);
@@ -3605,21 +3607,20 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
 	invalidate_inode_pages2(fs_info->btree_inode->i_mapping);
 
 fail_sb_buffer:
 	btrfs_stop_all_workers(fs_info);
 	btrfs_free_block_groups(fs_info);
 fail_alloc:
 	btrfs_mapping_tree_free(&fs_info->mapping_tree);
 
 	iput(fs_info->btree_inode);
 fail:
-	btrfs_close_devices(fs_info->fs_devices);
 	ASSERT(ret < 0);
 	return ret;
 }
 ALLOW_ERROR_INJECTION(open_ctree, ERRNO);
 
 static void btrfs_end_super_write(struct bio *bio)
 {
 	struct btrfs_device *device = bio->bi_private;
 	struct bio_vec *bvec;
 	struct bvec_iter_all iter_all;
@@ -4385,21 +4386,20 @@ void __cold close_ctree(struct btrfs_fs_info *fs_info)
 	 * have had an IO error and have left over tree log blocks that aren't
 	 * cleaned up until the fs roots are freed.  This makes the block group
 	 * accounting appear to be wrong because there's pending reserved bytes,
 	 * so make sure we do the block group cleanup afterwards.
 	 */
 	btrfs_free_block_groups(fs_info);
 
 	iput(fs_info->btree_inode);
 
 	btrfs_mapping_tree_free(&fs_info->mapping_tree);
-	btrfs_close_devices(fs_info->fs_devices);
 }
 
 void btrfs_mark_buffer_dirty(struct btrfs_trans_handle *trans,
 			     struct extent_buffer *buf)
 {
 	struct btrfs_fs_info *fs_info = buf->fs_info;
 	u64 transid = btrfs_header_generation(buf);
 
 #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
 	/*
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index ef256b944c72a..9616ce63c5630 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -1450,55 +1450,52 @@ static struct dentry *btrfs_mount_root(struct file_system_type *fs_type,
 	fs_devices = device->fs_devices;
 	fs_info->fs_devices = fs_devices;
 
 	error = btrfs_open_devices(fs_devices, mode, fs_type);
 	mutex_unlock(&uuid_mutex);
 	if (error)
 		goto error_fs_info;
 
 	if (!(flags & SB_RDONLY) && fs_devices->rw_devices == 0) {
 		error = -EACCES;
-		goto error_close_devices;
+		goto error_fs_info;
 	}
 
 	bdev = fs_devices->latest_dev->bdev;
 	s = sget(fs_type, btrfs_test_super, btrfs_set_super, flags | SB_NOSEC,
 		 fs_info);
 	if (IS_ERR(s)) {
 		error = PTR_ERR(s);
-		goto error_close_devices;
+		goto error_fs_info;
 	}
 
 	if (s->s_root) {
-		btrfs_close_devices(fs_devices);
 		btrfs_free_fs_info(fs_info);
 		if ((flags ^ s->s_flags) & SB_RDONLY)
 			error = -EBUSY;
 	} else {
 		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;
 		error = btrfs_fill_super(s, fs_devices, data);
 	}
 	if (!error)
 		error = security_sb_set_mnt_opts(s, new_sec_opts, 0, NULL);
 	security_free_mnt_opts(&new_sec_opts);
 	if (error) {
 		deactivate_locked_super(s);
 		return ERR_PTR(error);
 	}
 
 	return dget(s->s_root);
 
-error_close_devices:
-	btrfs_close_devices(fs_devices);
 error_fs_info:
 	btrfs_free_fs_info(fs_info);
 error_sec_opts:
 	security_free_mnt_opts(&new_sec_opts);
 	return ERR_PTR(error);
 }
 
 /*
  * Mount function which is called by VFS layer.
  *
-- 
2.43.0

[PATCH 2/3] f2fs: move release of block devices to after kill_block_super()

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Call destroy_device_list() and free the f2fs_sb_info from
kill_f2fs_super(), after the call to kill_block_super().  This is
necessary to order it after the call to fscrypt_destroy_keyring() once
generic_shutdown_super() starts calling fscrypt_destroy_keyring() just
after calling ->put_super.  This is because fscrypt_destroy_keyring()
may call into f2fs_get_devices() via the fscrypt_operations.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/f2fs/super.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 033af907c3b1d..ba95a341a9a36 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -1710,42 +1710,39 @@ static void f2fs_put_super(struct super_block *sb)
 	f2fs_destroy_node_manager(sbi);
 	f2fs_destroy_segment_manager(sbi);
 
 	/* flush s_error_work before sbi destroy */
 	flush_work(&sbi->s_error_work);
 
 	f2fs_destroy_post_read_wq(sbi);
 
 	kvfree(sbi->ckpt);
 
-	sb->s_fs_info = NULL;
 	if (sbi->s_chksum_driver)
 		crypto_free_shash(sbi->s_chksum_driver);
 	kfree(sbi->raw_super);
 
-	destroy_device_list(sbi);
 	f2fs_destroy_page_array_cache(sbi);
 	f2fs_destroy_xattr_caches(sbi);
 	mempool_destroy(sbi->write_io_dummy);
 #ifdef CONFIG_QUOTA
 	for (i = 0; i < MAXQUOTAS; i++)
 		kfree(F2FS_OPTION(sbi).s_qf_names[i]);
 #endif
 	fscrypt_free_dummy_policy(&F2FS_OPTION(sbi).dummy_enc_policy);
 	destroy_percpu_info(sbi);
 	f2fs_destroy_iostat(sbi);
 	for (i = 0; i < NR_PAGE_TYPE; i++)
 		kvfree(sbi->write_io[i]);
 #if IS_ENABLED(CONFIG_UNICODE)
 	utf8_unload(sb->s_encoding);
 #endif
-	kfree(sbi);
 }
 
 int f2fs_sync_fs(struct super_block *sb, int sync)
 {
 	struct f2fs_sb_info *sbi = F2FS_SB(sb);
 	int err = 0;
 
 	if (unlikely(f2fs_cp_error(sbi)))
 		return 0;
 	if (unlikely(is_sbi_flag_set(sbi, SBI_CP_DISABLED)))
@@ -4895,23 +4892,23 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 }
 
 static struct dentry *f2fs_mount(struct file_system_type *fs_type, int flags,
 			const char *dev_name, void *data)
 {
 	return mount_bdev(fs_type, flags, dev_name, data, f2fs_fill_super);
 }
 
 static void kill_f2fs_super(struct super_block *sb)
 {
-	if (sb->s_root) {
-		struct f2fs_sb_info *sbi = F2FS_SB(sb);
+	struct f2fs_sb_info *sbi = F2FS_SB(sb);
 
+	if (sb->s_root) {
 		set_sbi_flag(sbi, SBI_IS_CLOSE);
 		f2fs_stop_gc_thread(sbi);
 		f2fs_stop_discard_thread(sbi);
 
 #ifdef CONFIG_F2FS_FS_COMPRESSION
 		/*
 		 * latter evict_inode() can bypass checking and invalidating
 		 * compress inode cache.
 		 */
 		if (test_opt(sbi, COMPRESS_CACHE))
@@ -4924,20 +4921,25 @@ static void kill_f2fs_super(struct super_block *sb)
 				.reason = CP_UMOUNT,
 			};
 			stat_inc_cp_call_count(sbi, TOTAL_CALL);
 			f2fs_write_checkpoint(sbi, &cpc);
 		}
 
 		if (is_sbi_flag_set(sbi, SBI_IS_RECOVERED) && f2fs_readonly(sb))
 			sb->s_flags &= ~SB_RDONLY;
 	}
 	kill_block_super(sb);
+	if (sbi) {
+		destroy_device_list(sbi);
+		kfree(sbi);
+		sb->s_fs_info = NULL;
+	}
 }
 
 static struct file_system_type f2fs_fs_type = {
 	.owner		= THIS_MODULE,
 	.name		= "f2fs",
 	.mount		= f2fs_mount,
 	.kill_sb	= kill_f2fs_super,
 	.fs_flags	= FS_REQUIRES_DEV | FS_ALLOW_IDMAP,
 };
 MODULE_ALIAS_FS("f2fs");
-- 
2.43.0

[PATCH 3/3] fs: move fscrypt keyring destruction to after ->put_super

[Eric Biggers]

From: Josef Bacik <josef@toxicpanda.com>

btrfs has a variety of asynchronous things we do with inodes that can
potentially last until ->put_super, when we shut everything down and
clean up all of our async work.  Due to this we need to move
fscrypt_destroy_keyring() to after ->put_super, otherwise we get
warnings about still having active references on the master key.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/super.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/super.c b/fs/super.c
index 076392396e724..faf7d248145d2 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -674,34 +674,34 @@ void generic_shutdown_super(struct super_block *sb)
 		/* Evict all inodes with zero refcount. */
 		evict_inodes(sb);
 
 		/*
 		 * Clean up and evict any inodes that still have references due
 		 * to fsnotify or the security policy.
 		 */
 		fsnotify_sb_delete(sb);
 		security_sb_delete(sb);
 
-		/*
-		 * Now that all potentially-encrypted inodes have been evicted,
-		 * the fscrypt keyring can be destroyed.
-		 */
-		fscrypt_destroy_keyring(sb);
-
 		if (sb->s_dio_done_wq) {
 			destroy_workqueue(sb->s_dio_done_wq);
 			sb->s_dio_done_wq = NULL;
 		}
 
 		if (sop->put_super)
 			sop->put_super(sb);
 
+		/*
+		 * Now that all potentially-encrypted inodes have been evicted,
+		 * the fscrypt keyring can be destroyed.
+		 */
+		fscrypt_destroy_keyring(sb);
+
 		if (CHECK_DATA_CORRUPTION(!list_empty(&sb->s_inodes),
 				"VFS: Busy inodes after unmount of %s (%s)",
 				sb->s_id, sb->s_type->name)) {
 			/*
 			 * Adding a proper bailout path here would be hard, but
 			 * we can at least make it more likely that a later
 			 * iput_final() or such crashes cleanly.
 			 */
 			struct inode *inode;
 
-- 
2.43.0

Re: [PATCH 1/3] btrfs: call btrfs_close_devices from ->kill_sb

[Christoph Hellwig]

On Tue, Dec 12, 2023 at 08:00:16PM -0800, Eric Biggers wrote:
> From: Christoph Hellwig <hch@lst.de>
> 
> blkdev_put must not be called under sb->s_umount to avoid a lock order
> reversal with disk->open_mutex once call backs from block devices to
> the file system using the holder ops are supported.  Move the call
> to btrfs_close_devices into btrfs_free_fs_info so that it is closed
> from ->kill_sb (which is also called from the mount failure handling
> path unlike ->put_super) as well as when an fs_info is freed because
> an existing superblock already exists.

Thanks, this looks roughly the same to what I have locally.

I did in fact forward port everything missing from the get_super
series yesterday, but on my test setup btrfs/142 hangs even in the
baseline setup.  I went back to Linux before giving up for now.

Josef, any chane you could throw this branch:

    git://git.infradead.org/users/hch/misc.git btrfs-holder

into your CI setup and see if it sticks?  Except for the trivial last
three patches this is basically what you reviewed already, although
there was some heavy rebasing due to the mount API converison.

Re: [PATCH 3/3] fs: move fscrypt keyring destruction to after ->put_super

[Christoph Hellwig]

Looks good:

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

Re: [PATCH 3/3] fs: move fscrypt keyring destruction to after ->put_super

[Neal Gompa]

On Tue, Dec 12, 2023 at 11:01 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> From: Josef Bacik <josef@toxicpanda.com>
>
> btrfs has a variety of asynchronous things we do with inodes that can
> potentially last until ->put_super, when we shut everything down and
> clean up all of our async work.  Due to this we need to move
> fscrypt_destroy_keyring() to after ->put_super, otherwise we get
> warnings about still having active references on the master key.
>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  fs/super.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/fs/super.c b/fs/super.c
> index 076392396e724..faf7d248145d2 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -674,34 +674,34 @@ void generic_shutdown_super(struct super_block *sb)
>                 /* Evict all inodes with zero refcount. */
>                 evict_inodes(sb);
>
>                 /*
>                  * Clean up and evict any inodes that still have references due
>                  * to fsnotify or the security policy.
>                  */
>                 fsnotify_sb_delete(sb);
>                 security_sb_delete(sb);
>
> -               /*
> -                * Now that all potentially-encrypted inodes have been evicted,
> -                * the fscrypt keyring can be destroyed.
> -                */
> -               fscrypt_destroy_keyring(sb);
> -
>                 if (sb->s_dio_done_wq) {
>                         destroy_workqueue(sb->s_dio_done_wq);
>                         sb->s_dio_done_wq = NULL;
>                 }
>
>                 if (sop->put_super)
>                         sop->put_super(sb);
>
> +               /*
> +                * Now that all potentially-encrypted inodes have been evicted,
> +                * the fscrypt keyring can be destroyed.
> +                */
> +               fscrypt_destroy_keyring(sb);
> +
>                 if (CHECK_DATA_CORRUPTION(!list_empty(&sb->s_inodes),
>                                 "VFS: Busy inodes after unmount of %s (%s)",
>                                 sb->s_id, sb->s_type->name)) {
>                         /*
>                          * Adding a proper bailout path here would be hard, but
>                          * we can at least make it more likely that a later
>                          * iput_final() or such crashes cleanly.
>                          */
>                         struct inode *inode;
>
> --
> 2.43.0
>
>

This makes sense to me.

Reviewed-by: Neal Gompa <neal@gompa.dev>



--
真実はいつも一つ！/ Always, there's only one truth!

Re: [PATCH 1/3] btrfs: call btrfs_close_devices from ->kill_sb

[Josef Bacik]

On Wed, Dec 13, 2023 at 09:41:23AM +0100, Christoph Hellwig wrote:
> On Tue, Dec 12, 2023 at 08:00:16PM -0800, Eric Biggers wrote:
> > From: Christoph Hellwig <hch@lst.de>
> > 
> > blkdev_put must not be called under sb->s_umount to avoid a lock order
> > reversal with disk->open_mutex once call backs from block devices to
> > the file system using the holder ops are supported.  Move the call
> > to btrfs_close_devices into btrfs_free_fs_info so that it is closed
> > from ->kill_sb (which is also called from the mount failure handling
> > path unlike ->put_super) as well as when an fs_info is freed because
> > an existing superblock already exists.
> 
> Thanks, this looks roughly the same to what I have locally.
> 
> I did in fact forward port everything missing from the get_super
> series yesterday, but on my test setup btrfs/142 hangs even in the
> baseline setup.  I went back to Linux before giving up for now.
> 
> Josef, any chane you could throw this branch:
> 
>     git://git.infradead.org/users/hch/misc.git btrfs-holder
> 
> into your CI setup and see if it sticks?  Except for the trivial last
> three patches this is basically what you reviewed already, although
> there was some heavy rebasing due to the mount API converison.
> 

Yup, sorry Christoph I missed this email when you sent it, I'll throw it in
there now.  Thanks,

Josef

Re: [f2fs-dev] [PATCH 1/3] btrfs: call btrfs_close_devices from ->kill_sb

[Josef Bacik]

On Wed, Dec 13, 2023 at 09:41:23AM +0100, Christoph Hellwig wrote:
> On Tue, Dec 12, 2023 at 08:00:16PM -0800, Eric Biggers wrote:
> > From: Christoph Hellwig <hch@lst.de>
> > 
> > blkdev_put must not be called under sb->s_umount to avoid a lock order
> > reversal with disk->open_mutex once call backs from block devices to
> > the file system using the holder ops are supported.  Move the call
> > to btrfs_close_devices into btrfs_free_fs_info so that it is closed
> > from ->kill_sb (which is also called from the mount failure handling
> > path unlike ->put_super) as well as when an fs_info is freed because
> > an existing superblock already exists.
> 
> Thanks, this looks roughly the same to what I have locally.
> 
> I did in fact forward port everything missing from the get_super
> series yesterday, but on my test setup btrfs/142 hangs even in the
> baseline setup.  I went back to Linux before giving up for now.
> 
> Josef, any chane you could throw this branch:
> 
>     git://git.infradead.org/users/hch/misc.git btrfs-holder
> 
> into your CI setup and see if it sticks?  Except for the trivial last
> three patches this is basically what you reviewed already, although
> there was some heavy rebasing due to the mount API converison.

I ran it through, you broke a test that isn't upstream yet to test the old mount
api double mount thing that I have a test for

https://github.com/btrfs/fstests/commit/2796723e77adb0f9da1059acf13fc402467f7ac4

In this case we end up leaking a reference on the fs_devices.  If you add this
fixup to "btrfs: call btrfs_close_devices from ->kill_sb" it fixes that failure.
I'm re-running with that fixup applied, but I assume the rest is fine.  Thanks,

Josef

Re: [f2fs-dev] [PATCH 1/3] btrfs: call btrfs_close_devices from ->kill_sb

[Christoph Hellwig]

On Fri, Dec 15, 2023 at 04:45:50PM -0500, Josef Bacik wrote:
> I ran it through, you broke a test that isn't upstream yet to test the old mount
> api double mount thing that I have a test for
> 
> https://github.com/btrfs/fstests/commit/2796723e77adb0f9da1059acf13fc402467f7ac4
> 
> In this case we end up leaking a reference on the fs_devices.  If you add this
> fixup to "btrfs: call btrfs_close_devices from ->kill_sb" it fixes that failure.
> I'm re-running with that fixup applied, but I assume the rest is fine.  Thanks,

Is "this fixup" referring to a patch that was supposed to be attached
but is't? :)

Re: [f2fs-dev] [PATCH 1/3] btrfs: call btrfs_close_devices from ->kill_sb

[Josef Bacik]

On Sat, Dec 16, 2023 at 05:12:21AM +0100, Christoph Hellwig wrote:
> On Fri, Dec 15, 2023 at 04:45:50PM -0500, Josef Bacik wrote:
> > I ran it through, you broke a test that isn't upstream yet to test the old mount
> > api double mount thing that I have a test for
> > 
> > https://github.com/btrfs/fstests/commit/2796723e77adb0f9da1059acf13fc402467f7ac4
> > 
> > In this case we end up leaking a reference on the fs_devices.  If you add this
> > fixup to "btrfs: call btrfs_close_devices from ->kill_sb" it fixes that failure.
> > I'm re-running with that fixup applied, but I assume the rest is fine.  Thanks,
> 
> Is "this fixup" referring to a patch that was supposed to be attached
> but is't? :)

Sorry, vacation brain, here you go.

Josef

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index f93fe2e5e378..2dfa2274b193 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -1950,10 +1950,20 @@ static int btrfs_get_tree_super(struct fs_context *fc)
  */
 static struct vfsmount *btrfs_reconfigure_for_mount(struct fs_context *fc)
 {
+	struct btrfs_fs_info *fs_info = fc->s_fs_info;
 	struct vfsmount *mnt;
 	int ret;
 	const bool ro2rw = !(fc->sb_flags & SB_RDONLY);
 
+	/*
+	 * We got a reference to our fs_devices, so we need to close it here to
+	 * make sure we don't leak our reference on the fs_devices.
+	 */
+	if (fs_info->fs_devices) {
+		btrfs_close_devices(fs_info->fs_devices);
+		fs_info->fs_devices = NULL;
+	}
+
 	/*
 	 * We got an EBUSY because our SB_RDONLY flag didn't match the existing
 	 * super block, so invert our setting here and retry the mount so we

Re: [PATCH 2/3] f2fs: move release of block devices to after kill_block_super()

[Eric Biggers]

On Tue, Dec 12, 2023 at 08:00:17PM -0800, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Call destroy_device_list() and free the f2fs_sb_info from
> kill_f2fs_super(), after the call to kill_block_super().  This is
> necessary to order it after the call to fscrypt_destroy_keyring() once
> generic_shutdown_super() starts calling fscrypt_destroy_keyring() just
> after calling ->put_super.  This is because fscrypt_destroy_keyring()
> may call into f2fs_get_devices() via the fscrypt_operations.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  fs/f2fs/super.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)

Jaegeuk and Chao, when you have a chance can you review or ack this?  I'm
thinking of taking patches 2-3 of this series through the fscrypt tree for 6.8.

- Eric

Re: [f2fs-dev] [PATCH 2/3] f2fs: move release of block devices to after kill_block_super()

[Chao Yu]

On 2023/12/13 12:00, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Call destroy_device_list() and free the f2fs_sb_info from
> kill_f2fs_super(), after the call to kill_block_super().  This is
> necessary to order it after the call to fscrypt_destroy_keyring() once
> generic_shutdown_super() starts calling fscrypt_destroy_keyring() just
> after calling ->put_super.  This is because fscrypt_destroy_keyring()
> may call into f2fs_get_devices() via the fscrypt_operations.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>   fs/f2fs/super.c | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index 033af907c3b1d..ba95a341a9a36 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -1710,42 +1710,39 @@ static void f2fs_put_super(struct super_block *sb)
>   	f2fs_destroy_node_manager(sbi);
>   	f2fs_destroy_segment_manager(sbi);
>   
>   	/* flush s_error_work before sbi destroy */
>   	flush_work(&sbi->s_error_work);
>   
>   	f2fs_destroy_post_read_wq(sbi);
>   
>   	kvfree(sbi->ckpt);
>   
> -	sb->s_fs_info = NULL;
>   	if (sbi->s_chksum_driver)
>   		crypto_free_shash(sbi->s_chksum_driver);
>   	kfree(sbi->raw_super);
>   
> -	destroy_device_list(sbi);
>   	f2fs_destroy_page_array_cache(sbi);
>   	f2fs_destroy_xattr_caches(sbi);
>   	mempool_destroy(sbi->write_io_dummy);
>   #ifdef CONFIG_QUOTA
>   	for (i = 0; i < MAXQUOTAS; i++)
>   		kfree(F2FS_OPTION(sbi).s_qf_names[i]);
>   #endif
>   	fscrypt_free_dummy_policy(&F2FS_OPTION(sbi).dummy_enc_policy);
>   	destroy_percpu_info(sbi);
>   	f2fs_destroy_iostat(sbi);
>   	for (i = 0; i < NR_PAGE_TYPE; i++)
>   		kvfree(sbi->write_io[i]);
>   #if IS_ENABLED(CONFIG_UNICODE)
>   	utf8_unload(sb->s_encoding);
>   #endif
> -	kfree(sbi);
>   }
>   
>   int f2fs_sync_fs(struct super_block *sb, int sync)
>   {
>   	struct f2fs_sb_info *sbi = F2FS_SB(sb);
>   	int err = 0;
>   
>   	if (unlikely(f2fs_cp_error(sbi)))
>   		return 0;
>   	if (unlikely(is_sbi_flag_set(sbi, SBI_CP_DISABLED)))
> @@ -4895,23 +4892,23 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
>   }
>   
>   static struct dentry *f2fs_mount(struct file_system_type *fs_type, int flags,
>   			const char *dev_name, void *data)
>   {
>   	return mount_bdev(fs_type, flags, dev_name, data, f2fs_fill_super);
>   }
>   
>   static void kill_f2fs_super(struct super_block *sb)
>   {
> -	if (sb->s_root) {
> -		struct f2fs_sb_info *sbi = F2FS_SB(sb);
> +	struct f2fs_sb_info *sbi = F2FS_SB(sb);
>   
> +	if (sb->s_root) {
>   		set_sbi_flag(sbi, SBI_IS_CLOSE);
>   		f2fs_stop_gc_thread(sbi);
>   		f2fs_stop_discard_thread(sbi);
>   
>   #ifdef CONFIG_F2FS_FS_COMPRESSION
>   		/*
>   		 * latter evict_inode() can bypass checking and invalidating
>   		 * compress inode cache.
>   		 */
>   		if (test_opt(sbi, COMPRESS_CACHE))
> @@ -4924,20 +4921,25 @@ static void kill_f2fs_super(struct super_block *sb)
>   				.reason = CP_UMOUNT,
>   			};
>   			stat_inc_cp_call_count(sbi, TOTAL_CALL);
>   			f2fs_write_checkpoint(sbi, &cpc);
>   		}
>   
>   		if (is_sbi_flag_set(sbi, SBI_IS_RECOVERED) && f2fs_readonly(sb))
>   			sb->s_flags &= ~SB_RDONLY;
>   	}
>   	kill_block_super(sb);
> +	if (sbi) {

Can you please add one single line comment here to expand why we
need to delay destroying device_list?

Other code part looks good to me.

Thanks,

> +		destroy_device_list(sbi);
> +		kfree(sbi);
> +		sb->s_fs_info = NULL;
> +	}
>   }
>   
>   static struct file_system_type f2fs_fs_type = {
>   	.owner		= THIS_MODULE,
>   	.name		= "f2fs",
>   	.mount		= f2fs_mount,
>   	.kill_sb	= kill_f2fs_super,
>   	.fs_flags	= FS_REQUIRES_DEV | FS_ALLOW_IDMAP,
>   };
>   MODULE_ALIAS_FS("f2fs");