From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-10.2 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,MENTIONS_GIT_HOSTING, SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 88B8FC433DB for ; Fri, 12 Feb 2021 17:14:17 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 46F0A64E42 for ; Fri, 12 Feb 2021 17:14:17 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231233AbhBLRNy (ORCPT ); Fri, 12 Feb 2021 12:13:54 -0500 Received: from mx2.suse.de ([195.135.220.15]:41236 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229451AbhBLRNv (ORCPT ); Fri, 12 Feb 2021 12:13:51 -0500 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id 261AAAD29; Fri, 12 Feb 2021 17:13:09 +0000 (UTC) Date: Fri, 12 Feb 2021 17:12:58 +0000 From: Michal Rostecki To: Anand Jain Cc: Chris Mason , Josef Bacik , David Sterba , "open list:BTRFS FILE SYSTEM" , open list , Michal Rostecki Subject: Re: [PATCH RFC 6/6] btrfs: Add roundrobin raid1 read policy Message-ID: <20210212171246.GA20817@wotan.suse.de> References: <20210209203041.21493-1-mrostecki@suse.de> <20210209203041.21493-7-mrostecki@suse.de> <20210211155533.GB1263@wotan.suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210211155533.GB1263@wotan.suse.de> User-Agent: Mutt/1.10.1 (2018-07-13) Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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