Re: [PATCH RFC 4/4] btrfs: introduce new read_policy round-robin

[Anand Jain <anand.jain@oracle.com>]

On 28/10/20 10:44 pm, Josef Bacik wrote:
> On 10/28/20 9:26 AM, Anand Jain wrote:
>> Add round-robin read policy to route the read IO to the next device in 
>> the
>> round-robin order. The chunk allocation and thus the stripe-index follows
>> the order of free space available on devices. So to make the round-robin
>> effective it shall follow the devid order instead of the stripe-index
>> order.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>> RFC: because I am not too sure if any workload or block layer
>> configurations shall suit round-robin read_policy.
>>
>>   fs/btrfs/sysfs.c   |  2 +-
>>   fs/btrfs/volumes.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++
>>   fs/btrfs/volumes.h |  2 ++
>>   3 files changed, 53 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
>> index d2a974e1a1c4..293311c79321 100644
>> --- a/fs/btrfs/sysfs.c
>> +++ b/fs/btrfs/sysfs.c
>> @@ -908,7 +908,7 @@ static bool btrfs_strmatch(const char *given, 
>> const char *golden)
>>   /* Must follow the order as in enum btrfs_read_policy */
>>   static const char * const btrfs_read_policy_name[] = { "pid", 
>> "latency",
>> -                               "device" };
>> +                               "device", "roundrobin" };
>>   static ssize_t btrfs_read_policy_show(struct kobject *kobj,
>>                         struct kobj_attribute *a, char *buf)
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index 7ac675504051..fa1b1a3ebc87 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -5469,6 +5469,52 @@ int btrfs_is_parity_mirror(struct btrfs_fs_info 
>> *fs_info, u64 logical, u64 len)
>>       return ret;
>>   }
>> +struct stripe_mirror {
>> +    u64 devid;
>> +    int map;
>> +};
>> +
>> +static int btrfs_cmp_devid(const void *a, const void *b)
>> +{
>> +    struct stripe_mirror *s1 = (struct stripe_mirror *)a;
>> +    struct stripe_mirror *s2 = (struct stripe_mirror *)b;
>> +
>> +    if (s1->devid < s2->devid)
>> +        return -1;
>> +    if (s1->devid > s2->devid)
>> +        return 1;
>> +    return 0;
>> +}
>> +
>> +static int btrfs_find_read_round_robin(struct map_lookup *map, int 
>> first,
>> +                       int num_stripe)
>> +{
>> +    struct stripe_mirror stripes[4] = {0}; //4: for testing, works 
>> for now.
>> +    struct btrfs_fs_devices *fs_devices;
>> +    u64 devid;
>> +    int index, j, cnt;
>> +    int next_stripe;
>> +
>> +    index = 0;
>> +    for (j = first; j < first + num_stripe; j++) {
>> +        devid = map->stripes[j].dev->devid;
>> +
>> +        stripes[index].devid = devid;
>> +        stripes[index].map = j;
>> +
>> +        index++;
>> +    }
>> +
>> +    sort(stripes, num_stripe, sizeof(struct stripe_mirror),
>> +         btrfs_cmp_devid, NULL);
>> +
>> +    fs_devices = map->stripes[first].dev->fs_devices;
>> +    cnt = atomic_inc_return(&fs_devices->total_reads);
>> +    next_stripe = stripes[cnt % num_stripe].map;
> 
> This is heavy handed for policy decisions, just do something like
> 
> stripe_nr = atomic_inc_return(&fs_devices->total_reds) % num_stripes;
> return stripes[stripe_nr].map;
> 
> There's no reason to sort the stripes by devid every time we call this.  
> Thanks,


But that won't be round-robin at the device level, which I think we are
trying to achieve. It shall be round-robin at the strip level. As
stripe id isn't guaranteed to be on the same device across chunks.
Because chunk allocation follows the device free space size order.

But I agree the proposal in this patch is heavy. So I was thinking if we 
could fix the chunk allocation to sort by devid instead of size.? Any idea?

Thanks, Anand

> 
> Josef