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.
>
> 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.
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);
>>
