Re: [PATCH v4 1/6] fs: enhance and rename shutdown() callback to remove_bdev()

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

在 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.
>>>
>>
> 
>