From: Chaitanya Kulkarni <Chaitanya.Kulkarni@wdc.com>
To: Christoph Hellwig <hch@lst.de>
Cc: "linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
"linux-nvme@lists.infradead.org" <linux-nvme@lists.infradead.org>,
"sagi@grimberg.me" <sagi@grimberg.me>,
Damien Le Moal <Damien.LeMoal@wdc.com>
Subject: Re: [PATCH V9 4/9] nvmet: add ZBD over ZNS backend support
Date: Wed, 13 Jan 2021 04:57:15 +0000 [thread overview]
Message-ID: <BYAPR04MB4965CE08B1DE8BC36585991686A90@BYAPR04MB4965.namprd04.prod.outlook.com> (raw)
In-Reply-To: 20210112074805.GA24443@lst.de
On 1/11/21 23:48, Christoph Hellwig wrote:
>> diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
>> index a50b7bcac67a..bdf09d8faa48 100644
>> --- a/drivers/nvme/target/admin-cmd.c
>> +++ b/drivers/nvme/target/admin-cmd.c
>> @@ -191,6 +191,15 @@ static void nvmet_execute_get_log_cmd_effects_ns(struct nvmet_req *req)
>> log->iocs[nvme_cmd_dsm] = cpu_to_le32(1 << 0);
>> log->iocs[nvme_cmd_write_zeroes] = cpu_to_le32(1 << 0);
>> break;
>> + case NVME_CSI_ZNS:
>> + if (IS_ENABLED(CONFIG_BLK_DEV_ZONED)) {
>> + u32 *iocs = log->iocs;
>> +
>> + iocs[nvme_cmd_zone_append] = cpu_to_le32(1 << 0);
>> + iocs[nvme_cmd_zone_mgmt_send] = cpu_to_le32(1 << 0);
>> + iocs[nvme_cmd_zone_mgmt_recv] = cpu_to_le32(1 << 0);
>> + }
>> + break;
> We need to return errors if the command set is not actually supported.
> I also think splitting this into one helper per command set would
> be nice.
>
Okay.
>> @@ -644,6 +653,17 @@ static void nvmet_execuIt should be te_identify_desclist(struct nvmet_req *req)
>> if (status)
>> goto out;
>>
>> + if (IS_ENABLED(CONFIG_BLK_DEV_ZONED)) {
>> + u16 nvme_cis_zns = NVME_CSI_ZNS;
>> +
>> + if (req->ns->csi == NVME_CSI_ZNS)
>> + status = nvmet_copy_ns_identifier(req, NVME_NIDT_CSI,
>> + NVME_NIDT_CSI_LEN,
>> + &nvme_cis_zns, &off);
>> + if (status)
>> + goto out;
>> + }
> We need to add the CSI for every namespace, i.e. something like:
>
> status = nvmet_copy_ns_identifier(req, NVME_NIDT_CSI, NVME_NIDT_CSI_LEN,
> &req->ns->csi);
> if (status)
> goto out;
>
> and this hunk needs to go into the CSI patch.
even better, we can get rid of the local variables...
>> if (sg_zero_buffer(req->sg, req->sg_cnt, NVME_IDENTIFY_DATA_SIZE - off,
>> off) != NVME_IDENTIFY_DATA_SIZE - off)
>> status = NVME_SC_INTERNAL | NVME_SC_DNR;
>> @@ -660,8 +680,16 @@ static void nvmet_execute_identify(struct nvmet_req *req)
>> switch (req->cmd->identify.cns) {
>> case NVME_ID_CNS_NS:
>> return nvmet_execute_identify_ns(req);
>> + case NVME_ID_CNS_CS_NS:
>> + if (req->cmd->identify.csi == NVME_CSI_ZNS)
>> + return nvmet_execute_identify_cns_cs_ns(req);
>> + break;
>> case NVME_ID_CNS_CTRL:
>> return nvmet_execute_identify_ctrl(req);
>> + case NVME_ID_CNS_CS_CTRL:
>> + if (req->cmd->identify.csi == NVME_CSI_ZNS)
>> + return nvmet_execute_identify_cns_cs_ctrl(req);
>> + break;
> How does the CSI get mirrored into the cns field?
>
There is only one cns and one csi value we set from host/zns.c
This is just to reject req if we receive anything else or there is
any change on the host we fail.
>> diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
>> index 672e4009f8d6..17d5da062a5a 100644
>> --- a/drivers/nvme/target/core.c
>> +++ b/drivers/nvme/target/core.c
>> @@ -1107,6 +1107,7 @@ static inline u8 nvmet_cc_iocqes(u32 cc)
>> static inline bool nvmet_cc_css_check(u8 cc_css)
>> {
>> switch (cc_css <<= NVME_CC_CSS_SHIFT) {
>> + case NVME_CC_CSS_CSI:
>> case NVME_CC_CSS_NVM:
>> return true;
>> default:
>> @@ -1173,6 +1174,8 @@ static void nvmet_init_cap(struct nvmet_ctrl *ctrl)
>> {
>> /* command sets supported: NVMe command set: */
>> ctrl->cap = (1ULL << 37);
>> + if (IS_ENABLED(CONFIG_BLK_DEV_ZONED))
>> + ctrl->cap |= (1ULL << 43);
>> /* CC.EN timeout in 500msec units: */
>> ctrl->cap |= (15ULL << 24);
>> /* maximum queue entries supported: */
> This needs to go into a separate patch for multiple command set support.
> We can probably merge the CAP and CC bits with the CSI support, though.
Do you mean previous patch ? but we don't add handlers non-default I/O
command set until this patch..
>> + if (IS_ENABLED(CONFIG_BLK_DEV_ZONED) && bdev_is_zoned(ns->bdev)) {
> bdev_is_zoned should be probably stubbed out for !CONFIG_BLK_DEV_ZONED
> these days.
Are you saying something like following in the prep patch ?or should
just remove
theIS_ENABLED(CONFIG_BLK_DEV_ZONED)part in above if?
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 028ccc9bdf8d..124086c1a0ba 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1570,6 +1570,9 @@ static inline bool bdev_is_zoned(struct
block_device *bdev)
{
struct request_queue *q = bdev_get_queue(bdev);
+ if (!IS_ENABLED(CONFIG_BLK_DEV_ZONED))
+ return false;
+
if (q)
return blk_queue_is_zoned(q);
>> +/*
>> + * ZNS related command implementation and helpers.
>> + */
> Well, that is the description of the whole file, isn't it? I don't think
> this comment adds much value.
Stupid comment, will remove it.
>> + /*
>> + * For ZBC and ZAC devices, writes into sequential zones must be aligned
>> + * to the device physical block size. So use this value as the logical
>> + * block size to avoid errors.
>> + */
> I do not understand the logic here, given that NVMe does not have
> conventional zones.
>
It should be :-
/*
* For ZBC and ZAC devices, writes into sequential zones must be aligned
* to the device physical block size. So use this value as the *physical*
* block size to avoid errors.
*/
next prev parent reply other threads:[~2021-01-13 4:58 UTC|newest]
Thread overview: 49+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-12 4:26 [PATCH V9 0/9] nvmet: add ZBD backend support Chaitanya Kulkarni
2021-01-12 4:26 ` [PATCH V9 1/9] block: export bio_add_hw_pages() Chaitanya Kulkarni
2021-01-12 5:40 ` Damien Le Moal
2021-01-12 7:24 ` Christoph Hellwig
2021-01-13 1:20 ` Chaitanya Kulkarni
2021-01-12 4:26 ` [PATCH V9 2/9] nvmet: add lba to sect conversion helpers Chaitanya Kulkarni
2021-01-12 5:08 ` Damien Le Moal
2021-01-18 18:19 ` Christoph Hellwig
2021-01-12 4:26 ` [PATCH V9 3/9] nvmet: add NVM command set identifier support Chaitanya Kulkarni
2021-01-12 7:27 ` Christoph Hellwig
2021-01-13 4:16 ` Chaitanya Kulkarni
2021-01-18 18:21 ` Christoph Hellwig
2021-01-12 4:26 ` [PATCH V9 4/9] nvmet: add ZBD over ZNS backend support Chaitanya Kulkarni
2021-01-12 5:32 ` Damien Le Moal
2021-01-12 6:11 ` Chaitanya Kulkarni
2021-01-12 6:31 ` Damien Le Moal
2021-01-12 7:48 ` Christoph Hellwig
2021-01-12 7:52 ` Damien Le Moal
2021-01-18 18:25 ` Christoph Hellwig
2021-01-19 0:02 ` Damien Le Moal
2021-01-19 4:28 ` Damien Le Moal
2021-01-19 6:15 ` hch
2021-01-13 4:57 ` Chaitanya Kulkarni [this message]
2021-01-18 18:27 ` Christoph Hellwig
2021-01-12 4:26 ` [PATCH V9 5/9] nvmet: add bio get helper for different backends Chaitanya Kulkarni
2021-01-12 5:37 ` Damien Le Moal
2021-01-12 5:55 ` Chaitanya Kulkarni
2021-01-12 7:33 ` Christoph Hellwig
2021-01-13 5:03 ` Chaitanya Kulkarni
2021-01-18 18:28 ` Christoph Hellwig
2021-01-19 4:57 ` Chaitanya Kulkarni
2021-01-12 4:26 ` [PATCH V9 6/9] nvmet: add bio init " Chaitanya Kulkarni
2021-01-12 5:40 ` Damien Le Moal
2021-01-12 5:57 ` Chaitanya Kulkarni
2021-01-12 6:27 ` Damien Le Moal
2021-01-12 7:33 ` Christoph Hellwig
2021-01-13 5:04 ` Chaitanya Kulkarni
2021-01-18 18:33 ` Christoph Hellwig
2021-01-12 4:26 ` [PATCH V9 7/9] nvmet: add bio put " Chaitanya Kulkarni
2021-01-12 4:26 ` [PATCH V9 8/9] nvmet: add common I/O length check helper Chaitanya Kulkarni
2021-01-12 7:35 ` Christoph Hellwig
2021-01-13 5:07 ` Chaitanya Kulkarni
2021-01-18 18:34 ` Christoph Hellwig
2021-01-12 4:26 ` [PATCH V9 9/9] nvmet: call nvmet_bio_done() for zone append Chaitanya Kulkarni
2021-01-12 7:36 ` Christoph Hellwig
2021-01-13 5:13 ` Chaitanya Kulkarni
2021-01-12 6:12 ` [PATCH V9 0/9] nvmet: add ZBD backend support Chaitanya Kulkarni
2021-02-10 22:42 ` Chaitanya Kulkarni
2021-02-11 7:20 ` hch
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=BYAPR04MB4965CE08B1DE8BC36585991686A90@BYAPR04MB4965.namprd04.prod.outlook.com \
--to=chaitanya.kulkarni@wdc.com \
--cc=Damien.LeMoal@wdc.com \
--cc=hch@lst.de \
--cc=linux-block@vger.kernel.org \
--cc=linux-nvme@lists.infradead.org \
--cc=sagi@grimberg.me \
/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).