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

Josef Bacik
2020-10-28T23:23:53Z

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,

Josef