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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.