On 10.07.19 г. 15:02 ч., Qu Wenruo wrote:
>
>
> On 2019/7/10 下午7:19, Nikolay Borisov wrote:
>>
>>
> [...]
>>> +static int check_cross_tree_key_order(struct extent_buffer *left,
>>> + struct extent_buffer *right)
>>> +{
>>> + 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 0;
>>> + if (level) {
>>> + btrfs_node_key_to_cpu(left, &left_last, nr_left - 1);
>>> + btrfs_node_key_to_cpu(right, &right_first, 0);
>>> + } else {
>>> + btrfs_item_key_to_cpu(left, &left_last, nr_left - 1);
>>> + btrfs_item_key_to_cpu(right, &right_first, 0);
>>> + }
>>> + if (btrfs_comp_cpu_keys(&left_last, &right_first) >= 0) {
>>> + btrfs_crit(left->fs_info,
>>> +"bad key order cross tree blocks, left last (%llu %u %llu) right first
>>> (%llu %u %llu",
>>> + left_last.objectid, left_last.type,
>>> + left_last.offset, right_first.objectid,
>>> + right_first.type, right_first.offset);
>>> + return -EUCLEAN;
>>> + }
>>> + return 0;
>>> +}
>>> +
>>
>> nit: I wonder if it will make it a bit easier to reason about the code
>> if that function is renamed to valid_cross_block_key_order and make it
>> return true or false, then it's callers will do if
>> (!valid_cross_block_key_ordered) {
>> return -EUCLEAN
>> }>
> I'm always uncertain what's the correct schema for check function.
>
> Sometimes we have bool check_whatever() sometimes we have bool
> is_whatever(), and sometimes we have int check_whatever().
>
> If we have a good guidance for btrfs, it will be a no-brain choice.
There is no such guidance in this case my logic is that this function
really returns a boolean value - 0 or -EUCLEAN to make that explicit I'd
define it as returning bool and rename it to valid_cross_block_key_order
to really emphasize the fact it's a predicated-type of function. Thus if
someone reads the they will be certain that this function return either
true or false depending on whether the input parameters make sense.
Whereas right now I will have to go and look at the implementation to
determine what are the return values because of the "int" return type.
Again, that's just me and if you deem it doesn't bring value then feel
free to ignore it.
>
>>
>> I guess it won't be much different than it is now.
>>
>>> /*
>>> * try to push data from one node into the next node left in the
>>> * tree.
>>> @@ -3263,6 +3309,10 @@ static int push_node_left(struct btrfs_trans_handle
>>> *trans,
>>> } else
>>> push_items = min(src_nritems - 8, push_items);
>>>
>>> + /* dst is the left eb src is the middle eb */
>> nit: missing ',' between 'eb' and 'src'. But this is very minor.
>
> I'd prefer the rename the parameter of push_node_left() directly in
> another patch so that we won't need this comment at all.
>
> @dst @src?! That makes no sense compared to @left and @right.
I somewhat agree, however you will also need to rename the respective nr
items variables -> src_nritems/dst_nritems.
>
> Thanks,
> Qu
>
>>
>>
>>> + ret = check_cross_tree_key_order(dst, src);
>>> + if (ret < 0)
>>> + return ret;
>>> ret = tree_mod_log_eb_copy(dst, src, dst_nritems, 0, push_items);
>>> if (ret) {
>>> btrfs_abort_transaction(trans, ret);
>>> @@ -3331,6 +3381,10 @@ static int balance_node_right(struct
>>> btrfs_trans_handle *trans,
>>> if (max_push < push_items)
>>> push_items = max_push;
>>>
>>> + /* dst is the right eb, src is the middle eb */
>>> + ret = check_cross_tree_key_order(src, dst);
>>> + if (ret < 0)
>>> + return ret;
>>> ret = tree_mod_log_insert_move(dst, push_items, 0, dst_nritems);
>>> BUG_ON(ret < 0);
>>> memmove_extent_buffer(dst, btrfs_node_key_ptr_offset(push_items),
>>> @@ -3810,6 +3864,12 @@ static int push_leaf_right(struct btrfs_trans_handle
>>> *trans, struct btrfs_root
>>> if (left_nritems == 0)
>>> goto out_unlock;
>>>
>>> + ret = check_cross_tree_key_order(left, right);
>>> + if (ret < 0) {
>>> + btrfs_tree_unlock(right);
>>> + free_extent_buffer(right);
>>> + return ret;
>>> + }
>>> if (path->slots[0] == left_nritems && !empty) {
>>> /* Key greater than all keys in the leaf, right neighbor has
>>> * enough room for it and we're not emptying our leaf to delete
>>> @@ -4048,6 +4108,9 @@ static int push_leaf_left(struct btrfs_trans_handle
>>> *trans, struct btrfs_root
>>> goto out;
>>> }
>>>
>>> + ret = check_cross_tree_key_order(left, right);
>>> + if (ret < 0)
>>> + goto out;
>>> return __push_leaf_left(path, min_data_size,
>>> empty, left, free_space, right_nritems,
>>> max_slot);
>>>
>
