Re: [PATCH 3/4] btrfs: introduce new "rescue=ignoremetacsums" mount option

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

在 2024/6/17 03:47, David Sterba 写道:
> On Fri, Jun 14, 2024 at 06:58:20AM +0930, Qu Wenruo wrote:
>>
>>
>> 在 2024/6/13 05:08, David Sterba 写道:
>>> On Tue, Jun 11, 2024 at 02:51:37PM +0930, Qu Wenruo wrote:
>>>> This patch introduces "rescue=ignoremetacsums" to ignore metadata csums,
>>>> meanwhile all the other metadata sanity checks are still kept as is.
>>>>
>>>> This new mount option is mostly to allow the kernel to mount an
>>>> interrupted checksum conversion (at the metadata csum overwrite stage).
>>>>
>>>> And since the main part of metadata sanity checks is inside
>>>> tree-checker, we shouldn't lose much safety, and the new mount option is
>>>> rescue mount option it requires full read-only mount.
>>>>
>>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>>
>>>> --- a/fs/btrfs/disk-io.c
>>>> +++ b/fs/btrfs/disk-io.c
>>>> @@ -367,6 +367,7 @@ int btrfs_validate_extent_buffer(struct extent_buffer *eb,
>>>>    	u8 result[BTRFS_CSUM_SIZE];
>>>>    	const u8 *header_csum;
>>>>    	int ret = 0;
>>>> +	bool ignore_csum = btrfs_test_opt(fs_info, IGNOREMETACSUMS);
>>>
>>> const
>>>
>>>> --- a/fs/btrfs/messages.c
>>>> +++ b/fs/btrfs/messages.c
>>>> @@ -20,7 +20,7 @@ static const char fs_state_chars[] = {
>>>>    	[BTRFS_FS_STATE_TRANS_ABORTED]		= 'A',
>>>>    	[BTRFS_FS_STATE_DEV_REPLACING]		= 'R',
>>>>    	[BTRFS_FS_STATE_DUMMY_FS_INFO]		= 0,
>>>> -	[BTRFS_FS_STATE_NO_CSUMS]		= 'C',
>>>> +	[BTRFS_FS_STATE_NO_DATA_CSUMS]		= 'C',
>>>
>>> There should be the status also when the metadata checksums are not
>>> validated, the letters are arbitrary but should reflect the state if
>>> possible, I'd suggest to use 'S' here.
>>
>> I'd prefer to change the NO_DATA_CSUMS one to use 'D' or 'd' (for data),
>> meanwhile for metadata we go 'M' or 'm'.
>
> Changing would break backward compatibility, now it's part of user
> visible interface. It's not an ABI or API but at least we should do such
> changes without considering the consequences.
>
>> But on the other hand, I do not think data/meta csum ignoring really
>> deserves a dedicated state char.
>>
>> It's not really that special compared to trans aborted or dummy fs.
>> (The same for dev-replacing)
>
> The idea of the descriptors is to make it visible that the filesystem is
> in some unusual state, skipping checksum verification can make a
> difference when reading blocks that would normally not pass the check.

On the other hand, I have already changed the metadata csum mismatch output.
It should be enough to know if we're in a skipping-metadata-csum state.

Furthermore, all of these are in rescue mode with forced RO, which
should already be a thing to mention during report/debugging.

That's why I do not think skipping data/metadata csums really needs such
dedicated handling.

Thanks,
Qu