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

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

On Mon, Nov 29, 2021 at 04:35:41PM +0900, Damien Le Moal wrote:
> 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.

There is no problem with the existing code.
I simply think that it is strictly more correct and slightly less confusing
to use the BLK_ZONE_ enums when accessing members of struct blk_zone.

I didn't see the weirdness of doing so, especially considering that NVMe
uses the BLK_ZONE_ enums when assigning members of struct blk_zone, and
since struct blk_zone, which is the type we are using here, is itself
defined in (and only in) the UAPI header include/uapi/linux/blkzoned.h.

Anyway, I will drop this patch from the series and send out a V2 of patch 2/2.


Kind regards,
Niklas