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

Hannes Reinecke
2021-07-26T08:47:41Z

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.

>> 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?

Hmm. Not sure if I like that, but then it might be the best option after
all. So you can add my:

Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer