Messages in this thread | | | Date | Fri, 12 Feb 2021 17:12:58 +0000 | From | Michal Rostecki <> | Subject | Re: [PATCH RFC 6/6] btrfs: Add roundrobin raid1 read policy |
| |
Hi Anand,
re: inflight calculation
On Thu, Feb 11, 2021 at 03:55:33PM +0000, Michal Rostecki wrote: > > It is better to have random workloads in the above three categories > > of configs. > > > > Apart from the above three configs, there is also > > all-non-rotational with hetero > > For example, ssd and nvme together both are non-rotational. > > And, > > all-rotational with hetero > > For example, rotational disks with different speeds. > > > > > > The inflight calculation is local to btrfs. If the device is busy due to > > external factors, it would not switch to the better performing device. > > > > Good point. Maybe I should try to use the part stats instead of storing > inflight locally in btrfs.
I tried today to reuse the inflight calculation which is done for iostat. And I came to conclusion that it's impossible without exporting some methods from the block/ subsystem.
The thing is that there are two methods of calculating inflight. Which one of them is used, depends on queue_is_mq():
https://github.com/kdave/btrfs-devel/blob/9d294a685fbcb256ce8c5f7fd88a7596d0f52a8a/block/genhd.c#L1163
And if that condition is true, I noticed that part_stats return 0, even though there are processed requests (I checked with fio inside VM).
In general, those two methods of checking inflight are:
1) part_stats - which would be trivial to reuse, just a matter of one simple for_each_possible_cpu() loop with part_stat_local_read_cpu()
https://github.com/kdave/btrfs-devel/blob/9d294a685fbcb256ce8c5f7fd88a7596d0f52a8a/block/genhd.c#L133-L146
2) blk_mq_in_flight() - which has a lot of code and unexported structs inside the block/ directory, double function callback; definitely not easy to reimplement easily in btrfs without copying dozens of lines
https://github.com/kdave/btrfs-devel/blob/9d294a685fbcb256ce8c5f7fd88a7596d0f52a8a/block/blk-mq.c#L115-L123
Well, I tried copying the whole blk_mq_in_flight() function with all dependencies anyway, hard to do without causing modpost errors.
So, to sum it up, I think that making 2) possible to reuse in btrfs would require at lest exporting the blk_mq_in_flight() function, therefore the change would have to go through linux-block tree. Which maybe would be a good thing to do in long term, not sure if it should block my patchset entirely.
The question is if we are fine with inflight stats inside btrfs. Personally I think we sholdn't be concerned too much about it. The inflight counter in my patches is a percpu counted, used in places which already do some atomic operations.
Thanks, Michal
| |