linux-ext4 - Re: [PATCH v4 1/6] fs: enhance and rename shutdown() callback to remove

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <debab09d-5eb8-4742-96f4-b2c39233f9b7@gmx.com>
Date: Wed, 9 Jul 2025 07:42:11 +0930
From: Qu Wenruo <quwenruo.btrfs@....com>
To: "Darrick J. Wong" <djwong@...nel.org>, Jan Kara <jack@...e.cz>
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, 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/9 05:50, Darrick J. Wong 写道:
[...]
>> Well, I'd also say just go for own fs_holder_ops if it was not for the
>> awkward "get super from bdev" step. As Christian wrote we've encapsulated
>> that in fs/super.c and bdev_super_lock() in particular but the calling
>> conventions for the fs_holder_ops are not very nice (holding
>> bdev_holder_lock, need to release it before grabbing practically anything
>> else) so I'd have much greater peace of mind if this didn't spread too
>> much. Once you call bdev_super_lock() and hold on to sb with s_umount held,
>> things are much more conventional for the fs land so I'd like if this
>> step happened before any fs hook got called. So I prefer something like
>> Qu's proposal of separate sb op for device removal over exporting
>> bdev_super_lock(). Like:
>>
>> static void fs_bdev_mark_dead(struct block_device *bdev, bool surprise)
>> {
>>          struct super_block *sb;
>>
>>          sb = bdev_super_lock(bdev, false);
>>          if (!sb)
>>                  return;
>>
>> 	if (sb->s_op->remove_bdev) {
>> 		sb->s_op->remove_bdev(sb, bdev, surprise);

The only concern here is, remove_bdev() will handle two different things:

- Mark the target devices as missing and continue working
   Of course that's when the fs can handle it.

- Shutdown
   If the fs can not handle it.

And if the fs has to shutdown, we're missing all the shrinks in the 
shutdown path.

Thus I'd prefer the remove_bdev() to have a return value, so that we can 
fallback to shutdown path if the remove_bdev() failed.

This also aligns more towards Brauner's idea that we may want to expose 
if the fs can handle the removal.

Thanks,
Qu

>> 		return;
>> 	}
> 
> It feels odd but I could live with this, particularly since that's the
> direction that brauner is laying down. :)
> 
> Do we still need to super_unlock_shared here?
> 
> --D
> 
>>
>> 	if (!surprise)
>> 		sync_filesystem(sb);
>> 	shrink_dcache_sb(sb);
>> 	evict_inodes(sb);
>> 	if (sb->s_op->shutdown)
>> 		sb->s_op->shutdown(sb);
>>
>> 	super_unlock_shared(sb);
>> }
>>
>>> 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.
>>
>> Yes, I should find time to revive that patch series... It was not *that*
>> hard to do.
>>
>> 								Honza
>> -- 
>> Jan Kara <jack@...e.com>
>> SUSE Labs, CR
>>