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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210212171246.GA20817@wotan.suse.de>
Date:   Fri, 12 Feb 2021 17:12:58 +0000
From:   Michal Rostecki <mrostecki@...e.de>
To:     Anand Jain <anand.jain@...cle.com>
Cc:     Chris Mason <clm@...com>, Josef Bacik <josef@...icpanda.com>,
        David Sterba <dsterba@...e.com>,
        "open list:BTRFS FILE SYSTEM" <linux-btrfs@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>,
        Michal Rostecki <mrostecki@...e.com>
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