Re: [PATCH v2] btrfs: zoned: use greedy gc for auto reclaim

[Nikolay Borisov <nborisov@suse.com>]

On 12.10.21 г. 15:37, Johannes Thumshirn wrote:
> Currently auto reclaim of unusable zones reclaims the block-groups in the
> order they have been added to the reclaim list.
> 
> Change this to a greedy algorithm by sorting the list so we have the
> block-groups with the least amount of valid bytes reclaimed first.
> 
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> 
> ---
> Changes since v1:
> -  Changed list_sort() comparator to 'boolean' style
> 
> Changes since RFC:
> - Updated the patch description
> - Don't sort the list under the spin_lock (David)

<snip>


> @@ -1510,17 +1528,20 @@ void btrfs_reclaim_bgs_work(struct work_struct *work)
>  	}
>  
>  	spin_lock(&fs_info->unused_bgs_lock);
> -	while (!list_empty(&fs_info->reclaim_bgs)) {
> +	list_splice_init(&fs_info->reclaim_bgs, &reclaim_list);
> +	spin_unlock(&fs_info->unused_bgs_lock);
> +
> +	list_sort(NULL, &reclaim_list, reclaim_bgs_cmp);
> +	while (!list_empty(&reclaim_list)) {

Nit: Now that you've switched to a local reclaim_list you can convert
the while to a list_for_each_entry_safe, since it's guaranteed that new
entries can't be added while you are iterating the list, which is
generally the reason why a while() is preferred to one of the iteration
helpers.

>  		u64 zone_unusable;
>  		int ret = 0;
>  
> -		bg = list_first_entry(&fs_info->reclaim_bgs,
> +		bg = list_first_entry(&reclaim_list,
>  				      struct btrfs_block_group,
>  				      bg_list);
>  		list_del_init(&bg->bg_list);
>  
>  		space_info = bg->space_info;
> -		spin_unlock(&fs_info->unused_bgs_lock);
>  
>  		/* Don't race with allocators so take the groups_sem */
>  		down_write(&space_info->groups_sem);
> @@ -1568,12 +1589,12 @@ void btrfs_reclaim_bgs_work(struct work_struct *work)
>  				  bg->start);
>  
>  next:
> -		spin_lock(&fs_info->unused_bgs_lock);
>  		if (ret == -EAGAIN && list_empty(&bg->bg_list))
>  			list_add_tail(&bg->bg_list, &again_list);
>  		else
>  			btrfs_put_block_group(bg);
>  	}
> +	spin_lock(&fs_info->unused_bgs_lock);
>  	list_splice_tail(&again_list, &fs_info->reclaim_bgs);
>  	spin_unlock(&fs_info->unused_bgs_lock);
>  	mutex_unlock(&fs_info->reclaim_bgs_lock);
>