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

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2019/9/30 下午7:36, Nikolay Borisov wrote:
>
>
> 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.

Yes, the ASSERT() doesn't make much sense by itself.

However I still believe it won't be a problem.

It's compiler's job to remove such dead ASSERT(), but for human reader,
I still believe this ASSERT() could still make sense, especially when
the caller or callee can get more and more complex.

Thanks,
Qu

>
>> +
>> +	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;
>>