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

[Pankaj Raghav]

Hi Christoph,

On 2022-03-16 11:50, Christoph Hellwig wrote:
> On Wed, Mar 16, 2022 at 10:34:23AM +0100, Pankaj Raghav wrote:

>> -		test_bit(NVME_NS_FORCE_RO, &ns->flags));
>> +	set_disk_ro(disk, (id->nsattr & NVME_NS_ATTR_RO));
> 
> This will now set a namespace that was read-only due to unsupported ZNS
> features writable during revalidation.

>>  	blk_queue_max_active_zones(q, le32_to_cpu(id->mar) + 1);
>> +	nvme_set_disk_mode_ro(ns);

> And this will set a disk that is read-only due to NVME_NS_ATTR_RO
> writable if the controller supports all essential ZNS features.
You are right, this is not the right way to fix it.

What do you think about setting the disk permission after the disk info?
That won't introduce any race and also makes sure all parameters are
correctly set:
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 51c08f206cbf..b047fc4e311a 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1854,9 +1854,6 @@ static void nvme_update_disk_info(struct gendisk
*disk,
 	nvme_config_discard(disk, ns);
 	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));
 }

 static inline bool nvme_first_scan(struct gendisk *disk)
@@ -1915,6 +1912,7 @@ static int nvme_update_ns_info(struct nvme_ns *ns,
struct nvme_id_ns *id)
 			goto out_unfreeze;
 	}

+	nvme_set_disk_mode(ns, id);
 	set_bit(NVME_NS_READY, &ns->flags);
 	blk_mq_unfreeze_queue(ns->disk->queue);

diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index e7ccdb119ede..fc75564fb0d1 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -607,6 +607,12 @@ static inline bool nvme_is_path_error(u16 status)
 	return (status & 0x700) == 0x300;
 }

+static inline void nvme_set_disk_mode(struct nvme_ns *ns, struct
nvme_id_ns *id)
+{
+	set_disk_ro(ns->disk, test_bit(NVME_NS_FORCE_RO, &ns->flags) ||
+				      (id->nsattr & NVME_NS_ATTR_RO));
+}
+
 /*
  * 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

The only issue I see with this approach is we are breaking the
encapsulation of updating disk related stuff outside nvme_update_disk_info.
-- 
Regards,
Pankaj