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.
>
> Thanks,
> Qu
>
> >
> > > 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.
>
