Re: [PATCH V6 6/8] block, bfq: retrieve independent access ranges from request queue

From: Damien Le Moal <damien.lemoal@opensource.wdc.com>
To: Paolo Valente <paolo.valente@linaro.org>
Cc: Jens Axboe <axboe@kernel.dk>,
	linux-block <linux-block@vger.kernel.org>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	Arie van der Hoeven <arie.vanderhoeven@seagate.com>,
	Rory Chen <rory.c.chen@seagate.com>,
	Federico Gavioli <f.gavioli97@gmail.com>
Subject: Re: [PATCH V6 6/8] block, bfq: retrieve independent access ranges from request queue
Date: Wed, 7 Dec 2022 08:34:09 +0900	[thread overview]
Message-ID: <9eba7529-8879-fbba-4e17-f174ef401513@opensource.wdc.com> (raw)
In-Reply-To: <518C279B-8896-470A-9D8C-974F3BB886DB@linaro.org>

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.

-- 
Damien Le Moal
Western Digital Research