Message-ID: <02584a40-a2c0-4565-ab46-50c1a4100b21@gmx.com>
Date: Tue, 8 Jul 2025 11:39:42 +0930
From: Qu Wenruo <quwenruo.btrfs@....com>
To: "Darrick J. Wong" <djwong@...nel.org>
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()



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

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

Powered by blists - more mailing lists