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

Paolo Valente
2021-07-27T14:08:01Z

> 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