Message-ID: <20250708004532.GA2672018@frogsfrogsfrogs>
Date: Mon, 7 Jul 2025 17:45:32 -0700
From: "Darrick J. Wong" <djwong@...nel.org>
To: Qu Wenruo <quwenruo.btrfs@....com>
Cc: Dave Chinner <david@...morbit.com>, Qu Wenruo <wqu@...e.com>,
	linux-btrfs@...r.kernel.org, linux-fsdevel@...r.kernel.org,
	viro@...iv.linux.org.uk, brauner@...nel.org, jack@...e.cz,
	linux-ext4@...r.kernel.org, linux-f2fs-devel@...ts.sourceforge.net,
	ntfs3@...ts.linux.dev, linux-xfs@...r.kernel.org
Subject: Re: [PATCH v4 1/6] fs: enhance and rename shutdown() callback to
 remove_bdev()

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

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

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

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

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

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

--D

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

Powered by blists - more mailing lists