[PATCH v4 1/6] fs: enhance and rename shutdown() callback to remove_bdev()

[PATCH v4 1/6] fs: enhance and rename shutdown() callback to remove_bdev()

[Qu Wenruo]

Currently all the filesystems implementing the
super_opearations::shutdown() callback can not afford losing a device.

Thus fs_bdev_mark_dead() will just call the shutdown() callback for the
involved filesystem.

But it will no longer be the case, with multi-device filesystems like
btrfs and bcachefs the filesystem can handle certain device loss without
shutting down the whole filesystem.

To allow those multi-device filesystems to be integrated to use
fs_holder_ops:

- Replace super_opearation::shutdown() with
  super_opearations::remove_bdev()
  To better describe when the callback is called.

- Add a new @bdev parameter to remove_bdev() callback
  To allow the fs to determine which device is missing, and do the
  proper handling when needed.

For the existing shutdown callback users, the change is minimal.
They only need to follow the rename and the new parameter list.
The new @bdev parameter can be ignored if the filesystem can not afford
losing any device, and continue using the old shutdown behavior.

Btrfs is going to implement the callback soon, which will either
shutdown the fs or continue read-write operations.

Cc: linux-fsdevel@vger.kernel.org
Cc: linux-ext4@vger.kernel.org
Cc: linux-f2fs-devel@lists.sourceforge.net
Cc: ntfs3@lists.linux.dev
Cc: linux-xfs@vger.kernel.org
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/exfat/super.c   | 4 ++--
 fs/ext4/super.c    | 4 ++--
 fs/f2fs/super.c    | 4 ++--
 fs/ntfs3/super.c   | 6 +++---
 fs/super.c         | 4 ++--
 fs/xfs/xfs_super.c | 7 ++++---
 include/linux/fs.h | 7 ++++++-
 7 files changed, 21 insertions(+), 15 deletions(-)

diff --git a/fs/exfat/super.c b/fs/exfat/super.c
index 7ed858937d45..a0e11166b194 100644
--- a/fs/exfat/super.c
+++ b/fs/exfat/super.c
@@ -172,7 +172,7 @@ int exfat_force_shutdown(struct super_block *sb, u32 flags)
 	return 0;
 }
 
-static void exfat_shutdown(struct super_block *sb)
+static void exfat_remove_bdev(struct super_block *sb, struct block_device *bdev)
 {
 	exfat_force_shutdown(sb, EXFAT_GOING_DOWN_NOSYNC);
 }
@@ -202,7 +202,7 @@ static const struct super_operations exfat_sops = {
 	.put_super	= exfat_put_super,
 	.statfs		= exfat_statfs,
 	.show_options	= exfat_show_options,
-	.shutdown	= exfat_shutdown,
+	.remove_bdev	= exfat_remove_bdev,
 };
 
 enum {
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index c7d39da7e733..d75b416401ae 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1456,7 +1456,7 @@ static void ext4_destroy_inode(struct inode *inode)
 			 EXT4_I(inode)->i_reserved_data_blocks);
 }
 
-static void ext4_shutdown(struct super_block *sb)
+static void ext4_remove_bdev(struct super_block *sb, struct block_device *bdev)
 {
        ext4_force_shutdown(sb, EXT4_GOING_FLAGS_NOLOGFLUSH);
 }
@@ -1620,7 +1620,7 @@ static const struct super_operations ext4_sops = {
 	.unfreeze_fs	= ext4_unfreeze,
 	.statfs		= ext4_statfs,
 	.show_options	= ext4_show_options,
-	.shutdown	= ext4_shutdown,
+	.remove_bdev	= ext4_remove_bdev,
 #ifdef CONFIG_QUOTA
 	.quota_read	= ext4_quota_read,
 	.quota_write	= ext4_quota_write,
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index bbf1dad6843f..8667af9f76e4 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -2640,7 +2640,7 @@ static int f2fs_remount(struct super_block *sb, int *flags, char *data)
 	return err;
 }
 
-static void f2fs_shutdown(struct super_block *sb)
+static void f2fs_remove_bdev(struct super_block *sb, struct block_device *bdev)
 {
 	f2fs_do_shutdown(F2FS_SB(sb), F2FS_GOING_DOWN_NOSYNC, false, false);
 }
@@ -3264,7 +3264,7 @@ static const struct super_operations f2fs_sops = {
 	.unfreeze_fs	= f2fs_unfreeze,
 	.statfs		= f2fs_statfs,
 	.remount_fs	= f2fs_remount,
-	.shutdown	= f2fs_shutdown,
+	.remove_bdev	= f2fs_remove_bdev,
 };
 
 #ifdef CONFIG_FS_ENCRYPTION
diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c
index 920a1ab47b63..3e69dc805e3a 100644
--- a/fs/ntfs3/super.c
+++ b/fs/ntfs3/super.c
@@ -762,9 +762,9 @@ static int ntfs_show_options(struct seq_file *m, struct dentry *root)
 }
 
 /*
- * ntfs_shutdown - super_operations::shutdown
+ * ntfs_remove_bdev - super_operations::remove_bdev
  */
-static void ntfs_shutdown(struct super_block *sb)
+static void ntfs_remove_bdev(struct super_block *sb, struct block_device *bdev)
 {
 	set_bit(NTFS_FLAGS_SHUTDOWN_BIT, &ntfs_sb(sb)->flags);
 }
@@ -821,7 +821,7 @@ static const struct super_operations ntfs_sops = {
 	.put_super = ntfs_put_super,
 	.statfs = ntfs_statfs,
 	.show_options = ntfs_show_options,
-	.shutdown = ntfs_shutdown,
+	.remove_bdev = ntfs_remove_bdev,
 	.sync_fs = ntfs_sync_fs,
 	.write_inode = ntfs3_write_inode,
 };
diff --git a/fs/super.c b/fs/super.c
index 80418ca8e215..c972efb38f6a 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -1463,8 +1463,8 @@ static void fs_bdev_mark_dead(struct block_device *bdev, bool surprise)
 		sync_filesystem(sb);
 	shrink_dcache_sb(sb);
 	evict_inodes(sb);
-	if (sb->s_op->shutdown)
-		sb->s_op->shutdown(sb);
+	if (sb->s_op->remove_bdev)
+		sb->s_op->remove_bdev(sb, bdev);
 
 	super_unlock_shared(sb);
 }
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 0bc4b5489078..8e307b036133 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1276,8 +1276,9 @@ xfs_fs_free_cached_objects(
 }
 
 static void
-xfs_fs_shutdown(
-	struct super_block	*sb)
+xfs_fs_remove_bdev(
+	struct super_block	*sb,
+	struct block_device	*bdev)
 {
 	xfs_force_shutdown(XFS_M(sb), SHUTDOWN_DEVICE_REMOVED);
 }
@@ -1308,7 +1309,7 @@ static const struct super_operations xfs_super_operations = {
 	.show_options		= xfs_fs_show_options,
 	.nr_cached_objects	= xfs_fs_nr_cached_objects,
 	.free_cached_objects	= xfs_fs_free_cached_objects,
-	.shutdown		= xfs_fs_shutdown,
+	.remove_bdev		= xfs_fs_remove_bdev,
 	.show_stats		= xfs_fs_show_stats,
 };
 
diff --git a/include/linux/fs.h b/include/linux/fs.h
index b085f161ed22..b08af63d2d4f 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2367,7 +2367,12 @@ struct super_operations {
 				  struct shrink_control *);
 	long (*free_cached_objects)(struct super_block *,
 				    struct shrink_control *);
-	void (*shutdown)(struct super_block *sb);
+	/*
+	 * Called when block device @bdev belonging to @sb is removed.
+	 *
+	 * If the fs can't afford the device loss, it should be shutdown.
+	 */
+	void (*remove_bdev)(struct super_block *sb, struct block_device *bdev);
 };
 
 /*
-- 
2.50.0

Re: (subset) [PATCH v4 1/6] fs: enhance and rename shutdown() callback to remove_bdev()

[Christian Brauner]

On Fri, 04 Jul 2025 10:12:29 +0930, Qu Wenruo wrote:
> Currently all the filesystems implementing the
> super_opearations::shutdown() callback can not afford losing a device.
> 
> Thus fs_bdev_mark_dead() will just call the shutdown() callback for the
> involved filesystem.
> 
> But it will no longer be the case, with multi-device filesystems like
> btrfs and bcachefs the filesystem can handle certain device loss without
> shutting down the whole filesystem.
> 
> [...]

Moving this into a stable branch that won't change anymore and that you
can pull into btrfs/use as base for your patches.

---

Applied to the vfs-6.17.super branch of the vfs/vfs.git tree.
Patches in the vfs-6.17.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-6.17.super

[1/6] fs: enhance and rename shutdown() callback to remove_bdev()
      https://git.kernel.org/vfs/vfs/c/165fa94de612

Re: [PATCH v4 1/6] fs: enhance and rename shutdown() callback to remove_bdev()

[Jan Kara]

OK, now that you've made me read the changelog :) :

On Fri 04-07-25 10:12:29, Qu Wenruo wrote:
> Currently all the filesystems implementing the
> super_opearations::shutdown() callback can not afford losing a device.
        ^^^ operations

> Thus fs_bdev_mark_dead() will just call the shutdown() callback for the
> involved filesystem.
> 
> But it will no longer be the case, with multi-device filesystems like
> btrfs and bcachefs the filesystem can handle certain device loss without
> shutting down the whole filesystem.
> 
> To allow those multi-device filesystems to be integrated to use
> fs_holder_ops:
> 
> - Replace super_opearation::shutdown() with
>   super_opearations::remove_bdev()
    ^^ again typos in work "operations"

>   To better describe when the callback is called.
> 
> - Add a new @bdev parameter to remove_bdev() callback
>   To allow the fs to determine which device is missing, and do the
>   proper handling when needed.
> 
> For the existing shutdown callback users, the change is minimal.
> They only need to follow the rename and the new parameter list.
> The new @bdev parameter can be ignored if the filesystem can not afford
> losing any device, and continue using the old shutdown behavior.
> 
> Btrfs is going to implement the callback soon, which will either
> shutdown the fs or continue read-write operations.
> 
> Cc: linux-fsdevel@vger.kernel.org
> Cc: linux-ext4@vger.kernel.org
> Cc: linux-f2fs-devel@lists.sourceforge.net
> Cc: ntfs3@lists.linux.dev
> Cc: linux-xfs@vger.kernel.org
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Still feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/exfat/super.c   | 4 ++--
>  fs/ext4/super.c    | 4 ++--
>  fs/f2fs/super.c    | 4 ++--
>  fs/ntfs3/super.c   | 6 +++---
>  fs/super.c         | 4 ++--
>  fs/xfs/xfs_super.c | 7 ++++---
>  include/linux/fs.h | 7 ++++++-
>  7 files changed, 21 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/exfat/super.c b/fs/exfat/super.c
> index 7ed858937d45..a0e11166b194 100644
> --- a/fs/exfat/super.c
> +++ b/fs/exfat/super.c
> @@ -172,7 +172,7 @@ int exfat_force_shutdown(struct super_block *sb, u32 flags)
>  	return 0;
>  }
>  
> -static void exfat_shutdown(struct super_block *sb)
> +static void exfat_remove_bdev(struct super_block *sb, struct block_device *bdev)
>  {
>  	exfat_force_shutdown(sb, EXFAT_GOING_DOWN_NOSYNC);
>  }
> @@ -202,7 +202,7 @@ static const struct super_operations exfat_sops = {
>  	.put_super	= exfat_put_super,
>  	.statfs		= exfat_statfs,
>  	.show_options	= exfat_show_options,
> -	.shutdown	= exfat_shutdown,
> +	.remove_bdev	= exfat_remove_bdev,
>  };
>  
>  enum {
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index c7d39da7e733..d75b416401ae 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -1456,7 +1456,7 @@ static void ext4_destroy_inode(struct inode *inode)
>  			 EXT4_I(inode)->i_reserved_data_blocks);
>  }
>  
> -static void ext4_shutdown(struct super_block *sb)
> +static void ext4_remove_bdev(struct super_block *sb, struct block_device *bdev)
>  {
>         ext4_force_shutdown(sb, EXT4_GOING_FLAGS_NOLOGFLUSH);
>  }
> @@ -1620,7 +1620,7 @@ static const struct super_operations ext4_sops = {
>  	.unfreeze_fs	= ext4_unfreeze,
>  	.statfs		= ext4_statfs,
>  	.show_options	= ext4_show_options,
> -	.shutdown	= ext4_shutdown,
> +	.remove_bdev	= ext4_remove_bdev,
>  #ifdef CONFIG_QUOTA
>  	.quota_read	= ext4_quota_read,
>  	.quota_write	= ext4_quota_write,
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index bbf1dad6843f..8667af9f76e4 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -2640,7 +2640,7 @@ static int f2fs_remount(struct super_block *sb, int *flags, char *data)
>  	return err;
>  }
>  
> -static void f2fs_shutdown(struct super_block *sb)
> +static void f2fs_remove_bdev(struct super_block *sb, struct block_device *bdev)
>  {
>  	f2fs_do_shutdown(F2FS_SB(sb), F2FS_GOING_DOWN_NOSYNC, false, false);
>  }
> @@ -3264,7 +3264,7 @@ static const struct super_operations f2fs_sops = {
>  	.unfreeze_fs	= f2fs_unfreeze,
>  	.statfs		= f2fs_statfs,
>  	.remount_fs	= f2fs_remount,
> -	.shutdown	= f2fs_shutdown,
> +	.remove_bdev	= f2fs_remove_bdev,
>  };
>  
>  #ifdef CONFIG_FS_ENCRYPTION
> diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c
> index 920a1ab47b63..3e69dc805e3a 100644
> --- a/fs/ntfs3/super.c
> +++ b/fs/ntfs3/super.c
> @@ -762,9 +762,9 @@ static int ntfs_show_options(struct seq_file *m, struct dentry *root)
>  }
>  
>  /*
> - * ntfs_shutdown - super_operations::shutdown
> + * ntfs_remove_bdev - super_operations::remove_bdev
>   */
> -static void ntfs_shutdown(struct super_block *sb)
> +static void ntfs_remove_bdev(struct super_block *sb, struct block_device *bdev)
>  {
>  	set_bit(NTFS_FLAGS_SHUTDOWN_BIT, &ntfs_sb(sb)->flags);
>  }
> @@ -821,7 +821,7 @@ static const struct super_operations ntfs_sops = {
>  	.put_super = ntfs_put_super,
>  	.statfs = ntfs_statfs,
>  	.show_options = ntfs_show_options,
> -	.shutdown = ntfs_shutdown,
> +	.remove_bdev = ntfs_remove_bdev,
>  	.sync_fs = ntfs_sync_fs,
>  	.write_inode = ntfs3_write_inode,
>  };
> diff --git a/fs/super.c b/fs/super.c
> index 80418ca8e215..c972efb38f6a 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -1463,8 +1463,8 @@ static void fs_bdev_mark_dead(struct block_device *bdev, bool surprise)
>  		sync_filesystem(sb);
>  	shrink_dcache_sb(sb);
>  	evict_inodes(sb);
> -	if (sb->s_op->shutdown)
> -		sb->s_op->shutdown(sb);
> +	if (sb->s_op->remove_bdev)
> +		sb->s_op->remove_bdev(sb, bdev);
>  
>  	super_unlock_shared(sb);
>  }
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 0bc4b5489078..8e307b036133 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1276,8 +1276,9 @@ xfs_fs_free_cached_objects(
>  }
>  
>  static void
> -xfs_fs_shutdown(
> -	struct super_block	*sb)
> +xfs_fs_remove_bdev(
> +	struct super_block	*sb,
> +	struct block_device	*bdev)
>  {
>  	xfs_force_shutdown(XFS_M(sb), SHUTDOWN_DEVICE_REMOVED);
>  }
> @@ -1308,7 +1309,7 @@ static const struct super_operations xfs_super_operations = {
>  	.show_options		= xfs_fs_show_options,
>  	.nr_cached_objects	= xfs_fs_nr_cached_objects,
>  	.free_cached_objects	= xfs_fs_free_cached_objects,
> -	.shutdown		= xfs_fs_shutdown,
> +	.remove_bdev		= xfs_fs_remove_bdev,
>  	.show_stats		= xfs_fs_show_stats,
>  };
>  
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index b085f161ed22..b08af63d2d4f 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2367,7 +2367,12 @@ struct super_operations {
>  				  struct shrink_control *);
>  	long (*free_cached_objects)(struct super_block *,
>  				    struct shrink_control *);
> -	void (*shutdown)(struct super_block *sb);
> +	/*
> +	 * Called when block device @bdev belonging to @sb is removed.
> +	 *
> +	 * If the fs can't afford the device loss, it should be shutdown.
> +	 */
> +	void (*remove_bdev)(struct super_block *sb, struct block_device *bdev);
>  };
>  
>  /*
> -- 
> 2.50.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v4 1/6] fs: enhance and rename shutdown() callback to remove_bdev()

[Dave Chinner]

On Fri, Jul 04, 2025 at 10:12:29AM +0930, Qu Wenruo wrote:
> Currently all the filesystems implementing the
> super_opearations::shutdown() callback can not afford losing a device.
> 
> Thus fs_bdev_mark_dead() will just call the shutdown() callback for the
> involved filesystem.
> 
> But it will no longer be the case, with multi-device filesystems like
> btrfs and bcachefs the filesystem can handle certain device loss without
> shutting down the whole filesystem.
> 
> To allow those multi-device filesystems to be integrated to use
> fs_holder_ops:
> 
> - Replace super_opearation::shutdown() with
>   super_opearations::remove_bdev()
>   To better describe when the callback is called.

This conflates cause with action.

The shutdown callout is an action that the filesystem must execute,
whilst "remove bdev" is a cause notification that might require an
action to be take.

Yes, the cause could be someone doing hot-unplug of the block
device, but it could also be something going wrong in software
layers below the filesystem. e.g. dm-thinp having an unrecoverable
corruption or ENOSPC errors.

We already have a "cause" notification: blk_holder_ops->mark_dead().

The generic fs action that is taken by this notification is
fs_bdev_mark_dead().  That action is to invalidate caches and shut
down the filesystem.

btrfs needs to do something different to a blk_holder_ops->mark_dead
notification. i.e. it needs an action that is different to
fs_bdev_mark_dead().

Indeed, this is how bcachefs already handles "single device
died" events for multi-device filesystems - see
bch2_fs_bdev_mark_dead().

Hence Btrfs should be doing the same thing as bcachefs. The
bdev_handle_ops structure exists precisly because it allows the
filesystem to handle block device events in the exact manner they
require....

> - Add a new @bdev parameter to remove_bdev() callback
>   To allow the fs to determine which device is missing, and do the
>   proper handling when needed.
> 
> For the existing shutdown callback users, the change is minimal.

Except for the change in API semantics. ->shutdown is an external
shutdown trigger for the filesystem, not a generic "block device
removed" notification.

Hooking blk_holder_ops->mark_dead means that btrfs can also provide
a ->shutdown implementation for when something external other than a
block device removal needs to shut down the filesystem....

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

Re: [PATCH v4 1/6] fs: enhance and rename shutdown() callback to remove_bdev()

[Qu Wenruo]

在 2025/7/8 08:32, Dave Chinner 写道:
> On Fri, Jul 04, 2025 at 10:12:29AM +0930, Qu Wenruo wrote:
>> Currently all the filesystems implementing the
>> super_opearations::shutdown() callback can not afford losing a device.
>>
>> Thus fs_bdev_mark_dead() will just call the shutdown() callback for the
>> involved filesystem.
>>
>> But it will no longer be the case, with multi-device filesystems like
>> btrfs and bcachefs the filesystem can handle certain device loss without
>> shutting down the whole filesystem.
>>
>> To allow those multi-device filesystems to be integrated to use
>> fs_holder_ops:
>>
>> - Replace super_opearation::shutdown() with
>>    super_opearations::remove_bdev()
>>    To better describe when the callback is called.
> 
> This conflates cause with action.
> 
> The shutdown callout is an action that the filesystem must execute,
> whilst "remove bdev" is a cause notification that might require an
> action to be take.
> 
> Yes, the cause could be someone doing hot-unplug of the block
> device, but it could also be something going wrong in software
> layers below the filesystem. e.g. dm-thinp having an unrecoverable
> corruption or ENOSPC errors.
> 
> We already have a "cause" notification: blk_holder_ops->mark_dead().
> 
> The generic fs action that is taken by this notification is
> fs_bdev_mark_dead().  That action is to invalidate caches and shut
> down the filesystem.
> 
> btrfs needs to do something different to a blk_holder_ops->mark_dead
> notification. i.e. it needs an action that is different to
> fs_bdev_mark_dead().
> 
> Indeed, this is how bcachefs already handles "single device
> died" events for multi-device filesystems - see
> bch2_fs_bdev_mark_dead().

I do not think it's the correct way to go, especially when there is 
already fs_holder_ops.

We're always going towards a more generic solution, other than letting 
the individual fs to do the same thing slightly differently.

Yes, the naming is not perfect and mixing cause and action, but the end 
result is still a more generic and less duplicated code base.

> 
> Hence Btrfs should be doing the same thing as bcachefs. The
> bdev_handle_ops structure exists precisly because it allows the
> filesystem to handle block device events in the exact manner they
> require....
> 
>> - Add a new @bdev parameter to remove_bdev() callback
>>    To allow the fs to determine which device is missing, and do the
>>    proper handling when needed.
>>
>> For the existing shutdown callback users, the change is minimal.
> 
> Except for the change in API semantics. ->shutdown is an external
> shutdown trigger for the filesystem, not a generic "block device
> removed" notification.

The problem is, there is no one utilizing ->shutdown() out of 
fs_bdev_mark_dead().

If shutdown ioctl is handled through super_operations::shutdown, it will 
be more meaningful to split shutdown and dev removal.

But that's not the case, and different fses even have slightly different 
handling for the shutdown flags (not all fses even utilize journal to 
protect their metadata).

Thanks,
Qu


> 
> Hooking blk_holder_ops->mark_dead means that btrfs can also provide
> a ->shutdown implementation for when something external other than a
> block device removal needs to shut down the filesystem....
> 
> -Dave.

Re: [PATCH v4 1/6] fs: enhance and rename shutdown() callback to remove_bdev()

[Darrick J. Wong]

On Tue, Jul 08, 2025 at 08:52:47AM +0930, Qu Wenruo wrote:
> 
> 
> 在 2025/7/8 08:32, Dave Chinner 写道:
> > On Fri, Jul 04, 2025 at 10:12:29AM +0930, Qu Wenruo wrote:
> > > Currently all the filesystems implementing the
> > > super_opearations::shutdown() callback can not afford losing a device.
> > > 
> > > Thus fs_bdev_mark_dead() will just call the shutdown() callback for the
> > > involved filesystem.
> > > 
> > > But it will no longer be the case, with multi-device filesystems like
> > > btrfs and bcachefs the filesystem can handle certain device loss without
> > > shutting down the whole filesystem.
> > > 
> > > To allow those multi-device filesystems to be integrated to use
> > > fs_holder_ops:
> > > 
> > > - Replace super_opearation::shutdown() with
> > >    super_opearations::remove_bdev()
> > >    To better describe when the callback is called.
> > 
> > This conflates cause with action.
> > 
> > The shutdown callout is an action that the filesystem must execute,
> > whilst "remove bdev" is a cause notification that might require an
> > action to be take.
> > 
> > Yes, the cause could be someone doing hot-unplug of the block
> > device, but it could also be something going wrong in software
> > layers below the filesystem. e.g. dm-thinp having an unrecoverable
> > corruption or ENOSPC errors.
> > 
> > We already have a "cause" notification: blk_holder_ops->mark_dead().
> > 
> > The generic fs action that is taken by this notification is
> > fs_bdev_mark_dead().  That action is to invalidate caches and shut
> > down the filesystem.
> > 
> > btrfs needs to do something different to a blk_holder_ops->mark_dead
> > notification. i.e. it needs an action that is different to
> > fs_bdev_mark_dead().
> > 
> > Indeed, this is how bcachefs already handles "single device
> > died" events for multi-device filesystems - see
> > bch2_fs_bdev_mark_dead().
> 
> I do not think it's the correct way to go, especially when there is already
> fs_holder_ops.
> 
> We're always going towards a more generic solution, other than letting the
> individual fs to do the same thing slightly differently.

On second thought -- it's weird that you'd flush the filesystem and
shrink the inode/dentry caches in a "your device went away" handler.
Fancy filesystems like bcachefs and btrfs would likely just shift IO to
a different bdev, right?  And there's no good reason to run shrinkers on
either of those fses, right?

> Yes, the naming is not perfect and mixing cause and action, but the end
> result is still a more generic and less duplicated code base.

I think dchinner makes a good point that if your filesystem can do
something clever on device removal, it should provide its own block
device holder ops instead of using fs_holder_ops.  I don't understand
why you need a "generic" solution for btrfs when it's not going to do
what the others do anyway.

Awkward naming is often a sign that further thought (or at least
separation of code) is needed.

As an aside:
'twould be nice if we could lift the *FS_IOC_SHUTDOWN dispatch out of
everyone's ioctl functions into the VFS, and then move the "I am dead"
state into super_block so that you could actually shut down any
filesystem, not just the seven that currently implement it.

--D

> > Hence Btrfs should be doing the same thing as bcachefs. The
> > bdev_handle_ops structure exists precisly because it allows the
> > filesystem to handle block device events in the exact manner they
> > require....
> > 
> > > - Add a new @bdev parameter to remove_bdev() callback
> > >    To allow the fs to determine which device is missing, and do the
> > >    proper handling when needed.
> > > 
> > > For the existing shutdown callback users, the change is minimal.
> > 
> > Except for the change in API semantics. ->shutdown is an external
> > shutdown trigger for the filesystem, not a generic "block device
> > removed" notification.
> 
> The problem is, there is no one utilizing ->shutdown() out of
> fs_bdev_mark_dead().
> 
> If shutdown ioctl is handled through super_operations::shutdown, it will be
> more meaningful to split shutdown and dev removal.
> 
> But that's not the case, and different fses even have slightly different
> handling for the shutdown flags (not all fses even utilize journal to
> protect their metadata).
> 
> Thanks,
> Qu
> 
> 
> > 
> > Hooking blk_holder_ops->mark_dead means that btrfs can also provide
> > a ->shutdown implementation for when something external other than a
> > block device removal needs to shut down the filesystem....
> > 
> > -Dave.
> 

Re: [PATCH v4 1/6] fs: enhance and rename shutdown() callback to remove_bdev()

[Qu Wenruo]

在 2025/7/8 10:15, Darrick J. Wong 写道:
[...]
>>
>> I do not think it's the correct way to go, especially when there is already
>> fs_holder_ops.
>>
>> We're always going towards a more generic solution, other than letting the
>> individual fs to do the same thing slightly differently.
> 
> On second thought -- it's weird that you'd flush the filesystem and
> shrink the inode/dentry caches in a "your device went away" handler.
> Fancy filesystems like bcachefs and btrfs would likely just shift IO to
> a different bdev, right?  And there's no good reason to run shrinkers on
> either of those fses, right?

That's right, some part of fs_bdev_mark_dead() is not making much sense 
if the fs can handle the dev loss.

> 
>> Yes, the naming is not perfect and mixing cause and action, but the end
>> result is still a more generic and less duplicated code base.
> 
> I think dchinner makes a good point that if your filesystem can do
> something clever on device removal, it should provide its own block
> device holder ops instead of using fs_holder_ops.

Then re-implement a lot of things like bdev_super_lock()?

I'd prefer not.


fs_holder_ops solves a lot of things like handling mounting/inactive 
fses, and pushing it back again to the fs code is just causing more 
duplication.

Not really worthy if we only want a single different behavior.

Thus I strongly prefer to do with the existing fs_holder_ops, no matter 
if it's using/renaming the shutdown() callback, or a new callback.

>  I don't understand
> why you need a "generic" solution for btrfs when it's not going to do
> what the others do anyway.

Because there is only one behavior different.

Other things like freezing/thawing/syncing are all the same.

Thanks,
Qu

> 
> Awkward naming is often a sign that further thought (or at least
> separation of code) is needed.
> 
> As an aside:
> 'twould be nice if we could lift the *FS_IOC_SHUTDOWN dispatch out of
> everyone's ioctl functions into the VFS, and then move the "I am dead"
> state into super_block so that you could actually shut down any
> filesystem, not just the seven that currently implement it.
> 
> --D
> 
>>> Hence Btrfs should be doing the same thing as bcachefs. The
>>> bdev_handle_ops structure exists precisly because it allows the
>>> filesystem to handle block device events in the exact manner they
>>> require....
>>>
>>>> - Add a new @bdev parameter to remove_bdev() callback
>>>>     To allow the fs to determine which device is missing, and do the
>>>>     proper handling when needed.
>>>>
>>>> For the existing shutdown callback users, the change is minimal.
>>>
>>> Except for the change in API semantics. ->shutdown is an external
>>> shutdown trigger for the filesystem, not a generic "block device
>>> removed" notification.
>>
>> The problem is, there is no one utilizing ->shutdown() out of
>> fs_bdev_mark_dead().
>>
>> If shutdown ioctl is handled through super_operations::shutdown, it will be
>> more meaningful to split shutdown and dev removal.
>>
>> But that's not the case, and different fses even have slightly different
>> handling for the shutdown flags (not all fses even utilize journal to
>> protect their metadata).
>>
>> Thanks,
>> Qu
>>
>>
>>>
>>> Hooking blk_holder_ops->mark_dead means that btrfs can also provide
>>> a ->shutdown implementation for when something external other than a
>>> block device removal needs to shut down the filesystem....
>>>
>>> -Dave.
>>
> 

Re: [PATCH v4 1/6] fs: enhance and rename shutdown() callback to remove_bdev()

[Qu Wenruo]

在 2025/7/8 11:39, Qu Wenruo 写道:
> 
> 
> 在 2025/7/8 10:15, Darrick J. Wong 写道:
> [...]
>>>
>>> I do not think it's the correct way to go, especially when there is 
>>> already
>>> fs_holder_ops.
>>>
>>> We're always going towards a more generic solution, other than 
>>> letting the
>>> individual fs to do the same thing slightly differently.
>>
>> On second thought -- it's weird that you'd flush the filesystem and
>> shrink the inode/dentry caches in a "your device went away" handler.
>> Fancy filesystems like bcachefs and btrfs would likely just shift IO to
>> a different bdev, right?  And there's no good reason to run shrinkers on
>> either of those fses, right?
> 
> That's right, some part of fs_bdev_mark_dead() is not making much sense 
> if the fs can handle the dev loss.
> 
>>
>>> Yes, the naming is not perfect and mixing cause and action, but the end
>>> result is still a more generic and less duplicated code base.
>>
>> I think dchinner makes a good point that if your filesystem can do
>> something clever on device removal, it should provide its own block
>> device holder ops instead of using fs_holder_ops.
> 
> Then re-implement a lot of things like bdev_super_lock()?
> 
> I'd prefer not.
> 
> 
> fs_holder_ops solves a lot of things like handling mounting/inactive 
> fses, and pushing it back again to the fs code is just causing more 
> duplication.
> 
> Not really worthy if we only want a single different behavior.
> 
> Thus I strongly prefer to do with the existing fs_holder_ops, no matter 
> if it's using/renaming the shutdown() callback, or a new callback.

Previously Christoph is against a new ->remove_bdev() callback, as it is 
conflicting with the existing ->shutdown().

So what about a new ->handle_bdev_remove() callback, that we do 
something like this inside fs_bdev_mark_dead():

{
	bdev_super_lock();
	if (!surprise)
		sync_filesystem();

	if (s_op->handle_bdev_remove) {
		ret = s_op->handle_bdev_remove();
		if (!ret) {
			super_unlock_shared();
			return;
		}
	}
	shrink_dcache_sb();
	evict_inodes();
	if (s_op->shutdown)
		s_op->shutdown();
}

So that the new ->handle_bdev_remove() is not conflicting with
->shutdown() but an optional one.

And if the fs can not handle the removal, just let
->handle_bdev_remove() return an error so that we fallback to the 
existing shutdown routine.

Would this be more acceptable?

Thanks,
Qu

> 
>>  I don't understand
>> why you need a "generic" solution for btrfs when it's not going to do
>> what the others do anyway.
> 
> Because there is only one behavior different.
> 
> Other things like freezing/thawing/syncing are all the same.
> 
> Thanks,
> Qu
> 
>>
>> Awkward naming is often a sign that further thought (or at least
>> separation of code) is needed.
>>
>> As an aside:
>> 'twould be nice if we could lift the *FS_IOC_SHUTDOWN dispatch out of
>> everyone's ioctl functions into the VFS, and then move the "I am dead"
>> state into super_block so that you could actually shut down any
>> filesystem, not just the seven that currently implement it.
>>
>> --D
>>
>>>> Hence Btrfs should be doing the same thing as bcachefs. The
>>>> bdev_handle_ops structure exists precisly because it allows the
>>>> filesystem to handle block device events in the exact manner they
>>>> require....
>>>>
>>>>> - Add a new @bdev parameter to remove_bdev() callback
>>>>>     To allow the fs to determine which device is missing, and do the
>>>>>     proper handling when needed.
>>>>>
>>>>> For the existing shutdown callback users, the change is minimal.
>>>>
>>>> Except for the change in API semantics. ->shutdown is an external
>>>> shutdown trigger for the filesystem, not a generic "block device
>>>> removed" notification.
>>>
>>> The problem is, there is no one utilizing ->shutdown() out of
>>> fs_bdev_mark_dead().
>>>
>>> If shutdown ioctl is handled through super_operations::shutdown, it 
>>> will be
>>> more meaningful to split shutdown and dev removal.
>>>
>>> But that's not the case, and different fses even have slightly different
>>> handling for the shutdown flags (not all fses even utilize journal to
>>> protect their metadata).
>>>
>>> Thanks,
>>> Qu
>>>
>>>
>>>>
>>>> Hooking blk_holder_ops->mark_dead means that btrfs can also provide
>>>> a ->shutdown implementation for when something external other than a
>>>> block device removal needs to shut down the filesystem....
>>>>
>>>> -Dave.
>>>
>>
> 
> 

Re: [PATCH v4 1/6] fs: enhance and rename shutdown() callback to remove_bdev()

[Dave Chinner]

On Tue, Jul 08, 2025 at 12:36:48PM +0930, Qu Wenruo wrote:
> 在 2025/7/8 11:39, Qu Wenruo 写道:
> > 在 2025/7/8 10:15, Darrick J. Wong 写道:
> > > > Yes, the naming is not perfect and mixing cause and action, but the end
> > > > result is still a more generic and less duplicated code base.
> > > 
> > > I think dchinner makes a good point that if your filesystem can do
> > > something clever on device removal, it should provide its own block
> > > device holder ops instead of using fs_holder_ops.
> > 
> > Then re-implement a lot of things like bdev_super_lock()?

IDGI. Simply add:

EXPORT_SYMBOL_GPL(get_bdev_super);

And the problem is solved.

> > I'd prefer not.
> > 
> > 
> > fs_holder_ops solves a lot of things like handling mounting/inactive
> > fses, and pushing it back again to the fs code is just causing more
> > duplication.

This is all encapsulated in get_bdev_super(), so btrfs doesn't need
to implement any of this. get_bdev_super/deactivate_super is the API
you should be using with the blk_holder_ops methods.

> > Not really worthy if we only want a single different behavior.

This is the *3rd* different behaviour for ->mark_dead. We
have the generic behaviour, the bcachefs behaviour, and now the
btrfs behaviour (whatever that may be).

> > Thus I strongly prefer to do with the existing fs_holder_ops, no matter
> > if it's using/renaming the shutdown() callback, or a new callback.
> 
> Previously Christoph is against a new ->remove_bdev() callback, as it is
> conflicting with the existing ->shutdown().
> 
> So what about a new ->handle_bdev_remove() callback, that we do something
> like this inside fs_bdev_mark_dead():
> 
> {
> 	bdev_super_lock();
> 	if (!surprise)
> 		sync_filesystem();
> 
> 	if (s_op->handle_bdev_remove) {
> 		ret = s_op->handle_bdev_remove();
> 		if (!ret) {
> 			super_unlock_shared();
> 			return;
> 		}
> 	}
> 	shrink_dcache_sb();
> 	evict_inodes();
> 	if (s_op->shutdown)
> 		s_op->shutdown();
> }
> 
> So that the new ->handle_bdev_remove() is not conflicting with
> ->shutdown() but an optional one.
> 
> And if the fs can not handle the removal, just let
> ->handle_bdev_remove() return an error so that we fallback to the existing
> shutdown routine.
> 
> Would this be more acceptable?

No.

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

Re: [PATCH v4 1/6] fs: enhance and rename shutdown() callback to remove_bdev()

[Qu Wenruo]

在 2025/7/8 14:35, Dave Chinner 写道:
[...]
>>> Not really worthy if we only want a single different behavior.
> 
> This is the *3rd* different behaviour for ->mark_dead. We
> have the generic behaviour, the bcachefs behaviour, and now the
> btrfs behaviour (whatever that may be).

Then why not merging the btrfs/bcachefs callback into one generic 
callback? Other than introducing 3 different bdev_holder_ops?

This looks exactly the opposite what VFS is trying to do.

Thanks,
Qu

> 
>>> Thus I strongly prefer to do with the existing fs_holder_ops, no matter
>>> if it's using/renaming the shutdown() callback, or a new callback.
>>
>> Previously Christoph is against a new ->remove_bdev() callback, as it is
>> conflicting with the existing ->shutdown().
>>
>> So what about a new ->handle_bdev_remove() callback, that we do something
>> like this inside fs_bdev_mark_dead():
>>
>> {
>> 	bdev_super_lock();
>> 	if (!surprise)
>> 		sync_filesystem();
>>
>> 	if (s_op->handle_bdev_remove) {
>> 		ret = s_op->handle_bdev_remove();
>> 		if (!ret) {
>> 			super_unlock_shared();
>> 			return;
>> 		}
>> 	}
>> 	shrink_dcache_sb();
>> 	evict_inodes();
>> 	if (s_op->shutdown)
>> 		s_op->shutdown();
>> }
>>
>> So that the new ->handle_bdev_remove() is not conflicting with
>> ->shutdown() but an optional one.
>>
>> And if the fs can not handle the removal, just let
>> ->handle_bdev_remove() return an error so that we fallback to the existing
>> shutdown routine.
>>
>> Would this be more acceptable?
> 
> No.
> 
> -Dave.

Re: [PATCH v4 1/6] fs: enhance and rename shutdown() callback to remove_bdev()

[Christian Brauner]

On Mon, Jul 07, 2025 at 05:45:32PM -0700, Darrick J. Wong wrote:
> On Tue, Jul 08, 2025 at 08:52:47AM +0930, Qu Wenruo wrote:
> > 
> > 
> > 在 2025/7/8 08:32, Dave Chinner 写道:
> > > On Fri, Jul 04, 2025 at 10:12:29AM +0930, Qu Wenruo wrote:
> > > > Currently all the filesystems implementing the
> > > > super_opearations::shutdown() callback can not afford losing a device.
> > > > 
> > > > Thus fs_bdev_mark_dead() will just call the shutdown() callback for the
> > > > involved filesystem.
> > > > 
> > > > But it will no longer be the case, with multi-device filesystems like
> > > > btrfs and bcachefs the filesystem can handle certain device loss without
> > > > shutting down the whole filesystem.
> > > > 
> > > > To allow those multi-device filesystems to be integrated to use
> > > > fs_holder_ops:
> > > > 
> > > > - Replace super_opearation::shutdown() with
> > > >    super_opearations::remove_bdev()
> > > >    To better describe when the callback is called.
> > > 
> > > This conflates cause with action.
> > > 
> > > The shutdown callout is an action that the filesystem must execute,
> > > whilst "remove bdev" is a cause notification that might require an
> > > action to be take.
> > > 
> > > Yes, the cause could be someone doing hot-unplug of the block
> > > device, but it could also be something going wrong in software
> > > layers below the filesystem. e.g. dm-thinp having an unrecoverable
> > > corruption or ENOSPC errors.
> > > 
> > > We already have a "cause" notification: blk_holder_ops->mark_dead().
> > > 
> > > The generic fs action that is taken by this notification is
> > > fs_bdev_mark_dead().  That action is to invalidate caches and shut
> > > down the filesystem.
> > > 
> > > btrfs needs to do something different to a blk_holder_ops->mark_dead
> > > notification. i.e. it needs an action that is different to
> > > fs_bdev_mark_dead().
> > > 
> > > Indeed, this is how bcachefs already handles "single device
> > > died" events for multi-device filesystems - see
> > > bch2_fs_bdev_mark_dead().
> > 
> > I do not think it's the correct way to go, especially when there is already
> > fs_holder_ops.
> > 
> > We're always going towards a more generic solution, other than letting the
> > individual fs to do the same thing slightly differently.
> 
> On second thought -- it's weird that you'd flush the filesystem and
> shrink the inode/dentry caches in a "your device went away" handler.
> Fancy filesystems like bcachefs and btrfs would likely just shift IO to
> a different bdev, right?  And there's no good reason to run shrinkers on
> either of those fses, right?
> 
> > Yes, the naming is not perfect and mixing cause and action, but the end
> > result is still a more generic and less duplicated code base.
> 
> I think dchinner makes a good point that if your filesystem can do
> something clever on device removal, it should provide its own block
> device holder ops instead of using fs_holder_ops.  I don't understand
> why you need a "generic" solution for btrfs when it's not going to do
> what the others do anyway.

I think letting filesystems implement their own holder ops should be
avoided if we can. Christoph may chime in here. I have no appettite for
exporting stuff like get_bdev_super() unless absolutely necessary. We
tried to move all that handling into the VFS to eliminate a slew of
deadlocks we detected and fixed. I have no appetite to repeat that
cycle.

The shutdown method is implemented only by block-based filesystems and
arguably shutdown was always a misnomer because it assumed that the
filesystem needs to actually shut down when it is called. IOW, we made
it so that it is a call to action but that doesn't have to be the case.
Calling it ->remove_bdev() is imo the correct thing because it gives
block based filesystem the ability to handle device events how they see
fit.

Once we will have non-block based filesystems that need a method to
always shut down the filesystem itself we might have to revisit this
design anyway but no one had that use-case yet.

> 
> Awkward naming is often a sign that further thought (or at least
> separation of code) is needed.
> 
> As an aside:
> 'twould be nice if we could lift the *FS_IOC_SHUTDOWN dispatch out of
> everyone's ioctl functions into the VFS, and then move the "I am dead"
> state into super_block so that you could actually shut down any
> filesystem, not just the seven that currently implement it.

That goes back to my earlier point. Fwiw, I think that's valuable work.

Re: [PATCH v4 1/6] fs: enhance and rename shutdown() callback to remove_bdev()

[Jan Kara]

On Mon 07-07-25 17:45:32, Darrick J. Wong wrote:
> On Tue, Jul 08, 2025 at 08:52:47AM +0930, Qu Wenruo wrote:
> > 在 2025/7/8 08:32, Dave Chinner 写道:
> > > On Fri, Jul 04, 2025 at 10:12:29AM +0930, Qu Wenruo wrote:
> > > > Currently all the filesystems implementing the
> > > > super_opearations::shutdown() callback can not afford losing a device.
> > > > 
> > > > Thus fs_bdev_mark_dead() will just call the shutdown() callback for the
> > > > involved filesystem.
> > > > 
> > > > But it will no longer be the case, with multi-device filesystems like
> > > > btrfs and bcachefs the filesystem can handle certain device loss without
> > > > shutting down the whole filesystem.
> > > > 
> > > > To allow those multi-device filesystems to be integrated to use
> > > > fs_holder_ops:
> > > > 
> > > > - Replace super_opearation::shutdown() with
> > > >    super_opearations::remove_bdev()
> > > >    To better describe when the callback is called.
> > > 
> > > This conflates cause with action.
> > > 
> > > The shutdown callout is an action that the filesystem must execute,
> > > whilst "remove bdev" is a cause notification that might require an
> > > action to be take.
> > > 
> > > Yes, the cause could be someone doing hot-unplug of the block
> > > device, but it could also be something going wrong in software
> > > layers below the filesystem. e.g. dm-thinp having an unrecoverable
> > > corruption or ENOSPC errors.
> > > 
> > > We already have a "cause" notification: blk_holder_ops->mark_dead().
> > > 
> > > The generic fs action that is taken by this notification is
> > > fs_bdev_mark_dead().  That action is to invalidate caches and shut
> > > down the filesystem.
> > > 
> > > btrfs needs to do something different to a blk_holder_ops->mark_dead
> > > notification. i.e. it needs an action that is different to
> > > fs_bdev_mark_dead().
> > > 
> > > Indeed, this is how bcachefs already handles "single device
> > > died" events for multi-device filesystems - see
> > > bch2_fs_bdev_mark_dead().
> > 
> > I do not think it's the correct way to go, especially when there is already
> > fs_holder_ops.
> > 
> > We're always going towards a more generic solution, other than letting the
> > individual fs to do the same thing slightly differently.
> 
> On second thought -- it's weird that you'd flush the filesystem and
> shrink the inode/dentry caches in a "your device went away" handler.
> Fancy filesystems like bcachefs and btrfs would likely just shift IO to
> a different bdev, right?  And there's no good reason to run shrinkers on
> either of those fses, right?

I agree it is awkward and bcachefs avoids these in case of removal it can
handle gracefully AFAICS.

> > Yes, the naming is not perfect and mixing cause and action, but the end
> > result is still a more generic and less duplicated code base.
> 
> I think dchinner makes a good point that if your filesystem can do
> something clever on device removal, it should provide its own block
> device holder ops instead of using fs_holder_ops.  I don't understand
> why you need a "generic" solution for btrfs when it's not going to do
> what the others do anyway.

Well, I'd also say just go for own fs_holder_ops if it was not for the
awkward "get super from bdev" step. As Christian wrote we've encapsulated
that in fs/super.c and bdev_super_lock() in particular but the calling
conventions for the fs_holder_ops are not very nice (holding
bdev_holder_lock, need to release it before grabbing practically anything
else) so I'd have much greater peace of mind if this didn't spread too
much. Once you call bdev_super_lock() and hold on to sb with s_umount held,
things are much more conventional for the fs land so I'd like if this
step happened before any fs hook got called. So I prefer something like
Qu's proposal of separate sb op for device removal over exporting
bdev_super_lock(). Like:

static void fs_bdev_mark_dead(struct block_device *bdev, bool surprise)
{
        struct super_block *sb;

        sb = bdev_super_lock(bdev, false);
        if (!sb)
                return;

	if (sb->s_op->remove_bdev) {
		sb->s_op->remove_bdev(sb, bdev, surprise);
		return;
	}

	if (!surprise)
		sync_filesystem(sb);
	shrink_dcache_sb(sb);
	evict_inodes(sb);
	if (sb->s_op->shutdown)
		sb->s_op->shutdown(sb);

	super_unlock_shared(sb);
}

> As an aside:
> 'twould be nice if we could lift the *FS_IOC_SHUTDOWN dispatch out of
> everyone's ioctl functions into the VFS, and then move the "I am dead"
> state into super_block so that you could actually shut down any
> filesystem, not just the seven that currently implement it.

Yes, I should find time to revive that patch series... It was not *that*
hard to do.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v4 1/6] fs: enhance and rename shutdown() callback to remove_bdev()

[Darrick J. Wong]

On Tue, Jul 08, 2025 at 12:20:00PM +0200, Jan Kara wrote:
> On Mon 07-07-25 17:45:32, Darrick J. Wong wrote:
> > On Tue, Jul 08, 2025 at 08:52:47AM +0930, Qu Wenruo wrote:
> > > 在 2025/7/8 08:32, Dave Chinner 写道:
> > > > On Fri, Jul 04, 2025 at 10:12:29AM +0930, Qu Wenruo wrote:
> > > > > Currently all the filesystems implementing the
> > > > > super_opearations::shutdown() callback can not afford losing a device.
> > > > > 
> > > > > Thus fs_bdev_mark_dead() will just call the shutdown() callback for the
> > > > > involved filesystem.
> > > > > 
> > > > > But it will no longer be the case, with multi-device filesystems like
> > > > > btrfs and bcachefs the filesystem can handle certain device loss without
> > > > > shutting down the whole filesystem.
> > > > > 
> > > > > To allow those multi-device filesystems to be integrated to use
> > > > > fs_holder_ops:
> > > > > 
> > > > > - Replace super_opearation::shutdown() with
> > > > >    super_opearations::remove_bdev()
> > > > >    To better describe when the callback is called.
> > > > 
> > > > This conflates cause with action.
> > > > 
> > > > The shutdown callout is an action that the filesystem must execute,
> > > > whilst "remove bdev" is a cause notification that might require an
> > > > action to be take.
> > > > 
> > > > Yes, the cause could be someone doing hot-unplug of the block
> > > > device, but it could also be something going wrong in software
> > > > layers below the filesystem. e.g. dm-thinp having an unrecoverable
> > > > corruption or ENOSPC errors.
> > > > 
> > > > We already have a "cause" notification: blk_holder_ops->mark_dead().
> > > > 
> > > > The generic fs action that is taken by this notification is
> > > > fs_bdev_mark_dead().  That action is to invalidate caches and shut
> > > > down the filesystem.
> > > > 
> > > > btrfs needs to do something different to a blk_holder_ops->mark_dead
> > > > notification. i.e. it needs an action that is different to
> > > > fs_bdev_mark_dead().
> > > > 
> > > > Indeed, this is how bcachefs already handles "single device
> > > > died" events for multi-device filesystems - see
> > > > bch2_fs_bdev_mark_dead().
> > > 
> > > I do not think it's the correct way to go, especially when there is already
> > > fs_holder_ops.
> > > 
> > > We're always going towards a more generic solution, other than letting the
> > > individual fs to do the same thing slightly differently.
> > 
> > On second thought -- it's weird that you'd flush the filesystem and
> > shrink the inode/dentry caches in a "your device went away" handler.
> > Fancy filesystems like bcachefs and btrfs would likely just shift IO to
> > a different bdev, right?  And there's no good reason to run shrinkers on
> > either of those fses, right?
> 
> I agree it is awkward and bcachefs avoids these in case of removal it can
> handle gracefully AFAICS.
> 
> > > Yes, the naming is not perfect and mixing cause and action, but the end
> > > result is still a more generic and less duplicated code base.
> > 
> > I think dchinner makes a good point that if your filesystem can do
> > something clever on device removal, it should provide its own block
> > device holder ops instead of using fs_holder_ops.  I don't understand
> > why you need a "generic" solution for btrfs when it's not going to do
> > what the others do anyway.
> 
> Well, I'd also say just go for own fs_holder_ops if it was not for the
> awkward "get super from bdev" step. As Christian wrote we've encapsulated
> that in fs/super.c and bdev_super_lock() in particular but the calling
> conventions for the fs_holder_ops are not very nice (holding
> bdev_holder_lock, need to release it before grabbing practically anything
> else) so I'd have much greater peace of mind if this didn't spread too
> much. Once you call bdev_super_lock() and hold on to sb with s_umount held,
> things are much more conventional for the fs land so I'd like if this
> step happened before any fs hook got called. So I prefer something like
> Qu's proposal of separate sb op for device removal over exporting
> bdev_super_lock(). Like:
> 
> static void fs_bdev_mark_dead(struct block_device *bdev, bool surprise)
> {
>         struct super_block *sb;
> 
>         sb = bdev_super_lock(bdev, false);
>         if (!sb)
>                 return;
> 
> 	if (sb->s_op->remove_bdev) {
> 		sb->s_op->remove_bdev(sb, bdev, surprise);
> 		return;
> 	}

It feels odd but I could live with this, particularly since that's the
direction that brauner is laying down. :)

Do we still need to super_unlock_shared here?

--D

> 
> 	if (!surprise)
> 		sync_filesystem(sb);
> 	shrink_dcache_sb(sb);
> 	evict_inodes(sb);
> 	if (sb->s_op->shutdown)
> 		sb->s_op->shutdown(sb);
> 
> 	super_unlock_shared(sb);
> }
> 
> > As an aside:
> > 'twould be nice if we could lift the *FS_IOC_SHUTDOWN dispatch out of
> > everyone's ioctl functions into the VFS, and then move the "I am dead"
> > state into super_block so that you could actually shut down any
> > filesystem, not just the seven that currently implement it.
> 
> Yes, I should find time to revive that patch series... It was not *that*
> hard to do.
> 
> 								Honza
> -- 
> Jan Kara <jack@suse.com>
> SUSE Labs, CR
> 

Re: [PATCH v4 1/6] fs: enhance and rename shutdown() callback to remove_bdev()

[Qu Wenruo]

在 2025/7/9 05:50, Darrick J. Wong 写道:
[...]
>> Well, I'd also say just go for own fs_holder_ops if it was not for the
>> awkward "get super from bdev" step. As Christian wrote we've encapsulated
>> that in fs/super.c and bdev_super_lock() in particular but the calling
>> conventions for the fs_holder_ops are not very nice (holding
>> bdev_holder_lock, need to release it before grabbing practically anything
>> else) so I'd have much greater peace of mind if this didn't spread too
>> much. Once you call bdev_super_lock() and hold on to sb with s_umount held,
>> things are much more conventional for the fs land so I'd like if this
>> step happened before any fs hook got called. So I prefer something like
>> Qu's proposal of separate sb op for device removal over exporting
>> bdev_super_lock(). Like:
>>
>> static void fs_bdev_mark_dead(struct block_device *bdev, bool surprise)
>> {
>>          struct super_block *sb;
>>
>>          sb = bdev_super_lock(bdev, false);
>>          if (!sb)
>>                  return;
>>
>> 	if (sb->s_op->remove_bdev) {
>> 		sb->s_op->remove_bdev(sb, bdev, surprise);

The only concern here is, remove_bdev() will handle two different things:

- Mark the target devices as missing and continue working
   Of course that's when the fs can handle it.

- Shutdown
   If the fs can not handle it.

And if the fs has to shutdown, we're missing all the shrinks in the 
shutdown path.

Thus I'd prefer the remove_bdev() to have a return value, so that we can 
fallback to shutdown path if the remove_bdev() failed.

This also aligns more towards Brauner's idea that we may want to expose 
if the fs can handle the removal.

Thanks,
Qu

>> 		return;
>> 	}
> 
> It feels odd but I could live with this, particularly since that's the
> direction that brauner is laying down. :)
> 
> Do we still need to super_unlock_shared here?
> 
> --D
> 
>>
>> 	if (!surprise)
>> 		sync_filesystem(sb);
>> 	shrink_dcache_sb(sb);
>> 	evict_inodes(sb);
>> 	if (sb->s_op->shutdown)
>> 		sb->s_op->shutdown(sb);
>>
>> 	super_unlock_shared(sb);
>> }
>>
>>> As an aside:
>>> 'twould be nice if we could lift the *FS_IOC_SHUTDOWN dispatch out of
>>> everyone's ioctl functions into the VFS, and then move the "I am dead"
>>> state into super_block so that you could actually shut down any
>>> filesystem, not just the seven that currently implement it.
>>
>> Yes, I should find time to revive that patch series... It was not *that*
>> hard to do.
>>
>> 								Honza
>> -- 
>> Jan Kara <jack@suse.com>
>> SUSE Labs, CR
>>

Re: [PATCH v4 1/6] fs: enhance and rename shutdown() callback to remove_bdev()

[Dave Chinner]

On Tue, Jul 08, 2025 at 09:55:14AM +0200, Christian Brauner wrote:
> On Mon, Jul 07, 2025 at 05:45:32PM -0700, Darrick J. Wong wrote:
> > On Tue, Jul 08, 2025 at 08:52:47AM +0930, Qu Wenruo wrote:
> > > 
> > > 
> > > 在 2025/7/8 08:32, Dave Chinner 写道:
> > > > On Fri, Jul 04, 2025 at 10:12:29AM +0930, Qu Wenruo wrote:
> > > > > Currently all the filesystems implementing the
> > > > > super_opearations::shutdown() callback can not afford losing a device.
> > > > > 
> > > > > Thus fs_bdev_mark_dead() will just call the shutdown() callback for the
> > > > > involved filesystem.
> > > > > 
> > > > > But it will no longer be the case, with multi-device filesystems like
> > > > > btrfs and bcachefs the filesystem can handle certain device loss without
> > > > > shutting down the whole filesystem.
> > > > > 
> > > > > To allow those multi-device filesystems to be integrated to use
> > > > > fs_holder_ops:
> > > > > 
> > > > > - Replace super_opearation::shutdown() with
> > > > >    super_opearations::remove_bdev()
> > > > >    To better describe when the callback is called.
> > > > 
> > > > This conflates cause with action.
> > > > 
> > > > The shutdown callout is an action that the filesystem must execute,
> > > > whilst "remove bdev" is a cause notification that might require an
> > > > action to be take.
> > > > 
> > > > Yes, the cause could be someone doing hot-unplug of the block
> > > > device, but it could also be something going wrong in software
> > > > layers below the filesystem. e.g. dm-thinp having an unrecoverable
> > > > corruption or ENOSPC errors.
> > > > 
> > > > We already have a "cause" notification: blk_holder_ops->mark_dead().
> > > > 
> > > > The generic fs action that is taken by this notification is
> > > > fs_bdev_mark_dead().  That action is to invalidate caches and shut
> > > > down the filesystem.
> > > > 
> > > > btrfs needs to do something different to a blk_holder_ops->mark_dead
> > > > notification. i.e. it needs an action that is different to
> > > > fs_bdev_mark_dead().
> > > > 
> > > > Indeed, this is how bcachefs already handles "single device
> > > > died" events for multi-device filesystems - see
> > > > bch2_fs_bdev_mark_dead().
> > > 
> > > I do not think it's the correct way to go, especially when there is already
> > > fs_holder_ops.
> > > 
> > > We're always going towards a more generic solution, other than letting the
> > > individual fs to do the same thing slightly differently.
> > 
> > On second thought -- it's weird that you'd flush the filesystem and
> > shrink the inode/dentry caches in a "your device went away" handler.
> > Fancy filesystems like bcachefs and btrfs would likely just shift IO to
> > a different bdev, right?  And there's no good reason to run shrinkers on
> > either of those fses, right?
> > 
> > > Yes, the naming is not perfect and mixing cause and action, but the end
> > > result is still a more generic and less duplicated code base.
> > 
> > I think dchinner makes a good point that if your filesystem can do
> > something clever on device removal, it should provide its own block
> > device holder ops instead of using fs_holder_ops.  I don't understand
> > why you need a "generic" solution for btrfs when it's not going to do
> > what the others do anyway.
> 
> I think letting filesystems implement their own holder ops should be
> avoided if we can. Christoph may chime in here. I have no appettite for
> exporting stuff like get_bdev_super() unless absolutely necessary. We
> tried to move all that handling into the VFS to eliminate a slew of
> deadlocks we detected and fixed. I have no appetite to repeat that
> cycle.

Except it isn't actually necessary.

Everyone here seems to be assuming that the filesystem *must* take
an active superblock reference to process a device removal event,
and that is *simply not true*.

bcachefs does not use get_bdev_super() or an active superblock
reference to process ->mark_dead events.

It has it's own internal reference counting on the struct bch_fs
attached to the bdev that ensures the filesystem structures can't go
away whilst ->mark_dead is being processed.  i.e. bcachefs is only
dependent on the bdev->bd_holder_lock() being held when
->mark_dead() is called and does not rely on the VFS for anything.

This means that device removal processing can be performed
without global filesystem/VFS locks needing to be held. Hence issues
like re-entrancy deadlocks when there are concurrent/cascading
device failures (e.g. a HBA dies, taking out multiple devices
simultaneously) are completely avoided...

It also avoids the problem of ->mark_dead events being generated
from a context that holds filesystem/vfs locks and then deadlocking
waiting for those locks to be released.

IOWs, a multi-device filesystem should really be implementing
->mark_dead itself, and should not be depending on being able to
lock the superblock to take an active reference to it.

It should be pretty clear that these are not issues that the generic
filesystem ->mark_dead implementation should be trying to
handle.....

> The shutdown method is implemented only by block-based filesystems and
> arguably shutdown was always a misnomer because it assumed that the
> filesystem needs to actually shut down when it is called.

Shutdown was not -assumed- as the operation that needed to be
performed. That was the feature that was *required* to fix
filesystem level problems that occur when the device underneath it
disappears.

->mark_dead() is the abstract filesystem notification from the block
device, fs_bdfev_mark_dead() is the -generic implementation- of the
functionality required by single block device filesystems. Part of
that functionality is shutting down the filesystem because it can
*no longer function without a backing device*.

multi-block device filesystems require compeltely different
implementations, and we already have one that -does not use active
superblock references-. IOWs, even if we add ->remove_bdev(sb)
callout, bcachefs will continue to use ->mark_dead() because low
level filesystem device management isn't (and shouldn't be!)
dependent on high level VFS structure reference counting....

> IOW, we made
> it so that it is a call to action but that doesn't have to be the case.
> Calling it ->remove_bdev() is imo the correct thing because it gives
> block based filesystem the ability to handle device events how they see
> fit.

And that's exactly what ->mark_dead already provides. 

And, as I've pointed out above, multi-device filesystems don't
actually need actively referenced superblocks to process device
removal notifications. Hence ->mark_dead is the correct interface
for them to use.

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

Re: [PATCH v4 1/6] fs: enhance and rename shutdown() callback to remove_bdev()

[Qu Wenruo]

在 2025/7/9 08:29, Dave Chinner 写道:
> On Tue, Jul 08, 2025 at 09:55:14AM +0200, Christian Brauner wrote:
>> On Mon, Jul 07, 2025 at 05:45:32PM -0700, Darrick J. Wong wrote:
>>> On Tue, Jul 08, 2025 at 08:52:47AM +0930, Qu Wenruo wrote:
>>>>
>>>>
>>>> 在 2025/7/8 08:32, Dave Chinner 写道:
>>>>> On Fri, Jul 04, 2025 at 10:12:29AM +0930, Qu Wenruo wrote:
>>>>>> Currently all the filesystems implementing the
>>>>>> super_opearations::shutdown() callback can not afford losing a device.
>>>>>>
>>>>>> Thus fs_bdev_mark_dead() will just call the shutdown() callback for the
>>>>>> involved filesystem.
>>>>>>
>>>>>> But it will no longer be the case, with multi-device filesystems like
>>>>>> btrfs and bcachefs the filesystem can handle certain device loss without
>>>>>> shutting down the whole filesystem.
>>>>>>
>>>>>> To allow those multi-device filesystems to be integrated to use
>>>>>> fs_holder_ops:
>>>>>>
>>>>>> - Replace super_opearation::shutdown() with
>>>>>>     super_opearations::remove_bdev()
>>>>>>     To better describe when the callback is called.
>>>>>
>>>>> This conflates cause with action.
>>>>>
>>>>> The shutdown callout is an action that the filesystem must execute,
>>>>> whilst "remove bdev" is a cause notification that might require an
>>>>> action to be take.
>>>>>
>>>>> Yes, the cause could be someone doing hot-unplug of the block
>>>>> device, but it could also be something going wrong in software
>>>>> layers below the filesystem. e.g. dm-thinp having an unrecoverable
>>>>> corruption or ENOSPC errors.
>>>>>
>>>>> We already have a "cause" notification: blk_holder_ops->mark_dead().
>>>>>
>>>>> The generic fs action that is taken by this notification is
>>>>> fs_bdev_mark_dead().  That action is to invalidate caches and shut
>>>>> down the filesystem.
>>>>>
>>>>> btrfs needs to do something different to a blk_holder_ops->mark_dead
>>>>> notification. i.e. it needs an action that is different to
>>>>> fs_bdev_mark_dead().
>>>>>
>>>>> Indeed, this is how bcachefs already handles "single device
>>>>> died" events for multi-device filesystems - see
>>>>> bch2_fs_bdev_mark_dead().
>>>>
>>>> I do not think it's the correct way to go, especially when there is already
>>>> fs_holder_ops.
>>>>
>>>> We're always going towards a more generic solution, other than letting the
>>>> individual fs to do the same thing slightly differently.
>>>
>>> On second thought -- it's weird that you'd flush the filesystem and
>>> shrink the inode/dentry caches in a "your device went away" handler.
>>> Fancy filesystems like bcachefs and btrfs would likely just shift IO to
>>> a different bdev, right?  And there's no good reason to run shrinkers on
>>> either of those fses, right?
>>>
>>>> Yes, the naming is not perfect and mixing cause and action, but the end
>>>> result is still a more generic and less duplicated code base.
>>>
>>> I think dchinner makes a good point that if your filesystem can do
>>> something clever on device removal, it should provide its own block
>>> device holder ops instead of using fs_holder_ops.  I don't understand
>>> why you need a "generic" solution for btrfs when it's not going to do
>>> what the others do anyway.
>>
>> I think letting filesystems implement their own holder ops should be
>> avoided if we can. Christoph may chime in here. I have no appettite for
>> exporting stuff like get_bdev_super() unless absolutely necessary. We
>> tried to move all that handling into the VFS to eliminate a slew of
>> deadlocks we detected and fixed. I have no appetite to repeat that
>> cycle.
> 
> Except it isn't actually necessary.
> 
> Everyone here seems to be assuming that the filesystem *must* take
> an active superblock reference to process a device removal event,
> and that is *simply not true*.
> 
> bcachefs does not use get_bdev_super() or an active superblock
> reference to process ->mark_dead events.

Yes, bcachefs doesn't go this path, but the reason is more important.

Is it just because there is no such callback so that Kent has to come up 
his own solution, or something else?

If the former case, all your argument here makes no sense.

Adding Kent here to see if he wants a more generic s_op->remove_bdev() 
solution or the current each fs doing its own mark_dead() callback.

Thanks,
Qu

Re: [PATCH v4 1/6] fs: enhance and rename shutdown() callback to remove_bdev()

[Kent Overstreet]

On Wed, Jul 09, 2025 at 08:37:05AM +0930, Qu Wenruo wrote:
> 在 2025/7/9 08:29, Dave Chinner 写道:
> > On Tue, Jul 08, 2025 at 09:55:14AM +0200, Christian Brauner wrote:
> > > On Mon, Jul 07, 2025 at 05:45:32PM -0700, Darrick J. Wong wrote:
> > > > On Tue, Jul 08, 2025 at 08:52:47AM +0930, Qu Wenruo wrote:
> > > > > 
> > > > > 
> > > > > 在 2025/7/8 08:32, Dave Chinner 写道:
> > > > > > On Fri, Jul 04, 2025 at 10:12:29AM +0930, Qu Wenruo wrote:
> > > > > > > Currently all the filesystems implementing the
> > > > > > > super_opearations::shutdown() callback can not afford losing a device.
> > > > > > > 
> > > > > > > Thus fs_bdev_mark_dead() will just call the shutdown() callback for the
> > > > > > > involved filesystem.
> > > > > > > 
> > > > > > > But it will no longer be the case, with multi-device filesystems like
> > > > > > > btrfs and bcachefs the filesystem can handle certain device loss without
> > > > > > > shutting down the whole filesystem.
> > > > > > > 
> > > > > > > To allow those multi-device filesystems to be integrated to use
> > > > > > > fs_holder_ops:
> > > > > > > 
> > > > > > > - Replace super_opearation::shutdown() with
> > > > > > >     super_opearations::remove_bdev()
> > > > > > >     To better describe when the callback is called.
> > > > > > 
> > > > > > This conflates cause with action.
> > > > > > 
> > > > > > The shutdown callout is an action that the filesystem must execute,
> > > > > > whilst "remove bdev" is a cause notification that might require an
> > > > > > action to be take.
> > > > > > 
> > > > > > Yes, the cause could be someone doing hot-unplug of the block
> > > > > > device, but it could also be something going wrong in software
> > > > > > layers below the filesystem. e.g. dm-thinp having an unrecoverable
> > > > > > corruption or ENOSPC errors.
> > > > > > 
> > > > > > We already have a "cause" notification: blk_holder_ops->mark_dead().
> > > > > > 
> > > > > > The generic fs action that is taken by this notification is
> > > > > > fs_bdev_mark_dead().  That action is to invalidate caches and shut
> > > > > > down the filesystem.
> > > > > > 
> > > > > > btrfs needs to do something different to a blk_holder_ops->mark_dead
> > > > > > notification. i.e. it needs an action that is different to
> > > > > > fs_bdev_mark_dead().
> > > > > > 
> > > > > > Indeed, this is how bcachefs already handles "single device
> > > > > > died" events for multi-device filesystems - see
> > > > > > bch2_fs_bdev_mark_dead().
> > > > > 
> > > > > I do not think it's the correct way to go, especially when there is already
> > > > > fs_holder_ops.
> > > > > 
> > > > > We're always going towards a more generic solution, other than letting the
> > > > > individual fs to do the same thing slightly differently.
> > > > 
> > > > On second thought -- it's weird that you'd flush the filesystem and
> > > > shrink the inode/dentry caches in a "your device went away" handler.
> > > > Fancy filesystems like bcachefs and btrfs would likely just shift IO to
> > > > a different bdev, right?  And there's no good reason to run shrinkers on
> > > > either of those fses, right?
> > > > 
> > > > > Yes, the naming is not perfect and mixing cause and action, but the end
> > > > > result is still a more generic and less duplicated code base.
> > > > 
> > > > I think dchinner makes a good point that if your filesystem can do
> > > > something clever on device removal, it should provide its own block
> > > > device holder ops instead of using fs_holder_ops.  I don't understand
> > > > why you need a "generic" solution for btrfs when it's not going to do
> > > > what the others do anyway.
> > > 
> > > I think letting filesystems implement their own holder ops should be
> > > avoided if we can. Christoph may chime in here. I have no appettite for
> > > exporting stuff like get_bdev_super() unless absolutely necessary. We
> > > tried to move all that handling into the VFS to eliminate a slew of
> > > deadlocks we detected and fixed. I have no appetite to repeat that
> > > cycle.
> > 
> > Except it isn't actually necessary.
> > 
> > Everyone here seems to be assuming that the filesystem *must* take
> > an active superblock reference to process a device removal event,
> > and that is *simply not true*.
> > 
> > bcachefs does not use get_bdev_super() or an active superblock
> > reference to process ->mark_dead events.
> 
> Yes, bcachefs doesn't go this path, but the reason is more important.
> 
> Is it just because there is no such callback so that Kent has to come up his
> own solution, or something else?
> 
> If the former case, all your argument here makes no sense.
> 
> Adding Kent here to see if he wants a more generic s_op->remove_bdev()
> solution or the current each fs doing its own mark_dead() callback.

Consider that the thing that has a block device open might not even be a
filesystem, or at least a VFS filesystem.

It could be a stacking block device driver - md or md - and those
absolutely should be implementing .mark_dead() (likely passing it
through on up the stack), or something else entirely.

In bcachefs's case, we might not even have created the VFS super_block
yet: we don't do that until after recovery finishes, and indeed we can't
because creating a super_block and leaving it in !SB_BORN will cause
such fun as sync calls to hang for the entire system...

Re: [PATCH v4 1/6] fs: enhance and rename shutdown() callback to remove_bdev()

[Qu Wenruo]

在 2025/7/9 10:05, Kent Overstreet 写道:
> On Wed, Jul 09, 2025 at 08:37:05AM +0930, Qu Wenruo wrote:
>> 在 2025/7/9 08:29, Dave Chinner 写道:
>>> On Tue, Jul 08, 2025 at 09:55:14AM +0200, Christian Brauner wrote:
>>>> On Mon, Jul 07, 2025 at 05:45:32PM -0700, Darrick J. Wong wrote:
>>>>> On Tue, Jul 08, 2025 at 08:52:47AM +0930, Qu Wenruo wrote:
>>>>>>
>>>>>>
>>>>>> 在 2025/7/8 08:32, Dave Chinner 写道:
>>>>>>> On Fri, Jul 04, 2025 at 10:12:29AM +0930, Qu Wenruo wrote:
>>>>>>>> Currently all the filesystems implementing the
>>>>>>>> super_opearations::shutdown() callback can not afford losing a device.
>>>>>>>>
>>>>>>>> Thus fs_bdev_mark_dead() will just call the shutdown() callback for the
>>>>>>>> involved filesystem.
>>>>>>>>
>>>>>>>> But it will no longer be the case, with multi-device filesystems like
>>>>>>>> btrfs and bcachefs the filesystem can handle certain device loss without
>>>>>>>> shutting down the whole filesystem.
>>>>>>>>
>>>>>>>> To allow those multi-device filesystems to be integrated to use
>>>>>>>> fs_holder_ops:
>>>>>>>>
>>>>>>>> - Replace super_opearation::shutdown() with
>>>>>>>>      super_opearations::remove_bdev()
>>>>>>>>      To better describe when the callback is called.
>>>>>>>
>>>>>>> This conflates cause with action.
>>>>>>>
>>>>>>> The shutdown callout is an action that the filesystem must execute,
>>>>>>> whilst "remove bdev" is a cause notification that might require an
>>>>>>> action to be take.
>>>>>>>
>>>>>>> Yes, the cause could be someone doing hot-unplug of the block
>>>>>>> device, but it could also be something going wrong in software
>>>>>>> layers below the filesystem. e.g. dm-thinp having an unrecoverable
>>>>>>> corruption or ENOSPC errors.
>>>>>>>
>>>>>>> We already have a "cause" notification: blk_holder_ops->mark_dead().
>>>>>>>
>>>>>>> The generic fs action that is taken by this notification is
>>>>>>> fs_bdev_mark_dead().  That action is to invalidate caches and shut
>>>>>>> down the filesystem.
>>>>>>>
>>>>>>> btrfs needs to do something different to a blk_holder_ops->mark_dead
>>>>>>> notification. i.e. it needs an action that is different to
>>>>>>> fs_bdev_mark_dead().
>>>>>>>
>>>>>>> Indeed, this is how bcachefs already handles "single device
>>>>>>> died" events for multi-device filesystems - see
>>>>>>> bch2_fs_bdev_mark_dead().
>>>>>>
>>>>>> I do not think it's the correct way to go, especially when there is already
>>>>>> fs_holder_ops.
>>>>>>
>>>>>> We're always going towards a more generic solution, other than letting the
>>>>>> individual fs to do the same thing slightly differently.
>>>>>
>>>>> On second thought -- it's weird that you'd flush the filesystem and
>>>>> shrink the inode/dentry caches in a "your device went away" handler.
>>>>> Fancy filesystems like bcachefs and btrfs would likely just shift IO to
>>>>> a different bdev, right?  And there's no good reason to run shrinkers on
>>>>> either of those fses, right?
>>>>>
>>>>>> Yes, the naming is not perfect and mixing cause and action, but the end
>>>>>> result is still a more generic and less duplicated code base.
>>>>>
>>>>> I think dchinner makes a good point that if your filesystem can do
>>>>> something clever on device removal, it should provide its own block
>>>>> device holder ops instead of using fs_holder_ops.  I don't understand
>>>>> why you need a "generic" solution for btrfs when it's not going to do
>>>>> what the others do anyway.
>>>>
>>>> I think letting filesystems implement their own holder ops should be
>>>> avoided if we can. Christoph may chime in here. I have no appettite for
>>>> exporting stuff like get_bdev_super() unless absolutely necessary. We
>>>> tried to move all that handling into the VFS to eliminate a slew of
>>>> deadlocks we detected and fixed. I have no appetite to repeat that
>>>> cycle.
>>>
>>> Except it isn't actually necessary.
>>>
>>> Everyone here seems to be assuming that the filesystem *must* take
>>> an active superblock reference to process a device removal event,
>>> and that is *simply not true*.
>>>
>>> bcachefs does not use get_bdev_super() or an active superblock
>>> reference to process ->mark_dead events.
>>
>> Yes, bcachefs doesn't go this path, but the reason is more important.
>>
>> Is it just because there is no such callback so that Kent has to come up his
>> own solution, or something else?
>>
>> If the former case, all your argument here makes no sense.
>>
>> Adding Kent here to see if he wants a more generic s_op->remove_bdev()
>> solution or the current each fs doing its own mark_dead() callback.
> 
> Consider that the thing that has a block device open might not even be a
> filesystem, or at least a VFS filesystem.
> 
> It could be a stacking block device driver - md or md - and those
> absolutely should be implementing .mark_dead() (likely passing it
> through on up the stack), or something else entirely.
> 
> In bcachefs's case, we might not even have created the VFS super_block
> yet: we don't do that until after recovery finishes, and indeed we can't
> because creating a super_block and leaving it in !SB_BORN will cause
> such fun as sync calls to hang for the entire system...
> 

Not related to the series, but IIRC if s_flags doesn't have SB_ACTIVE 
set, things like bdev_super_lock() won't choose that superblock, thus 
won't call ->sync() callback through the bdev callbacks.

And btrfs also follows the same scheme, only setting SB_ACTIVE after 
everything is done (including replaying the log etc), and so far we 
haven't yet hit such sync during mount.

Thanks,
Qu

Re: [PATCH v4 1/6] fs: enhance and rename shutdown() callback to remove_bdev()

[Kent Overstreet]

On Wed, Jul 09, 2025 at 10:25:08AM +0930, Qu Wenruo wrote:
> 在 2025/7/9 10:05, Kent Overstreet 写道:
> > Consider that the thing that has a block device open might not even be a
> > filesystem, or at least a VFS filesystem.
> > 
> > It could be a stacking block device driver - md or md - and those
> > absolutely should be implementing .mark_dead() (likely passing it
> > through on up the stack), or something else entirely.
> > 
> > In bcachefs's case, we might not even have created the VFS super_block
> > yet: we don't do that until after recovery finishes, and indeed we can't
> > because creating a super_block and leaving it in !SB_BORN will cause
> > such fun as sync calls to hang for the entire system...
> > 
> 
> Not related to the series, but IIRC if s_flags doesn't have SB_ACTIVE set,
> things like bdev_super_lock() won't choose that superblock, thus won't call
> ->sync() callback through the bdev callbacks.
> 
> And btrfs also follows the same scheme, only setting SB_ACTIVE after
> everything is done (including replaying the log etc), and so far we haven't
> yet hit such sync during mount.

Well, how long can that take? Have a look at iterate_supers(), it's
something to be wary of.

Fixing the fs/super.c locking is something I was looking at, it's doable
but it'd be a giant hassle - for bcachefs it wasn't worth it, bcachefs
has preexisting reasons for wanting to avoid the vfs superblock
dependency.

Anyways - the VFS trying to own .mark_dead() is a layering violation.

        VFS
------------------
        FS
------------------
        BLOCK

By default, the "FS" layer should be considered a black box by both the
block layer and VFS; reaching across that and assuming filesystem
behavior is a good way to paint yourself into a corner.

It's seductive because most filesystems are single device filesystems,
and for that case it makes total sense to provide helpers for
convenience, given that there's not much room for behaviour to deviate
in the single device case.

But in the multi device case: the behaviour is completely up to the
filesystem - in general we don't shut down the entire filesystem if a
single block device goes dead, if we're redundant we can just drop that
device and continue.

And if you're thinking you want to make use of locking provided by the
VFS - I would warn away from that line of thinking too, mixing up
locking from different layers creates all sorts of fun...

Re: [PATCH v4 1/6] fs: enhance and rename shutdown() callback to remove_bdev()

[Jan Kara]

Bcc:
Subject: Re: [PATCH v4 1/6] fs: enhance and rename shutdown() callback to
 remove_bdev()
Reply-To:
In-Reply-To: <aG2i3qP01m-vmFVE@dread.disaster.area>

On Wed 09-07-25 08:59:42, Dave Chinner wrote:
> On Tue, Jul 08, 2025 at 09:55:14AM +0200, Christian Brauner wrote:
> > On Mon, Jul 07, 2025 at 05:45:32PM -0700, Darrick J. Wong wrote:
> > > On Tue, Jul 08, 2025 at 08:52:47AM +0930, Qu Wenruo wrote:
> > > > 在 2025/7/8 08:32, Dave Chinner 写道:
> > > > > On Fri, Jul 04, 2025 at 10:12:29AM +0930, Qu Wenruo wrote:
> > > > > > Currently all the filesystems implementing the
> > > > > > super_opearations::shutdown() callback can not afford losing a device.
> > > > > > 
> > > > > > Thus fs_bdev_mark_dead() will just call the shutdown() callback for the
> > > > > > involved filesystem.
> > > > > > 
> > > > > > But it will no longer be the case, with multi-device filesystems like
> > > > > > btrfs and bcachefs the filesystem can handle certain device loss without
> > > > > > shutting down the whole filesystem.
> > > > > > 
> > > > > > To allow those multi-device filesystems to be integrated to use
> > > > > > fs_holder_ops:
> > > > > > 
> > > > > > - Replace super_opearation::shutdown() with
> > > > > >    super_opearations::remove_bdev()
> > > > > >    To better describe when the callback is called.
> > > > > 
> > > > > This conflates cause with action.
> > > > > 
> > > > > The shutdown callout is an action that the filesystem must execute,
> > > > > whilst "remove bdev" is a cause notification that might require an
> > > > > action to be take.
> > > > > 
> > > > > Yes, the cause could be someone doing hot-unplug of the block
> > > > > device, but it could also be something going wrong in software
> > > > > layers below the filesystem. e.g. dm-thinp having an unrecoverable
> > > > > corruption or ENOSPC errors.
> > > > > 
> > > > > We already have a "cause" notification: blk_holder_ops->mark_dead().
> > > > > 
> > > > > The generic fs action that is taken by this notification is
> > > > > fs_bdev_mark_dead().  That action is to invalidate caches and shut
> > > > > down the filesystem.
> > > > > 
> > > > > btrfs needs to do something different to a blk_holder_ops->mark_dead
> > > > > notification. i.e. it needs an action that is different to
> > > > > fs_bdev_mark_dead().
> > > > > 
> > > > > Indeed, this is how bcachefs already handles "single device
> > > > > died" events for multi-device filesystems - see
> > > > > bch2_fs_bdev_mark_dead().
> > > > 
> > > > I do not think it's the correct way to go, especially when there is already
> > > > fs_holder_ops.
> > > > 
> > > > We're always going towards a more generic solution, other than letting the
> > > > individual fs to do the same thing slightly differently.
> > > 
> > > On second thought -- it's weird that you'd flush the filesystem and
> > > shrink the inode/dentry caches in a "your device went away" handler.
> > > Fancy filesystems like bcachefs and btrfs would likely just shift IO to
> > > a different bdev, right?  And there's no good reason to run shrinkers on
> > > either of those fses, right?
> > > 
> > > > Yes, the naming is not perfect and mixing cause and action, but the end
> > > > result is still a more generic and less duplicated code base.
> > > 
> > > I think dchinner makes a good point that if your filesystem can do
> > > something clever on device removal, it should provide its own block
> > > device holder ops instead of using fs_holder_ops.  I don't understand
> > > why you need a "generic" solution for btrfs when it's not going to do
> > > what the others do anyway.
> > 
> > I think letting filesystems implement their own holder ops should be
> > avoided if we can. Christoph may chime in here. I have no appettite for
> > exporting stuff like get_bdev_super() unless absolutely necessary. We
> > tried to move all that handling into the VFS to eliminate a slew of
> > deadlocks we detected and fixed. I have no appetite to repeat that
> > cycle.
> 
> Except it isn't actually necessary.
> 
> Everyone here seems to be assuming that the filesystem *must* take
> an active superblock reference to process a device removal event,
> and that is *simply not true*.
> 
> bcachefs does not use get_bdev_super() or an active superblock
> reference to process ->mark_dead events.
>
> It has it's own internal reference counting on the struct bch_fs
> attached to the bdev that ensures the filesystem structures can't go
> away whilst ->mark_dead is being processed.  i.e. bcachefs is only
> dependent on the bdev->bd_holder_lock() being held when
> ->mark_dead() is called and does not rely on the VFS for anything.

Right, they have their own refcount which effectively blocks umount
in .put_super AFAICS and they use it instead of VFS refcounts for this.

> This means that device removal processing can be performed
> without global filesystem/VFS locks needing to be held. Hence issues
> like re-entrancy deadlocks when there are concurrent/cascading
> device failures (e.g. a HBA dies, taking out multiple devices
> simultaneously) are completely avoided...

Funnily enough how about:

bch2_fs_bdev_mark_dead()		umount()
  bdev_get_fs()
    bch2_ro_ref_tryget() -> grabs bch_fs->ro_ref
    mutex_unlock(&bdev->bd_holder_lock);
					deactivate_super()
					  down_write(&sb->s_umount);
					  deactivate_locked_super()
					    bch2_kill_sb()
					      generic_shutdown_super()
					        bch2_put_super()
						  __bch2_fs_stop()
						    bch2_ro_ref_put()
						    wait_event(c->ro_ref_wait, !refcount_read(&c->ro_ref));
  sb = c->vfs_sb;
  down_read(&sb->s_umount); -> deadlock

Which is a case in point why I would like to have a shared infrastructure
for bdev -> sb transition that's used as widely as possible. Because it
isn't easy to get the lock ordering right given all the constraints in the
VFS and block layer code paths for this transition that's going contrary to
the usual ordering sb -> bdev. And yes I do realize bcachefs grabs s_umount
not because it itself needs it but because it calls some VFS helpers
(sync_filesystem()) which expect it to be held so the pain is inflicted
by VFS here but that just demostrates the fact that VFS and FS locking are
deeply intertwined and you can hardly avoid dealing with VFS locking rules
in the filesystem itself.

> It also avoids the problem of ->mark_dead events being generated
> from a context that holds filesystem/vfs locks and then deadlocking
> waiting for those locks to be released.
> 
> IOWs, a multi-device filesystem should really be implementing
> ->mark_dead itself, and should not be depending on being able to
> lock the superblock to take an active reference to it.
> 
> It should be pretty clear that these are not issues that the generic
> filesystem ->mark_dead implementation should be trying to
> handle.....

Well, IMO every fs implementation needs to do the bdev -> sb transition and
make sb somehow stable. It may be that grabbing s_umount and active sb
reference is not what everybody wants but AFAIU btrfs as the second
multi-device filesystem would be fine with that and for bcachefs this
doesn't work only because they have special superblock instantiation
behavior on mount for independent reasons (i.e., not because active ref
+ s_umount would be problematic for them) if I understand Kent right.
So I'm still not fully convinced each multi-device filesystem should be
shipping their special method to get from device to stable sb reference.

> > The shutdown method is implemented only by block-based filesystems and
> > arguably shutdown was always a misnomer because it assumed that the
> > filesystem needs to actually shut down when it is called.
> 
> Shutdown was not -assumed- as the operation that needed to be
> performed. That was the feature that was *required* to fix
> filesystem level problems that occur when the device underneath it
> disappears.
> 
> ->mark_dead() is the abstract filesystem notification from the block
> device, fs_bdfev_mark_dead() is the -generic implementation- of the
> functionality required by single block device filesystems. Part of
> that functionality is shutting down the filesystem because it can
> *no longer function without a backing device*.
> 
> multi-block device filesystems require compeltely different
> implementations, and we already have one that -does not use active
> superblock references-. IOWs, even if we add ->remove_bdev(sb)
> callout, bcachefs will continue to use ->mark_dead() because low
> level filesystem device management isn't (and shouldn't be!)
> dependent on high level VFS structure reference counting....

I have to admit I don't get why device management shouldn't be dependent on
VFS refcounts / locking. IMO it is often dependent although I agree with
multiple devices you likely have to do *additional* locking. And yes, I can
imagine VFS locking could get in your way but the only tangible example we
have is bcachefs and btrfs seems to be a counter example showing even multi
device filesystem can live with VFS locking. So I don't think the case is
as clear as you try to frame it.

So conceptually I agree making filesystems as bdev holders implement their
own holder ops makes a lot of sense but because of lock ordering rules it
is not quite practical or easily maintainable choice I'm afraid.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v4 1/6] fs: enhance and rename shutdown() callback to remove_bdev()

[Kent Overstreet]

On Wed, Jul 09, 2025 at 07:23:07PM +0200, Jan Kara wrote:
> On Wed 09-07-25 08:59:42, Dave Chinner wrote:
> > This means that device removal processing can be performed
> > without global filesystem/VFS locks needing to be held. Hence issues
> > like re-entrancy deadlocks when there are concurrent/cascading
> > device failures (e.g. a HBA dies, taking out multiple devices
> > simultaneously) are completely avoided...
> 
> Funnily enough how about:
> 
> bch2_fs_bdev_mark_dead()		umount()
>   bdev_get_fs()
>     bch2_ro_ref_tryget() -> grabs bch_fs->ro_ref
>     mutex_unlock(&bdev->bd_holder_lock);
> 					deactivate_super()
> 					  down_write(&sb->s_umount);
> 					  deactivate_locked_super()
> 					    bch2_kill_sb()
> 					      generic_shutdown_super()
> 					        bch2_put_super()
> 						  __bch2_fs_stop()
> 						    bch2_ro_ref_put()
> 						    wait_event(c->ro_ref_wait, !refcount_read(&c->ro_ref));
>   sb = c->vfs_sb;
>   down_read(&sb->s_umount); -> deadlock
> 
> Which is a case in point why I would like to have a shared infrastructure
> for bdev -> sb transition that's used as widely as possible. Because it
> isn't easy to get the lock ordering right given all the constraints in the
> VFS and block layer code paths for this transition that's going contrary to
> the usual ordering sb -> bdev. And yes I do realize bcachefs grabs s_umount
> not because it itself needs it but because it calls some VFS helpers
> (sync_filesystem()) which expect it to be held so the pain is inflicted
> by VFS here but that just demostrates the fact that VFS and FS locking are
> deeply intertwined and you can hardly avoid dealing with VFS locking rules
> in the filesystem itself.

Getting rid of the s_umount use looks like the much saner and easier
fix - like the comment notes, it's only taken to avoid the warning in
sync_filesystem, we don't actually need it.

Locking gets easier when locks are private to individual subsystems,
protecting specific data structures that are private to those
subsystems.

> > It also avoids the problem of ->mark_dead events being generated
> > from a context that holds filesystem/vfs locks and then deadlocking
> > waiting for those locks to be released.
> > 
> > IOWs, a multi-device filesystem should really be implementing
> > ->mark_dead itself, and should not be depending on being able to
> > lock the superblock to take an active reference to it.
> > 
> > It should be pretty clear that these are not issues that the generic
> > filesystem ->mark_dead implementation should be trying to
> > handle.....
> 
> Well, IMO every fs implementation needs to do the bdev -> sb transition and
> make sb somehow stable. It may be that grabbing s_umount and active sb
> reference is not what everybody wants but AFAIU btrfs as the second
> multi-device filesystem would be fine with that and for bcachefs this
> doesn't work only because they have special superblock instantiation
> behavior on mount for independent reasons (i.e., not because active ref
> + s_umount would be problematic for them) if I understand Kent right.
> So I'm still not fully convinced each multi-device filesystem should be
> shipping their special method to get from device to stable sb reference.

Honestly, the sync_filesystem() call seems bogus.

If the block device is truly dead, what's it going to accomplish?

It's not like we get callbacks for "this device is going to be going
away soon", we only get that in reaction to something that's already
happened.

> > > The shutdown method is implemented only by block-based filesystems and
> > > arguably shutdown was always a misnomer because it assumed that the
> > > filesystem needs to actually shut down when it is called.
> > 
> > Shutdown was not -assumed- as the operation that needed to be
> > performed. That was the feature that was *required* to fix
> > filesystem level problems that occur when the device underneath it
> > disappears.
> > 
> > ->mark_dead() is the abstract filesystem notification from the block
> > device, fs_bdfev_mark_dead() is the -generic implementation- of the
> > functionality required by single block device filesystems. Part of
> > that functionality is shutting down the filesystem because it can
> > *no longer function without a backing device*.
> > 
> > multi-block device filesystems require compeltely different
> > implementations, and we already have one that -does not use active
> > superblock references-. IOWs, even if we add ->remove_bdev(sb)
> > callout, bcachefs will continue to use ->mark_dead() because low
> > level filesystem device management isn't (and shouldn't be!)
> > dependent on high level VFS structure reference counting....
> 
> I have to admit I don't get why device management shouldn't be dependent on
> VFS refcounts / locking. IMO it is often dependent although I agree with
> multiple devices you likely have to do *additional* locking. And yes, I can
> imagine VFS locking could get in your way but the only tangible example we
> have is bcachefs and btrfs seems to be a counter example showing even multi
> device filesystem can live with VFS locking. So I don't think the case is
> as clear as you try to frame it.

Individual devices coming and going has nothing to do with the VFS. If a
single device goes away and we're continuing in RW mode, _no_ VFS state
is affected whatsoever.

The only thing that's needed is a ref to prevent the filesystem from
going away, not a lock. But again given that a bch_fs doesn't
necessarily even have a VFS superblock it's not something we'd use
directly in .mark_dead, that synchronization is handled directly via
kill_sb -> generic_shutdown_super -> and all that...

We don't want bch_fs to outlive the VFS superblock if we do have a VFS
sb, because asynchronous shutdown and releasing of resources causes very
real problems (which already exist for other reasons...)

Re: [PATCH v4 1/6] fs: enhance and rename shutdown() callback to remove_bdev()

[Christian Brauner]

On Wed, Jul 09, 2025 at 08:59:42AM +1000, Dave Chinner wrote:
> On Tue, Jul 08, 2025 at 09:55:14AM +0200, Christian Brauner wrote:
> > On Mon, Jul 07, 2025 at 05:45:32PM -0700, Darrick J. Wong wrote:
> > > On Tue, Jul 08, 2025 at 08:52:47AM +0930, Qu Wenruo wrote:
> > > > 
> > > > 
> > > > 在 2025/7/8 08:32, Dave Chinner 写道:
> > > > > On Fri, Jul 04, 2025 at 10:12:29AM +0930, Qu Wenruo wrote:
> > > > > > Currently all the filesystems implementing the
> > > > > > super_opearations::shutdown() callback can not afford losing a device.
> > > > > > 
> > > > > > Thus fs_bdev_mark_dead() will just call the shutdown() callback for the
> > > > > > involved filesystem.
> > > > > > 
> > > > > > But it will no longer be the case, with multi-device filesystems like
> > > > > > btrfs and bcachefs the filesystem can handle certain device loss without
> > > > > > shutting down the whole filesystem.
> > > > > > 
> > > > > > To allow those multi-device filesystems to be integrated to use
> > > > > > fs_holder_ops:
> > > > > > 
> > > > > > - Replace super_opearation::shutdown() with
> > > > > >    super_opearations::remove_bdev()
> > > > > >    To better describe when the callback is called.
> > > > > 
> > > > > This conflates cause with action.
> > > > > 
> > > > > The shutdown callout is an action that the filesystem must execute,
> > > > > whilst "remove bdev" is a cause notification that might require an
> > > > > action to be take.
> > > > > 
> > > > > Yes, the cause could be someone doing hot-unplug of the block
> > > > > device, but it could also be something going wrong in software
> > > > > layers below the filesystem. e.g. dm-thinp having an unrecoverable
> > > > > corruption or ENOSPC errors.
> > > > > 
> > > > > We already have a "cause" notification: blk_holder_ops->mark_dead().
> > > > > 
> > > > > The generic fs action that is taken by this notification is
> > > > > fs_bdev_mark_dead().  That action is to invalidate caches and shut
> > > > > down the filesystem.
> > > > > 
> > > > > btrfs needs to do something different to a blk_holder_ops->mark_dead
> > > > > notification. i.e. it needs an action that is different to
> > > > > fs_bdev_mark_dead().
> > > > > 
> > > > > Indeed, this is how bcachefs already handles "single device
> > > > > died" events for multi-device filesystems - see
> > > > > bch2_fs_bdev_mark_dead().
> > > > 
> > > > I do not think it's the correct way to go, especially when there is already
> > > > fs_holder_ops.
> > > > 
> > > > We're always going towards a more generic solution, other than letting the
> > > > individual fs to do the same thing slightly differently.
> > > 
> > > On second thought -- it's weird that you'd flush the filesystem and
> > > shrink the inode/dentry caches in a "your device went away" handler.
> > > Fancy filesystems like bcachefs and btrfs would likely just shift IO to
> > > a different bdev, right?  And there's no good reason to run shrinkers on
> > > either of those fses, right?
> > > 
> > > > Yes, the naming is not perfect and mixing cause and action, but the end
> > > > result is still a more generic and less duplicated code base.
> > > 
> > > I think dchinner makes a good point that if your filesystem can do
> > > something clever on device removal, it should provide its own block
> > > device holder ops instead of using fs_holder_ops.  I don't understand
> > > why you need a "generic" solution for btrfs when it's not going to do
> > > what the others do anyway.
> > 
> > I think letting filesystems implement their own holder ops should be
> > avoided if we can. Christoph may chime in here. I have no appettite for
> > exporting stuff like get_bdev_super() unless absolutely necessary. We
> > tried to move all that handling into the VFS to eliminate a slew of
> > deadlocks we detected and fixed. I have no appetite to repeat that
> > cycle.
> 
> Except it isn't actually necessary.
> 
> Everyone here seems to be assuming that the filesystem *must* take
> an active superblock reference to process a device removal event,
> and that is *simply not true*.
> 
> bcachefs does not use get_bdev_super() or an active superblock
> reference to process ->mark_dead events.
> 
> It has it's own internal reference counting on the struct bch_fs
> attached to the bdev that ensures the filesystem structures can't go
> away whilst ->mark_dead is being processed.  i.e. bcachefs is only
> dependent on the bdev->bd_holder_lock() being held when
> ->mark_dead() is called and does not rely on the VFS for anything.
> 
> This means that device removal processing can be performed
> without global filesystem/VFS locks needing to be held. Hence issues
> like re-entrancy deadlocks when there are concurrent/cascading
> device failures (e.g. a HBA dies, taking out multiple devices
> simultaneously) are completely avoided...
> 
> It also avoids the problem of ->mark_dead events being generated
> from a context that holds filesystem/vfs locks and then deadlocking
> waiting for those locks to be released.
> 
> IOWs, a multi-device filesystem should really be implementing
> ->mark_dead itself, and should not be depending on being able to
> lock the superblock to take an active reference to it.
> 
> It should be pretty clear that these are not issues that the generic
> filesystem ->mark_dead implementation should be trying to
> handle.....
> 
> > The shutdown method is implemented only by block-based filesystems and
> > arguably shutdown was always a misnomer because it assumed that the
> > filesystem needs to actually shut down when it is called.
> 
> Shutdown was not -assumed- as the operation that needed to be
> performed. That was the feature that was *required* to fix
> filesystem level problems that occur when the device underneath it
> disappears.
> 
> ->mark_dead() is the abstract filesystem notification from the block
> device, fs_bdfev_mark_dead() is the -generic implementation- of the
> functionality required by single block device filesystems. Part of
> that functionality is shutting down the filesystem because it can
> *no longer function without a backing device*.
> 
> multi-block device filesystems require compeltely different
> implementations, and we already have one that -does not use active
> superblock references-. IOWs, even if we add ->remove_bdev(sb)
> callout, bcachefs will continue to use ->mark_dead() because low
> level filesystem device management isn't (and shouldn't be!)
> dependent on high level VFS structure reference counting....
> 
> > IOW, we made
> > it so that it is a call to action but that doesn't have to be the case.
> > Calling it ->remove_bdev() is imo the correct thing because it gives
> > block based filesystem the ability to handle device events how they see
> > fit.
> 
> And that's exactly what ->mark_dead already provides. 
> 
> And, as I've pointed out above, multi-device filesystems don't
> actually need actively referenced superblocks to process device
> removal notifications. Hence ->mark_dead is the correct interface
> for them to use.

I'm not sure what this is trying to argue about as we agree.

All current filesystems that use the fs_holder_ops require an active
superblock reference. If they want to rewrite themselves to not need an
active superblock reference and switch to custom holder ops then the VFS
doesn't care.

This is about what is currently the case. Everyone is aware that a
filesystem can do this differently.

If btrfs wants to rely on the VFS infrastructure then we will enable it
and we will help them move along and the only requirement is that we
don't have to bleed the VFS locking requirements into the specific
filesystem unnecessarily. That's all this is.

Re: [PATCH v4 1/6] fs: enhance and rename shutdown() callback to remove_bdev()

[Christian Brauner]

On Tue, Jul 08, 2025 at 01:20:50PM -0700, Darrick J. Wong wrote:
> On Tue, Jul 08, 2025 at 12:20:00PM +0200, Jan Kara wrote:
> > On Mon 07-07-25 17:45:32, Darrick J. Wong wrote:
> > > On Tue, Jul 08, 2025 at 08:52:47AM +0930, Qu Wenruo wrote:
> > > > 在 2025/7/8 08:32, Dave Chinner 写道:
> > > > > On Fri, Jul 04, 2025 at 10:12:29AM +0930, Qu Wenruo wrote:
> > > > > > Currently all the filesystems implementing the
> > > > > > super_opearations::shutdown() callback can not afford losing a device.
> > > > > > 
> > > > > > Thus fs_bdev_mark_dead() will just call the shutdown() callback for the
> > > > > > involved filesystem.
> > > > > > 
> > > > > > But it will no longer be the case, with multi-device filesystems like
> > > > > > btrfs and bcachefs the filesystem can handle certain device loss without
> > > > > > shutting down the whole filesystem.
> > > > > > 
> > > > > > To allow those multi-device filesystems to be integrated to use
> > > > > > fs_holder_ops:
> > > > > > 
> > > > > > - Replace super_opearation::shutdown() with
> > > > > >    super_opearations::remove_bdev()
> > > > > >    To better describe when the callback is called.
> > > > > 
> > > > > This conflates cause with action.
> > > > > 
> > > > > The shutdown callout is an action that the filesystem must execute,
> > > > > whilst "remove bdev" is a cause notification that might require an
> > > > > action to be take.
> > > > > 
> > > > > Yes, the cause could be someone doing hot-unplug of the block
> > > > > device, but it could also be something going wrong in software
> > > > > layers below the filesystem. e.g. dm-thinp having an unrecoverable
> > > > > corruption or ENOSPC errors.
> > > > > 
> > > > > We already have a "cause" notification: blk_holder_ops->mark_dead().
> > > > > 
> > > > > The generic fs action that is taken by this notification is
> > > > > fs_bdev_mark_dead().  That action is to invalidate caches and shut
> > > > > down the filesystem.
> > > > > 
> > > > > btrfs needs to do something different to a blk_holder_ops->mark_dead
> > > > > notification. i.e. it needs an action that is different to
> > > > > fs_bdev_mark_dead().
> > > > > 
> > > > > Indeed, this is how bcachefs already handles "single device
> > > > > died" events for multi-device filesystems - see
> > > > > bch2_fs_bdev_mark_dead().
> > > > 
> > > > I do not think it's the correct way to go, especially when there is already
> > > > fs_holder_ops.
> > > > 
> > > > We're always going towards a more generic solution, other than letting the
> > > > individual fs to do the same thing slightly differently.
> > > 
> > > On second thought -- it's weird that you'd flush the filesystem and
> > > shrink the inode/dentry caches in a "your device went away" handler.
> > > Fancy filesystems like bcachefs and btrfs would likely just shift IO to
> > > a different bdev, right?  And there's no good reason to run shrinkers on
> > > either of those fses, right?
> > 
> > I agree it is awkward and bcachefs avoids these in case of removal it can
> > handle gracefully AFAICS.
> > 
> > > > Yes, the naming is not perfect and mixing cause and action, but the end
> > > > result is still a more generic and less duplicated code base.
> > > 
> > > I think dchinner makes a good point that if your filesystem can do
> > > something clever on device removal, it should provide its own block
> > > device holder ops instead of using fs_holder_ops.  I don't understand
> > > why you need a "generic" solution for btrfs when it's not going to do
> > > what the others do anyway.
> > 
> > Well, I'd also say just go for own fs_holder_ops if it was not for the
> > awkward "get super from bdev" step. As Christian wrote we've encapsulated
> > that in fs/super.c and bdev_super_lock() in particular but the calling
> > conventions for the fs_holder_ops are not very nice (holding
> > bdev_holder_lock, need to release it before grabbing practically anything
> > else) so I'd have much greater peace of mind if this didn't spread too
> > much. Once you call bdev_super_lock() and hold on to sb with s_umount held,
> > things are much more conventional for the fs land so I'd like if this
> > step happened before any fs hook got called. So I prefer something like
> > Qu's proposal of separate sb op for device removal over exporting
> > bdev_super_lock(). Like:
> > 
> > static void fs_bdev_mark_dead(struct block_device *bdev, bool surprise)
> > {
> >         struct super_block *sb;
> > 
> >         sb = bdev_super_lock(bdev, false);
> >         if (!sb)
> >                 return;
> > 
> > 	if (sb->s_op->remove_bdev) {
> > 		sb->s_op->remove_bdev(sb, bdev, surprise);
> > 		return;
> > 	}
> 
> It feels odd but I could live with this, particularly since that's the
> direction that brauner is laying down. :)

I want to reiterate that no one is saying "under no circumstances
implement your own holder ops". But if you rely on the VFS locking then
you better not spill it's guts into your filesystem and make us export
this bloody locking that half the world had implemented wrong in their
drivers in the first places spewing endless syzbot deadlocks reports
that we had to track down and fix. That will not happen again similar
way we don't bleed all the nastiness of other locking paths.

Please all stop long philosophical treatises about things no on has ever
argued. btrfs wants to rely on the VFS infra. That is fine and well. We
will support and enable this.

I think the two method idea is fine given that they now are clearly
delineated.

Thanks for providing some clarity here, Darrick and Qu.

Re: [PATCH v4 1/6] fs: enhance and rename shutdown() callback to remove_bdev()

[Qu Wenruo]

在 2025/7/10 18:10, Christian Brauner 写道:
> On Tue, Jul 08, 2025 at 01:20:50PM -0700, Darrick J. Wong wrote:
>> On Tue, Jul 08, 2025 at 12:20:00PM +0200, Jan Kara wrote:
>>> On Mon 07-07-25 17:45:32, Darrick J. Wong wrote:
>>>> On Tue, Jul 08, 2025 at 08:52:47AM +0930, Qu Wenruo wrote:
>>>>> 在 2025/7/8 08:32, Dave Chinner 写道:
>>>>>> On Fri, Jul 04, 2025 at 10:12:29AM +0930, Qu Wenruo wrote:
>>>>>>> Currently all the filesystems implementing the
>>>>>>> super_opearations::shutdown() callback can not afford losing a device.
>>>>>>>
>>>>>>> Thus fs_bdev_mark_dead() will just call the shutdown() callback for the
>>>>>>> involved filesystem.
>>>>>>>
>>>>>>> But it will no longer be the case, with multi-device filesystems like
>>>>>>> btrfs and bcachefs the filesystem can handle certain device loss without
>>>>>>> shutting down the whole filesystem.
>>>>>>>
>>>>>>> To allow those multi-device filesystems to be integrated to use
>>>>>>> fs_holder_ops:
>>>>>>>
>>>>>>> - Replace super_opearation::shutdown() with
>>>>>>>     super_opearations::remove_bdev()
>>>>>>>     To better describe when the callback is called.
>>>>>>
>>>>>> This conflates cause with action.
>>>>>>
>>>>>> The shutdown callout is an action that the filesystem must execute,
>>>>>> whilst "remove bdev" is a cause notification that might require an
>>>>>> action to be take.
>>>>>>
>>>>>> Yes, the cause could be someone doing hot-unplug of the block
>>>>>> device, but it could also be something going wrong in software
>>>>>> layers below the filesystem. e.g. dm-thinp having an unrecoverable
>>>>>> corruption or ENOSPC errors.
>>>>>>
>>>>>> We already have a "cause" notification: blk_holder_ops->mark_dead().
>>>>>>
>>>>>> The generic fs action that is taken by this notification is
>>>>>> fs_bdev_mark_dead().  That action is to invalidate caches and shut
>>>>>> down the filesystem.
>>>>>>
>>>>>> btrfs needs to do something different to a blk_holder_ops->mark_dead
>>>>>> notification. i.e. it needs an action that is different to
>>>>>> fs_bdev_mark_dead().
>>>>>>
>>>>>> Indeed, this is how bcachefs already handles "single device
>>>>>> died" events for multi-device filesystems - see
>>>>>> bch2_fs_bdev_mark_dead().
>>>>>
>>>>> I do not think it's the correct way to go, especially when there is already
>>>>> fs_holder_ops.
>>>>>
>>>>> We're always going towards a more generic solution, other than letting the
>>>>> individual fs to do the same thing slightly differently.
>>>>
>>>> On second thought -- it's weird that you'd flush the filesystem and
>>>> shrink the inode/dentry caches in a "your device went away" handler.
>>>> Fancy filesystems like bcachefs and btrfs would likely just shift IO to
>>>> a different bdev, right?  And there's no good reason to run shrinkers on
>>>> either of those fses, right?
>>>
>>> I agree it is awkward and bcachefs avoids these in case of removal it can
>>> handle gracefully AFAICS.
>>>
>>>>> Yes, the naming is not perfect and mixing cause and action, but the end
>>>>> result is still a more generic and less duplicated code base.
>>>>
>>>> I think dchinner makes a good point that if your filesystem can do
>>>> something clever on device removal, it should provide its own block
>>>> device holder ops instead of using fs_holder_ops.  I don't understand
>>>> why you need a "generic" solution for btrfs when it's not going to do
>>>> what the others do anyway.
>>>
>>> Well, I'd also say just go for own fs_holder_ops if it was not for the
>>> awkward "get super from bdev" step. As Christian wrote we've encapsulated
>>> that in fs/super.c and bdev_super_lock() in particular but the calling
>>> conventions for the fs_holder_ops are not very nice (holding
>>> bdev_holder_lock, need to release it before grabbing practically anything
>>> else) so I'd have much greater peace of mind if this didn't spread too
>>> much. Once you call bdev_super_lock() and hold on to sb with s_umount held,
>>> things are much more conventional for the fs land so I'd like if this
>>> step happened before any fs hook got called. So I prefer something like
>>> Qu's proposal of separate sb op for device removal over exporting
>>> bdev_super_lock(). Like:
>>>
>>> static void fs_bdev_mark_dead(struct block_device *bdev, bool surprise)
>>> {
>>>          struct super_block *sb;
>>>
>>>          sb = bdev_super_lock(bdev, false);
>>>          if (!sb)
>>>                  return;
>>>
>>> 	if (sb->s_op->remove_bdev) {
>>> 		sb->s_op->remove_bdev(sb, bdev, surprise);
>>> 		return;
>>> 	}
>>
>> It feels odd but I could live with this, particularly since that's the
>> direction that brauner is laying down. :)
> 
> I want to reiterate that no one is saying "under no circumstances
> implement your own holder ops". But if you rely on the VFS locking then
> you better not spill it's guts into your filesystem and make us export
> this bloody locking that half the world had implemented wrong in their
> drivers in the first places spewing endless syzbot deadlocks reports
> that we had to track down and fix. That will not happen again similar
> way we don't bleed all the nastiness of other locking paths.
> 
> Please all stop long philosophical treatises about things no on has ever
> argued. btrfs wants to rely on the VFS infra. That is fine and well. We
> will support and enable this.
> 
> I think the two method idea is fine given that they now are clearly
> delineated.
> 
> Thanks for providing some clarity here, Darrick and Qu.
> 

So the next update would be something like this for fs_bdev_mark_dead():

	sb = bdev_super_lock();
	if (!sb)
		return;
	if (!surprise)
		sync_filesystem(sb);
+	if (sb->s_op->remove_bdev) {
+		ret = sb->s_op->remove_bdev();
+		if (!ret) {
+			/* Fs can handle the dev loss. */
+			super_unlock_shared();
+			return;
+		}
+	}
+	/* Fs can not handle the dev loss, shutdown. */
	shrink_dcache_sb();
	evict_inodes();
	if (sb->s_op->shutdown)
		sb->s_op->shutdown();
	super_unlock_shared();

This means ->remove_bdev() must have a return value to indicate if the 
fs can handle the loss.
And any error, no matter if it's not enough tolerance from the fs or 
some other problem during the dev loss handling, the old shutdown 
behavior will be triggered.

Would this be an acceptable solution?

Thanks,
Qu

Re: [PATCH v4 1/6] fs: enhance and rename shutdown() callback to remove_bdev()

[Christoph Hellwig]

On Mon, Jul 07, 2025 at 05:45:32PM -0700, Darrick J. Wong wrote:
> On second thought -- it's weird that you'd flush the filesystem and
> shrink the inode/dentry caches in a "your device went away" handler.
> Fancy filesystems like bcachefs and btrfs would likely just shift IO to
> a different bdev, right?  And there's no good reason to run shrinkers on
> either of those fses, right?

No nmeed for fancy file systems, this is weird no matter what.  But it
is what Linux has done for 30+ years, so I kept it when refactoring
this code to sit in a callback.

> > Yes, the naming is not perfect and mixing cause and action, but the end
> > result is still a more generic and less duplicated code base.
> 
> I think dchinner makes a good point that if your filesystem can do
> something clever on device removal, it should provide its own block
> device holder ops instead of using fs_holder_ops.  I don't understand
> why you need a "generic" solution for btrfs when it's not going to do
> what the others do anyway.

Why?  You're most likely to get the locking wrong, and so on.

What might make sense is to move the sync_filesystem, shrink_dcache_sb
and evict_inodes into the method.  That way file systems where we

> As an aside:
> 'twould be nice if we could lift the *FS_IOC_SHUTDOWN dispatch out of
> everyone's ioctl functions into the VFS, and then move the "I am dead"
> state into super_block so that you could actually shut down any
> filesystem, not just the seven that currently implement it.

Sure.  Someone just needs to do the work..

Re: [PATCH v4 1/6] fs: enhance and rename shutdown() callback to remove_bdev()

[Christoph Hellwig]

On Tue, Jul 08, 2025 at 09:55:14AM +0200, Christian Brauner wrote:
> I think letting filesystems implement their own holder ops should be
> avoided if we can. Christoph may chime in here.

Ccing helps for that..

>
> I have no appettite for
> exporting stuff like get_bdev_super() unless absolutely necessary. We
> tried to move all that handling into the VFS to eliminate a slew of
> deadlocks we detected and fixed. I have no appetite to repeat that
> cycle.

Exactly.

> The shutdown method is implemented only by block-based filesystems and
> arguably shutdown was always a misnomer because it assumed that the
> filesystem needs to actually shut down when it is called. IOW, we made
> it so that it is a call to action but that doesn't have to be the case.
> Calling it ->remove_bdev() is imo the correct thing because it gives
> block based filesystem the ability to handle device events how they see
> fit.
> 
> Once we will have non-block based filesystems that need a method to
> always shut down the filesystem itself we might have to revisit this
> design anyway but no one had that use-case yet.

I'm not sure what non-block file systems would need it for except
maybe for a generic IOC_SHUTDOWN implementation, but that would have
a different signature anyway as it needs to pass flags that don't
fit here.  So that would be a separate method.

Re: [PATCH v4 1/6] fs: enhance and rename shutdown() callback to remove_bdev()

[Jan Kara]

On Wed 09-07-25 13:49:12, Kent Overstreet wrote:
> On Wed, Jul 09, 2025 at 07:23:07PM +0200, Jan Kara wrote:
> > > It also avoids the problem of ->mark_dead events being generated
> > > from a context that holds filesystem/vfs locks and then deadlocking
> > > waiting for those locks to be released.
> > > 
> > > IOWs, a multi-device filesystem should really be implementing
> > > ->mark_dead itself, and should not be depending on being able to
> > > lock the superblock to take an active reference to it.
> > > 
> > > It should be pretty clear that these are not issues that the generic
> > > filesystem ->mark_dead implementation should be trying to
> > > handle.....
> > 
> > Well, IMO every fs implementation needs to do the bdev -> sb transition and
> > make sb somehow stable. It may be that grabbing s_umount and active sb
> > reference is not what everybody wants but AFAIU btrfs as the second
> > multi-device filesystem would be fine with that and for bcachefs this
> > doesn't work only because they have special superblock instantiation
> > behavior on mount for independent reasons (i.e., not because active ref
> > + s_umount would be problematic for them) if I understand Kent right.
> > So I'm still not fully convinced each multi-device filesystem should be
> > shipping their special method to get from device to stable sb reference.
> 
> Honestly, the sync_filesystem() call seems bogus.
> 
> If the block device is truly dead, what's it going to accomplish?

Notice that fs_bdev_mark_dead() calls sync_filesystem() only in case
'surprise' argument is false - meaning this is actually a notification
*before* the device is going away. I.e., graceful device hot unplug when
you can access the device to clean up as much as possible.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v4 1/6] fs: enhance and rename shutdown() callback to remove_bdev()

[Kent Overstreet]

On Thu, Jul 10, 2025 at 03:10:04PM +0200, Jan Kara wrote:
> On Wed 09-07-25 13:49:12, Kent Overstreet wrote:
> > On Wed, Jul 09, 2025 at 07:23:07PM +0200, Jan Kara wrote:
> > > > It also avoids the problem of ->mark_dead events being generated
> > > > from a context that holds filesystem/vfs locks and then deadlocking
> > > > waiting for those locks to be released.
> > > > 
> > > > IOWs, a multi-device filesystem should really be implementing
> > > > ->mark_dead itself, and should not be depending on being able to
> > > > lock the superblock to take an active reference to it.
> > > > 
> > > > It should be pretty clear that these are not issues that the generic
> > > > filesystem ->mark_dead implementation should be trying to
> > > > handle.....
> > > 
> > > Well, IMO every fs implementation needs to do the bdev -> sb transition and
> > > make sb somehow stable. It may be that grabbing s_umount and active sb
> > > reference is not what everybody wants but AFAIU btrfs as the second
> > > multi-device filesystem would be fine with that and for bcachefs this
> > > doesn't work only because they have special superblock instantiation
> > > behavior on mount for independent reasons (i.e., not because active ref
> > > + s_umount would be problematic for them) if I understand Kent right.
> > > So I'm still not fully convinced each multi-device filesystem should be
> > > shipping their special method to get from device to stable sb reference.
> > 
> > Honestly, the sync_filesystem() call seems bogus.
> > 
> > If the block device is truly dead, what's it going to accomplish?
> 
> Notice that fs_bdev_mark_dead() calls sync_filesystem() only in case
> 'surprise' argument is false - meaning this is actually a notification
> *before* the device is going away. I.e., graceful device hot unplug when
> you can access the device to clean up as much as possible.

That doesn't seem to be hooked up to anything?

blk_mark_disk_dead() -> blk_report_disk_dead(), surprise is always true

disk_force_media_change(), same

The only call where it's falso is in s390 code. If we know that a disk
is going away, that would be a userspace thing, and they can just
unmount.

Re: [PATCH v4 1/6] fs: enhance and rename shutdown() callback to remove_bdev()

[Christian Brauner]

On Thu, Jul 10, 2025 at 07:24:46PM +0930, Qu Wenruo wrote:
> 
> 
> 在 2025/7/10 18:10, Christian Brauner 写道:
> > On Tue, Jul 08, 2025 at 01:20:50PM -0700, Darrick J. Wong wrote:
> > > On Tue, Jul 08, 2025 at 12:20:00PM +0200, Jan Kara wrote:
> > > > On Mon 07-07-25 17:45:32, Darrick J. Wong wrote:
> > > > > On Tue, Jul 08, 2025 at 08:52:47AM +0930, Qu Wenruo wrote:
> > > > > > 在 2025/7/8 08:32, Dave Chinner 写道:
> > > > > > > On Fri, Jul 04, 2025 at 10:12:29AM +0930, Qu Wenruo wrote:
> > > > > > > > Currently all the filesystems implementing the
> > > > > > > > super_opearations::shutdown() callback can not afford losing a device.
> > > > > > > > 
> > > > > > > > Thus fs_bdev_mark_dead() will just call the shutdown() callback for the
> > > > > > > > involved filesystem.
> > > > > > > > 
> > > > > > > > But it will no longer be the case, with multi-device filesystems like
> > > > > > > > btrfs and bcachefs the filesystem can handle certain device loss without
> > > > > > > > shutting down the whole filesystem.
> > > > > > > > 
> > > > > > > > To allow those multi-device filesystems to be integrated to use
> > > > > > > > fs_holder_ops:
> > > > > > > > 
> > > > > > > > - Replace super_opearation::shutdown() with
> > > > > > > >     super_opearations::remove_bdev()
> > > > > > > >     To better describe when the callback is called.
> > > > > > > 
> > > > > > > This conflates cause with action.
> > > > > > > 
> > > > > > > The shutdown callout is an action that the filesystem must execute,
> > > > > > > whilst "remove bdev" is a cause notification that might require an
> > > > > > > action to be take.
> > > > > > > 
> > > > > > > Yes, the cause could be someone doing hot-unplug of the block
> > > > > > > device, but it could also be something going wrong in software
> > > > > > > layers below the filesystem. e.g. dm-thinp having an unrecoverable
> > > > > > > corruption or ENOSPC errors.
> > > > > > > 
> > > > > > > We already have a "cause" notification: blk_holder_ops->mark_dead().
> > > > > > > 
> > > > > > > The generic fs action that is taken by this notification is
> > > > > > > fs_bdev_mark_dead().  That action is to invalidate caches and shut
> > > > > > > down the filesystem.
> > > > > > > 
> > > > > > > btrfs needs to do something different to a blk_holder_ops->mark_dead
> > > > > > > notification. i.e. it needs an action that is different to
> > > > > > > fs_bdev_mark_dead().
> > > > > > > 
> > > > > > > Indeed, this is how bcachefs already handles "single device
> > > > > > > died" events for multi-device filesystems - see
> > > > > > > bch2_fs_bdev_mark_dead().
> > > > > > 
> > > > > > I do not think it's the correct way to go, especially when there is already
> > > > > > fs_holder_ops.
> > > > > > 
> > > > > > We're always going towards a more generic solution, other than letting the
> > > > > > individual fs to do the same thing slightly differently.
> > > > > 
> > > > > On second thought -- it's weird that you'd flush the filesystem and
> > > > > shrink the inode/dentry caches in a "your device went away" handler.
> > > > > Fancy filesystems like bcachefs and btrfs would likely just shift IO to
> > > > > a different bdev, right?  And there's no good reason to run shrinkers on
> > > > > either of those fses, right?
> > > > 
> > > > I agree it is awkward and bcachefs avoids these in case of removal it can
> > > > handle gracefully AFAICS.
> > > > 
> > > > > > Yes, the naming is not perfect and mixing cause and action, but the end
> > > > > > result is still a more generic and less duplicated code base.
> > > > > 
> > > > > I think dchinner makes a good point that if your filesystem can do
> > > > > something clever on device removal, it should provide its own block
> > > > > device holder ops instead of using fs_holder_ops.  I don't understand
> > > > > why you need a "generic" solution for btrfs when it's not going to do
> > > > > what the others do anyway.
> > > > 
> > > > Well, I'd also say just go for own fs_holder_ops if it was not for the
> > > > awkward "get super from bdev" step. As Christian wrote we've encapsulated
> > > > that in fs/super.c and bdev_super_lock() in particular but the calling
> > > > conventions for the fs_holder_ops are not very nice (holding
> > > > bdev_holder_lock, need to release it before grabbing practically anything
> > > > else) so I'd have much greater peace of mind if this didn't spread too
> > > > much. Once you call bdev_super_lock() and hold on to sb with s_umount held,
> > > > things are much more conventional for the fs land so I'd like if this
> > > > step happened before any fs hook got called. So I prefer something like
> > > > Qu's proposal of separate sb op for device removal over exporting
> > > > bdev_super_lock(). Like:
> > > > 
> > > > static void fs_bdev_mark_dead(struct block_device *bdev, bool surprise)
> > > > {
> > > >          struct super_block *sb;
> > > > 
> > > >          sb = bdev_super_lock(bdev, false);
> > > >          if (!sb)
> > > >                  return;
> > > > 
> > > > 	if (sb->s_op->remove_bdev) {
> > > > 		sb->s_op->remove_bdev(sb, bdev, surprise);
> > > > 		return;
> > > > 	}
> > > 
> > > It feels odd but I could live with this, particularly since that's the
> > > direction that brauner is laying down. :)
> > 
> > I want to reiterate that no one is saying "under no circumstances
> > implement your own holder ops". But if you rely on the VFS locking then
> > you better not spill it's guts into your filesystem and make us export
> > this bloody locking that half the world had implemented wrong in their
> > drivers in the first places spewing endless syzbot deadlocks reports
> > that we had to track down and fix. That will not happen again similar
> > way we don't bleed all the nastiness of other locking paths.
> > 
> > Please all stop long philosophical treatises about things no on has ever
> > argued. btrfs wants to rely on the VFS infra. That is fine and well. We
> > will support and enable this.
> > 
> > I think the two method idea is fine given that they now are clearly
> > delineated.
> > 
> > Thanks for providing some clarity here, Darrick and Qu.
> > 
> 
> So the next update would be something like this for fs_bdev_mark_dead():
> 
> 	sb = bdev_super_lock();
> 	if (!sb)
> 		return;
> 	if (!surprise)
> 		sync_filesystem(sb);
> +	if (sb->s_op->remove_bdev) {
> +		ret = sb->s_op->remove_bdev();
> +		if (!ret) {
> +			/* Fs can handle the dev loss. */
> +			super_unlock_shared();
> +			return;
> +		}
> +	}
> +	/* Fs can not handle the dev loss, shutdown. */
> 	shrink_dcache_sb();
> 	evict_inodes();
> 	if (sb->s_op->shutdown)
> 		sb->s_op->shutdown();
> 	super_unlock_shared();
> 
> This means ->remove_bdev() must have a return value to indicate if the fs
> can handle the loss.
> And any error, no matter if it's not enough tolerance from the fs or some
> other problem during the dev loss handling, the old shutdown behavior will
> be triggered.
> 
> Would this be an acceptable solution?

This works for me.

Re: [PATCH v4 1/6] fs: enhance and rename shutdown() callback to remove_bdev()

[Jan Kara]

On Thu 10-07-25 14:41:18, Kent Overstreet wrote:
> On Thu, Jul 10, 2025 at 03:10:04PM +0200, Jan Kara wrote:
> > On Wed 09-07-25 13:49:12, Kent Overstreet wrote:
> > > On Wed, Jul 09, 2025 at 07:23:07PM +0200, Jan Kara wrote:
> > > > > It also avoids the problem of ->mark_dead events being generated
> > > > > from a context that holds filesystem/vfs locks and then deadlocking
> > > > > waiting for those locks to be released.
> > > > > 
> > > > > IOWs, a multi-device filesystem should really be implementing
> > > > > ->mark_dead itself, and should not be depending on being able to
> > > > > lock the superblock to take an active reference to it.
> > > > > 
> > > > > It should be pretty clear that these are not issues that the generic
> > > > > filesystem ->mark_dead implementation should be trying to
> > > > > handle.....
> > > > 
> > > > Well, IMO every fs implementation needs to do the bdev -> sb transition and
> > > > make sb somehow stable. It may be that grabbing s_umount and active sb
> > > > reference is not what everybody wants but AFAIU btrfs as the second
> > > > multi-device filesystem would be fine with that and for bcachefs this
> > > > doesn't work only because they have special superblock instantiation
> > > > behavior on mount for independent reasons (i.e., not because active ref
> > > > + s_umount would be problematic for them) if I understand Kent right.
> > > > So I'm still not fully convinced each multi-device filesystem should be
> > > > shipping their special method to get from device to stable sb reference.
> > > 
> > > Honestly, the sync_filesystem() call seems bogus.
> > > 
> > > If the block device is truly dead, what's it going to accomplish?
> > 
> > Notice that fs_bdev_mark_dead() calls sync_filesystem() only in case
> > 'surprise' argument is false - meaning this is actually a notification
> > *before* the device is going away. I.e., graceful device hot unplug when
> > you can access the device to clean up as much as possible.
> 
> That doesn't seem to be hooked up to anything?

__del_gendisk()
  if (!test_bit(GD_DEAD, &disk->state))
    blk_report_disk_dead(disk, false);

Is the path which results in "surprise" to be false. I have to admit I
didn't check deeper into drivers whether this is hooked up properly but
del_gendisk() is a standard call to tear down a disk so it would seem so
from the first glance.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR