Re: [PATCH v4 3/7] btrfs: block-group: Refactor how we delete one block group item

From: Qu Wenruo <quwenruo.btrfs@gmx.com>
To: Johannes Thumshirn <Johannes.Thumshirn@wdc.com>,
	Qu Wenruo <wqu@suse.com>,
	"linux-btrfs@vger.kernel.org" <linux-btrfs@vger.kernel.org>
Subject: Re: [PATCH v4 3/7] btrfs: block-group: Refactor how we delete one block group item
Date: Tue, 5 May 2020 16:55:42 +0800	[thread overview]
Message-ID: <bd019a91-406b-f913-1324-d4803f9b2223@gmx.com> (raw)
In-Reply-To: <SN4PR0401MB35980F5B55AB37C71E789C1D9BA70@SN4PR0401MB3598.namprd04.prod.outlook.com>

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

On 2020/5/5 下午4:47, Johannes Thumshirn wrote:
> On 05/05/2020 01:58, Qu Wenruo wrote:
>> When deleting a block group item, it's pretty straight forward, just
>> delete the item pointed by the key.
>>
>> However it will not be that straight-forward for incoming skinny block
>> group item.
>>
>> So refactor the block group item deletion into a new function,
>> remove_block_group_item(), also to make the already lengthy
>> btrfs_remove_block_group() a little shorter.
> 
> I think this patch is useful even without the skinny_bg feature.
> 
>> +static int remove_block_group_item(struct btrfs_trans_handle *trans,
>> +				   struct btrfs_path *path,
>> +				   struct btrfs_block_group *block_group)
>> +{
>> +	struct btrfs_fs_info *fs_info = trans->fs_info;
>> +	struct btrfs_root *root;
> 
> Tiny nitpick, why not:
> 
> 	struct btrfs_root *root = fs_info->extent_root;
> 
> Like it was in brtfs_remove_block_group()?

That's mostly for the skinny_bg_tree (6th) patch, as in that patch,
skinny_bg_tree feature goes to pick bg_root other than extent root.

So I didn't initialize root here, but leaves it assigned the same timing
as key.

Thanks,
Qu

> 
> Anyways looks good to me,
> Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> 

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