On Thu, Jun 26, 2025 at 06:48:29PM +0930, Qu Wenruo wrote:
>
>
> 在 2025/6/26 18:40, Christian Brauner 写道:
> > On Thu, Jun 26, 2025 at 06:14:03PM +0930, Qu Wenruo wrote:
> > >
> > >
> > > 在 2025/6/26 18:08, Christian Brauner 写道:
> > > > 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@xxxxxxxx>
> > > > > ---
> > > > > 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.
> > >
> > > So what do you really want?
> > >
> > > The original path to expand the shutdown() callback is rejected, and now the
> > > new callback is also rejected.
> > >
> > > I guess the only thing left is to rename shutdown() to remove_bdev(), add
> > > the new parameters and keep existing fses doing what they do (aka,
> > > shutdown)?
> >
> > Yes. My original understanding had been that ->remove_bdev() would be
> > called in a different codepath than ->shutdown() and they would be
> > complimentary. So sorry for the back and forth here. If that's not the
> > case I don't see any point in having two distinct methods.
>
> That's fine, just want to do a final confirmation that everyone is fine with
> such change:
>
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index b085f161ed22..c11b9219863b 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2367,7 +2367,8 @@ struct super_operations {
> struct shrink_control *);
> long (*free_cached_objects)(struct super_block *,
> struct shrink_control *);
> - void (*shutdown)(struct super_block *sb);
> + void (*remove_bdev)(struct super_block *sb, struct block_device
> *bdev,
> + bool surprise);
> };
This looks good to me!
