[PATCH] nvme: Fix zns drives without append support to export correct permissions

[Pankaj Raghav]

Hi Damien,

On 2022-03-12 09:14, Damien Le Moal wrote:
> On 3/12/22 05:16, Pankaj Raghav wrote:
>> Fixes: 73d90386b559 ("nvme: cleanup zone information initialization")
>> Signed-off-by: Pankaj Raghav <p.raghav at samsung.com>
>> ---
>>  drivers/nvme/host/core.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>> index 51c08f206cbf..67a78653b07c 100644
>> --- a/drivers/nvme/host/core.c
>> +++ b/drivers/nvme/host/core.c
>> @@ -1913,6 +1913,11 @@ static int nvme_update_ns_info(struct nvme_ns *ns, struct nvme_id_ns *id)
>>  		ret = nvme_update_zone_info(ns, lbaf);
>>  		if (ret)
>>  			goto out_unfreeze;
>> +		/* nvme_update_zone_info might set the namespace to be marked
>> +		 * as read-only. Call nvme_update_disk_info so that the disk
>> +		 * is updated with the appropriate permission.
>> +		 */
> 
> Multi-line comment block should start with a "/*" line without text.
> 
>> +		nvme_update_disk_info(ns->disk, ns, id);
> 
> It may make more sense to move the "set_disk_ro()" call at the end of
> nvme_update_zone_info() into a different helper (say
> nvme_set_disk_mode()) and call that new helper here to avoid going
> through again all the settings that nvme_update_disk_info() does.
That is a good idea.

Based on a quick check, we don't do NVME_NS_FORCE_NO anywhere except
zns.c, so would it make sense to do the following change then?

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 51c08f206cbf..cde33f2a3a5a 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1855,8 +1855,7 @@ static void nvme_update_disk_info(struct gendisk
*disk,
 	blk_queue_max_write_zeroes_sectors(disk->queue,
 					   ns->ctrl->max_zeroes_sectors);

-	set_disk_ro(disk, (id->nsattr & NVME_NS_ATTR_RO) ||
-		test_bit(NVME_NS_FORCE_RO, &ns->flags));
+	set_disk_ro(disk, (id->nsattr & NVME_NS_ATTR_RO));
 }

 static inline bool nvme_first_scan(struct gendisk *disk)
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index e7ccdb119ede..b6800bdd6ea9 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -607,6 +607,11 @@ static inline bool nvme_is_path_error(u16 status)
 	return (status & 0x700) == 0x300;
 }

+static inline void nvme_set_disk_mode_ro(struct nvme_ns *ns)
+{
+	set_disk_ro(ns->disk, test_bit(NVME_NS_FORCE_RO, &ns->flags));
+}
+
 /*
  * Fill in the status and result information from the CQE, and then
figure out
  * if blk-mq will need to use IPI magic to complete the request, and if
yes do
diff --git a/drivers/nvme/host/zns.c b/drivers/nvme/host/zns.c
index 9f81beb4df4e..4ab685fa02b4 100644
--- a/drivers/nvme/host/zns.c
+++ b/drivers/nvme/host/zns.c
@@ -113,6 +113,7 @@ int nvme_update_zone_info(struct nvme_ns *ns,
unsigned lbaf)
 	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);
+	nvme_set_disk_mode_ro(ns);
 free_data:
 	kfree(id);
 	return status;

-- 
Regards,
Pankaj