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

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

[Niklas Cassel]

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;
 
 	ret = cb(&zone, idx, data);
-- 
2.33.1

[PATCH 2/2] scsi: sd_zbc: Clean up sd_zbc_parse_report() setting of wp

[Niklas Cassel]

From: Niklas Cassel <niklas.cassel@wdc.com>

Make sd_zbc_parse_report() use if/else when setting the write pointer,
instead of setting it unconditionally and then conditionally updating it.

Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 drivers/scsi/sd_zbc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c
index 024f1bec6e5a..3e25ded3ac0f 100644
--- a/drivers/scsi/sd_zbc.c
+++ b/drivers/scsi/sd_zbc.c
@@ -61,10 +61,11 @@ static int sd_zbc_parse_report(struct scsi_disk *sdkp, u8 *buf,
 	zone.len = logical_to_sectors(sdp, get_unaligned_be64(&buf[8]));
 	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 != BLK_ZONE_TYPE_CONVENTIONAL &&
 	    zone.cond == BLK_ZONE_COND_FULL)
 		zone.wp = zone.start + zone.len;
+	else
+		zone.wp = logical_to_sectors(sdp, get_unaligned_be64(&buf[24]));
 
 	ret = cb(&zone, idx, data);
 	if (ret)
-- 
2.33.1

Re: [PATCH 2/2] scsi: sd_zbc: Clean up sd_zbc_parse_report() setting of wp

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH 2/2] scsi: sd_zbc: Clean up sd_zbc_parse_report() setting of wp

[Damien Le Moal]

On 2021/11/26 21:56, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@wdc.com>
> 
> Make sd_zbc_parse_report() use if/else when setting the write pointer,
> instead of setting it unconditionally and then conditionally updating it.
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> ---
>  drivers/scsi/sd_zbc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c
> index 024f1bec6e5a..3e25ded3ac0f 100644
> --- a/drivers/scsi/sd_zbc.c
> +++ b/drivers/scsi/sd_zbc.c
> @@ -61,10 +61,11 @@ static int sd_zbc_parse_report(struct scsi_disk *sdkp, u8 *buf,
>  	zone.len = logical_to_sectors(sdp, get_unaligned_be64(&buf[8]));
>  	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 != BLK_ZONE_TYPE_CONVENTIONAL &&
>  	    zone.cond == BLK_ZONE_COND_FULL)
>  		zone.wp = zone.start + zone.len;
> +	else
> +		zone.wp = logical_to_sectors(sdp, get_unaligned_be64(&buf[24]));
>  
>  	ret = cb(&zone, idx, data);
>  	if (ret)
> 

Looks good to me, modulo the switch to using BLK_ZONE_* enums.


-- 
Damien Le Moal
Western Digital Research

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

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

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

[Damien Le Moal]

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


-- 
Damien Le Moal
Western Digital Research

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

[Niklas Cassel]

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

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

[Damien Le Moal]

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

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

[Niklas Cassel]

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