Re: [PATCH RFC 6/6] btrfs: Add roundrobin raid1 read policy

[Michal Rostecki]

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