| Message ID | 20201207153237.1073887-4-nborisov@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Overhaul free objectid code | expand |
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.
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 this assures highest_objectid is also within range. But If you want to leave it - I'm fine with it.
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.
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c index 4c3dda0034b5..eaaece2bf0c8 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -1373,8 +1373,6 @@ static int btrfs_init_fs_root(struct btrfs_root *root, dev_t anon_dev) goto fail; } - ASSERT(root->highest_objectid <= BTRFS_LAST_FREE_OBJECTID); - mutex_unlock(&root->objectid_mutex); return 0; @@ -2651,7 +2649,6 @@ static int __cold init_tree_roots(struct btrfs_fs_info *fs_info) continue; } - ASSERT(tree_root->highest_objectid <= BTRFS_LAST_FREE_OBJECTID); ret = btrfs_read_roots(fs_info); if (ret < 0) {
The invariants the asserts are checking are already verified by the tree checker, just remove them. Signed-off-by: Nikolay Borisov <nborisov@suse.com> --- fs/btrfs/disk-io.c | 3 --- 1 file changed, 3 deletions(-)