| Message ID | 20211126125533.266015-1-Niklas.Cassel@wdc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] scsi: sd_zbc: Compare against block layer enum values | expand |
Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
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 ? 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).
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
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.
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
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; ret = cb(&zone, idx, data);