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

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

[Josef Bacik]

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,

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);
+	}
+	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) {
-- 
2.21.0

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

[Qu Wenruo]

[-- 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 --]

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

[Josef Bacik]

On Fri, Aug 02, 2019 at 09:54:12PM +0800, Qu Wenruo wrote:
> 
> 
> 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.
> 

I don't want to make it default for the reason you describe below.  I'm thinking
of some new log-writes test that checks an fs at a transaction commit where
we've emptied a ton of block groups but haven't removed them yet.

<snip>

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

Which is why I ran it through xfstests first to verify this wasn't too weird.
What I'm try to capture is the case where something has really gone wrong, so
there will be tons of empty block groups, not just one.  I think generally
speaking having 1 left over makes sense for the testcases that fill and then
delete a bunch, so I don't want those to fail.  Thanks,

Josef

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

[David Sterba]

On Fri, Aug 02, 2019 at 09:06:35AM -0400, 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,
> 
> 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);
> +	}
> +	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",

The option name does not match the description, it could be something
like --check-empty-bg-count and certainly should not use the short
option. That are reserved for most common usecases, I don't think
this is the case.

>  	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) {
> -- 
> 2.21.0