On Wed, Sep 11, 2019 at 03:46:24PM +0800, Qu Wenruo wrote:
> Although we have btrfs_verify_level_key() function to check the first
> key and level at tree block read time, it has its limitation due to tree
> lock context, it's not reliable handling new tree blocks.
>
How is it not reliable with new tree blocks?
> So btrfs_verify_level_key() is good as a pre-check, but it can't ensure
> new tree blocks are still sane at runtime.
>
I mean I guess this is good, but we have to keep the parent locked when we're
adding new blocks anyway, so I'm not entirely sure what this gains us? You are
essentially duplicating the checks that we already do on reads, and then adding
the first_key check.
I'll go along with the first_key check being relatively useful, but why exactly
do we need all this infrastructure when we can just check it as we walk down the
tree?
<snip>
> @@ -2887,24 +2982,28 @@ int btrfs_search_slot(struct btrfs_trans_handle
> *trans, struct btrfs_root *root,
> }
>
> if (!p->skip_locking) {
> - level = btrfs_header_level(b);
> - if (level <= write_lock_level) {
> + if (level - 1 <= write_lock_level) {
> err = btrfs_try_tree_write_lock(b);
> if (!err) {
> btrfs_set_path_blocking(p);
> btrfs_tree_lock(b);
> }
> - p->locks[level] = BTRFS_WRITE_LOCK;
> + p->locks[level - 1] = BTRFS_WRITE_LOCK;
> } else {
> err = btrfs_tree_read_lock_atomic(b);
> if (!err) {
> btrfs_set_path_blocking(p);
> btrfs_tree_read_lock(b);
> }
> - p->locks[level] = BTRFS_READ_LOCK;
> + p->locks[level - 1] = BTRFS_READ_LOCK;
> }
> - p->nodes[level] = b;
> + p->nodes[level - 1] = b;
> }
This makes no sense to me. Why do we need to change how we do level here just
for the btrfs_verify_child() check? We've already init'ed verify further up,
and we're not changing b here, so messing with level here just makes the code
less clear. Thanks,
Josef
