Re: [PATCH RFC] btrfs: Introduce btrfs child tree block verification system

[WenRuo Qu <wqu@suse.com>]

On 2019/9/12 下午5:59, Josef Bacik wrote:
> On Thu, Sep 12, 2019 at 07:38:14AM +0800, Qu Wenruo wrote:
>>
>>
>> On 2019/9/12 上午12:02, Josef Bacik wrote:
>>> 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?
>>
>> Current btrfs_verify_level_key() skips first_key verification for any
>> tree blocks newer than last committed.
>>
>>>
>>>> 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?
>>
>> For cases like tree search on current node, where all tree blocks can be
>> newly CoWed tree blocks.
>>
> 
> But again we have the parent locked in these cases, so we can still do the check
> even if the parent has been cow'ed, so I'm not clear what the point is?  Like
> for sure add an extra check during search to check the first_key I guess, but
> all the extra checks seem superflous.

But child isn't locked if it's from the eb cache other than read from disk.
We can get a stable node_ptr_key from parent without any problem.

But to verify first_key, we need to lock the child to verify its first
key against the first key we got from parent.

So in short, the whole facility and all those calls are just to verify
first_key and nritems *reliably*.

> 
>> If bit flip happens affecting those new tree blocks, we can detect them
>> at runtime, and that's the only time we can catch such error.
>>
> 
> Sure but we can't really detect bitflips in lots of places.  I'm not sure that
> justifies this extra infrastructure.

The facility is only to check first_key and nritems for *all* ebs, not
only ebs read from disk.

You can definitely remove the level/transid check, as long as we choose
to keep the btrfs_verify_level_key().
But I'm afraid I tend to remove btrfs_verify_level_key() and merge all
its check in this new facility, as we can check it more accurately.

> 
>> Write time tree checker doesn't go beyond single leave/node, thus has no
>> way to detect such parent-child mismatch case.
>>
> 
> Yeah that I'll give you.  But again as long as we check while we're searching
> we'll be fine.  The only case we'll miss is if there's a bitflip in between the
> time we modified the thing and we write it out.  Your code doesn't catch this
> case either, cause frankly it's kind of impossible without actually walking and
> verifying at writeout time.
> 
>>>  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?
>>
>> You can't really do the nritems and first key check at the current
>> timing of btrfs_verify_level_key() for new tree blocks due to lock context.
>>
>> That's the only reason the new infrastructure is here, to block the only
>> hole of btrfs_verify_level_key().
>>
>>>
>>> <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?
>>
>> Because we can't trust the level from @b unless we have verified it.
>>
>> (Although level is always checked in btrfs_verify_level_key(), but that
>> function is not 100% sure to be kept as is).
> 
> But we have the level at the time we read the block, which we verify, so it'll
> already be correct here right?  So we're just adding the first_key check here.
> Thanks,

Right as long as we're not removing btrfs_verify_level_key().

Thanks,
Qu

> 
> Josef
>