Re: [PATCH 1/2] scsi: sd_zbc: Compare against block layer enum values

[Damien Le Moal <damien.lemoal@opensource.wdc.com>]

On 2021/11/27 18:58, Niklas Cassel wrote:
> On Sat, Nov 27, 2021 at 10:00:57AM +0900, Damien Le Moal wrote:
>> On 2021/11/26 21:55, Niklas Cassel wrote:
>>> From: Niklas Cassel <niklas.cassel@wdc.com>
>>>
>>> sd_zbc_parse_report() fills in a struct blk_zone, which is the block layer
>>> representation of a zone. This struct is also what will be copied to user
>>> for a BLKREPORTZONE ioctl.
>>>
>>> Since sd_zbc_parse_report() compares against zone.type and zone.cond, which
>>> are members of a struct blk_zone, the correct enum values to compare
>>> against are the enum values defined by the block layer.
>>>
>>> These specific enum values for ZBC and the block layer happen to have the
>>> same enum constants, but they could theoretically have been different.
>>>
>>> Compare against the block layer enum values, to make it more obvious that
>>> struct blk_zone is the block layer representation of a zone, and not the
>>> SCSI/ZBC representation of a zone.
>>>
>>> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
>>> ---
>>>  drivers/scsi/sd_zbc.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c
>>> index ed06798983f8..024f1bec6e5a 100644
>>> --- a/drivers/scsi/sd_zbc.c
>>> +++ b/drivers/scsi/sd_zbc.c
>>> @@ -62,8 +62,8 @@ static int sd_zbc_parse_report(struct scsi_disk *sdkp, u8 *buf,
>>>  	zone.capacity = zone.len;
>>>  	zone.start = logical_to_sectors(sdp, get_unaligned_be64(&buf[16]));
>>>  	zone.wp = logical_to_sectors(sdp, get_unaligned_be64(&buf[24]));
>>> -	if (zone.type != ZBC_ZONE_TYPE_CONV &&
>>> -	    zone.cond == ZBC_ZONE_COND_FULL)
>>> +	if (zone.type != BLK_ZONE_TYPE_CONVENTIONAL &&
>>> +	    zone.cond == BLK_ZONE_COND_FULL)
>>>  		zone.wp = zone.start + zone.len;
>>
>> For the sake of avoiding layering violation, I would keep the code as is, unles
>> Martin and James are OK with this ?
> 
> Sorry, but I don't understand this comment.
> 
> The whole point of sd_zbc_parse_report() is to take a ZBC zone representation,
> stored in u8 *buf, and to convert it to a struct blk_zone used by the block
> layer.

Yes. So what is the problem with using the scsi_proto.h defined ZBC_ZONE_*
macros ? We are deep in scsi territory with this code, so using an UAPI defined
macro is weird.

> Similarly, nvme_zone_parse_entry() takes a ZNS zone representation, stored in a
> struct nvme_zone_descriptor *entry, and to convert it to a struct blk_zone.
> 
> 
> When comparing against struct members inside entry, the NVMe enums have to be
> used, i.e. NVME_ZONE_TYPE_SEQWRITE_REQ.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/nvme/host/zns.c#n158
> 
> However, assigning, or comparing against struct members of struct blk_zone,
> the blk layer enums have to be used, i.e. BLK_ZONE_TYPE_SEQWRITE_REQ:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/nvme/host/zns.c#n164
> 
> And why did you give me your Reviewed-by on the NVMe patch that uses the
> blk later enums here:
> https://lore.kernel.org/linux-nvme/ef1c39ab-7b56-6a37-0f4f-1ca111d5b48b@opensource.wdc.com/T/#t
> 
> Be consistent, either ack both or nack both :)

I am not nacking anything. I am giving my opinion, which is that I find this
code change useless.

>> A more sensible patch may be to add a static checking that all BLK_ZONE_COND_*
>> and BLK_ZONE_TYPE_* enum values are equal to the ZBC defined values in
>> include/scsi/scsi_proto.h (ZBC_ZONE_COND_* and ZBC_ZONE_TYPE_* macros).
> 
> The blk-zoned block layer is obviously modeled after ZBC, that is why all the
> enum constants happen to be the same. But this obviously doesn't have to be
> true for all existing/future lower level interfaces which supports zones.

If you are worried that sd_zbc_parse_report() does not fill the values as
defined for struct blk_zone, then add something like:

static_assert(BLK_ZONE_COND_FULL == ZBC_ZONE_COND_FULL);
static_assert(BLK_ZONE_TYPE_CONVENTIONAL == ZBC_ZONE_TYPE_CONV);

at the beginning of that function.

blk_dev_revalidate_zones() will check everything is valid anyway.

-- 
Damien Le Moal
Western Digital Research