在 2025/6/26 19:44, Christoph Hellwig 写道:
On Thu, Jun 26, 2025 at 10:38:11AM +0200, Christian Brauner wrote:
+ 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.
Yes. As I mentioned before I think we just want to transition everyone
to the remove_bdev API. But our life is probably easier if Qu's series
only adds the new one so that we can do the transitions through the
file system trees instead of needing a global API change. Or am I
overthinking this?
Then let's do the transition right now.
The transition itself is pretty straightforward, just renaming and
appending the parameter list, and keeping the old shutdown behavior.
AFAIK only btrfs and bcachefs will take advantage of the new interface
for now.
+ * @surprse: indicates a surprise removal. If true the device/media is
surprse is misspelled here. But I don't see how passing this on to
the file system would be useful, because at this point everything is
a surprise for the file system.
If I understand the @surprise parameter correctly, it should allow the
fs to do read/write as usual if it's not a surprise removal.
And btrfs will take the chance to fully writeback all the dirty pages
(more than the default shutdown behavior which only writebacks the
current transaction, no dirty data pages.).
But in the real world, for test case like generic/730, the @surprise
flag is either not properly respected, I'm getting @surprise == false
but the block device is already gone.
So I'm not sure what's the real expected behavior here, and the new flag
is only for future expansion for now.
Thanks,
Qu
