btrfs: add remove_bdev() callback

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.

[...]

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 [email protected]

Cc [email protected]

Cc [email protected]

Cc [email protected]

Cc [email protected]

Signed-off-by Qu Wenruo <[email protected]>

---
 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(-)

fs/exfat/super.c +2 -2

index 7ed858937d45..a0e11166b194 100644
@@ -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 {

fs/ext4/super.c +2 -2

index c7d39da7e733..d75b416401ae 100644
@@ -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,

fs/f2fs/super.c +2 -2

index bbf1dad6843f..8667af9f76e4 100644
@@ -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

fs/ntfs3/super.c +3 -3

index 920a1ab47b63..3e69dc805e3a 100644
@@ -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,
 };

fs/super.c +2 -2

index 80418ca8e215..c972efb38f6a 100644
@@ -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);
 }

fs/xfs/xfs_super.c +4 -3

index 0bc4b5489078..8e307b036133 100644
@@ -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,
 };
 

include/linux/fs.h +6 -1

index b085f161ed22..b08af63d2d4f 100644
@@ -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);
 };
 
 /*

Signature · 1 line

2.50.0

This includes the following callbacks of file_operations:

- read_iter()
- write_iter()
- mmap()
- open()
- remap_file_range()
- uring_cmd()
- splice_read()
This requires a small wrapper to do the extra shutdown check, then call the regular filemap_splice_read() function

This should reject most of the file operations on a shutdown btrfs.

The callback ioctl() is intentionally skipped, as ext4 doesn't do the shutdown check on ioctl() either, thus I believe there is some special require for ioctl() callback even if the fs is fully shutdown.

Signed-off-by Qu Wenruo <[email protected]>

---
 fs/btrfs/file.c    | 25 ++++++++++++++++++++++++-
 fs/btrfs/ioctl.c   |  3 +++
 fs/btrfs/reflink.c |  3 +++
 3 files changed, 30 insertions(+), 1 deletion(-)

fs/btrfs/file.c +0 -0

index 05b046c6806f..cb7d1d53fc13 100644

The shutdown ioctl should follow the XFS one, which use magic number 'X', and ioctl number 125, with a u32 as flags.

For now btrfs don't distinguish DEFAULT and LOGFLUSH flags (just like f2fs), both will freeze the fs first (implies committing the current transaction), setting the SHUTDOWN flag and finally thaw the fs.

For NOLOGFLUSH flag, the freeze/thaw part is skipped thus the current transaction is aborted.

The new shutdown ioctl is hidden behind experimental features for more testing.

Signed-off-by Qu Wenruo <[email protected]>

---
 fs/btrfs/ioctl.c           | 40 ++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/btrfs.h |  9 +++++++++
 2 files changed, 49 insertions(+)

fs/btrfs/ioctl.c +5 -0

index 2f3b7be13bea..94eb7a8499db 100644
@@ -5194,6 +5194,36 @@ static int btrfs_ioctl_subvol_sync(struct btrfs_fs_info *fs_info, void __user *a
 	return 0;
 }
 
+#ifdef CONFIG_BTRFS_EXPERIMENTAL
+static int btrfs_emergency_shutdown(struct btrfs_fs_info *fs_info, u32 flags)
+{
+	int ret = 0;
+

fs/btrfs/ioctl.c +15 -0

+		if (ret)
+			return ret;
+		btrfs_force_shutdown(fs_info);
+		ret = thaw_super(fs_info->sb, FREEZE_HOLDER_KERNEL, NULL);
+		if (ret)
+			return ret;
+		break;
+	case BTRFS_SHUTDOWN_FLAGS_NOLOGFLUSH:
+		btrfs_force_shutdown(fs_info);
+		break;
+	}
+	return ret;
+}
+#endif
+
 long btrfs_ioctl(struct file *file, unsigned int
 		cmd, unsigned long arg)
 {

include/uapi/linux/btrfs.h +6 -0

@@ -1096,6 +1096,12 @@ enum btrfs_err_code {
 	BTRFS_ERROR_DEV_RAID1C4_MIN_NOT_MET,
 };
 
+/* Flags for IOC_SHUTDOWN, should match XFS' flags. */
+#define BTRFS_SHUTDOWN_FLAGS_DEFAULT	0x0
+#define BTRFS_SHUTDOWN_FLAGS_LOGFLUSH	0x1
+#define BTRFS_SHUTDOWN_FLAGS_NOLOGFLUSH	0x2
+#define BTRFS_SHUTDOWN_FLAGS_LAST	0x3
+
 #define BTRFS_IOC_SNAP_CREATE _IOW(BTRFS_IOCTL_MAGIC, 1, \
 				   struct btrfs_ioctl_vol_args)
 #define BTRFS_IOC_DEFRAG _IOW(BTRFS_IOCTL_MAGIC, 2, \

Quoted 32 lines · Qu Wenruo

The shutdown ioctl should follow the XFS one, which use magic number 'X', and ioctl number 125, with a u32 as flags.

For now btrfs don't distinguish DEFAULT and LOGFLUSH flags (just like f2fs), both will freeze the fs first (implies committing the current transaction), setting the SHUTDOWN flag and finally thaw the fs.

For NOLOGFLUSH flag, the freeze/thaw part is skipped thus the current transaction is aborted.

The new shutdown ioctl is hidden behind experimental features for more testing.

Signed-off-by Qu Wenruo <[email protected]>

---
  fs/btrfs/ioctl.c           | 40 ++++++++++++++++++++++++++++++++++++++
  include/uapi/linux/btrfs.h |  9 +++++++++
  2 files changed, 49 insertions(+)

fs/btrfs/ioctl.c +5 -0

index 2f3b7be13bea..94eb7a8499db 100644
@@ -5194,6 +5194,36 @@ static int btrfs_ioctl_subvol_sync(struct btrfs_fs_info *fs_info, void __user *a
  	return 0;
  }
 
+#ifdef CONFIG_BTRFS_EXPERIMENTAL
+static int btrfs_emergency_shutdown(struct btrfs_fs_info *fs_info, u32 flags)
+{
+	int ret = 0;
+

fs/btrfs/ioctl.c static int btrfs_ioctl_subvol_sync(struct btrfs_fs_info *fs_info, void __user *a · line 5211

Quoted 10 lines

fs/btrfs/ioctl.c +10 -0

+	if (flags >= BTRFS_SHUTDOWN_FLAGS_LAST)
+		return -EINVAL;
+
+	if (btrfs_is_shutdown(fs_info))
+		return 0;
+
+	switch (flags) {
+	case BTRFS_SHUTDOWN_FLAGS_LOGFLUSH:
+	case BTRFS_SHUTDOWN_FLAGS_DEFAULT:
+		ret = freeze_super(fs_info->sb, FREEZE_HOLDER_KERNEL, NULL);

Recently I've looked at scrub blocking filesystem freezing and it does not work because it blocks on the semaphore taken in mnt_want_write, also taken in freeze_super().

I have an idea for fix, basically pause scrub, undo mnt_want_write and then call freeze_super. So we'll need that too for shutdown. Once implemented the fixup would be to use btrfs_freeze_super callback here.

Quoted 36 lines

fs/btrfs/ioctl.c +15 -0

+		if (ret)
+			return ret;
+		btrfs_force_shutdown(fs_info);
+		ret = thaw_super(fs_info->sb, FREEZE_HOLDER_KERNEL, NULL);
+		if (ret)
+			return ret;
+		break;
+	case BTRFS_SHUTDOWN_FLAGS_NOLOGFLUSH:
+		btrfs_force_shutdown(fs_info);
+		break;
+	}
+	return ret;
+}
+#endif
+
  long btrfs_ioctl(struct file *file, unsigned int
  		cmd, unsigned long arg)
  {

include/uapi/linux/btrfs.h +6 -0

@@ -1096,6 +1096,12 @@ enum btrfs_err_code {
  	BTRFS_ERROR_DEV_RAID1C4_MIN_NOT_MET,
  };
 
+/* Flags for IOC_SHUTDOWN, should match XFS' flags. */
+#define BTRFS_SHUTDOWN_FLAGS_DEFAULT	0x0
+#define BTRFS_SHUTDOWN_FLAGS_LOGFLUSH	0x1
+#define BTRFS_SHUTDOWN_FLAGS_NOLOGFLUSH	0x2
+#define BTRFS_SHUTDOWN_FLAGS_LAST	0x3
+
  #define BTRFS_IOC_SNAP_CREATE _IOW(BTRFS_IOCTL_MAGIC, 1, \
  				   struct btrfs_ioctl_vol_args)
  #define BTRFS_IOC_DEFRAG _IOW(BTRFS_IOCTL_MAGIC, 2, \

include/uapi/linux/btrfs.h enum btrfs_err_code { · line 1227

Quoted 5 lines

include/uapi/linux/btrfs.h +2 -0

@@ -1217,6 +1223,9 @@ enum btrfs_err_code {
  #define BTRFS_IOC_SUBVOL_SYNC_WAIT _IOW(BTRFS_IOCTL_MAGIC, 65, \
  					struct btrfs_ioctl_subvol_wait)
 
+/* Shutdown ioctl should follow XFS's interfaces, thus not using btrfs magic. */
+#define BTRFS_IOC_SHUTDOWN	_IOR('X', 125, __u32)

In XFS it's

#define XFS_IOC_GOINGDOWN _IOR ('X', 125, uint32_t)

It's right to use the same definition and ioctl value as this will be a generic ioctl eventually, with 3 users at least. I like the name SHUTDOWN better, ext4 also uses that.

Quoted 19 lines · David Sterba

Quoted 4 lines

fs/btrfs/ioctl.c +4 -0

+static int btrfs_emergency_shutdown(struct btrfs_fs_info *fs_info, u32 flags)
+{
+	int ret = 0;
+

fs/btrfs/ioctl.c static int btrfs_ioctl_subvol_sync(struct btrfs_fs_info *fs_info, void __user *a · line 5211

Quoted 10 lines

fs/btrfs/ioctl.c +10 -0

+	if (flags >= BTRFS_SHUTDOWN_FLAGS_LAST)
+		return -EINVAL;
+
+	if (btrfs_is_shutdown(fs_info))
+		return 0;
+
+	switch (flags) {
+	case BTRFS_SHUTDOWN_FLAGS_LOGFLUSH:
+	case BTRFS_SHUTDOWN_FLAGS_DEFAULT:
+		ret = freeze_super(fs_info->sb, FREEZE_HOLDER_KERNEL, NULL);

Recently I've looked at scrub blocking filesystem freezing and it does not work because it blocks on the semaphore taken in mnt_want_write, also taken in freeze_super().

Quoted 3 lines

I have an idea for fix, basically pause scrub, undo mnt_want_write and then call freeze_super. So we'll need that too for shutdown. Once implemented the fixup would be to use btrfs_freeze_super callback here.

It may not be that simple.

freeze_super() itself is doing extra works related to the stage/freeze_owner/etc.

I'm not sure if it's a good idea to completely skip that part.

I'd prefer scrub to check the frozen stage, and if it's already in any FREEZE stages, exit early.

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.

Quoted 29 lines · Qu Wenruo

Quoted 19 lines · David Sterba

Quoted 4 lines

fs/btrfs/ioctl.c +4 -0

+static int btrfs_emergency_shutdown(struct btrfs_fs_info *fs_info, u32 flags)
+{
+	int ret = 0;
+

fs/btrfs/ioctl.c static int btrfs_ioctl_subvol_sync(struct btrfs_fs_info *fs_info, void __user *a · line 5211

Quoted 10 lines

fs/btrfs/ioctl.c +10 -0

+	if (flags >= BTRFS_SHUTDOWN_FLAGS_LAST)
+		return -EINVAL;
+
+	if (btrfs_is_shutdown(fs_info))
+		return 0;
+
+	switch (flags) {
+	case BTRFS_SHUTDOWN_FLAGS_LOGFLUSH:
+	case BTRFS_SHUTDOWN_FLAGS_DEFAULT:
+		ret = freeze_super(fs_info->sb, FREEZE_HOLDER_KERNEL, NULL);

Recently I've looked at scrub blocking filesystem freezing and it does not work because it blocks on the semaphore taken in mnt_want_write, also taken in freeze_super().

Quoted 3 lines

I have an idea for fix, basically pause scrub, undo mnt_want_write and then call freeze_super. So we'll need that too for shutdown. Once implemented the fixup would be to use btrfs_freeze_super callback here.

It may not be that simple.

freeze_super() itself is doing extra works related to the stage/freeze_owner/etc.

Quoted 4 lines

I'm not sure if it's a good idea to completely skip that part.

I'd prefer scrub to check the frozen stage, and if it's already in any FREEZE stages, exit early.

I have working prototype for pausing scrub that does not need to exit, so far I've tested it with fsfreeze in a VM, I still need to test actual freezing for suspend purposes.

Quoted 10 lines · 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.

Quoted 6 lines

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....

Quoted 4 lines

- 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.

Quoted 1 line

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.

Quoted 35 lines · Dave Chinner

Quoted 10 lines · 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.

Quoted 6 lines

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.

Quoted 7 lines

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.

Quoted 9 lines

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....

Quoted 4 lines

- 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.

Quoted 5 lines

Quoted 1 line

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

Quoted 6 lines

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.

Quoted 3 lines

I have working prototype for pausing scrub that does not need to exit, so far I've tested it with fsfreeze in a VM, I still need to test actual freezing for suspend purposes.

Not sure how would you test with running scrub and freeze, but please enable lockdep for the potential reversed lock sequence related to btrfs specific locks and s_umount/s_writers.rw_sem.

Quoted 4 lines

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

Quoted 2 lines

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?

Quoted 3 lines

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

Quoted 36 lines

Quoted 9 lines

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....

Quoted 4 lines

- 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.

Quoted 5 lines

Quoted 1 line

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

Quoted 6 lines

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.

在 2025/7/8 10:15, Darrick J. Wong 写道:
[...]

Quoted 4 lines

Quoted 4 lines

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

Quoted 8 lines

Quoted 2 lines

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.

Quoted 7 lines

Quoted 3 lines

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.

Quoted 4 lines

 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

Quoted 49 lines

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

Quoted 36 lines

Quoted 9 lines

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....

Quoted 4 lines

- 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.

Quoted 5 lines

Quoted 1 line

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

Quoted 6 lines

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.

Quoted 9 lines · Qu Wenruo

Quoted 6 lines · Darrick J. Wong

Quoted 2 lines

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()?

Quoted 6 lines

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.

Quoted 1 line

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

Quoted 2 lines

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?

Quoted 1 line

Quoted 1 line

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).

Quoted 28 lines

Quoted 3 lines

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();
}

Quoted 8 lines

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.

Quoted 46 lines · Qu Wenruo

Quoted 35 lines · Dave Chinner

Quoted 10 lines · 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.

Quoted 6 lines

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.

Quoted 7 lines

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().

Quoted 5 lines

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?

Quoted 2 lines

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.

Quoted 47 lines · Qu Wenruo

Quoted 35 lines · Dave Chinner

Quoted 10 lines · 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.

Quoted 6 lines

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.

Quoted 7 lines

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.

Quoted 2 lines

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?

Quoted 2 lines

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.

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.

Quoted 48 lines · Darrick J. Wong

Quoted 47 lines · Qu Wenruo

Quoted 35 lines · Dave Chinner

Quoted 10 lines · 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.

Quoted 6 lines

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.

Quoted 7 lines

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.

Quoted 8 lines

Quoted 2 lines

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.

Quoted 8 lines

Quoted 2 lines

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);
}

Quoted 5 lines

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

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

Quoted 16 lines

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;

Quoted 6 lines

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

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

Quoted 2 lines

		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

Quoted 25 lines

	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);
}

Quoted 5 lines

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

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

Quoted 59 lines · Darrick J. Wong

Quoted 46 lines · Qu Wenruo

Quoted 35 lines · Dave Chinner

Quoted 10 lines · 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.

Quoted 6 lines

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.

Quoted 7 lines

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().

Quoted 5 lines

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?

Quoted 8 lines

Quoted 2 lines

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.

Quoted 69 lines · Christian Brauner

Quoted 59 lines · Darrick J. Wong

Quoted 46 lines · Qu Wenruo

Quoted 35 lines · Dave Chinner

Quoted 10 lines · 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.

Quoted 6 lines

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.

Quoted 7 lines

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().

Quoted 5 lines

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?

Quoted 8 lines

Quoted 2 lines

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.

Quoted 6 lines

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.

Quoted 77 lines · Dave Chinner

Quoted 69 lines · Christian Brauner

Quoted 59 lines · Darrick J. Wong

Quoted 46 lines · Qu Wenruo

Quoted 35 lines · Dave Chinner

Quoted 10 lines · 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.

Quoted 6 lines

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.

Quoted 7 lines

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().

Quoted 5 lines

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?

Quoted 8 lines

Quoted 2 lines

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.

Quoted 6 lines

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.

Quoted 8 lines

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.

Quoted 89 lines · Qu Wenruo

Quoted 77 lines · Dave Chinner

Quoted 69 lines · Christian Brauner

Quoted 59 lines · Darrick J. Wong

Quoted 46 lines · Qu Wenruo

Quoted 35 lines · Dave Chinner

Quoted 10 lines · 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.

Quoted 6 lines

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.

Quoted 7 lines

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().

Quoted 5 lines

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?

Quoted 8 lines

Quoted 2 lines

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.

Quoted 6 lines

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.

Quoted 8 lines

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.

Quoted 7 lines

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...

Quoted 12 lines · 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.

Quoted 69 lines · Christian Brauner

Quoted 59 lines · Darrick J. Wong

Quoted 46 lines · Qu Wenruo

Quoted 35 lines · Dave Chinner

Quoted 10 lines · 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.

Quoted 6 lines

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.

Quoted 7 lines

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().

Quoted 5 lines

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?

Quoted 8 lines

Quoted 2 lines

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.

Quoted 6 lines

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.....

Quoted 3 lines

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....

Quoted 5 lines

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.

Quoted 90 lines · Jan Kara

Quoted 48 lines · Darrick J. Wong

Quoted 47 lines · Qu Wenruo

Quoted 35 lines · Dave Chinner

Quoted 10 lines · 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.

Quoted 6 lines

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.

Quoted 7 lines

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.

Quoted 8 lines

Quoted 2 lines

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.

Quoted 8 lines

Quoted 2 lines

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;

Quoted 4 lines

	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. :)

Quoted 91 lines · Darrick J. Wong

Quoted 90 lines · Jan Kara

Quoted 48 lines · Darrick J. Wong

Quoted 47 lines · Qu Wenruo

Quoted 35 lines · Dave Chinner

Quoted 10 lines · 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.

Quoted 6 lines

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.

Quoted 7 lines

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.

Quoted 8 lines

Quoted 2 lines

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.

Quoted 8 lines

Quoted 2 lines

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;

Quoted 7 lines

Quoted 4 lines

	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.

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?

Quoted 2 lines

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.

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.

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.

Quoted 108 lines · Christian Brauner

Quoted 91 lines · Darrick J. Wong

Quoted 90 lines · Jan Kara

Quoted 48 lines · Darrick J. Wong

Quoted 47 lines · Qu Wenruo

Quoted 35 lines · Dave Chinner

Quoted 10 lines · 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.

Quoted 6 lines

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.

Quoted 7 lines

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.

Quoted 8 lines

Quoted 2 lines

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.

Quoted 8 lines

Quoted 2 lines

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;

Quoted 7 lines

Quoted 4 lines

	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.

Quoted 9 lines

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);

(quoted diff) +9 -0

+	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?