Re: [PATCH v2 0/3] raid1 balancing methods

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

On 22/10/24 02:42, David Sterba wrote:
> On Mon, Oct 21, 2024 at 11:36:10PM +0800, Anand Jain wrote:
>>> I think it's safe to start with the round-round robin policy, but the
>>> syntax is strange, why the [ ] are mandatory? Also please call it
>>> round-robin, or 'rr' for short.
>>
>> I'm fine with round-robin. The [ ] part is not mandatory; if the
>> min_contiguous_read value is not specified, it will default to a
>> predefined value.
>>
>>> The default of sector size is IMHO a wrong value, switching devices so
>>> often will drop the performance just because of the io request overhead.
>>
>>>   From what I rememer values around 200k were reasonable, so either 192k
>>> or 256k should be the default. We may also drop the configurable value
>>> at all and provide a few hard coded sizes like rr-256k, rr-512k, rr-1m,
>>> if not only to drop parsing of user strings.
>>
>> I'm okay with a default value of 256k. For the experimental feature,
>> we can keep it configurable, allowing the opportunity to experiment
>> with other values as well
> 
> Yeah, for experimenting it makes sense to make it flexible, no need to
> patch and reboot the kernel. For final we should settle on some
> reasonable values.
> 
>>>> 5. Tested FIO random read/write and defrag compression workloads with
>>>>      min_contiguous_read set to sector size, 192k, and 256k.
>>>>
>>>>      RAID1 balancing method rotation is better for multi-process workloads
>>>>      such as fio and also single-process workload such as defragmentation.
>>>>
>>>>        $ fio --filename=/btrfs/foo --size=5Gi --direct=1 --rw=randrw --bs=4k \
>>>>           --ioengine=libaio --iodepth=256 --runtime=120 --numjobs=4 \
>>>>           --time_based --group_reporting --name=iops-test-job --eta-newline=1
>>>>
>>>>
>>>> |         |            |            | Read I/O count  |
>>>> |         | Read       | Write      | devid1 | devid2 |
>>>> |---------|------------|------------|--------|--------|
>>>> | pid     | 20.3MiB/s  | 20.5MiB/s  | 313895 | 313895 |
>>>> | rotation|            |            |        |        |
>>>> |     4096| 20.4MiB/s  | 20.5MiB/s  | 313895 | 313895 |
>>>> |   196608| 20.2MiB/s  | 20.2MiB/s  | 310152 | 310175 |
>>>> |   262144| 20.3MiB/s  | 20.4MiB/s  | 312180 | 312191 |
>>>> |  latency| 18.4MiB/s  | 18.4MiB/s  | 272980 | 291683 |
>>>> | devid:1 | 14.8MiB/s  | 14.9MiB/s  | 456376 | 0      |
>>>>
>>>>      rotation RAID1 balancing technique performs more than 2x better for
>>>>      single-process defrag.
>>>>
>>>>         $ time -p btrfs filesystem defrag -r -f -c /btrfs
>>>>
>>>>
>>>> |         | Time  | Read I/O Count  |
>>>> |         | Real  | devid1 | devid2 |
>>>> |---------|-------|--------|--------|
>>>> | pid     | 18.00s| 3800   | 0      |
>>>> | rotation|       |        |        |
>>>> |     4096|  8.95s| 1900   | 1901   |
>>>> |   196608|  8.50s| 1881   | 1919   |
>>>> |   262144|  8.80s| 1881   | 1919   |
>>>> | latency | 17.18s| 3800   | 0      |
>>>> | devid:2 | 17.48s| 0      | 3800   |
>>>>
>>>> Rotation keeps all devices active, and for now, the Rotation RAID1
>>>> balancing method is preferable as default. More workload testing is
>>>> needed while the code is EXPERIMENTAL.
>>>
>>> Yeah round-robin will be a good defalt, we only need to verify the chunk
>>> size and then do the switch in the next release.
>>>
>>
>> Yes..
>>
>>>> While Latency is better during the failing/unstable block layer transport.
>>>> As of now these two techniques, are needed to be further independently
>>>> tested with different worloads, and in the long term we should be merge
>>>> these technique to a unified heuristic.
>>>
>>> This sounds like he latency is good for a specific case and maybe a
>>> fallback if the device becomes faulty, but once the layer below becomes
>>> unstable we may need to skip reading from the device. This is also a
>>> different mode of operation than balancing reads.
>>>
>>
>> If the latency on the faulty path is so high that it shouldn't pick that
>> path at all, so it works. However, the round-robin balancing is unaware
>> of dynamic faults on the device path. IMO, a round-robin method that is
>> latency aware (with ~20% variance) would be better.
> 
> We should not mix the faulty device handling mode to the read balancing,
> at least for now. A back off algorithm that checks number of failed io
> requests should precede the balancing.
> 
>>>> Rotation keeps all devices active, and for now, the Rotation RAID1
>>>> balancing method should be the default. More workload testing is needed
>>>> while the code is EXPERIMENTAL.
>>>>
>>>> Latency is smarter with unstable block layer transport.
>>>>
>>>> Both techniques need independent testing across workloads, with the goal of
>>>> eventually merging them into a unified approach? for the long term.
>>>>
>>>> Devid is a hands-on approach, provides manual or user-space script control.
>>>>
>>>> These RAID1 balancing methods are tunable via the sysfs knob.
>>>> The mount -o option and btrfs properties are under consideration.
>>>
>>> To move forward with the feature I think the round robin and preferred
>>> device id can be merged. I'm not sure about the latency but if it's
>>> under experimental we can take it as is and tune later.
>>
>> I hope the experimental feature also means we can change the name of the
>> balancing method at any time. Once we have tested a fair combination of
>> block device types, we'll definitely need a method that can
>> automatically tune based on the device type, which will require adding
>> or dropping balancing methods accordingly.
> 
> Yes we can change the names. The automatic tuning would need some
> feedback that measures the load and tries to improve the throughput,
> this is where we got stuck last time. So for now let's do some
> starightforward policy that on average works better than the current pid
> policy. I hope that tha round-robin-256k can be a good default, but of
> course we need more data for that.

Sending v3 with rotation renamed to round-robin. Code review
appreciated; I'll wait a day.

Thanks, Anand