From: Damien Le Moal <Damien.LeMoal@wdc.com>
To: "Javier González" <javier@javigon.com>,
"linux-nvme@lists.infradead.org" <linux-nvme@lists.infradead.org>
Cc: "axboe@kernel.dk" <axboe@kernel.dk>,
"SelvaKumar S" <selvakuma.s1@samsung.com>,
"sagi@grimberg.me" <sagi@grimberg.me>,
"Kanchan Joshi" <joshi.k@samsung.com>,
"Johannes Thumshirn" <Johannes.Thumshirn@wdc.com>,
"Nitesh Shetty" <nj.shetty@samsung.com>,
"linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
"kbusch@kernel.org" <kbusch@kernel.org>,
"Javier González" <javier.gonz@samsung.com>,
"mb@lightnvm.io" <mb@lightnvm.io>, "hch@lst.de" <hch@lst.de>
Subject: Re: [PATCH v3 4/4] block: add attributes to zone report
Date: Thu, 2 Jul 2020 09:54:12 +0000 [thread overview]
Message-ID: <CY4PR04MB3751CAC724F706063EA0D87EE76D0@CY4PR04MB3751.namprd04.prod.outlook.com> (raw)
In-Reply-To: 20200702092438.63717-5-javier@javigon.com
On 2020/07/02 18:25, Javier González wrote:
> From: Javier González <javier.gonz@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@samsung.com>
> Signed-off-by: SelvaKumar S <selvakuma.s1@samsung.com>
> Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>
> Signed-off-by: Nitesh Shetty <nj.shetty@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 daf0d91bcdf6..926904d1827b 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_SUP_OFFLINE;
> + q->zone_flags = BLK_ZONE_REP_CAPACITY | BLK_ZONE_REP_CAPACITY | BLK_ZONE_SUP_OFFLINE;
Wrong flag added, as already signaled.
> 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;
Mask on 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 8b7c4705cef7..b48b8cb129a2 100644
> --- a/include/uapi/linux/blkzoned.h
> +++ b/include/uapi/linux/blkzoned.h
> @@ -77,16 +77,39 @@ enum blk_zone_cond {
> * enum blk_zone_report_flags - Feature flags of reported zone descriptors.
> *
> * @BLK_ZONE_REP_CAPACITY: Zone descriptor has capacity field.
> + * @BLK_ZONE_REP_ATTR: Zone attributes reported.
> * @BLK_ZONE_SUP_OFFLINE : Zone device supports explicit offline transition.
> */
> enum blk_zone_report_flags {
> /* Report feature flags */
> BLK_ZONE_REP_CAPACITY = (1 << 0),
> + BLK_ZONE_REP_ATTR = (1 << 2),
>
> /* Supported capabilities */
> BLK_ZONE_SUP_OFFLINE = (1 << 1),
> };
>
> +/**
> + * 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
> + * @BLK_ZONE_ATTR_FZR: Finish Zone Recommended required by controller
> + * @BLK_ZONE_ATTR_RZR: Reset Zone Recommended required by controller
> + * @BLK_ZONE_ATTR_NSQ: Non Sequential zone
> + * @BLK_ZONE_ATTR_NRW: Need Reset Write Pointer required by controller
> + * @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,
> +};
> +
> /**
> * struct blk_zone - Zone descriptor for BLKREPORTZONE ioctl.
> *
> @@ -113,7 +136,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];
> };
>
Similarly to BLK_ZONE_REP_CAPACITY, I do not see the point of having
BLK_ZONE_REP_ATTR added to q->zone_flags. Rename that one q->zone_features and
have report zones do:
re.flags = BLK_ZONE_REP_CAPACITY | BLK_ZONE_REP_ATTR | q->zone_features;
Devices such as nullblk can simply have zone->attr at 0, always. That is still
correct. BLK_ZONE_REP_ATTR means "have attr field", not "attributes are set".
seems a lot cleaner and simpler to me. For zone_features, it would be zone
offline only, but as already discussed, I do not think that a feature flag is
the best approach as that complicates support for device mapper.
--
Damien Le Moal
Western Digital Research
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
prev parent reply other threads:[~2020-07-02 9:54 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-02 9:24 [PATCH v3 0/4] ZNS: Extra features for current patches Javier González
2020-07-02 9:24 ` [PATCH v3 1/4] block: Add zone flags to queue zone prop Javier González
2020-07-02 9:46 ` Damien Le Moal
2020-07-02 10:18 ` Javier González
2020-07-02 9:24 ` [PATCH v3 2/4] block: add support for zone offline transition Javier González
2020-07-02 9:47 ` Damien Le Moal
2020-07-02 9:24 ` [PATCH v3 3/4] nvme: Add consistency check for zone count Javier González
2020-07-02 9:24 ` [PATCH v3 4/4] block: add attributes to zone report Javier González
2020-07-02 9:54 ` Damien Le Moal [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CY4PR04MB3751CAC724F706063EA0D87EE76D0@CY4PR04MB3751.namprd04.prod.outlook.com \
--to=damien.lemoal@wdc.com \
--cc=Johannes.Thumshirn@wdc.com \
--cc=axboe@kernel.dk \
--cc=hch@lst.de \
--cc=javier.gonz@samsung.com \
--cc=javier@javigon.com \
--cc=joshi.k@samsung.com \
--cc=kbusch@kernel.org \
--cc=linux-block@vger.kernel.org \
--cc=linux-nvme@lists.infradead.org \
--cc=mb@lightnvm.io \
--cc=nj.shetty@samsung.com \
--cc=sagi@grimberg.me \
--cc=selvakuma.s1@samsung.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).