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.
> 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.
> 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,
Josef
