[PATCH 4/4] block: add attributes to zone report

[Damien Le Moal]

On 2020/07/02 15:55, Javier González wrote:
> From: Javier González <javier.gonz at samsung.com>
> 
> Add zone attributes field to the blk_zone structure. Use ZNS attributes
> as base for zoned block devices and add the current atributes in
> ZAC/ZBC.
> 
> Signed-off-by: Javier González <javier.gonz at samsung.com>
> Signed-off-by: SelvaKumar S <selvakuma.s1 at samsung.com>
> Signed-off-by: Kanchan Joshi <joshi.k at samsung.com>
> Signed-off-by: Nitesh Shetty <nj.shetty at samsung.com>
> ---
>  drivers/nvme/host/zns.c       |  3 ++-
>  drivers/scsi/sd.c             |  2 +-
>  drivers/scsi/sd_zbc.c         |  8 ++++++--
>  include/uapi/linux/blkzoned.h | 26 +++++++++++++++++++++++++-
>  4 files changed, 34 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/nvme/host/zns.c b/drivers/nvme/host/zns.c
> index d785d179a343..749382a14968 100644
> --- a/drivers/nvme/host/zns.c
> +++ b/drivers/nvme/host/zns.c
> @@ -118,7 +118,7 @@ int nvme_update_zone_info(struct gendisk *disk, struct nvme_ns *ns,
>  	}
>  
>  	q->limits.zoned = BLK_ZONED_HM;
> -	q->zone_flags = BLK_ZONE_REP_CAPACITY | BLK_ZONE_REP_OFFLINE;
> +	q->zone_flags = BLK_ZONE_REP_CAPACITY | BLK_ZONE_REP_CAPACITY | BLK_ZONE_REP_OFFLINE;

Adding BLK_ZONE_REP_CAPACITY a second time ? You meant adding BLK_ZONE_REP_ATTR,
no ?

>  	blk_queue_flag_set(QUEUE_FLAG_ZONE_RESETALL, q);
>  free_data:
>  	kfree(id);
> @@ -197,6 +197,7 @@ static int nvme_zone_parse_entry(struct nvme_ns *ns,
>  	zone.capacity = nvme_lba_to_sect(ns, le64_to_cpu(entry->zcap));
>  	zone.start = nvme_lba_to_sect(ns, le64_to_cpu(entry->zslba));
>  	zone.wp = nvme_lba_to_sect(ns, le64_to_cpu(entry->wp));
> +	zone.attr = entry->za;

It may be a good idea to limit this to the user level defined attributes.

>  
>  	return cb(&zone, idx, data);
>  }
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index b9c920bace28..63270598aa76 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -2967,7 +2967,7 @@ static void sd_read_block_characteristics(struct scsi_disk *sdkp)
>  	if (sdkp->device->type == TYPE_ZBC) {
>  		/* Host-managed */
>  		q->limits.zoned = BLK_ZONED_HM;
> -		q->zone_flags = BLK_ZONE_REP_CAPACITY;
> +		q->zone_flags = BLK_ZONE_REP_CAPACITY | BLK_ZONE_REP_ATTR;
>  	} else {
>  		sdkp->zoned = (buffer[8] >> 4) & 3;
>  		if (sdkp->zoned == 1 && !disk_has_partitions(sdkp->disk)) {
> diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c
> index 183a20720da9..51c7f82b59c5 100644
> --- a/drivers/scsi/sd_zbc.c
> +++ b/drivers/scsi/sd_zbc.c
> @@ -53,10 +53,14 @@ static int sd_zbc_parse_report(struct scsi_disk *sdkp, u8 *buf,
>  
>  	zone.type = buf[0] & 0x0f;
>  	zone.cond = (buf[1] >> 4) & 0xf;
> -	if (buf[1] & 0x01)
> +	if (buf[1] & 0x01) {
> +		zone.attr |= BLK_ZONE_ATTR_NRW;
>  		zone.reset = 1;
> -	if (buf[1] & 0x02)
> +	}
> +	if (buf[1] & 0x02) {
> +		zone.attr |= BLK_ZONE_ATTR_NSQ;
>  		zone.non_seq = 1;
> +	}
>  
>  	zone.len = logical_to_sectors(sdp, get_unaligned_be64(&buf[8]));
>  	zone.capacity = zone.len;
> diff --git a/include/uapi/linux/blkzoned.h b/include/uapi/linux/blkzoned.h
> index e5adf4a9f4b0..315617827acd 100644
> --- a/include/uapi/linux/blkzoned.h
> +++ b/include/uapi/linux/blkzoned.h
> @@ -78,10 +78,33 @@ enum blk_zone_cond {
>   *
>   * @BLK_ZONE_REP_CAPACITY: Zone descriptor has capacity field.
>   * @BLK_ZONE_REP_OFFLINE : Zone device supports offline transition.
> + * @BLK_ZONE_REP_ATTR: Zone attributes reported.
>   */
>  enum blk_zone_report_flags {
>  	BLK_ZONE_REP_CAPACITY	= (1 << 0),
>  	BLK_ZONE_REP_OFFLINE	= (1 << 1),
> +	BLK_ZONE_REP_ATTR	= (1 << 2),
> +};
> +
> +/**
> + * enum blk_zone_attr - Zone Attributes
> + *
> + * Attributes of the zone. Reported in struct blk_zone -> attr
> + *
> + * @BLK_ZONE_ATTR_ZFC: Zone Finished by Controller due to a zone active excursion

ZNS drives that have zone active excursion are not supported for now, so this
attribute can never be returned.

> + * @BLK_ZONE_ATTR_FZR: Finish Zone Recommended required by controller

"Recommended required" is a rather odd wording. If it is required then it is not
recommended, it is mandatory. I would simplify this to

Finish zone recommended

> + * @BLK_ZONE_ATTR_RZR: Reset Zone Recommended required by controller

Same comment as above.

> + * @BLK_ZONE_ATTR_NSQ: Non Sequential zone

This is the same as the non_seq field, so please keep the same definition:

Non-sequential write resources active

> + * @BLK_ZONE_ATTR_NRW: Need Reset Write Pointer required by controller

This is the same as the reset field, so please keep the same definition:

"Reset write pointer recommended"

> + * @BLK_ZONE_ATTR_ZDEV: Zone Descriptor Extension Valid in zone report
> + */
> +enum blk_zone_attr {
> +	BLK_ZONE_ATTR_ZFC	= 1 << 0,
> +	BLK_ZONE_ATTR_FZR	= 1 << 1,
> +	BLK_ZONE_ATTR_RZR	= 1 << 2,
> +	BLK_ZONE_ATTR_NSQ	= 1 << 3,
> +	BLK_ZONE_ATTR_NRW	= 1 << 4,
> +	BLK_ZONE_ATTR_ZDEV	= 1 << 7,

What is the purpose of BLK_ZONE_ATTR_ZDEV ?

>  };
>  
>  /**
> @@ -110,7 +133,8 @@ struct blk_zone {
>  	__u8	cond;		/* Zone condition */
>  	__u8	non_seq;	/* Non-sequential write resources active */
>  	__u8	reset;		/* Reset write pointer recommended */
> -	__u8	resv[4];
> +	__u8	attr;		/* Zone attributes */
> +	__u8	resv[3];
>  	__u64	capacity;	/* Zone capacity in number of sectors */
>  	__u8	reserved[24];
>  };
> 


-- 
Damien Le Moal
Western Digital Research