Re: [PATCH v4 4/4] btrfs: ctree: Checking key orders before merged tree blocks

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

On 2020/8/13 下午10:21, David Sterba wrote:
> On Wed, Aug 12, 2020 at 02:05:09PM +0800, Qu Wenruo wrote:
[...]
>> ---
>>  fs/btrfs/ctree.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 68 insertions(+)
>>
>> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
>> index 70e49d8d4f6c..497abb397ea1 100644
>> --- a/fs/btrfs/ctree.c
>> +++ b/fs/btrfs/ctree.c
>> @@ -3159,6 +3159,52 @@ void btrfs_set_item_key_safe(struct btrfs_fs_info *fs_info,
>>  		fixup_low_keys(path, &disk_key, 1);
>>  }
>>
>> +/*
>> + * Check the cross tree block key ordering.
>> + *
>> + * Tree-checker only works inside one tree block, thus the following
>> + * corruption can not be rejected by tree-checker:
>> + * Leaf @left			| Leaf @right
>> + * --------------------------------------------------------------
>> + * | 1 | 2 | 3 | 4 | 5 | f6 |   | 7 | 8 |
>> + *
>> + * Key f6 in leaf @left itself is valid, but not valid when the next
>> + * key in leaf @right is 7.
>> + * This can only be checked at tree block merge time.
>> + * And since tree checker has ensured all key order in each tree block
>> + * is correct, we only need to bother the last key of @left and the first
>> + * key of @right.
>> + */
>> +static bool valid_cross_tree_key_order(struct extent_buffer *left,
>> +				       struct extent_buffer *right)
>
> I think this naming is confusing, my first thought was that keys from
> two trees were being checked, but this is for two leaves in the same
> tree.
>
> The arguments should be constified.
>
> Elsewhere we use a check_<something> naming scheme with return value
> true - problem, and 0/false - all ok. The 'valid' is the reverse and
> also not following the scheme.

Any good candidate?

My current top list candidate is, check_sibling_keys().

Thanks,
Qu

>
>> +{
>> +	struct btrfs_key left_last;
>> +	struct btrfs_key right_first;
>> +	int level = btrfs_header_level(left);
>> +	int nr_left = btrfs_header_nritems(left);
>> +	int nr_right = btrfs_header_nritems(right);
>> +
>> +	/* No key to check in one of the tree blocks */
>> +	if (!nr_left || !nr_right)
>> +		return true;
>> +	if (level) {
>
> You don't need a temporary variable for one-time use, btrfs_header_level
> is understandable here.
>