From: Damien Le Moal <Damien.LeMoal@wdc.com>
To: Christoph Hellwig <hch@lst.de>,
"linux-nvme@lists.infradead.org" <linux-nvme@lists.infradead.org>
Cc: Keith Busch <kbusch@kernel.org>, Jens Axboe <axboe@kernel.dk>,
Sagi Grimberg <sagi@grimberg.me>
Subject: Re: [PATCH 17/20] nvme: query namespae identifiers before adding the namespace
Date: Mon, 28 Sep 2020 15:11:43 +0000 [thread overview]
Message-ID: <CY4PR04MB3751E29EE08865FD1D804FE4E7350@CY4PR04MB3751.namprd04.prod.outlook.com> (raw)
In-Reply-To: 20200928123502.435373-18-hch@lst.de
On 2020/09/28 21:35, Christoph Hellwig wrote:
> Check the namespace identifier list first thing when scanning namespaces.
> This keeps the code to query the CSI common between the alloc and validate
> path, and helps to structure the code better for multiple command set
> support.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> drivers/nvme/host/core.c | 116 ++++++++++++++++++---------------------
> 1 file changed, 53 insertions(+), 63 deletions(-)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 9c137d8819f756..ad18c32b36e7b6 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1281,6 +1281,8 @@ static int nvme_identify_ns_descs(struct nvme_ctrl *ctrl, unsigned nsid,
> int status, pos, len;
> void *data;
>
> + if (ctrl->vs < NVME_VS(1, 3, 0) && !nvme_multi_css(ctrl))
> + return 0;
> if (ctrl->quirks & NVME_QUIRK_NO_NS_DESC_LIST)
> return 0;
>
> @@ -1335,8 +1337,8 @@ static int nvme_identify_ns_list(struct nvme_ctrl *dev, unsigned nsid, __le32 *n
> NVME_IDENTIFY_DATA_SIZE);
> }
>
> -static int nvme_identify_ns(struct nvme_ctrl *ctrl,
> - unsigned nsid, struct nvme_id_ns **id)
> +static int nvme_identify_ns(struct nvme_ctrl *ctrl, unsigned nsid,
> + struct nvme_ns_ids *ids, struct nvme_id_ns **id)
> {
> struct nvme_command c = { };
> int error;
> @@ -1359,6 +1361,14 @@ static int nvme_identify_ns(struct nvme_ctrl *ctrl,
> error = -ENODEV;
> if ((*id)->ncap == 0) /* namespace not allocated or attached */
> goto out_free_id;
> +
> + if (ctrl->vs >= NVME_VS(1, 1, 0) &&
> + !memchr_inv(ids->eui64, 0, sizeof(ids->eui64)))
> + memcpy(ids->eui64, (*id)->eui64, sizeof(ids->eui64));
> + if (ctrl->vs >= NVME_VS(1, 2, 0) &&
> + !memchr_inv(ids->nguid, 0, sizeof(ids->nguid)))
> + memcpy(ids->nguid, (*id)->nguid, sizeof(ids->nguid));
> +
> return 0;
>
> out_free_id:
> @@ -1884,20 +1894,6 @@ static void nvme_config_write_zeroes(struct gendisk *disk, struct nvme_ns *ns)
> nvme_lba_to_sect(ns, max_blocks));
> }
>
> -static int nvme_report_ns_ids(struct nvme_ctrl *ctrl, unsigned int nsid,
> - struct nvme_id_ns *id, struct nvme_ns_ids *ids)
> -{
> - memset(ids, 0, sizeof(*ids));
> -
> - if (ctrl->vs >= NVME_VS(1, 1, 0))
> - memcpy(ids->eui64, id->eui64, sizeof(id->eui64));
> - if (ctrl->vs >= NVME_VS(1, 2, 0))
> - memcpy(ids->nguid, id->nguid, sizeof(id->nguid));
> - if (ctrl->vs >= NVME_VS(1, 3, 0) || nvme_multi_css(ctrl))
> - return nvme_identify_ns_descs(ctrl, nsid, ids);
> - return 0;
> -}
> -
> static bool nvme_ns_ids_valid(struct nvme_ns_ids *ids)
> {
> return !uuid_is_null(&ids->uuid) ||
> @@ -2117,30 +2113,16 @@ static void nvme_set_chunk_sectors(struct nvme_ns *ns, struct nvme_id_ns *id)
> static int nvme_update_ns_info(struct nvme_ns *ns, struct nvme_id_ns *id)
> {
> unsigned lbaf = id->flbas & NVME_NS_FLBAS_LBA_MASK;
> - struct nvme_ctrl *ctrl = ns->ctrl;
> int ret;
>
> blk_mq_freeze_queue(ns->disk->queue);
> ns->lba_shift = id->lbaf[lbaf].ds;
> - nvme_set_queue_limits(ctrl, ns->queue);
> + nvme_set_queue_limits(ns->ctrl, ns->queue);
>
> - switch (ns->head->ids.csi) {
> - case NVME_CSI_NVM:
> - break;
> - case NVME_CSI_ZNS:
> + if (ns->head->ids.csi == NVME_CSI_ZNS) {
> ret = nvme_update_zone_info(ns, lbaf);
> - if (ret) {
> - dev_warn(ctrl->device,
> - "failed to add zoned namespace:%u ret:%d\n",
> - ns->head->ns_id, ret);
> + if (ret)
> goto out_unfreeze;
> - }
> - break;
> - default:
> - dev_warn(ctrl->device, "unknown csi:%u ns:%u\n",
> - ns->head->ids.csi, ns->head->ns_id);
> - ret = -ENODEV;
> - goto out_unfreeze;
> }
>
> ret = nvme_configure_metadata(ns, id);
> @@ -2174,11 +2156,10 @@ static int nvme_update_ns_info(struct nvme_ns *ns, struct nvme_id_ns *id)
> return ret;
> }
>
> -static int nvme_validate_ns(struct nvme_ns *ns)
> +static int nvme_validate_ns(struct nvme_ns *ns, struct nvme_ns_ids *ids)
> {
> struct nvme_ctrl *ctrl = ns->ctrl;
> struct nvme_id_ns *id;
> - struct nvme_ns_ids ids;
> int ret = 0;
>
> if (test_bit(NVME_NS_DEAD, &ns->flags)) {
> @@ -2186,15 +2167,11 @@ static int nvme_validate_ns(struct nvme_ns *ns)
> return -ENODEV;
> }
>
> - ret = nvme_identify_ns(ctrl, ns->head->ns_id, &id);
> + ret = nvme_identify_ns(ctrl, ns->head->ns_id, ids, &id);
> if (ret)
> goto out;
>
> - ret = nvme_report_ns_ids(ctrl, ns->head->ns_id, id, &ids);
> - if (ret)
> - goto free_id;
> -
> - if (!nvme_ns_ids_equal(&ns->head->ids, &ids)) {
> + if (!nvme_ns_ids_equal(&ns->head->ids, ids)) {
> dev_err(ctrl->device,
> "identifiers changed for nsid %d\n", ns->head->ns_id);
> ret = -ENODEV;
> @@ -3794,25 +3771,16 @@ static struct nvme_ns_head *nvme_alloc_ns_head(struct nvme_ctrl *ctrl,
> }
>
> static int nvme_init_ns_head(struct nvme_ns *ns, unsigned nsid,
> - struct nvme_id_ns *id)
> + struct nvme_ns_ids *ids, bool is_shared)
> {
> struct nvme_ctrl *ctrl = ns->ctrl;
> - bool is_shared = id->nmic & NVME_NS_NMIC_SHARED;
> struct nvme_ns_head *head = NULL;
> - struct nvme_ns_ids ids;
> int ret = 0;
>
> - ret = nvme_report_ns_ids(ctrl, nsid, id, &ids);
> - if (ret) {
> - if (ret < 0)
> - return ret;
> - return blk_status_to_errno(nvme_error_status(ret));
> - }
> -
> mutex_lock(&ctrl->subsys->lock);
> head = nvme_find_ns_head(ctrl->subsys, nsid);
> if (!head) {
> - head = nvme_alloc_ns_head(ctrl, nsid, &ids);
> + head = nvme_alloc_ns_head(ctrl, nsid, ids);
> if (IS_ERR(head)) {
> ret = PTR_ERR(head);
> goto out_unlock;
> @@ -3825,7 +3793,7 @@ static int nvme_init_ns_head(struct nvme_ns *ns, unsigned nsid,
> "Duplicate unshared namespace %d\n", nsid);
> goto out_put_ns_head;
> }
> - if (!nvme_ns_ids_equal(&head->ids, &ids)) {
> + if (!nvme_ns_ids_equal(&head->ids, ids)) {
> dev_err(ctrl->device,
> "IDs don't match for shared namespace %d\n",
> nsid);
> @@ -3873,7 +3841,8 @@ struct nvme_ns *nvme_find_get_ns(struct nvme_ctrl *ctrl, unsigned nsid)
> }
> EXPORT_SYMBOL_NS_GPL(nvme_find_get_ns, NVME_TARGET_PASSTHRU);
>
> -static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)
> +static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid,
> + struct nvme_ns_ids *ids)
> {
> struct nvme_ns *ns;
> struct gendisk *disk;
> @@ -3881,7 +3850,7 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)
> char disk_name[DISK_NAME_LEN];
> int node = ctrl->numa_node, flags = GENHD_FL_EXT_DEVT, ret;
>
> - if (nvme_identify_ns(ctrl, nsid, &id))
> + if (nvme_identify_ns(ctrl, nsid, ids, &id))
> return;
>
> ns = kzalloc_node(sizeof(*ns), GFP_KERNEL, node);
> @@ -3903,7 +3872,7 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)
> ns->ctrl = ctrl;
> kref_init(&ns->kref);
>
> - ret = nvme_init_ns_head(ns, nsid, id);
> + ret = nvme_init_ns_head(ns, nsid, ids, id->nmic & NVME_NS_NMIC_SHARED);
> if (ret)
> goto out_free_queue;
> nvme_set_disk_name(disk_name, ns, ctrl, &flags);
> @@ -4006,20 +3975,41 @@ static void nvme_ns_remove_by_nsid(struct nvme_ctrl *ctrl, u32 nsid)
>
> static void nvme_validate_or_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)
> {
> + struct nvme_ns_ids ids = { };
> struct nvme_ns *ns;
> int ret;
>
> + if (nvme_identify_ns_descs(ctrl, nsid, &ids))
> + return;
> +
> ns = nvme_find_get_ns(ctrl, nsid);
> - if (!ns) {
> - nvme_alloc_ns(ctrl, nsid);
> + if (ns) {
> + ret = nvme_validate_ns(ns, &ids);
> + revalidate_disk_size(ns->disk, ret == 0);
> + if (ret)
> + nvme_ns_remove(ns);
> + nvme_put_ns(ns);
> return;
> }
>
> - ret = nvme_validate_ns(ns);
> - revalidate_disk_size(ns->disk, ret == 0);
> - if (ret)
> - nvme_ns_remove(ns);
> - nvme_put_ns(ns);
> + switch (ids.csi) {
> + case NVME_CSI_NVM:
> + nvme_alloc_ns(ctrl, nsid, &ids);
> + break;
> + case NVME_CSI_ZNS:
> + if (!IS_ENABLED(CONFIG_BLK_DEV_ZONED)) {
> + dev_warn(ctrl->device,
> + "nsid %u not supported without CONFIG_BLK_DEV_ZONED\n",
> + nsid);
> + break;
> + }
> + nvme_alloc_ns(ctrl, nsid, &ids);
> + break;
> + default:
> + dev_warn(ctrl->device, "unknown csi %u for nsid %u\n",
> + ids.csi, nsid);
> + break;
> + }
> }
>
> static void nvme_remove_invalid_namespaces(struct nvme_ctrl *ctrl,
>
Looks good.
Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>
--
Damien Le Moal
Western Digital Research
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
next prev parent reply other threads:[~2020-09-28 15:11 UTC|newest]
Thread overview: 90+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-28 12:34 refactor the nvme scanning and validation code Christoph Hellwig
2020-09-28 12:34 ` [PATCH 01/20] block: optimize blk_queue_zoned_model for !CONFIG_BLK_DEV_ZONED Christoph Hellwig
2020-09-28 14:11 ` Damien Le Moal
2020-09-29 8:29 ` Sagi Grimberg
2020-09-29 18:25 ` Christoph Hellwig
2020-09-29 21:12 ` Chaitanya Kulkarni
2020-09-28 12:34 ` [PATCH 02/20] nvme: fix initialization of the zone bitmaps Christoph Hellwig
2020-09-28 14:13 ` Damien Le Moal
2020-09-28 14:16 ` Damien Le Moal
2020-09-28 14:26 ` Christoph Hellwig
2020-09-29 21:27 ` Keith Busch
2020-09-28 12:34 ` [PATCH 03/20] nvme: remove the disk argument to nvme_update_zone_info Christoph Hellwig
2020-09-28 14:17 ` Damien Le Moal
2020-09-29 8:32 ` Sagi Grimberg
2020-09-29 21:15 ` Chaitanya Kulkarni
2020-09-28 12:34 ` [PATCH 04/20] nvme: rename nvme_validate_ns to nvme_validate_or_alloc_ns Christoph Hellwig
2020-09-28 14:19 ` Damien Le Moal
2020-09-29 8:33 ` Sagi Grimberg
2020-09-29 21:17 ` Chaitanya Kulkarni
2020-09-28 12:34 ` [PATCH 05/20] nvme: rename _nvme_revalidate_disk Christoph Hellwig
2020-09-28 14:20 ` Damien Le Moal
2020-09-29 8:34 ` Sagi Grimberg
2020-09-29 21:18 ` Chaitanya Kulkarni
2020-09-28 12:34 ` [PATCH 06/20] nvme: rename __nvme_revalidate_disk Christoph Hellwig
2020-09-28 14:21 ` Damien Le Moal
2020-09-29 8:35 ` Sagi Grimberg
2020-09-29 21:20 ` Chaitanya Kulkarni
2020-09-28 12:34 ` [PATCH 07/20] nvme: lift the check for an unallocated namespace into nvme_identify_ns Christoph Hellwig
2020-09-28 14:27 ` Damien Le Moal
2020-09-29 18:29 ` Christoph Hellwig
2020-09-29 8:36 ` Sagi Grimberg
2020-09-29 21:22 ` Chaitanya Kulkarni
2020-09-28 12:34 ` [PATCH 08/20] nvme: call nvme_identify_ns as the first thing in nvme_alloc_ns_block Christoph Hellwig
2020-09-28 14:32 ` Damien Le Moal
2020-09-29 8:39 ` Sagi Grimberg
2020-09-29 18:30 ` Christoph Hellwig
2020-09-29 21:24 ` Chaitanya Kulkarni
2020-09-28 12:34 ` [PATCH 09/20] nvme: factor out a nvme_configure_metadata helper Christoph Hellwig
2020-09-28 14:35 ` Damien Le Moal
2020-09-29 8:40 ` Sagi Grimberg
2020-09-29 21:27 ` Chaitanya Kulkarni
2020-09-28 12:34 ` [PATCH 10/20] nvme: freeze the queue over ->lba_shift updates Christoph Hellwig
2020-09-28 14:49 ` Damien Le Moal
2020-09-29 8:48 ` Sagi Grimberg
2020-09-29 18:32 ` Christoph Hellwig
2020-09-29 19:07 ` Sagi Grimberg
2020-10-02 16:03 ` Sagi Grimberg
2020-10-05 8:32 ` Christoph Hellwig
2020-09-28 12:34 ` [PATCH 11/20] nvme: clean up the check for too large logic block sizes Christoph Hellwig
2020-09-28 14:50 ` Damien Le Moal
2020-09-29 18:33 ` Christoph Hellwig
2020-09-29 8:50 ` Sagi Grimberg
2020-09-29 18:34 ` Christoph Hellwig
2020-09-28 12:34 ` [PATCH 12/20] nvme: remove the 0 lba_shift check in nvme_update_ns_info Christoph Hellwig
2020-09-28 14:51 ` Damien Le Moal
2020-09-29 8:52 ` Sagi Grimberg
2020-09-29 18:34 ` Christoph Hellwig
2020-09-29 21:46 ` Chaitanya Kulkarni
2020-09-28 12:34 ` [PATCH 13/20] nvme: set the queue limits " Christoph Hellwig
2020-09-28 14:55 ` Damien Le Moal
2020-09-29 8:54 ` Sagi Grimberg
2020-09-29 21:52 ` Chaitanya Kulkarni
2020-09-30 6:12 ` Christoph Hellwig
2020-09-28 12:34 ` [PATCH 14/20] nvme: update the known admin effects Christoph Hellwig
2020-09-28 14:57 ` Damien Le Moal
2020-09-29 8:55 ` Sagi Grimberg
2020-09-29 21:54 ` Chaitanya Kulkarni
2020-09-28 12:34 ` [PATCH 15/20] nvme: remove nvme_update_formats Christoph Hellwig
2020-09-28 15:02 ` Damien Le Moal
2020-09-28 12:34 ` [PATCH 16/20] nvme: revalidate zone bitmaps in nvme_update_ns_info Christoph Hellwig
2020-09-28 15:06 ` Damien Le Moal
2020-09-29 18:37 ` Christoph Hellwig
2020-09-28 12:34 ` [PATCH 17/20] nvme: query namespae identifiers before adding the namespace Christoph Hellwig
2020-09-28 15:11 ` Damien Le Moal [this message]
2020-09-30 9:44 ` Niklas Cassel
2020-09-30 10:04 ` Niklas Cassel
2020-10-01 17:14 ` Christoph Hellwig
2020-10-01 17:43 ` Niklas Cassel
2020-10-02 6:41 ` Christoph Hellwig
2020-09-28 12:35 ` [PATCH 18/20] nvme: move nvme_validate_ns Christoph Hellwig
2020-09-28 15:12 ` Damien Le Moal
2020-09-30 0:22 ` Chaitanya Kulkarni
2020-09-30 6:13 ` Christoph Hellwig
2020-09-28 12:35 ` [PATCH 19/20] nvme: refactor nvme_validate_ns Christoph Hellwig
2020-09-28 15:15 ` Damien Le Moal
2020-09-29 18:40 ` Christoph Hellwig
2020-09-28 12:35 ` [PATCH 20/20] nvme: remove nvme_identify_ns_list Christoph Hellwig
2020-09-29 23:59 ` Chaitanya Kulkarni
2020-09-29 16:51 ` refactor the nvme scanning and validation code Keith Busch
2020-09-30 6:41 ` Christoph Hellwig
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=CY4PR04MB3751E29EE08865FD1D804FE4E7350@CY4PR04MB3751.namprd04.prod.outlook.com \
--to=damien.lemoal@wdc.com \
--cc=axboe@kernel.dk \
--cc=hch@lst.de \
--cc=kbusch@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).