[PATCH] nvme: enable ro namespace for ZNS without append

[PATCH] nvme: enable ro namespace for ZNS without append

[Javier González]

Allow ZNS NVMe SSDs to present a read-only namespace when append is not
supported, instead of rejecting the namespace directly.

This allows (i) the namespace to be used in read-only mode, which is not
a problem as the append command only affects the write path, and (ii) 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.

This patch includes comments from Christoph, Niklas and Keith that
applied to a different approach setting capacity to 0
  https://www.spinics.net/lists/linux-block/msg60747.html

The reminder of the original patch will be submitted separately.

Signed-off-by: Javier González <javier.gonz@samsung.com>
---
 drivers/nvme/host/core.c |  5 +++++
 drivers/nvme/host/nvme.h |  1 +
 drivers/nvme/host/zns.c  | 20 ++++++++++++--------
 3 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index c190c56bf702..8967ab9089b3 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2036,6 +2036,11 @@ static void nvme_update_disk_info(struct gendisk *disk,
 	else
 		set_disk_ro(disk, false);
 
+#ifdef CONFIG_BLK_DEV_ZONED
+	if (blk_queue_is_zoned(disk->queue) && !ns->zoned_ns_supp)
+		set_disk_ro(disk, true);
+#endif
+
 	blk_mq_unfreeze_queue(disk->queue);
 }
 
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 87737fa32360..ff4fe645ab9b 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 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..4d005132ab95 100644
--- a/drivers/nvme/host/zns.c
+++ b/drivers/nvme/host/zns.c
@@ -42,22 +42,25 @@ int nvme_update_zone_info(struct gendisk *disk, struct nvme_ns *ns,
 	struct request_queue *q = disk->queue;
 	struct nvme_command c = { };
 	struct nvme_id_ns_zns *id;
+	bool zoned_ns_supp = true;
 	int status;
 
 	/* Driver requires zone append support */
 	if (!(le32_to_cpu(log->iocs[nvme_cmd_zone_append]) &
 			NVME_CMD_EFFECTS_CSUPP)) {
+		zoned_ns_supp = false;
 		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) {
-		status = nvme_set_max_append(ns->ctrl);
-		if (status)
-			return status;
+	} else {
+		/* Lazily query controller append limit for the first
+		 * zoned namespace
+		 */
+		if (!ns->ctrl->max_zone_append) {
+			status = nvme_set_max_append(ns->ctrl);
+			if (status)
+				return status;
+		}
 	}
 
 	id = kzalloc(sizeof(*id), GFP_KERNEL);
@@ -95,6 +98,7 @@ int nvme_update_zone_info(struct gendisk *disk, struct nvme_ns *ns,
 	}
 
 	q->limits.zoned = BLK_ZONED_HM;
+	ns->zoned_ns_supp = zoned_ns_supp;
 	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);
-- 
2.17.1


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: enable ro namespace for ZNS without append

[Christoph Hellwig]

> +#ifdef CONFIG_BLK_DEV_ZONED
> +	if (blk_queue_is_zoned(disk->queue) && !ns->zoned_ns_supp)
> +		set_disk_ro(disk, true);
> +#endif

I think we can simplify this at bit.  Add a new NVME_NS_FORCE_RO flag
to ns->flags, set it in nvme_update_zone_info, and just query it here
without any ifdefs.

>  	struct request_queue *q = disk->queue;
>  	struct nvme_command c = { };
>  	struct nvme_id_ns_zns *id;
> +	bool zoned_ns_supp = true;
>  	int status;
>  
>  	/* Driver requires zone append support */
>  	if (!(le32_to_cpu(log->iocs[nvme_cmd_zone_append]) &
>  			NVME_CMD_EFFECTS_CSUPP)) {
> +		zoned_ns_supp = false;
>  		dev_warn(ns->ctrl->device,
>  			"append not supported for zoned namespace:%d\n",
>  			ns->head->ns_id);
> +	} else {
> +		/* Lazily query controller append limit for the first
> +		 * zoned namespace
> +		 */
> +		if (!ns->ctrl->max_zone_append) {
> +			status = nvme_set_max_append(ns->ctrl);
> +			if (status)
> +				return status;
> +		}

While you're at it:  my inverse the polarity of the if check as that reads just
a little more natural, and maybe mention in the warning that the namespace is
forced to read-only mode?

Otherwise this looks good.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: enable ro namespace for ZNS without append

[Javier González]

> On 6 Nov 2020, at 15.14, Christoph Hellwig <hch@lst.de> wrote:
> 
> 
>> 
>> +#ifdef CONFIG_BLK_DEV_ZONED
>> +    if (blk_queue_is_zoned(disk->queue) && !ns->zoned_ns_supp)
>> +        set_disk_ro(disk, true);
>> +#endif
> 
> I think we can simplify this at bit.  Add a new NVME_NS_FORCE_RO flag
> to ns->flags, set it in nvme_update_zone_info, and just query it here
> without any ifdefs.
> 
>>    struct request_queue *q = disk->queue;
>>    struct nvme_command c = { };
>>    struct nvme_id_ns_zns *id;
>> +    bool zoned_ns_supp = true;
>>    int status;
>> 
>>    /* Driver requires zone append support */
>>    if (!(le32_to_cpu(log->iocs[nvme_cmd_zone_append]) &
>>            NVME_CMD_EFFECTS_CSUPP)) {
>> +        zoned_ns_supp = false;
>>        dev_warn(ns->ctrl->device,
>>            "append not supported for zoned namespace:%d\n",
>>            ns->head->ns_id);
>> +    } else {
>> +        /* Lazily query controller append limit for the first
>> +         * zoned namespace
>> +         */
>> +        if (!ns->ctrl->max_zone_append) {
>> +            status = nvme_set_max_append(ns->ctrl);
>> +            if (status)
>> +                return status;
>> +        }
> 
> While you're at it:  my inverse the polarity of the if check as that reads just
> a little more natural, and maybe mention in the warning that the namespace is
> forced to read-only mode?
> 
> Otherwise this looks good.

Thanks. I’ll send a V2. 
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme