Re: [PATCH 08/10] btrfs-progs: check: Add support for freespace tree fixing

[Omar Sandoval <osandov@osandov.com>]

On Mon, Oct 01, 2018 at 05:46:19PM +0300, Nikolay Borisov wrote:
> Now that all the prerequisite code for proper support of free space
> tree repair is in, it's time to wire it in. This is achieved by first
> hooking the freespace tree to the __free_extent/alloc_reserved_tree_block
> functions. And then introducing a wrapper function to contains the
> existing check_space_cache and the newly introduced repair code.
> Finally, it's important to note that FST repair code first clears the
> existing FST in case of any problem found and rebuilds it from scratch.

Reviewed-by: Omar Sandoval <osandov@fb.com>

A couple of really trivial nitpicks below that you should feel free to
ignore ;)

> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  check/main.c | 47 ++++++++++++++++++++++++++++++-----------------
>  1 file changed, 30 insertions(+), 17 deletions(-)
> 
> diff --git a/check/main.c b/check/main.c
> index b361cd7e26a0..4daf85aad82c 100644
> --- a/check/main.c
> +++ b/check/main.c
> @@ -5392,14 +5392,6 @@ static int check_space_cache(struct btrfs_root *root)
>  	int ret;
>  	int error = 0;
>  
> -	if (btrfs_super_cache_generation(root->fs_info->super_copy) != -1ULL &&
> -	    btrfs_super_generation(root->fs_info->super_copy) !=
> -	    btrfs_super_cache_generation(root->fs_info->super_copy)) {
> -		printf("cache and super generation don't match, space cache "
> -		       "will be invalidated\n");
> -		return 0;
> -	}
> -
>  	while (1) {
>  		ctx.item_count++;
>  		cache = btrfs_lookup_first_block_group(root->fs_info, start);
> @@ -9417,7 +9409,6 @@ static int do_clear_free_space_cache(struct btrfs_fs_info *fs_info,
>  			ret = 1;
>  			goto close_out;
>  		}
> -		printf("Clearing free space cache\n");

Just out of curiosity, why did you delete this message? The one in the
v2 case is still there.

>  		ret = clear_free_space_cache(fs_info);
>  		if (ret) {
>  			error("failed to clear free space cache");
> @@ -9444,6 +9435,35 @@ static int do_clear_free_space_cache(struct btrfs_fs_info *fs_info,
>  	return ret;
>  }
>  
> +static int validate_free_space_cache(struct btrfs_root *root)

At first glance, I wouldn't know what the difference is between
check_space_cache() and validate_free_space_cache(); they sound like the
same thing. Maybe rename this to check_and_repair_space_cache() or just
fold the rebuild into check_space_cache(), to be more in line with the
other check steps in fsck?