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

[David Sterba <dsterba@suse.cz>]

On Tue, Oct 12, 2021 at 04:56:01PM +0300, Nikolay Borisov wrote:
> 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.

Like the following? I'll fold it in if yes:

--- a/fs/btrfs/block-group.c
+++ b/fs/btrfs/block-group.c
@@ -1507,7 +1507,7 @@ void btrfs_reclaim_bgs_work(struct work_struct *work)
 {
        struct btrfs_fs_info *fs_info =
                container_of(work, struct btrfs_fs_info, reclaim_bgs_work);
-       struct btrfs_block_group *bg;
+       struct btrfs_block_group *bg, *tmp;
        struct btrfs_space_info *space_info;
        LIST_HEAD(again_list);
        LIST_HEAD(reclaim_list);
@@ -1532,13 +1532,10 @@ void btrfs_reclaim_bgs_work(struct work_struct *work)
        spin_unlock(&fs_info->unused_bgs_lock);
 
        list_sort(NULL, &reclaim_list, reclaim_bgs_cmp);
-       while (!list_empty(&reclaim_list)) {
+       list_for_each_entry_safe(bg, tmp, &reclaim_list, bg_list) {
                u64 zone_unusable;
                int ret = 0;
 
-               bg = list_first_entry(&reclaim_list,
-                                     struct btrfs_block_group,
-                                     bg_list);
                list_del_init(&bg->bg_list);
 
                space_info = bg->space_info;
---