Re: [PATCH 5/5] btrfs: zoned: make auto-reclaim less aggressive

[Damien Le Moal <damien.lemoal@opensource.wdc.com>]

On 3/23/22 20:52, Johannes Thumshirn wrote:
> On 23/03/2022 12:24, Pankaj Raghav wrote:
>>
>>
>> On 2022-03-23 11:39, Johannes Thumshirn wrote:
>>>
>>> It looks like we can't use btrfs_calc_available_free_space(), can
>>> you try this one on top:
>>>
>>> diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
>>> index f2a412427921..4a6c1f1a7223 100644
>>> --- a/fs/btrfs/zoned.c
>>> +++ b/fs/btrfs/zoned.c
>>> @@ -2082,23 +2082,27 @@ void btrfs_free_zone_cache(struct btrfs_fs_info *fs_info)
>>>  
>>>  bool btrfs_zoned_should_reclaim(struct btrfs_fs_info *fs_info)
>>>  {
>>> -       struct btrfs_space_info *sinfo;
>>> +       struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;
>>> +       struct btrfs_device *device;
>>>         u64 used = 0;
>>>         u64 total = 0;
>>>         u64 factor;
>>>  
>>> -       if (!btrfs_is_zoned(fs_info))
>>> -               return false;
>>> -
>>>         if (!fs_info->bg_reclaim_threshold)
>>>                 return false;
>>>  
>>> -       list_for_each_entry(sinfo, &fs_info->space_info, list) {
>>> -               total += sinfo->total_bytes;
>>> -               used += btrfs_calc_available_free_space(fs_info, sinfo,
>>> -                                                       BTRFS_RESERVE_NO_FLUSH);
>>> +
>>> +       mutex_lock(&fs_devices->device_list_mutex);
>>> +       list_for_each_entry(device, &fs_devices->devices, dev_list) {
>>> +               if (!device->bdev)
>>> +                       continue;
>>> +
>>> +               total += device->disk_total_bytes;
>>> +               used += device->bytes_used;

Does bytes_used include all the unusable blocks between zone cap and zone
size for all zones ? If yes, then the calculation will be OK. If not, then
you will get an artificially low factor not reflecting the need for defrag.

>>> +
>>>         }
>>> +       mutex_unlock(&fs_devices->device_list_mutex);
>>>  
>>> -       factor = div_u64(used * 100, total);
>>> +       factor = div64_u64(used * 100, total);
>>>         return factor >= fs_info->bg_reclaim_threshold;

Not sure if the factor variable is really necessary here.

>>>  }
>>>
>> size 1280M:
>> [   47.511871] btrfs: factor: 30 used: 402653184, total: 1342177280
>> [   48.542981] btrfs: factor: 30 used: 402653184, total: 1342177280
>> [   49.576005] btrfs: factor: 30 used: 402653184, total: 1342177280
>> size: 12800M:
>> [   33.971009] btrfs: factor: 3 used: 402653184, total: 13421772800
>> [   34.978602] btrfs: factor: 3 used: 402653184, total: 13421772800
>> [   35.991784] btrfs: factor: 3 used: 402653184, total: 13421772800
>> size: 12800M, zcap=96M zsze=128M:
>> [   64.639103] btrfs: factor: 3 used: 402653184, total: 13421772800
>> [   65.643778] btrfs: factor: 3 used: 402653184, total: 13421772800
>> [   66.661920] btrfs: factor: 3 used: 402653184, total: 13421772800
>>
>> This looks good. And the test btrfs/237 is failing, as it should be
>> because of the change in reclaim condition. Are you planning to update
>> this test in fstests later?
> 
> Yes, once I have an idea how to do. Probably just fill the FS until
> ~75% of the drive is filled and then use the original logic.
> 
>> I still have one more question: shouldn't we use the usable disk bytes
>> (zcap * nr_zones) instead of disk_total_bytes (zsze * nr_zones) to
>> calculate the `total` variable? The `used` value is a part of the usable
>> disk space so I feel it makes more sense to calculate the `factor` with
>> the usable disk bytes instead of the disk_total_bytes.
>>
> 
> disk_total_bytes comes from the value the underlying device driver set
> for the gendisk's capacity via set_capacity().


-- 
Damien Le Moal
Western Digital Research