Re: [PATCH 1/6] fs: add a new remove_bdev() super operations callback

[Christian Brauner <brauner@kernel.org>]

On Thu, Jun 26, 2025 at 09:23:42AM +0930, Qu Wenruo wrote:
> The new remove_bdev() call back is mostly for multi-device filesystems
> to handle device removal.
> 
> Some multi-devices filesystems like btrfs can have the ability to handle
> device lose according to the setup (e.g. all chunks have extra mirrors),
> thus losing a block device will not interrupt the normal operations.
> 
> Btrfs will soon implement this call back by:
> 
> - Automatically degrade the fs if read-write operations can be
>   maintained
> 
> - Shutdown the fs if read-write operations can not be maintained
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/super.c         |  4 +++-
>  include/linux/fs.h | 18 ++++++++++++++++++
>  2 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/super.c b/fs/super.c
> index 80418ca8e215..07845d2f9ec4 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -1463,7 +1463,9 @@ 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)
> +	if (sb->s_op->remove_bdev)
> +		sb->s_op->remove_bdev(sb, bdev, surprise);
> +	else if (sb->s_op->shutdown)
>  		sb->s_op->shutdown(sb);

This makes ->remove_bdev() and ->shutdown() mutually exclusive. I really
really dislike this pattern. It introduces the possibility that a
filesystem accidently implement both variants and assumes both are
somehow called. That can be solved by an assert at superblock initation
time but it's still nasty.

The other thing is that this just reeks of being the wrong api. We
should absolutely aim for the methods to not be mutually exclusive. I
hate that with a passion. That's just an ugly api and I want to have as
little of that as possible in our code.

>  
>  	super_unlock_shared(sb);
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index b085f161ed22..5e84e06c7354 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2367,7 +2367,25 @@ struct super_operations {
>  				  struct shrink_control *);
>  	long (*free_cached_objects)(struct super_block *,
>  				    struct shrink_control *);
> +	/*
> +	 * Callback to shutdown the fs.
> +	 *
> +	 * If a fs can not afford losing any block device, implement this callback.
> +	 */
>  	void (*shutdown)(struct super_block *sb);
> +
> +	/*
> +	 * Callback to handle a block device removal.
> +	 *
> +	 * Recommended to implement this for multi-device filesystems, as they
> +	 * may afford losing a block device and continue operations.
> +	 *
> +	 * @surprse:	indicates a surprise removal. If true the device/media is
> +	 *		already gone. Otherwise we're prepareing for an orderly
> +	 *		removal.
> +	 */
> +	void (*remove_bdev)(struct super_block *sb, struct block_device *bdev,
> +			    bool surprise);
>  };

Yeah, I think that's just not a good api. That looks a lot to me like we
should just collapse both functions even though earlier discussion said
we shouldn't. Just do:

s/shutdown/remove_bdev/

or

s/shutdown/shutdown_bdev()

The filesystem will know whether it has to kill the filesystem or if it
can keep going even if the device is lost. Hell, if we have to we could
just have it return whether it killed the superblock or just the device
by giving the method a return value. But for now it probably doesn't
matter.