Re: [PATCH 1/3] btrfs-progs: check/lowmem: Add check and repair for invalid inode generation

Nikolay Borisov
2019-09-30T11:36:41Z

On 24.09.19 г. 11:11 ч., Qu Wenruo wrote:
> There are at least two bug reports of kernel tree-checker complaining
> about invalid inode generation.
>
> All offending inodes seem to be caused by old kernel around 2014, with
> inode generation overflow.
>
> So add such check and repair ability to lowmem mode check first.
>
> This involves:
> - Calculate the inode generation upper limit
> If it's an inode from log tree, then the upper limit is
> super_generation + 1, otherwise it's super_generation.
>
> - Check if the inode generation is larger than the upper limit
>
> - Repair by resetting inode generation to current transaction
> generation
>
> Reported-by: Charles Wright <charles.v.wright@gmail.com>
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Tested-by: Nikolay Borisov <nborisov@suse.com>

There is one small nit with the assert once rectified you can add:

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

> ---
> check/mode-lowmem.c | 76 +++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 76 insertions(+)
>
> diff --git a/check/mode-lowmem.c b/check/mode-lowmem.c
> index 5f7f101d..7af29ba9 100644
> --- a/check/mode-lowmem.c
> +++ b/check/mode-lowmem.c
> @@ -2472,6 +2472,59 @@ static bool has_orphan_item(struct btrfs_root *root, u64 ino)
> return false;
> }
>
> +static int repair_inode_gen_lowmem(struct btrfs_root *root,
> + struct btrfs_path *path)
> +{
> + struct btrfs_trans_handle *trans;
> + struct btrfs_inode_item *ii;
> + struct btrfs_key key;
> + u64 transid;
> + int ret;
> +
> + trans = btrfs_start_transaction(root, 1);
> + if (IS_ERR(trans)) {
> + ret = PTR_ERR(trans);
> + errno = -ret;
> + error("failed to start transaction for inode gen repair: %m");
> + return ret;
> + }
> + transid = trans->transid;

> + btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
> + ASSERT(key.type == BTRFS_INODE_ITEM_KEY);

nit: This function's sole caller, check_inode_item, is guaranteed to be
called with a path pointing to BTRFS_INODE_ITEM_KEY thanks to the logic
in the 'for' loop in process_one_leaf. This renders the assert
redundant. At the very least I think it should be moved to
check_inode_item.

> +
> + btrfs_release_path(path);
> +
> + ret = btrfs_search_slot(trans, root, &key, path, 0, 1);
> + if (ret > 0) {
> + ret = -ENOENT;
> + error("no inode item found for ino %llu", key.objectid);
> + goto error;
> + }
> + if (ret < 0) {
> + errno = -ret;
> + error("failed to find inode item for ino %llu: %m",
> + key.objectid);
> + goto error;
> + }
> + ii = btrfs_item_ptr(path->nodes[0], path->slots[0],
> + struct btrfs_inode_item);
> + btrfs_set_inode_generation(path->nodes[0], ii, trans->transid);
> + btrfs_mark_buffer_dirty(path->nodes[0]);
> + ret = btrfs_commit_transaction(trans, root);
> + if (ret < 0) {
> + errno = -ret;
> + error("failed to commit transaction: %m");
> + goto error;
> + }
> + printf("reseting inode generation to %llu for ino %llu\n",
> + transid, key.objectid);
> + return ret;
> +
> +error:
> + btrfs_abort_transaction(trans, ret);
> + return ret;
> +}
> +
> /*
> * Check INODE_ITEM and related ITEMs (the same inode number)
> * 1. check link count
> @@ -2487,6 +2540,7 @@ static int check_inode_item(struct btrfs_root *root, struct btrfs_path *path)
> struct btrfs_inode_item *ii;
> struct btrfs_key key;
> struct btrfs_key last_key;
> + struct btrfs_super_block *super = root->fs_info->super_copy;
> u64 inode_id;
> u32 mode;
> u64 flags;
> @@ -2497,6 +2551,8 @@ static int check_inode_item(struct btrfs_root *root, struct btrfs_path *path)
> u64 refs = 0;
> u64 extent_end = 0;
> u64 extent_size = 0;
> + u64 generation;
> + u64 gen_uplimit;
> unsigned int dir;
> unsigned int nodatasum;
> bool is_orphan = false;
> @@ -2527,6 +2583,7 @@ static int check_inode_item(struct btrfs_root *root, struct btrfs_path *path)
> flags = btrfs_inode_flags(node, ii);
> dir = imode_to_type(mode) == BTRFS_FT_DIR;
> nlink = btrfs_inode_nlink(node, ii);
> + generation = btrfs_inode_generation(node, ii);
> nodatasum = btrfs_inode_flags(node, ii) & BTRFS_INODE_NODATASUM;
>
> if (!is_valid_imode(mode)) {
> @@ -2540,6 +2597,25 @@ static int check_inode_item(struct btrfs_root *root, struct btrfs_path *path)
> }
> }
>
> + if (btrfs_super_log_root(super) != 0 &&
> + root->objectid == BTRFS_TREE_LOG_OBJECTID)
> + gen_uplimit = btrfs_super_generation(super) + 1;
> + else
> + gen_uplimit = btrfs_super_generation(super);
> +
> + if (generation > gen_uplimit) {
> + error(
> + "invalid inode generation for ino %llu, have %llu expect [0, %llu)",
> + inode_id, generation, gen_uplimit);
> + if (repair) {
> + ret = repair_inode_gen_lowmem(root, path);
> + if (ret < 0)
> + err |= INVALID_GENERATION;
> + } else {
> + err |= INVALID_GENERATION;
> + }
> +
> + }
> if (S_ISLNK(mode) &&
> flags & (BTRFS_INODE_IMMUTABLE | BTRFS_INODE_APPEND)) {
> err |= INODE_FLAGS_ERROR;
>