Message ID | 20221103162623.10286-7-paolo.valente@linaro.org |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:6687:0:0:0:0:0 with SMTP id l7csp634607wru; Thu, 3 Nov 2022 09:29:06 -0700 (PDT) X-Google-Smtp-Source: AMsMyM76/BOA2oipzpVFtWav55D2cGqHkIa1QiNvTFXVGutUmhqT7A08cmhl97kiio4iqGUUf4ib X-Received: by 2002:a17:907:1ddd:b0:7ad:8371:b59c with SMTP id og29-20020a1709071ddd00b007ad8371b59cmr29697853ejc.429.1667492945949; Thu, 03 Nov 2022 09:29:05 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1667492945; cv=none; d=google.com; s=arc-20160816; b=Jku2sEN+R9i0vr8ZXAifary5QMXPqnRcWWucJbJ8xKyd+HkKshwT6QI1DAuG+Zjicu E6/+Sxm7L/TBhVKIlXwR9it+cAh/zriS7qnTMip99P88FRQuTosiWeRzrqcjjSqnZpFD T8dRH3CM28YYRg9Wo1iokAXuo/td++h+Y0FVKkxcfPEwO9mY5Q0ymE18D/OyFoSNgPyc bJqMgNxRs76Sqgifdqwvz8xELPcsJacmQ7fdnH7CCMG67B3ukmYiZK7s7RXlbQPDNN6c BAaeqf1CsWeXv7J7rIwRJlvB4i4lFJ6kbzgKmFxFSBIqqpnhhuFQlDJkE9rX4Pn1YTms qteg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=B0bqMU9HIzOHFWGEi5tgrDC+AGs+9ggljEGUZXzxUOo=; b=jPjCWujWRJp3VnnxbzDLBSaCJ/EiGLW5rTtaPTRdeso6zemRjI34ExonoGg7WdITlO 5pHQ6+5F+pHB1mjmD7qbCGnzm/JLEPYStlV/yUGjw+kaje/RL48RYCDlFOLied2cbo5J ftN57TXLam1KV1k8S4j8O9kYYH9uJp30w7EGXXIHxHhXANkddAVmbRUeMgUPOy1Af1XI DhYew/8wzrGU6ZBjgtqKkclIFHRvF29TXWHzfsif08t369oRyoALidZtjPVgSFnfMZRR FXThMk9O1OkqEkOnqq5OBZL2vrMZ4RSGT+o2YRj9nTa2qJkIY4yrZDLtNh4i5B+3NWAx X3+Q== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@linaro.org header.s=google header.b=qUtvrLsl; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id y19-20020a056402441300b004615c5728e8si1909081eda.494.2022.11.03.09.28.42; Thu, 03 Nov 2022 09:29:05 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@linaro.org header.s=google header.b=qUtvrLsl; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231790AbiKCQ1H (ORCPT <rfc822;yves.mi.zy@gmail.com> + 99 others); Thu, 3 Nov 2022 12:27:07 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50838 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231570AbiKCQ0q (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 3 Nov 2022 12:26:46 -0400 Received: from mail-ed1-x52a.google.com (mail-ed1-x52a.google.com [IPv6:2a00:1450:4864:20::52a]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 68B7F1A83D for <linux-kernel@vger.kernel.org>; Thu, 3 Nov 2022 09:26:45 -0700 (PDT) Received: by mail-ed1-x52a.google.com with SMTP id a13so3938494edj.0 for <linux-kernel@vger.kernel.org>; Thu, 03 Nov 2022 09:26:45 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=B0bqMU9HIzOHFWGEi5tgrDC+AGs+9ggljEGUZXzxUOo=; b=qUtvrLsl23naT+PvbRMt3AkcDQvD3B9m9jT9QYBiWOc3bHmZkWL1FAbBf1ebKXww6u aldyZ6EZbcJKTFsNyMSwKkUDRzfHYF82RBx2pAmodfiB+qyKgtF8vpolYRfPvUtRdMmW 5285x1E+bfYaInkxT2ewRZVojLa3vrowrs90tcoaewCSjR5augzWnB5U0A8l4oqrj4Kf OGcXhsq7FlcKfXnpX5StPkhAAeteqKHwKsF80r4dDTu9UgFC/c0aLhFbcESCBlVZHT7e bFEcdi+Gjo40TdNiQYW32riXSu/EoqaX9fmA2zfjUY4i9dboRjHCyLfSB101cCXk7Dgi v2Fg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=B0bqMU9HIzOHFWGEi5tgrDC+AGs+9ggljEGUZXzxUOo=; b=mEESM7iDWm/5AYw72BMneAb94A0zunh7TnzbtBtdGEpva3R9ETuFkrnGU5ePkL3q9P 552IeS9oreRU2Q1ZbW/DK94LUh55YPUjjuUoZRQlc5xyYe7PLy4OpUgqjjIdzjp2Nixp +F53NYOr84bfUip2k/tTNCY5tiJpW1T5a0nKoJ23TBivcgOnd/OBHWPcNKoVvFvcveuv 6aAsy/dpr75q8fVh2n214PBiISgBhIsewVxCiMXouhjwSTdUE1AIHzSptorA6bwWHxxA jpZ87c7S3BsPF/t/UtvEuyGwka/CnmIMttCZ0xK2Iv+zrYC2H6JOqnpF0TIUDQJuGsiG oF4Q== X-Gm-Message-State: ACrzQf1XJ579XZ7tusLwTPKkelXGmhbLKEf6yySjNuGTn0bX1uN5vlcE FcH2RTUVAgHQOXExmO5qY06yoQ== X-Received: by 2002:aa7:d9d1:0:b0:461:9556:23e6 with SMTP id v17-20020aa7d9d1000000b00461955623e6mr32275265eds.25.1667492803932; Thu, 03 Nov 2022 09:26:43 -0700 (PDT) Received: from MBP-di-Paolo.station (net-2-35-55-161.cust.vodafonedsl.it. [2.35.55.161]) by smtp.gmail.com with ESMTPSA id kx9-20020a170907774900b0078116c361d9sm702507ejc.10.2022.11.03.09.26.42 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Thu, 03 Nov 2022 09:26:43 -0700 (PDT) From: Paolo Valente <paolo.valente@linaro.org> To: Jens Axboe <axboe@kernel.dk> Cc: linux-block@vger.kernel.org, linux-kernel@vger.kernel.org, arie.vanderhoeven@seagate.com, rory.c.chen@seagate.com, Federico Gavioli <f.gavioli97@gmail.com>, Paolo Valente <paolo.valente@linaro.org> Subject: [PATCH V6 6/8] block, bfq: retrieve independent access ranges from request queue Date: Thu, 3 Nov 2022 17:26:21 +0100 Message-Id: <20221103162623.10286-7-paolo.valente@linaro.org> X-Mailer: git-send-email 2.20.1 In-Reply-To: <20221103162623.10286-1-paolo.valente@linaro.org> References: <20221103162623.10286-1-paolo.valente@linaro.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE, SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1748493083442216345?= X-GMAIL-MSGID: =?utf-8?q?1748493083442216345?= |
Series |
block, bfq: extend bfq to support multi-actuator drives
|
|
Commit Message
Paolo Valente
Nov. 3, 2022, 4:26 p.m. UTC
From: Federico Gavioli <f.gavioli97@gmail.com> This patch implements the code to gather the content of the independent_access_ranges structure from the request_queue and copy it into the queue's bfq_data. This copy is done at queue initialization. We copy the access ranges into the bfq_data to avoid taking the queue lock each time we access the ranges. This implementation, however, puts a limit to the maximum independent ranges supported by the scheduler. Such a limit is equal to the constant BFQ_MAX_ACTUATORS. This limit was placed to avoid the allocation of dynamic memory. Co-developed-by: Rory Chen <rory.c.chen@seagate.com> Signed-off-by: Rory Chen <rory.c.chen@seagate.com> Signed-off-by: Federico Gavioli <f.gavioli97@gmail.com> Signed-off-by: Paolo Valente <paolo.valente@linaro.org> --- block/bfq-iosched.c | 54 ++++++++++++++++++++++++++++++++++++++------- block/bfq-iosched.h | 5 +++++ 2 files changed, 51 insertions(+), 8 deletions(-)
Comments
On 11/4/22 01:26, Paolo Valente wrote: > From: Federico Gavioli <f.gavioli97@gmail.com> > > This patch implements the code to gather the content of the > independent_access_ranges structure from the request_queue and copy > it into the queue's bfq_data. This copy is done at queue initialization. > > We copy the access ranges into the bfq_data to avoid taking the queue > lock each time we access the ranges. > > This implementation, however, puts a limit to the maximum independent > ranges supported by the scheduler. Such a limit is equal to the constant > BFQ_MAX_ACTUATORS. This limit was placed to avoid the allocation of > dynamic memory. > > Co-developed-by: Rory Chen <rory.c.chen@seagate.com> > Signed-off-by: Rory Chen <rory.c.chen@seagate.com> > Signed-off-by: Federico Gavioli <f.gavioli97@gmail.com> > Signed-off-by: Paolo Valente <paolo.valente@linaro.org> > --- > block/bfq-iosched.c | 54 ++++++++++++++++++++++++++++++++++++++------- > block/bfq-iosched.h | 5 +++++ > 2 files changed, 51 insertions(+), 8 deletions(-) > > diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c > index c94b80e3f685..106c8820cc5c 100644 > --- a/block/bfq-iosched.c > +++ b/block/bfq-iosched.c > @@ -1831,10 +1831,26 @@ static bool bfq_bfqq_higher_class_or_weight(struct bfq_queue *bfqq, > /* get the index of the actuator that will serve bio */ > static unsigned int bfq_actuator_index(struct bfq_data *bfqd, struct bio *bio) > { > - /* > - * Multi-actuator support not complete yet, so always return 0 > - * for the moment. > - */ > + struct blk_independent_access_range *iar; > + unsigned int i; > + sector_t end; > + > + /* no search needed if one or zero ranges present */ > + if (bfqd->num_actuators < 2) > + return 0; > + > + /* bio_end_sector(bio) gives the sector after the last one */ > + end = bio_end_sector(bio) - 1; > + > + for (i = 0; i < bfqd->num_actuators; i++) { > + iar = &(bfqd->ia_ranges[i]); > + if (end >= iar->sector && end < iar->sector + iar->nr_sectors) > + return i; > + } > + > + WARN_ONCE(true, > + "bfq_actuator_index: bio sector out of ranges: end=%llu\n", > + end); > return 0; > } > > @@ -2479,7 +2495,6 @@ static void bfq_remove_request(struct request_queue *q, > > if (rq->cmd_flags & REQ_META) > bfqq->meta_pending--; > - whiteline change > } > > static bool bfq_bio_merge(struct request_queue *q, struct bio *bio, > @@ -7144,6 +7159,8 @@ static int bfq_init_queue(struct request_queue *q, struct elevator_type *e) > { > struct bfq_data *bfqd; > struct elevator_queue *eq; > + unsigned int i; > + struct blk_independent_access_ranges *ia_ranges = q->disk->ia_ranges; > > eq = elevator_alloc(q, e); > if (!eq) > @@ -7187,10 +7204,31 @@ static int bfq_init_queue(struct request_queue *q, struct elevator_type *e) > bfqd->queue = q; > > /* > - * Multi-actuator support not complete yet, default to single > - * actuator for the moment. > + * If the disk supports multiple actuators, we copy the independent > + * access ranges from the request queue structure. > */ > - bfqd->num_actuators = 1; > + spin_lock_irq(&q->queue_lock); > + if (ia_ranges) { > + /* > + * Check if the disk ia_ranges size exceeds the current bfq > + * actuator limit. > + */ > + if (ia_ranges->nr_ia_ranges > BFQ_MAX_ACTUATORS) { > + pr_crit("nr_ia_ranges higher than act limit: iars=%d, max=%d.\n", > + ia_ranges->nr_ia_ranges, BFQ_MAX_ACTUATORS); > + pr_crit("Falling back to single actuator mode.\n"); > + bfqd->num_actuators = 0; > + } else { > + bfqd->num_actuators = ia_ranges->nr_ia_ranges; > + > + for (i = 0; i < bfqd->num_actuators; i++) > + bfqd->ia_ranges[i] = ia_ranges->ia_range[i]; > + } > + } else { > + bfqd->num_actuators = 0; That is very weird. The default should be 1 actuator. ia_ranges->nr_ia_ranges is 0 when the disk does not provide any range information, meaning it is a regular disk with a single actuator. > + } > + > + spin_unlock_irq(&q->queue_lock); > > INIT_LIST_HEAD(&bfqd->dispatch); > > diff --git a/block/bfq-iosched.h b/block/bfq-iosched.h > index f1c2e77cbf9a..90130a893c8f 100644 > --- a/block/bfq-iosched.h > +++ b/block/bfq-iosched.h > @@ -811,6 +811,11 @@ struct bfq_data { > */ > unsigned int num_actuators; > > + /* > + * Disk independent access ranges for each actuator > + * in this device. > + */ > + struct blk_independent_access_range ia_ranges[BFQ_MAX_ACTUATORS]; I fail to see how keeping this information is useful, especially given that this struct contains a kobj. Why not just copy the sector & nr_sectors fields into struct bfq_queue ? That would also work for the single actuator case as then sector will be 0 and nr_sectors will be the disk total capacity. I think this patch should be first in the series. That will avoid having the empty bfq_actuator_index() function. > }; > > enum bfqq_state_flags {
> Il giorno 21 nov 2022, alle ore 02:01, Damien Le Moal <damien.lemoal@opensource.wdc.com> ha scritto: > ... > >> } >> >> static bool bfq_bio_merge(struct request_queue *q, struct bio *bio, >> @@ -7144,6 +7159,8 @@ static int bfq_init_queue(struct request_queue *q, struct elevator_type *e) >> { >> struct bfq_data *bfqd; >> struct elevator_queue *eq; >> + unsigned int i; >> + struct blk_independent_access_ranges *ia_ranges = q->disk->ia_ranges; >> >> eq = elevator_alloc(q, e); >> if (!eq) >> @@ -7187,10 +7204,31 @@ static int bfq_init_queue(struct request_queue *q, struct elevator_type *e) >> bfqd->queue = q; >> >> /* >> - * Multi-actuator support not complete yet, default to single >> - * actuator for the moment. >> + * If the disk supports multiple actuators, we copy the independent >> + * access ranges from the request queue structure. >> */ >> - bfqd->num_actuators = 1; >> + spin_lock_irq(&q->queue_lock); >> + if (ia_ranges) { >> + /* >> + * Check if the disk ia_ranges size exceeds the current bfq >> + * actuator limit. >> + */ >> + if (ia_ranges->nr_ia_ranges > BFQ_MAX_ACTUATORS) { >> + pr_crit("nr_ia_ranges higher than act limit: iars=%d, max=%d.\n", >> + ia_ranges->nr_ia_ranges, BFQ_MAX_ACTUATORS); >> + pr_crit("Falling back to single actuator mode.\n"); >> + bfqd->num_actuators = 0; >> + } else { >> + bfqd->num_actuators = ia_ranges->nr_ia_ranges; >> + >> + for (i = 0; i < bfqd->num_actuators; i++) >> + bfqd->ia_ranges[i] = ia_ranges->ia_range[i]; >> + } >> + } else { >> + bfqd->num_actuators = 0; > > That is very weird. The default should be 1 actuator. > ia_ranges->nr_ia_ranges is 0 when the disk does not provide any range > information, meaning it is a regular disk with a single actuator. Actually, IIUC this assignment to 0 seems to be done exactly when you say that it should be done, i.e., when the disk does not provide any range information (ia_ranges is NULL). Am I missing something else? Once again, all other suggestions applied. I'm about to submit a V7. Thanks, Paolo
On 12/6/22 17:06, Paolo Valente wrote: > > >> Il giorno 21 nov 2022, alle ore 02:01, Damien Le Moal <damien.lemoal@opensource.wdc.com> ha scritto: >> > > ... > >> >>> } >>> >>> static bool bfq_bio_merge(struct request_queue *q, struct bio *bio, >>> @@ -7144,6 +7159,8 @@ static int bfq_init_queue(struct request_queue *q, struct elevator_type *e) >>> { >>> struct bfq_data *bfqd; >>> struct elevator_queue *eq; >>> + unsigned int i; >>> + struct blk_independent_access_ranges *ia_ranges = q->disk->ia_ranges; >>> >>> eq = elevator_alloc(q, e); >>> if (!eq) >>> @@ -7187,10 +7204,31 @@ static int bfq_init_queue(struct request_queue *q, struct elevator_type *e) >>> bfqd->queue = q; >>> >>> /* >>> - * Multi-actuator support not complete yet, default to single >>> - * actuator for the moment. >>> + * If the disk supports multiple actuators, we copy the independent >>> + * access ranges from the request queue structure. >>> */ >>> - bfqd->num_actuators = 1; >>> + spin_lock_irq(&q->queue_lock); >>> + if (ia_ranges) { >>> + /* >>> + * Check if the disk ia_ranges size exceeds the current bfq >>> + * actuator limit. >>> + */ >>> + if (ia_ranges->nr_ia_ranges > BFQ_MAX_ACTUATORS) { >>> + pr_crit("nr_ia_ranges higher than act limit: iars=%d, max=%d.\n", >>> + ia_ranges->nr_ia_ranges, BFQ_MAX_ACTUATORS); >>> + pr_crit("Falling back to single actuator mode.\n"); >>> + bfqd->num_actuators = 0; >>> + } else { >>> + bfqd->num_actuators = ia_ranges->nr_ia_ranges; >>> + >>> + for (i = 0; i < bfqd->num_actuators; i++) >>> + bfqd->ia_ranges[i] = ia_ranges->ia_range[i]; >>> + } >>> + } else { >>> + bfqd->num_actuators = 0; >> >> That is very weird. The default should be 1 actuator. >> ia_ranges->nr_ia_ranges is 0 when the disk does not provide any range >> information, meaning it is a regular disk with a single actuator. > > Actually, IIUC this assignment to 0 seems to be done exactly when you > say that it should be done, i.e., when the disk does not provide any > range information (ia_ranges is NULL). Am I missing something else? No ranges reported means no extra actuators, so a single actuator an single LBA range for the entire device. In that case, bfq should process all IOs using bfqd->ia_ranges[0]. The get range function will always return that range. That makes the code clean and avoids different path for nr_ranges == 1 and nr_ranges > 1. No ? > > Once again, all other suggestions applied. I'm about to submit a V7. > > Thanks, > Paolo >
> Il giorno 6 dic 2022, alle ore 09:29, Damien Le Moal <damien.lemoal@opensource.wdc.com> ha scritto: > > On 12/6/22 17:06, Paolo Valente wrote: >> >> >>> Il giorno 21 nov 2022, alle ore 02:01, Damien Le Moal <damien.lemoal@opensource.wdc.com> ha scritto: >>> >> >> ... >> >>> >>>> } >>>> >>>> static bool bfq_bio_merge(struct request_queue *q, struct bio *bio, >>>> @@ -7144,6 +7159,8 @@ static int bfq_init_queue(struct request_queue *q, struct elevator_type *e) >>>> { >>>> struct bfq_data *bfqd; >>>> struct elevator_queue *eq; >>>> + unsigned int i; >>>> + struct blk_independent_access_ranges *ia_ranges = q->disk->ia_ranges; >>>> >>>> eq = elevator_alloc(q, e); >>>> if (!eq) >>>> @@ -7187,10 +7204,31 @@ static int bfq_init_queue(struct request_queue *q, struct elevator_type *e) >>>> bfqd->queue = q; >>>> >>>> /* >>>> - * Multi-actuator support not complete yet, default to single >>>> - * actuator for the moment. >>>> + * If the disk supports multiple actuators, we copy the independent >>>> + * access ranges from the request queue structure. >>>> */ >>>> - bfqd->num_actuators = 1; >>>> + spin_lock_irq(&q->queue_lock); >>>> + if (ia_ranges) { >>>> + /* >>>> + * Check if the disk ia_ranges size exceeds the current bfq >>>> + * actuator limit. >>>> + */ >>>> + if (ia_ranges->nr_ia_ranges > BFQ_MAX_ACTUATORS) { >>>> + pr_crit("nr_ia_ranges higher than act limit: iars=%d, max=%d.\n", >>>> + ia_ranges->nr_ia_ranges, BFQ_MAX_ACTUATORS); >>>> + pr_crit("Falling back to single actuator mode.\n"); >>>> + bfqd->num_actuators = 0; >>>> + } else { >>>> + bfqd->num_actuators = ia_ranges->nr_ia_ranges; >>>> + >>>> + for (i = 0; i < bfqd->num_actuators; i++) >>>> + bfqd->ia_ranges[i] = ia_ranges->ia_range[i]; >>>> + } >>>> + } else { >>>> + bfqd->num_actuators = 0; >>> >>> That is very weird. The default should be 1 actuator. >>> ia_ranges->nr_ia_ranges is 0 when the disk does not provide any range >>> information, meaning it is a regular disk with a single actuator. >> >> Actually, IIUC this assignment to 0 seems to be done exactly when you >> say that it should be done, i.e., when the disk does not provide any >> range information (ia_ranges is NULL). Am I missing something else? > > No ranges reported means no extra actuators, so a single actuator an > single LBA range for the entire device. I'm still confused, sorry. Where will I read sector ranges from, if no sector range information is available (ia_ranges is NULL)? > In that case, bfq should process > all IOs using bfqd->ia_ranges[0]. The get range function will always > return that range. That makes the code clean and avoids different path for > nr_ranges == 1 and nr_ranges > 1. No ? Apart from the above point, for which maybe there is some other source of information for getting ranges, I see the following issue. What you propose is to save sector information and trigger the range-checking for loop also for the above single-actuator case. Yet txecuting (one iteration of) that loop will will always result in getting a 0 as index. So, what's the point is saving data and executing code on each IO, for getting a static result that we already know we will get? Thanks, Paolo > >> >> Once again, all other suggestions applied. I'm about to submit a V7. >> >> Thanks, >> Paolo >> > > -- > Damien Le Moal > Western Digital Research
On 12/6/22 17:41, Paolo Valente wrote: > > >> Il giorno 6 dic 2022, alle ore 09:29, Damien Le Moal <damien.lemoal@opensource.wdc.com> ha scritto: >> >> On 12/6/22 17:06, Paolo Valente wrote: >>> >>> >>>> Il giorno 21 nov 2022, alle ore 02:01, Damien Le Moal <damien.lemoal@opensource.wdc.com> ha scritto: >>>> >>> >>> ... >>> >>>> >>>>> } >>>>> >>>>> static bool bfq_bio_merge(struct request_queue *q, struct bio *bio, >>>>> @@ -7144,6 +7159,8 @@ static int bfq_init_queue(struct request_queue *q, struct elevator_type *e) >>>>> { >>>>> struct bfq_data *bfqd; >>>>> struct elevator_queue *eq; >>>>> + unsigned int i; >>>>> + struct blk_independent_access_ranges *ia_ranges = q->disk->ia_ranges; >>>>> >>>>> eq = elevator_alloc(q, e); >>>>> if (!eq) >>>>> @@ -7187,10 +7204,31 @@ static int bfq_init_queue(struct request_queue *q, struct elevator_type *e) >>>>> bfqd->queue = q; >>>>> >>>>> /* >>>>> - * Multi-actuator support not complete yet, default to single >>>>> - * actuator for the moment. >>>>> + * If the disk supports multiple actuators, we copy the independent >>>>> + * access ranges from the request queue structure. >>>>> */ >>>>> - bfqd->num_actuators = 1; >>>>> + spin_lock_irq(&q->queue_lock); >>>>> + if (ia_ranges) { >>>>> + /* >>>>> + * Check if the disk ia_ranges size exceeds the current bfq >>>>> + * actuator limit. >>>>> + */ >>>>> + if (ia_ranges->nr_ia_ranges > BFQ_MAX_ACTUATORS) { >>>>> + pr_crit("nr_ia_ranges higher than act limit: iars=%d, max=%d.\n", >>>>> + ia_ranges->nr_ia_ranges, BFQ_MAX_ACTUATORS); >>>>> + pr_crit("Falling back to single actuator mode.\n"); >>>>> + bfqd->num_actuators = 0; >>>>> + } else { >>>>> + bfqd->num_actuators = ia_ranges->nr_ia_ranges; >>>>> + >>>>> + for (i = 0; i < bfqd->num_actuators; i++) >>>>> + bfqd->ia_ranges[i] = ia_ranges->ia_range[i]; >>>>> + } >>>>> + } else { >>>>> + bfqd->num_actuators = 0; >>>> >>>> That is very weird. The default should be 1 actuator. >>>> ia_ranges->nr_ia_ranges is 0 when the disk does not provide any range >>>> information, meaning it is a regular disk with a single actuator. >>> >>> Actually, IIUC this assignment to 0 seems to be done exactly when you >>> say that it should be done, i.e., when the disk does not provide any >>> range information (ia_ranges is NULL). Am I missing something else? >> >> No ranges reported means no extra actuators, so a single actuator an >> single LBA range for the entire device. > > I'm still confused, sorry. Where will I read sector ranges from, if > no sector range information is available (ia_ranges is NULL)? start = 0 and nr_sectors = bdev_nr_sectors(bdev). No ia_ranges to read. > >> In that case, bfq should process >> all IOs using bfqd->ia_ranges[0]. The get range function will always >> return that range. That makes the code clean and avoids different path for >> nr_ranges == 1 and nr_ranges > 1. No ? > > Apart from the above point, for which maybe there is some other > source of information for getting ranges, I see the following issue. > > What you propose is to save sector information and trigger the > range-checking for loop also for the above single-actuator case. Yet > txecuting (one iteration of) that loop will will always result in > getting a 0 as index. So, what's the point is saving data and > executing code on each IO, for getting a static result that we already > know we will get? Surely, you can add an "if (bfqd->num_actuators ==1)" optimization in strategic places to optimize for regular devices with a single actuator, which bfqd->num_actuators == 1 *exactly* describes. Having "bfqd->num_actuators = 0" makes no sense to me. But if you feel strongly about this, feel free to ignore this. > > Thanks, > Paolo > >> >>> >>> Once again, all other suggestions applied. I'm about to submit a V7. >>> >>> Thanks, >>> Paolo >>> >> >> -- >> Damien Le Moal >> Western Digital Research >
> Il giorno 6 dic 2022, alle ore 10:02, Damien Le Moal <damien.lemoal@opensource.wdc.com> ha scritto: > > On 12/6/22 17:41, Paolo Valente wrote: >> >> >>> Il giorno 6 dic 2022, alle ore 09:29, Damien Le Moal <damien.lemoal@opensource.wdc.com> ha scritto: >>> >>> On 12/6/22 17:06, Paolo Valente wrote: >>>> >>>> >>>>> Il giorno 21 nov 2022, alle ore 02:01, Damien Le Moal <damien.lemoal@opensource.wdc.com> ha scritto: >>>>> >>>> >>>> ... >>>> >>>>> >>>>>> } >>>>>> >>>>>> static bool bfq_bio_merge(struct request_queue *q, struct bio *bio, >>>>>> @@ -7144,6 +7159,8 @@ static int bfq_init_queue(struct request_queue *q, struct elevator_type *e) >>>>>> { >>>>>> struct bfq_data *bfqd; >>>>>> struct elevator_queue *eq; >>>>>> + unsigned int i; >>>>>> + struct blk_independent_access_ranges *ia_ranges = q->disk->ia_ranges; >>>>>> >>>>>> eq = elevator_alloc(q, e); >>>>>> if (!eq) >>>>>> @@ -7187,10 +7204,31 @@ static int bfq_init_queue(struct request_queue *q, struct elevator_type *e) >>>>>> bfqd->queue = q; >>>>>> >>>>>> /* >>>>>> - * Multi-actuator support not complete yet, default to single >>>>>> - * actuator for the moment. >>>>>> + * If the disk supports multiple actuators, we copy the independent >>>>>> + * access ranges from the request queue structure. >>>>>> */ >>>>>> - bfqd->num_actuators = 1; >>>>>> + spin_lock_irq(&q->queue_lock); >>>>>> + if (ia_ranges) { >>>>>> + /* >>>>>> + * Check if the disk ia_ranges size exceeds the current bfq >>>>>> + * actuator limit. >>>>>> + */ >>>>>> + if (ia_ranges->nr_ia_ranges > BFQ_MAX_ACTUATORS) { >>>>>> + pr_crit("nr_ia_ranges higher than act limit: iars=%d, max=%d.\n", >>>>>> + ia_ranges->nr_ia_ranges, BFQ_MAX_ACTUATORS); >>>>>> + pr_crit("Falling back to single actuator mode.\n"); >>>>>> + bfqd->num_actuators = 0; >>>>>> + } else { >>>>>> + bfqd->num_actuators = ia_ranges->nr_ia_ranges; >>>>>> + >>>>>> + for (i = 0; i < bfqd->num_actuators; i++) >>>>>> + bfqd->ia_ranges[i] = ia_ranges->ia_range[i]; >>>>>> + } >>>>>> + } else { >>>>>> + bfqd->num_actuators = 0; >>>>> >>>>> That is very weird. The default should be 1 actuator. >>>>> ia_ranges->nr_ia_ranges is 0 when the disk does not provide any range >>>>> information, meaning it is a regular disk with a single actuator. >>>> >>>> Actually, IIUC this assignment to 0 seems to be done exactly when you >>>> say that it should be done, i.e., when the disk does not provide any >>>> range information (ia_ranges is NULL). Am I missing something else? >>> >>> No ranges reported means no extra actuators, so a single actuator an >>> single LBA range for the entire device. >> >> I'm still confused, sorry. Where will I read sector ranges from, if >> no sector range information is available (ia_ranges is NULL)? > > start = 0 and nr_sectors = bdev_nr_sectors(bdev). > No ia_ranges to read. > ok, thanks >> >>> In that case, bfq should process >>> all IOs using bfqd->ia_ranges[0]. The get range function will always >>> return that range. That makes the code clean and avoids different path for >>> nr_ranges == 1 and nr_ranges > 1. No ? >> >> Apart from the above point, for which maybe there is some other >> source of information for getting ranges, I see the following issue. >> >> What you propose is to save sector information and trigger the >> range-checking for loop also for the above single-actuator case. Yet >> txecuting (one iteration of) that loop will will always result in >> getting a 0 as index. So, what's the point is saving data and >> executing code on each IO, for getting a static result that we already >> know we will get? > > Surely, you can add an "if (bfqd->num_actuators ==1)" optimization in > strategic places to optimize for regular devices with a single actuator, > which bfqd->num_actuators == 1 *exactly* describes. Having > "bfqd->num_actuators = 0" makes no sense to me. > Ok, I see your point at last, sorry. I'll check the code, but I think that there is no problem in moving from 0 to 1 actuators for the case ia_ranges == NULL. I meant to separate the case "single actuator with ia_ranges available" (num_actuators = 1), from the case "no ia_ranges available" (num_actuators = 0). But evidently things don't work as I thought, and using the same value (1) is ok. Just, let me avoid setting the fields bfqd->sector and bfqd->nr_sectors for a case where we don't use them. Thanks, Paolo > But if you feel strongly about this, feel free to ignore this. > >> >> Thanks, >> Paolo >> >>> >>>> >>>> Once again, all other suggestions applied. I'm about to submit a V7. >>>> >>>> Thanks, >>>> Paolo >>>> >>> >>> -- >>> Damien Le Moal >>> Western Digital Research >> > > -- > Damien Le Moal > Western Digital Research
On 12/7/22 00:43, Paolo Valente wrote: >>>> In that case, bfq should process >>>> all IOs using bfqd->ia_ranges[0]. The get range function will always >>>> return that range. That makes the code clean and avoids different path for >>>> nr_ranges == 1 and nr_ranges > 1. No ? >>> >>> Apart from the above point, for which maybe there is some other >>> source of information for getting ranges, I see the following issue. >>> >>> What you propose is to save sector information and trigger the >>> range-checking for loop also for the above single-actuator case. Yet >>> txecuting (one iteration of) that loop will will always result in >>> getting a 0 as index. So, what's the point is saving data and >>> executing code on each IO, for getting a static result that we already >>> know we will get? >> >> Surely, you can add an "if (bfqd->num_actuators ==1)" optimization in >> strategic places to optimize for regular devices with a single actuator, >> which bfqd->num_actuators == 1 *exactly* describes. Having >> "bfqd->num_actuators = 0" makes no sense to me. >> > > Ok, I see your point at last, sorry. I'll check the code, but I think > that there is no problem in moving from 0 to 1 actuators for the case > ia_ranges == NULL. I meant to separate the case "single actuator with > ia_ranges available" (num_actuators = 1), from the case "no ia_ranges > available" (num_actuators = 0). But evidently things don't work as I > thought, and using the same value (1) is ok. Any HDD always has at least 1 actuator. Per SCSI & ATA specs, ia_range will be present only and only if the device has *more than one actuator*. So the case "no ia_ranges available" means "num_actuator = 1" and the implied access range is the entire device capacity. > Just, let me avoid setting the fields bfqd->sector and > bfqd->nr_sectors for a case where we don't use them. Sure. But if you do not use them thanks to "if (num_actuators == 1)" optimizations, it would still not hurt to set these fields. That actually could be helpful for debugging. Overall, I think that the code should not differ much for the num_actuators == 1 case. Any actuator search loop would simply hit the first and only entry on the first iteration, so should not add any nasty overhead. Such single code path would likely greatly simplify the code.
> Il giorno 7 dic 2022, alle ore 00:34, Damien Le Moal <damien.lemoal@opensource.wdc.com> ha scritto: > > [...] >> Just, let me avoid setting the fields bfqd->sector and >> bfqd->nr_sectors for a case where we don't use them. > > Sure. But if you do not use them thanks to "if (num_actuators == 1)" > optimizations, it would still not hurt to set these fields. That actually > could be helpful for debugging. > Got it. I'm about to send a V9 that applies this last suggestion of yours. Thanks, Paolo
diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c index c94b80e3f685..106c8820cc5c 100644 --- a/block/bfq-iosched.c +++ b/block/bfq-iosched.c @@ -1831,10 +1831,26 @@ static bool bfq_bfqq_higher_class_or_weight(struct bfq_queue *bfqq, /* get the index of the actuator that will serve bio */ static unsigned int bfq_actuator_index(struct bfq_data *bfqd, struct bio *bio) { - /* - * Multi-actuator support not complete yet, so always return 0 - * for the moment. - */ + struct blk_independent_access_range *iar; + unsigned int i; + sector_t end; + + /* no search needed if one or zero ranges present */ + if (bfqd->num_actuators < 2) + return 0; + + /* bio_end_sector(bio) gives the sector after the last one */ + end = bio_end_sector(bio) - 1; + + for (i = 0; i < bfqd->num_actuators; i++) { + iar = &(bfqd->ia_ranges[i]); + if (end >= iar->sector && end < iar->sector + iar->nr_sectors) + return i; + } + + WARN_ONCE(true, + "bfq_actuator_index: bio sector out of ranges: end=%llu\n", + end); return 0; } @@ -2479,7 +2495,6 @@ static void bfq_remove_request(struct request_queue *q, if (rq->cmd_flags & REQ_META) bfqq->meta_pending--; - } static bool bfq_bio_merge(struct request_queue *q, struct bio *bio, @@ -7144,6 +7159,8 @@ static int bfq_init_queue(struct request_queue *q, struct elevator_type *e) { struct bfq_data *bfqd; struct elevator_queue *eq; + unsigned int i; + struct blk_independent_access_ranges *ia_ranges = q->disk->ia_ranges; eq = elevator_alloc(q, e); if (!eq) @@ -7187,10 +7204,31 @@ static int bfq_init_queue(struct request_queue *q, struct elevator_type *e) bfqd->queue = q; /* - * Multi-actuator support not complete yet, default to single - * actuator for the moment. + * If the disk supports multiple actuators, we copy the independent + * access ranges from the request queue structure. */ - bfqd->num_actuators = 1; + spin_lock_irq(&q->queue_lock); + if (ia_ranges) { + /* + * Check if the disk ia_ranges size exceeds the current bfq + * actuator limit. + */ + if (ia_ranges->nr_ia_ranges > BFQ_MAX_ACTUATORS) { + pr_crit("nr_ia_ranges higher than act limit: iars=%d, max=%d.\n", + ia_ranges->nr_ia_ranges, BFQ_MAX_ACTUATORS); + pr_crit("Falling back to single actuator mode.\n"); + bfqd->num_actuators = 0; + } else { + bfqd->num_actuators = ia_ranges->nr_ia_ranges; + + for (i = 0; i < bfqd->num_actuators; i++) + bfqd->ia_ranges[i] = ia_ranges->ia_range[i]; + } + } else { + bfqd->num_actuators = 0; + } + + spin_unlock_irq(&q->queue_lock); INIT_LIST_HEAD(&bfqd->dispatch); diff --git a/block/bfq-iosched.h b/block/bfq-iosched.h index f1c2e77cbf9a..90130a893c8f 100644 --- a/block/bfq-iosched.h +++ b/block/bfq-iosched.h @@ -811,6 +811,11 @@ struct bfq_data { */ unsigned int num_actuators; + /* + * Disk independent access ranges for each actuator + * in this device. + */ + struct blk_independent_access_range ia_ranges[BFQ_MAX_ACTUATORS]; }; enum bfqq_state_flags {