Re: [PATCH] btrfs-progs: add a --check-bg-usage option to fsck

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

[-- Attachment #1.1: Type: text/plain, Size: 5477 bytes --]



On 2019/8/2 下午9:06, Josef Bacik wrote:
> Sometimes when messing with the chunk allocator code we can end up
> over-allocating chunks.  Generally speaking I'll notice this when a
> random xfstest fails with ENOSPC when it shouldn't, but I'm super
> worried that I won't catch a problem until somebody has a fs completely
> filled up with empty block groups.  Add a fsck option to check for too
> many empty block groups.  This way I can set FSCK_OPTIONS="-B" to catch
> cases where we're too aggressive with the chunk allocator but not so
> aggressive that it causes problems in xfstests.
> 
> Thankfully this doesn't trip up currently, so this will just keep me
> from regressing us.  Thanks,

I think the empty bg check is valid.

Although I hope this check can be a warning for default check, and a new
option to report too many empty bgs as error.

> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  btrfsck.h    |  1 +
>  check/main.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 51 insertions(+), 2 deletions(-)
> 
> diff --git a/btrfsck.h b/btrfsck.h
> index ac7f5d48..5e779075 100644
> --- a/btrfsck.h
> +++ b/btrfsck.h
> @@ -44,6 +44,7 @@ struct block_group_record {
>  	u64 offset;
>  
>  	u64 flags;
> +	u64 used;
>  };
>  
>  struct block_group_tree {
> diff --git a/check/main.c b/check/main.c
> index 0cc6fdba..a3ff3791 100644
> --- a/check/main.c
> +++ b/check/main.c
> @@ -62,6 +62,7 @@ int no_holes = 0;
>  static int is_free_space_tree = 0;
>  int init_extent_tree = 0;
>  int check_data_csum = 0;
> +int check_bg_usage = 0;
>  struct btrfs_fs_info *global_info;
>  struct task_ctx ctx = { 0 };
>  struct cache_tree *roots_info_cache = NULL;
> @@ -5126,6 +5127,7 @@ btrfs_new_block_group_record(struct extent_buffer *leaf, struct btrfs_key *key,
>  
>  	ptr = btrfs_item_ptr(leaf, slot, struct btrfs_block_group_item);
>  	rec->flags = btrfs_disk_block_group_flags(leaf, ptr);
> +	rec->used = btrfs_disk_block_group_used(leaf, ptr);
>  
>  	INIT_LIST_HEAD(&rec->list);
>  
> @@ -8522,6 +8524,41 @@ out:
>  	return ret;
>  }
>  
> +static int check_block_group_usage(struct block_group_tree *block_group_cache)
> +{
> +	struct block_group_record *bg_rec;
> +	int empty_data = 0, empty_metadata = 0, empty_system = 0;
> +	int ret = 0;
> +
> +	list_for_each_entry(bg_rec, &block_group_cache->block_groups, list) {
> +		if (bg_rec->used)
> +			continue;
> +		if (bg_rec->flags & BTRFS_BLOCK_GROUP_DATA)
> +			empty_data++;
> +		else if (bg_rec->flags & BTRFS_BLOCK_GROUP_METADATA)
> +			empty_metadata++;
> +		else
> +			empty_system++;
> +	}
> +
> +	if (empty_data > 1) {
> +		ret = -EINVAL;
> +		fprintf(stderr, "Too many empty data block groups: %d\n",
> +			empty_data);
> +	}
> +	if (empty_metadata > 1) {
> +		ret = -EINVAL;
> +		fprintf(stderr, "Too many empty metadata block groups: %d\n",
> +			empty_metadata);
> +	}
> +	if (empty_system > 1) {
> +		ret = -EINVAL;
> +		fprintf(stderr, "Too many empty system block groups: %d\n",
> +			empty_system);
> +	}

This hard coded threshold (1) is too vague and maybe too restrict.
What will happen for things like a lot of data got removed and cleaner
didn't get kicked in quickly enough before unmount?

Thanks,
Qu

> +	return ret;
> +}
> +
>  static int check_chunks_and_extents(struct btrfs_fs_info *fs_info)
>  {
>  	struct rb_root dev_cache;
> @@ -8630,6 +8667,12 @@ again:
>  		err = ret;
>  	}
>  
> +	if (check_bg_usage) {
> +		ret = check_block_group_usage(&block_group_cache);
> +		if (ret)
> +			err = ret;
> +	}
> +
>  	ret = check_extent_refs(root, &extent_cache);
>  	if (ret < 0) {
>  		if (ret == -EAGAIN)
> @@ -9810,6 +9853,7 @@ static const char * const cmd_check_usage[] = {
>  	"       -E|--subvol-extents <subvolid>",
>  	"                                   print subvolume extents and sharing state",
>  	"       -p|--progress               indicate progress",
> +	"       -B|--check-bg-usage         check for too many empty block groups",
>  	NULL
>  };
>  
> @@ -9841,7 +9885,7 @@ static int cmd_check(const struct cmd_struct *cmd, int argc, char **argv)
>  			GETOPT_VAL_INIT_EXTENT, GETOPT_VAL_CHECK_CSUM,
>  			GETOPT_VAL_READONLY, GETOPT_VAL_CHUNK_TREE,
>  			GETOPT_VAL_MODE, GETOPT_VAL_CLEAR_SPACE_CACHE,
> -			GETOPT_VAL_FORCE };
> +			GETOPT_VAL_FORCE};
>  		static const struct option long_options[] = {
>  			{ "super", required_argument, NULL, 's' },
>  			{ "repair", no_argument, NULL, GETOPT_VAL_REPAIR },
> @@ -9864,10 +9908,11 @@ static int cmd_check(const struct cmd_struct *cmd, int argc, char **argv)
>  			{ "clear-space-cache", required_argument, NULL,
>  				GETOPT_VAL_CLEAR_SPACE_CACHE},
>  			{ "force", no_argument, NULL, GETOPT_VAL_FORCE },
> +			{ "check-bg-usage", no_argument, NULL, 'B' },
>  			{ NULL, 0, NULL, 0}
>  		};
>  
> -		c = getopt_long(argc, argv, "as:br:pEQ", long_options, NULL);
> +		c = getopt_long(argc, argv, "as:br:pEQB", long_options, NULL);
>  		if (c < 0)
>  			break;
>  		switch(c) {
> @@ -9875,6 +9920,9 @@ static int cmd_check(const struct cmd_struct *cmd, int argc, char **argv)
>  			case 'b':
>  				ctree_flags |= OPEN_CTREE_BACKUP_ROOT;
>  				break;
> +			case 'B':
> +				check_bg_usage = 1;
> +				break;
>  			case 's':
>  				num = arg_strtou64(optarg);
>  				if (num >= BTRFS_SUPER_MIRROR_MAX) {
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]