Re: [PATCH] nvme: report capacity 0 for non supported ZNS SSDs

[Niklas Cassel <Niklas.Cassel@xxxxxxx>]

On Thu, Oct 29, 2020 at 07:57:53PM +0100, Javier González wrote:
> Allow ZNS SSDs to be presented to the host even when they implement
> features that are not supported by the kernel zoned block device.
> 
> Instead of rejecting the SSD at the NVMe driver level, deal with this in
> the block layer by setting capacity to 0, as we do with other things
> such as unsupported PI configurations. This allows to use standard
> management tools such as nvme-cli to choose a different format or
> firmware slot that is compatible with the Linux zoned block device.
> 
> Signed-off-by: Javier González <javier.gonz@xxxxxxxxxxx>
> ---
>  drivers/nvme/host/core.c |  5 +++++
>  drivers/nvme/host/nvme.h |  1 +
>  drivers/nvme/host/zns.c  | 31 ++++++++++++++-----------------
>  3 files changed, 20 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index c190c56bf702..9ca4f0a6ff2c 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -2026,6 +2026,11 @@ static void nvme_update_disk_info(struct gendisk *disk,
>  			capacity = 0;
>  	}

Hello Javier,

>  
> +#ifdef CONFIG_BLK_DEV_ZONED

if (IS_ENABLED(CONFIG_BLK_DEV_ZONED)

is preferred over ifdefs.

> +	if (!ns->zone_sup)
> +		capacity = 0;
> +#endif
> +
>  	set_capacity_revalidate_and_notify(disk, capacity, false);
>  
>  	nvme_config_discard(disk, ns);
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index 87737fa32360..42cbe5bbc518 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -443,6 +443,7 @@ struct nvme_ns {
>  	u8 pi_type;
>  #ifdef CONFIG_BLK_DEV_ZONED
>  	u64 zsze;
> +	bool zone_sup;

Perhaps a more descriptive name? zoned_ns_supp ?

>  #endif
>  	unsigned long features;
>  	unsigned long flags;
> diff --git a/drivers/nvme/host/zns.c b/drivers/nvme/host/zns.c
> index 57cfd78731fb..77a7fed508ef 100644
> --- a/drivers/nvme/host/zns.c
> +++ b/drivers/nvme/host/zns.c
> @@ -44,20 +44,23 @@ int nvme_update_zone_info(struct gendisk *disk, struct nvme_ns *ns,
>  	struct nvme_id_ns_zns *id;
>  	int status;
>  
> -	/* Driver requires zone append support */
> +	ns->zone_sup = true;

I don't think it is wise to assign it to true here.
E.g. if kzalloc() failes, if nvme_submit_sync_cmd() fails,
or if nvme_set_max_append() fails, you have already set this to true,
but zoc or power of 2 checks were never performed.


Perhaps something like this would be more robust:

@@ -53,18 +53,19 @@ int nvme_update_zone_info(struct nvme_ns *ns, unsigned lbaf)
        struct nvme_command c = { };
        struct nvme_id_ns_zns *id;
        int status;
+       bool new_ns_supp = true;
+
+       /* default to NS not supported */
+       ns->zoned_ns_supp = false;

-       /* Driver requires zone append support */
        if (!(le32_to_cpu(log->iocs[nvme_cmd_zone_append]) &
                        NVME_CMD_EFFECTS_CSUPP)) {
                dev_warn(ns->ctrl->device,
                        "append not supported for zoned namespace:%d\n",
                        ns->head->ns_id);
-               return -EINVAL;
-       }
-
-       /* Lazily query controller append limit for the first zoned namespace */
-       if (!ns->ctrl->max_zone_append) {
+               new_ns_supp = false;
+       } else if (!ns->ctrl->max_zone_append) {
+               /* Lazily query controller append limit for the first zoned namespace */
                status = nvme_set_max_append(ns->ctrl);
                if (status)
                        return status;
@@ -80,19 +81,16 @@ int nvme_update_zone_info(struct nvme_ns *ns, unsigned lbaf)
        c.identify.csi = NVME_CSI_ZNS;

        status = nvme_submit_sync_cmd(ns->ctrl->admin_q, &c, id, sizeof(*id));
-       if (status)
-               goto free_data;
+       if (status) {
+               kfree(id);
+               return status;
+       }

-       /*
-        * We currently do not handle devices requiring any of the zoned
-        * operation characteristics.
-        */
        if (id->zoc) {
                dev_warn(ns->ctrl->device,
                        "zone operations:%x not supported for namespace:%u\n",
                        le16_to_cpu(id->zoc), ns->head->ns_id);
-               status = -EINVAL;
-               goto free_data;
+               new_ns_supp = false;
        }

        ns->zsze = nvme_lba_to_sect(ns, le64_to_cpu(id->lbafe[lbaf].zsze));
@@ -100,17 +98,14 @@ int nvme_update_zone_info(struct nvme_ns *ns, unsigned lbaf)
                dev_warn(ns->ctrl->device,
                        "invalid zone size:%llu for namespace:%u\n",
                        ns->zsze, ns->head->ns_id);
-               status = -EINVAL;
-               goto free_data;
+               new_ns_supp = false;
        }

+       ns->zoned_ns_supp = new_ns_supp;
        q->limits.zoned = BLK_ZONED_HM;
        blk_queue_flag_set(QUEUE_FLAG_ZONE_RESETALL, q);
        blk_queue_max_open_zones(q, le32_to_cpu(id->mor) + 1);
        blk_queue_max_active_zones(q, le32_to_cpu(id->mar) + 1);
-free_data:
-       kfree(id);
-       return status;
 }

 static void *nvme_zns_alloc_report_buffer(struct nvme_ns *ns,


Kind regards,
Niklas