Re: [PATCH] btrfs: kill update_block_group_flags

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

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



On 2020/1/7 上午12:50, Josef Bacik wrote:
> btrfs/061 has been failing consistently for me recently with a
> transaction abort.  We run out of space in the system chunk array, which
> means we've allocated way too many system chunks than we need.

Isn't that caused by scrubbing creating unnecessary system chunks?

IIRC I had a patch to address that problem by just simply not allocating
system chunks for scrub.
("btrfs: scrub: Don't check free space before marking a block  group RO")

Although that doesn't address the whole problem, but it should at least
reduce the possibility.


Furthermore, with the newer over-commit behavior for inc_block_group_ro
("btrfs: use btrfs_can_overcommit in inc_block_group_ro"), we won't
really allocate new system chunks anymore if we can over-commit.

With those two patches, I guess we should have solved the problem.
Or did I miss something?

Thanks,
Qu

> 
> Chris added this a long time ago for balance as a poor mans restriping.
> If you had a single disk and then added another disk and then did a
> balance, update_block_group_flags would then figure out which RAID level
> you needed.
> 
> Fast forward to today and we have restriping behavior, so we can
> explicitly tell the fs that we're trying to change the raid level.  This
> is accomplished through the normal get_alloc_profile path.
> 
> Furthermore this code actually causes btrfs/061 to fail, because we do
> things like mkfs -m dup -d single with multiple devices.  This trips
> this check
> 
> alloc_flags = update_block_group_flags(fs_info, cache->flags);
> if (alloc_flags != cache->flags) {
> 	ret = btrfs_chunk_alloc(trans, alloc_flags, CHUNK_ALLOC_FORCE);
> 
> in btrfs_inc_block_group_ro.  Because we're balancing and scrubbing, but
> not actually restriping, we keep forcing chunk allocation of RAID1
> chunks.  This eventually causes us to run out of system space and the
> file system aborts and flips read only.
> 
> We don't need this poor mans restriping any more, simply use the normal
> get_alloc_profile helper, which will get the correct alloc_flags and
> thus make the right decision for chunk allocation.  This keeps us from
> allocating a billion system chunks and falling over.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/block-group.c | 52 ++----------------------------------------
>  1 file changed, 2 insertions(+), 50 deletions(-)
> 
> diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> index c79eccf188c5..0257e6f1efb1 100644
> --- a/fs/btrfs/block-group.c
> +++ b/fs/btrfs/block-group.c
> @@ -1975,54 +1975,6 @@ int btrfs_make_block_group(struct btrfs_trans_handle *trans, u64 bytes_used,
>  	return 0;
>  }
>  
> -static u64 update_block_group_flags(struct btrfs_fs_info *fs_info, u64 flags)
> -{
> -	u64 num_devices;
> -	u64 stripped;
> -
> -	/*
> -	 * if restripe for this chunk_type is on pick target profile and
> -	 * return, otherwise do the usual balance
> -	 */
> -	stripped = get_restripe_target(fs_info, flags);
> -	if (stripped)
> -		return extended_to_chunk(stripped);
> -
> -	num_devices = fs_info->fs_devices->rw_devices;
> -
> -	stripped = BTRFS_BLOCK_GROUP_RAID0 | BTRFS_BLOCK_GROUP_RAID56_MASK |
> -		BTRFS_BLOCK_GROUP_RAID1_MASK | BTRFS_BLOCK_GROUP_RAID10;
> -
> -	if (num_devices == 1) {
> -		stripped |= BTRFS_BLOCK_GROUP_DUP;
> -		stripped = flags & ~stripped;
> -
> -		/* turn raid0 into single device chunks */
> -		if (flags & BTRFS_BLOCK_GROUP_RAID0)
> -			return stripped;
> -
> -		/* turn mirroring into duplication */
> -		if (flags & (BTRFS_BLOCK_GROUP_RAID1_MASK |
> -			     BTRFS_BLOCK_GROUP_RAID10))
> -			return stripped | BTRFS_BLOCK_GROUP_DUP;
> -	} else {
> -		/* they already had raid on here, just return */
> -		if (flags & stripped)
> -			return flags;
> -
> -		stripped |= BTRFS_BLOCK_GROUP_DUP;
> -		stripped = flags & ~stripped;
> -
> -		/* switch duplicated blocks with raid1 */
> -		if (flags & BTRFS_BLOCK_GROUP_DUP)
> -			return stripped | BTRFS_BLOCK_GROUP_RAID1;
> -
> -		/* this is drive concat, leave it alone */
> -	}
> -
> -	return flags;
> -}
> -
>  int btrfs_inc_block_group_ro(struct btrfs_block_group *cache)
>  
>  {
> @@ -2058,7 +2010,7 @@ int btrfs_inc_block_group_ro(struct btrfs_block_group *cache)
>  	 * if we are changing raid levels, try to allocate a corresponding
>  	 * block group with the new raid level.
>  	 */
> -	alloc_flags = update_block_group_flags(fs_info, cache->flags);
> +	alloc_flags = get_alloc_profile(fs_info, cache->flags);
>  	if (alloc_flags != cache->flags) {
>  		ret = btrfs_chunk_alloc(trans, alloc_flags, CHUNK_ALLOC_FORCE);
>  		/*
> @@ -2082,7 +2034,7 @@ int btrfs_inc_block_group_ro(struct btrfs_block_group *cache)
>  	ret = inc_block_group_ro(cache, 0);
>  out:
>  	if (cache->flags & BTRFS_BLOCK_GROUP_SYSTEM) {
> -		alloc_flags = update_block_group_flags(fs_info, cache->flags);
> +		alloc_flags = get_alloc_profile(fs_info, cache->flags);
>  		mutex_lock(&fs_info->chunk_mutex);
>  		check_system_chunk(trans, alloc_flags);
>  		mutex_unlock(&fs_info->chunk_mutex);
> 


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