在 2025/7/8 11:39, Qu Wenruo 写道:
在 2025/7/8 10:15, Darrick J. Wong 写道:
[...]
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?
That's right, some part of fs_bdev_mark_dead() is not making much sense
if the fs can handle the dev loss.
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.
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?
Thanks,
Qu
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
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.
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
