Re: [PATCH v2] btrfs: tree-checker: add qgroup status item check

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

在 2026/9/5 02:44, David Sterba 写道:
> On Wed, Aug 26, 2026 at 07:23:15PM +0930, Qu Wenruo wrote:
>> This adds the following checks:
>>
>> - Key check
>>
>> - Item size check
>>    Here we do not require completely matching the older or newer qgroup
>>    status item size, this is to allow future structure expansion.
>>
>> - Version check
>>    Again it's not a strict requirement for the version to match the
>>    support one, but to catch obvious bitflip.
>>
>>    And the kernel will already reject unknown version by disabling qgroup.
>>
>>    So we allow any version that is no larger than U8_MAX, which I believe
>>    we won't reach in the next few decades.
>>
>> - Flags check
>>    Mostly to catch any unexpected RUNTIME flags.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>> Changelog:
>> v2:
>> - Loosen the version check
>> - Loosen the item size check
>>    To allow older kernels to mount future newer qgroup expansions,
>>    other than completely rejecting the qgroup tree and failing the mount.
> 
> I don't see this patch in for-next but it seems OK for merge.

Mostly because Sashiko still has some complain about the over-strict 
checks on the flags.
AKA, any newly introduced flag will be rejected by this check, causing 
mount failure.

> 
>> +	/*
>> +	 * The version (1) hasn't changed for a long time, but even if we
>> +	 * are going to support a newer version, it won't suddenly jump
>> +	 * over 255 in the foreseeable future.
>> +	 */
>> +	if (unlikely(btrfs_qgroup_status_version(leaf, qsi) > U8_MAX)) {
> 
> Why is it U8_MAX instead of 255 as said in the comment?

U8_MAX is exactly 255. Although for the consistency I'll change the 
comment to use the same U8_MAX.

> I understand
> that it is for one byte, although the structure has __le64 for it.
> 
> I also don't expect the version to reach any high number, the version
> for such item is an outlier in the data structure format, we usually
> guess the version from the raw item size. Keeping the version inside
> without any reserved/unused filelds also make it pointless. The only
> meaningful way I see is to split the u64 to 1+3 bytes with 1st byte to
> be the version.
> 
> Actually, looking to the history, commit bd7c1ea3a302ab ("btrfs: qgroup:
> check generation when recording simple quota delta") already increased
> the structure size, so we don't even make use of the version.
> 
> As it is now it's basically wasting the 4 bytes and we won't increase it
> anyway so can think about how to scrap it completely and make the test
> stricter.

I think the version split (1 bytes for version, 3 bytes for future 
expansion) is the best solution.

Then I'm totally fine to drop this check.

> 
>> +		generic_err(leaf, slot,
>> +			    "suspicious qgroup status version, has %llu expect %u",
>> +			    btrfs_qgroup_status_version(leaf, qsi),
>> +			    BTRFS_QGROUP_STATUS_VERSION);
>> +		return -EUCLEAN;
>> +	}
>> +	flags = btrfs_qgroup_status_flags(leaf, qsi);
>> +	if (unlikely(flags & ~BTRFS_QGROUP_STATUS_FLAGS_MASK)) {
>> +		generic_err(leaf, slot,
>> +			    "unknown qgroup status flags, has 0x%llx unknown flags 0x%llx",
>> +			    flags, flags & ~BTRFS_QGROUP_STATUS_FLAGS_MASK);
>> +		return -EUCLEAN;
>> +	}
>> +	if (unlikely(flags & BTRFS_QGROUP_STATUS_FLAG_SIMPLE_MODE &&
>> +		     item_size < sizeof(*qsi))) {
>> +		generic_err(leaf, slot,
>> +			    "invalid qgroup status item size, has %u expect at least %zu",
>> +			    item_size, sizeof(*qsi));
>> +		return -EUCLEAN;
>> +	}
>> +	return 0;
>> +}
>