On 2020/06/02 21:50, Coly Li wrote:
>>> /* convert back to LBA of the bcache device*/
>>> zone->start -= data_offset;
>>> if (zone->cond != BLK_ZONE_COND_NOT_WP)
>>> zone->wp -= data_offset;
>>
>> Since you have the BLK_ZONE_COND_NOT_WP condition in the switch-case,
>> this is not needed.
>>
>>>
>>> switch (zone->cond) {
>>> case BLK_ZONE_COND_NOT_WP:
>>> /* No write pointer available */
>>> break;
>>> case BLK_ZONE_COND_EMPTY:
>>
>> zone->wp = zone->start;
>>
>
> Correct me if I am wrong. I assume when the zone is in empty condition,
> LBA of zone->wp should be exactly equal to zone->start before bcache
> does the conversion. Therefore 'zone->wp - data_offset' should still be
> equal to 'zone->start - data_offset'. Therefore explicitly handle it in
> 'case BLK_ZONE_COND_EMPTY:' should be equal to handle it in 'default:' part.
Yes, for an empty zone, the zone wp is always equal to the start sector of the
zone. So yes, for an empty zone, wp - data_offset is correct. But it is not
necessary since you already need to do zone->start -= data_offset, all you need
to do for an empty zone is: zone->wp = zone->start. That is less arithmetic (no
need for the subtraction), so is faster :)
This code you have:
if (zone->cond != BLK_ZONE_COND_NOT_WP)
zone->wp -= data_offset;
will indeed lead to the correct result for an empty zone, but as I explained, it
will also do the subtraction for zone conditions with an undefined value. No
need to do all this. A switch case handling all conditions makes things very clear.
--
Damien Le Moal
Western Digital Research
