Re: [PATCH v2] btrfs-progs: Replace OPEN_CTREE_NO_BLOCK_GROUPS with OPEN_CTREE_PARTIAL

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

On 2021/8/12 下午7:20, Nikolay Borisov wrote:
> When OPEN_CTREE_PARTIAL is used errors in loading the extent/csum/log
> trees are turned into non-fatal ones and those roots are initialized
> with dummy root nodes, which don't have uptodate flag set on the
> respective extent buffer. On the other hand reading block groups during
> mount is predicated on OPEN_CTREE_NO_BLOCK_GROUPS being unset and
> the extent tree's root extent buffer to have the uptodate flag set to
> true. Furthermore, OPEN_CTREE_NO_BLOCK_GROUP is used when there is a
> high chance the filesystem being opened can be damaged, similarly to
> OPEN_CTREE_PARTIAL.
>
> Considering this logic, it's not possible to load block groups when both
> OPEN_CTREE_NO_BLOCK_GROUPS and OPEN_CTREE_PARTIAL are passed and the
> extent tree is corrupted. This allows to eliminate OPEN_CTREE_NO_BLOCK_GROUPS
> and replace its usage with OPEN_CTREE_PARTIAL, since it's sufficient to check
> only for the extent tree's extent buffer's uptodate state, which is controlledi
> by OPEN_CTREE_PARTIAL. Additionally to retain the existing semantics, if an
> error is encountered while reading any of the block groups, simply free the
> block group caches and clearing the uptodate flag on the extent root's node.
>
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu
> ---
>
> V2:
> * Handle the case where we encounter an error on a child node of the exten tree
>    while opening the fs with PARTIAL flag. In this case simply delete any block groups
>    read.
>
>   check/main.c             |  2 +-
>   cmds/inspect-dump-tree.c |  2 +-
>   cmds/rescue.c            |  4 ++--
>   cmds/restore.c           |  2 +-
>   kernel-shared/disk-io.c  | 15 ++++++++++-----
>   kernel-shared/disk-io.h  |  7 ++++---
>   6 files changed, 19 insertions(+), 13 deletions(-)
>
> diff --git a/check/main.c b/check/main.c
> index a88518159830..84dd96fc167a 100644
> --- a/check/main.c
> +++ b/check/main.c
> @@ -10342,7 +10342,7 @@ static int cmd_check(const struct cmd_struct *cmd, int argc, char **argv)
>   			case GETOPT_VAL_INIT_EXTENT:
>   				init_extent_tree = 1;
>   				ctree_flags |= (OPEN_CTREE_WRITES |
> -						OPEN_CTREE_NO_BLOCK_GROUPS);
> +						OPEN_CTREE_PARTIAL);
>   				repair = 1;
>   				break;
>   			case GETOPT_VAL_CHECK_CSUM:
> diff --git a/cmds/inspect-dump-tree.c b/cmds/inspect-dump-tree.c
> index e1c90be7e81f..fca79cd11599 100644
> --- a/cmds/inspect-dump-tree.c
> +++ b/cmds/inspect-dump-tree.c
> @@ -330,7 +330,7 @@ static int cmd_inspect_dump_tree(const struct cmd_struct *cmd,
>   	 * to inspect fs with corrupted extent tree blocks, and show as many good
>   	 * tree blocks as possible.
>   	 */
> -	open_ctree_flags = OPEN_CTREE_PARTIAL | OPEN_CTREE_NO_BLOCK_GROUPS;
> +	open_ctree_flags = OPEN_CTREE_PARTIAL;
>   	cache_tree_init(&block_root);
>   	optind = 0;
>   	while (1) {
> diff --git a/cmds/rescue.c b/cmds/rescue.c
> index a98b255ad328..7ebe9b6c1e54 100644
> --- a/cmds/rescue.c
> +++ b/cmds/rescue.c
> @@ -194,8 +194,8 @@ static int cmd_rescue_zero_log(const struct cmd_struct *cmd,
>   		goto out;
>   	}
>
> -	root = open_ctree(devname, 0, OPEN_CTREE_WRITES | OPEN_CTREE_PARTIAL |
> -			  OPEN_CTREE_NO_BLOCK_GROUPS);
> +	root = open_ctree(devname, 0, OPEN_CTREE_WRITES | OPEN_CTREE_PARTIAL);
> +
>   	if (!root) {
>   		error("could not open ctree");
>   		return 1;
> diff --git a/cmds/restore.c b/cmds/restore.c
> index 39fcc69d8e19..f30d8b7532c0 100644
> --- a/cmds/restore.c
> +++ b/cmds/restore.c
> @@ -1214,7 +1214,7 @@ static struct btrfs_root *open_fs(const char *dev, u64 root_location,
>   		ocf.filename = dev;
>   		ocf.sb_bytenr = bytenr;
>   		ocf.root_tree_bytenr = root_location;
> -		ocf.flags = OPEN_CTREE_PARTIAL | OPEN_CTREE_NO_BLOCK_GROUPS;
> +		ocf.flags = OPEN_CTREE_PARTIAL;
>   		fs_info = open_ctree_fs_info(&ocf);
>   		if (fs_info)
>   			break;
> diff --git a/kernel-shared/disk-io.c b/kernel-shared/disk-io.c
> index 84990a521178..dfa4576c6371 100644
> --- a/kernel-shared/disk-io.c
> +++ b/kernel-shared/disk-io.c
> @@ -1044,8 +1044,7 @@ int btrfs_setup_all_roots(struct btrfs_fs_info *fs_info, u64 root_tree_bytenr,
>
>   	fs_info->generation = generation;
>   	fs_info->last_trans_committed = generation;
> -	if (extent_buffer_uptodate(fs_info->extent_root->node) &&
> -	    !(flags & OPEN_CTREE_NO_BLOCK_GROUPS)) {
> +	if (extent_buffer_uptodate(fs_info->extent_root->node)) {
>   		ret = btrfs_read_block_groups(fs_info);
>   		/*
>   		 * If we don't find any blockgroups (ENOENT) we're either
> @@ -1053,9 +1052,15 @@ int btrfs_setup_all_roots(struct btrfs_fs_info *fs_info, u64 root_tree_bytenr,
>   		 * anything else is error
>   		 */
>   		if (ret < 0 && ret != -ENOENT) {
> -			errno = -ret;
> -			error("failed to read block groups: %m");
> -			return ret;
> +			if (!(flags & OPEN_CTREE_PARTIAL)) {
> +				errno = -ret;
> +				error("failed to read block groups: %m");
> +				return ret;
> +			} else {
> +				btrfs_free_block_groups(fs_info);
> +				clear_extent_buffer_uptodate(
> +						fs_info->extent_root->node);
> +			}
>   		}
>   	}
>
> diff --git a/kernel-shared/disk-io.h b/kernel-shared/disk-io.h
> index 603ff8a3f945..16e13e4f5524 100644
> --- a/kernel-shared/disk-io.h
> +++ b/kernel-shared/disk-io.h
> @@ -32,7 +32,10 @@
>   enum btrfs_open_ctree_flags {
>   	/* Open filesystem for writes */
>   	OPEN_CTREE_WRITES		= (1U << 0),
> -	/* Allow to open filesystem with some broken tree roots (eg log root) */
> +	/*
> +	 * Allow to open filesystem with some broken tree roots
> +	 * (eg log root/csum/extent tree)
> +	 */
>   	OPEN_CTREE_PARTIAL		= (1U << 1),
>   	/* If primary root pinters are invalid, try backup copies */
>   	OPEN_CTREE_BACKUP_ROOT		= (1U << 2),
> @@ -40,8 +43,6 @@ enum btrfs_open_ctree_flags {
>   	OPEN_CTREE_RECOVER_SUPER	= (1U << 3),
>   	/* Restoring filesystem image */
>   	OPEN_CTREE_RESTORE		= (1U << 4),
> -	/* Do not read block groups (extent tree) */
> -	OPEN_CTREE_NO_BLOCK_GROUPS	= (1U << 5),
>   	/* Open all devices in O_EXCL mode */
>   	OPEN_CTREE_EXCLUSIVE		= (1U << 6),
>   	/* Do not scan devices */
>