| Message ID | 20221103162623.10286-7-paolo.valente@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block, bfq: extend bfq to support multi-actuator drives | expand |
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 {