Re: [PATCH v3 1/4] block: Add concurrent positioning ranges support

[Paolo Valente <paolo.valente@linaro.org>]

> Il giorno 26 lug 2021, alle ore 13:33, Damien Le Moal <Damien.LeMoal@wdc.com> ha scritto:
> 
> On 2021/07/26 17:47, Hannes Reinecke wrote:
>> On 7/26/21 10:30 AM, Damien Le Moal wrote:
>>> On 2021/07/26 16:34, Hannes Reinecke wrote:
>> [ .. ]
>>>> In principle it looks good, but what would be the appropriate action
>>>> when invalid ranges are being detected during revalidation?
>>>> The current code will leave the original ones intact, but I guess that's
>>>> questionable as the current settings are most likely invalid.
>>> 
>>> Nope. In that case, the old ranges are removed. In blk_queue_set_cranges(),
>>> there is:
>>> 
>>> +		if (!blk_check_ranges(disk, cr)) {
>>> +			kfree(cr);
>>> +			cr = NULL;
>>> +			goto reg;
>>> +		}
>>> 
>>> So for incorrect ranges, we will register "NULL", so no ranges. The old ranges
>>> are gone.
>>> 
>> 
>> Right. So that's the first concern addressed.
> 
> Not that at the scsi layer, if there is an error retrieving the ranges
> informations, blk_queue_set_cranges(q, NULL) is called, so the same happen: the
> ranges set are removed and no range information will appear in sysfs.
> 

As a very personal opinion, silent failures are often misleading when
trying to understand what is going wrong in a system.  But I guess
this is however the best option.

Thanks,
Paolo

>> 
>>>> I would vote for de-register the old ones and implement an error state
>>>> (using an error pointer?); that would signal that there _are_ ranges,
>>>> but we couldn't parse them properly.
>>>> Hmm?
>>> 
>>> With the current code, the information "there are ranges" will be completely
>>> gone if the ranges are bad... dmesg will have a message about it, but that's it.
>>> 
>> So there will be no additional information in sysfs in case of incorrect 
>> ranges?
> 
> Yep, there will be no queue/cranges directory. The drive will be the same as a
> single actuator one.
> 
>> Hmm. Not sure if I like that, but then it might be the best option after 
>> all. So you can add my:
> 
> Nothing much that we can do. If we fail to retrieve the ranges, or the ranges
> are incorrect, access optimization by FS or scheduler is not really possible.
> Note that the drive will still work. Only any eventual optimization will be
> turned off.
> 
>> Reviewed-by: Hannes Reinecke <hare@suse.de>
> 
> Thanks !
> 
>> 
>> Cheers,
>> 
>> Hannes
>> 
> 
> 
> -- 
> Damien Le Moal
> Western Digital Research