Re: [PATCH v3 1/3] btrfs: Introduce per-profile available space facility

Qu Wenruo
2020-01-07T02:14:01Z

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

On 2020/1/6 下午10:32, David Sterba wrote:
> On Mon, Jan 06, 2020 at 02:13:41PM +0800, Qu Wenruo wrote:
[...]
>> +static int calc_one_profile_avail(struct btrfs_fs_info *fs_info,
>> + enum btrfs_raid_types type)
>> +{
>> + struct btrfs_device_info *devices_info = NULL;
>> + struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;
>> + struct btrfs_device *device;
>> + u64 allocated;
>> + u64 result = 0;
>> + int ret = 0;
>> +
>> + ASSERT(type >= 0 && type < BTRFS_NR_RAID_TYPES);
>> +
>> + /* Not enough devices, quick exit, just update the result */
>> + if (fs_devices->rw_devices < btrfs_raid_array[type].devs_min)
>> + goto out;
>> +
>> + devices_info = kcalloc(fs_devices->rw_devices, sizeof(*devices_info),
>> + GFP_NOFS);
>
> Calling kcalloc is another potential slowdown, for the statfs
> considerations.

That's also what we did in statfs() before, so it shouldn't cause extra
problem.
Furthermore, we didn't use calc_one_profile_avail() directly in statfs()
directly.

Although I get your point, and personally speaking the memory allocation
and extra in-memory device iteration should be pretty fast compared to
__btrfs_alloc_chunk().

Thus I don't think this memory allocation would cause extra trouble,
except the error handling mentioned below.

[...]
>> + ret = calc_per_profile_avail(fs_info);
>
> Adding new failure modes

Another solution I have tried is make calc_per_profile_avail() return
void, ignoring the ENOMEM error, and just set the affected profile to 0
available space.

But that method is just delaying ENOMEM, and would cause strange
pre-profile values until next successful update or mount cycle.

Any idea on which method is less worse?

[...]
>>
>> --- a/fs/btrfs/volumes.h
>> +++ b/fs/btrfs/volumes.h
>> @@ -138,6 +138,13 @@ struct btrfs_device {
>> atomic_t dev_stat_values[BTRFS_DEV_STAT_VALUES_MAX];
>>
>> struct extent_io_tree alloc_state;
>> +
>> + /*
>> + * the "virtual" allocated space by virtual chunk allocator, which
>> + * is used to do accurate estimation on available space.
>> + * Doesn't affect real chunk allocator.
>> + */
>> + u64 virtual_allocated;
>
> I find it a bit confusing to use 'virtual', though I get what you mean.
> As this is per-device it accounts overall space, so the name should
> reflect somthing like that. I maybe have a more concrete suggestion once
> I read through the whole series.
>
Looking forward that naming.

Thanks,
Qu

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