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

[Niklas Cassel <Niklas.Cassel@wdc.com>]

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.

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 :)


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


Kind regards,
Niklas