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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2021/07/27 23:07, Paolo Valente wrote:
> 
> 
>> Il giorno 26 lug 2021, alle ore 13:33, Damien Le Moal <Damien.LeMoal@xxxxxxx> 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.

Failure are not silent: error messages are printed and will be visible in dmesg.

We can always completely ignore the drive and completely fail its initialization
in the case of a failed cranges initialization. But I find that rather extreme
since the drive is supposed to work anyway, even without any access optimization
for the multiple cranges case.

> 
> 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@xxxxxxx>
>>
>> Thanks !
>>
>>>
>>> Cheers,
>>>
>>> Hannes
>>>
>>
>>
>> -- 
>> Damien Le Moal
>> Western Digital Research
> 
> 


-- 
Damien Le Moal
Western Digital Research




[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Samba]     [Device Mapper]

  Powered by Linux