Re: [PATCH 1/4] btrfs: add read_policy latency

Anand Jain
2020-10-29T01:07:14Z

On 28/10/20 10:30 pm, Josef Bacik wrote:
> On 10/28/20 9:26 AM, Anand Jain wrote:
>> The read policy type latency routes the read IO based on the historical
>> average wait time experienced by the read IOs through the individual
>> device factored by 1/10 of inflight commands in the queue. The factor
>> 1/10 is because generally the block device queue depth is more than 1,
>> so there can be commands in the queue even before the previous commands
>> have been completed. This patch obtains the historical read IO stats from
>> the kernel block layer.

Oops I need to fix the change log.

>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>> v1: Drop part_stat_read_all instead use part_stat_read
>>      Drop inflight
>>   fs/btrfs/sysfs.c   |  3 ++-
>>   fs/btrfs/volumes.c | 39 ++++++++++++++++++++++++++++++++++++++-
>>   fs/btrfs/volumes.h |  1 +
>>   3 files changed, 41 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
>> index 4dbf90ff088a..88cbf7b2edf0 100644
>> --- a/fs/btrfs/sysfs.c
>> +++ b/fs/btrfs/sysfs.c
>> @@ -906,7 +906,8 @@ static bool btrfs_strmatch(const char *given,
>> const char *golden)
>>       return false;
>>   }
>> -static const char * const btrfs_read_policy_name[] = { "pid" };
>> +/* Must follow the order as in enum btrfs_read_policy */
>> +static const char * const btrfs_read_policy_name[] = { "pid",
>> "latency" };
>>   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 6bf487626f23..48587009b656 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -14,6 +14,7 @@
>>   #include <linux/semaphore.h>
>>   #include <linux/uuid.h>
>>   #include <linux/list_sort.h>
>> +#include <linux/part_stat.h>
>>   #include "misc.h"
>>   #include "ctree.h"
>>   #include "extent_map.h"
>> @@ -5468,6 +5469,39 @@ int btrfs_is_parity_mirror(struct btrfs_fs_info
>> *fs_info, u64 logical, u64 len)
>>       return ret;
>>   }
>> +static int btrfs_find_best_stripe(struct btrfs_fs_info *fs_info,
>> +                  struct map_lookup *map, int first,
>> +                  int num_stripe)
>> +{
>> +    u64 est_wait = 0;
>> +    int best_stripe = 0;
>> +    int index;
>> +
>> +    for (index = first; index < first + num_stripe; index++) {
>> +        u64 read_wait;
>> +        u64 avg_wait = 0;
>> +        unsigned long read_ios;
>> +        struct btrfs_device *device = map->stripes[index].dev;
>> +
>> +        read_wait = part_stat_read(device->bdev->bd_part, nsecs[READ]);
>> +        read_ios = part_stat_read(device->bdev->bd_part, ios[READ]);
>> +
>> +        if (read_wait && read_ios && read_wait >= read_ios)
>> +            avg_wait = div_u64(read_wait, read_ios);
>> +        else
>> +            btrfs_info_rl(device->fs_devices->fs_info,
>> +            "devid: %llu avg_wait ZERO read_wait %llu read_ios %lu",
>> +                      device->devid, read_wait, read_ios);
>
> Do we even care about this?  I feel like if we do at all it can be
> btrfs_debug or something.

Yeah I think btrfs_debug is better until we cover most the hardware?

>
>> +
>> +        if (est_wait == 0 || est_wait > avg_wait) {
>> +            est_wait = avg_wait;
>> +            best_stripe = index;
>> +        }
>> +    }
>> +
>> +    return best_stripe;
>> +}
>> +
>>   static int find_live_mirror(struct btrfs_fs_info *fs_info,
>>                   struct map_lookup *map, int first,
>>                   int dev_replace_is_ongoing)
>> @@ -5498,6 +5532,10 @@ static int find_live_mirror(struct
>> btrfs_fs_info *fs_info,
>>       case BTRFS_READ_POLICY_PID:
>>           preferred_mirror = first + current->pid % num_stripes;
>>           break;
>> +    case BTRFS_READ_POLICY_LATENCY:
>> +        preferred_mirror = btrfs_find_best_stripe(fs_info, map, first,
>> +                              num_stripes);
>> +        break;
>>       }
>>       if (dev_replace_is_ongoing &&
>> @@ -6114,7 +6152,6 @@ static int __btrfs_map_block(struct
>> btrfs_fs_info *fs_info,
>>       } else if (map->type & BTRFS_BLOCK_GROUP_RAID10) {
>>           u32 factor = map->num_stripes / map->sub_stripes;
>> -
>
> Unrelated change.  Thanks,

Oops. Thanks You I will fix.

Anand

>
> Josef