On 2020/2/20 下午8:49, Nikolay Borisov wrote:
> <snip>
>
>>
>> Suggested-by: Josef Bacik <josef@xxxxxxxxxxxxxx>
>> Signed-off-by: Qu Wenruo <wqu@xxxxxxxx>
>> ---
>> fs/btrfs/volumes.c | 216 ++++++++++++++++++++++++++++++++++++++++-----
>> fs/btrfs/volumes.h | 11 +++
>> 2 files changed, 207 insertions(+), 20 deletions(-)
>>
>
> <snip>
>
>> +
>> +/*
>> + * Return 0 if we allocated any virtual(*) chunk, and restore the size to
>> + * @allocated_size
>> + * Return -ENOSPC if we have no more space to allocate virtual chunk
>> + *
>> + * *: virtual chunk is a space holder for per-profile available space
>> + * calculator.
>> + * Such virtual chunks won't take on-disk space, thus called virtual, and
>> + * only affects per-profile available space calulation.
>> + */
>
> Document that the last parameter is an output value which contains the
> size of the allocated virtual chunk.
Isn't that mentioned in the 3rd line of the comment? (Although the
parameter name doesn't match after some updates).
...
> lockdep assert that chunk mutex is held since you access alloc_list.
>
>> + 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;
>
> You can directly return.
Then you won't update the @result_ret.
And don't forget that, @result_ret is from an uninitialized local array.
Thus we must go out tag.
>
>> +
>> + devices_info = kcalloc(fs_devices->rw_devices, sizeof(*devices_info),
>> + GFP_NOFS);
>> + if (!devices_info) {
>> + ret = -ENOMEM;
>> + goto out;
>
> Same here.
Same as above.
>
>> + }
>> + /* Clear virtual chunk used space for each device */
>> + list_for_each_entry(device, &fs_devices->alloc_list, dev_alloc_list)
>> + device->virtual_allocated = 0;
>> + while (ret == 0) {
>> + ret = alloc_virtual_chunk(fs_info, devices_info, type,
>> + &allocated);
> The 'allocated' variable is used only in this loop so declare it in the
> loop. Ideally we want variables to have the shortest possible lifecycle.
Then your new while() loop idea will still need @allocated declared at
at the beginning of the function.
Thanks,
Qu
>
>> + if (ret == 0)
>> + result += allocated;
>> + }
>
> Why do you need to call this in a loop calling alloc_virtual_chunk once
> simply calculate the available space for the given raid type.
>
>> + list_for_each_entry(device, &fs_devices->alloc_list, dev_alloc_list)
>> + device->virtual_allocated = 0;
>> +out:
>> + kfree(devices_info);
>> + if (ret < 0 && ret != -ENOSPC)
>> + return ret;
>> + *result_ret = result;
>> + return 0;
>> +}
>
> <snip>
>
>> @@ -259,6 +266,10 @@ struct btrfs_fs_devices {
>> struct kobject fsid_kobj;
>> struct kobject *devices_kobj;
>> struct completion kobj_unregister;
>> +
>> + /* Records per-type available space */
>> + spinlock_t per_profile_lock;
>> + u64 per_profile_avail[BTRFS_NR_RAID_TYPES];
>
> Since every avail quantity is independent of any other, can you turn
> this into an array of atomic64 values and get rid of the spinlock? My
> head spins how many locks we have in btrfs.
>
>> };
>>
>> #define BTRFS_BIO_INLINE_CSUM_SIZE 64
>>
