Re: [PATCH 3/6] btrfs: Remove useless ASSERTS

[David Sterba <dsterba@suse.cz>]

On Tue, Dec 15, 2020 at 07:48:18PM +0200, Nikolay Borisov wrote:
> 
> 
> On 15.12.20 г. 18:58 ч., David Sterba wrote:
> > On Mon, Dec 07, 2020 at 05:32:34PM +0200, Nikolay Borisov wrote:
> >> The invariants the asserts are checking are already verified by the
> >> tree checker, just remove them.
> > 
> > I haven't found where exactly does tree-checker verify the invariant and
> > also think that we can safely leave the asserts there. Even if it's for
> > a normally impossible case, assertions usually catch bugs after changing
> > some other code.
> > 
> 
>    2         if (unlikely((key->objectid < BTRFS_                                           
>      1                       key->objectid > BTRFS_ #define BTRFS_ROOT_TREE_DIR_OBJECTID 6ULL 
>   402                       key->objectid != BTRFS_ROOT_TREE_DIR_OBJECTID &&           
>      1                      key->objectid != BTRFS_FREE_INO_OBJECTID)) { 
> 
> 
> in check_inode_key. We verify that for every inode its objectid is
> within range, transitively

Ah so it's only indirectly implied.

> this assures highest_objectid is also
> within range. But If you want to leave it - I'm fine with it. 

Tree checker verifies that any inode that is read has the object id
within the bounds, that's fine. The highest free objectid is obtained
by doing reverse search, without reading (and checking) any existing
inode.

btrfs_init_root_free_objectid checks only object ids in the tree, not
necessarily inodes (though technically we use the objectids only for
inode-like items).

Things can be improved by doing proper checks inside
btrfs_init_root_free_objectid and drop the asserts, I can imagine a
crafted image that would trigger the asserts so we'd better handle that.