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

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

[Pankaj Raghav]

This commit 2f4c9ba23b88 ("nvme: export zoned namespaces without Zone
Append support read-only") exported zoned namespaces without append support
to be marked as ro. It does it by setting NVME_NS_FORCE_RO to the
ns->flags in nvme_update_zone_info and later nvme_update_disk_info will
check for this flag and set the disk as ro.

But later this commit 73d90386b559 ("nvme: cleanup zone information
initialization") rearranged nvme_update_disk_info to be called before
nvme_update_zone_info thereby not marking the disk as ro. The call order
cannot be just reverted because nvme_update_zone_info sets certain queue
parameters such as zone_write_granularity that depend on the prior call
to nvme_update_disk_info.

Call nvme_update_disk_info after nvme_update_zone_info again so that the
permission for ZNS drives are marked correctly.

Fixes: 73d90386b559 ("nvme: cleanup zone information initialization")
Signed-off-by: Pankaj Raghav <p.raghav@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.
+		 */
+		nvme_update_disk_info(ns->disk, ns, id);
 	}
 
 	set_bit(NVME_NS_READY, &ns->flags);
-- 
2.25.1

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

[Adam Manzanares]

On Fri, Mar 11, 2022 at 09:16:08PM +0100, Pankaj Raghav wrote:
> This commit 2f4c9ba23b88 ("nvme: export zoned namespaces without Zone
> Append support read-only") exported zoned namespaces without append support
> to be marked as ro. It does it by setting NVME_NS_FORCE_RO to the
> ns->flags in nvme_update_zone_info and later nvme_update_disk_info will
> check for this flag and set the disk as ro.
> 
> But later this commit 73d90386b559 ("nvme: cleanup zone information
> initialization") rearranged nvme_update_disk_info to be called before
> nvme_update_zone_info thereby not marking the disk as ro. The call order
> cannot be just reverted because nvme_update_zone_info sets certain queue
> parameters such as zone_write_granularity that depend on the prior call
> to nvme_update_disk_info.
> 
> Call nvme_update_disk_info after nvme_update_zone_info again so that the
> permission for ZNS drives are marked correctly.
> 
> Fixes: 73d90386b559 ("nvme: cleanup zone information initialization")
> Signed-off-by: Pankaj Raghav <p.raghav@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.
> +		 */
> +		nvme_update_disk_info(ns->disk, ns, id);

Shouldn't we make this conditional?

>  	}
>  
>  	set_bit(NVME_NS_READY, &ns->flags);
> -- 
> 2.25.1
> 

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

[Damien Le Moal]

On 3/12/22 05:16, Pankaj Raghav wrote:
> This commit 2f4c9ba23b88 ("nvme: export zoned namespaces without Zone
> Append support read-only") exported zoned namespaces without append support
> to be marked as ro. It does it by setting NVME_NS_FORCE_RO to the
> ns->flags in nvme_update_zone_info and later nvme_update_disk_info will
> check for this flag and set the disk as ro.
> 
> But later this commit 73d90386b559 ("nvme: cleanup zone information
> initialization") rearranged nvme_update_disk_info to be called before
> nvme_update_zone_info thereby not marking the disk as ro. The call order
> cannot be just reverted because nvme_update_zone_info sets certain queue
> parameters such as zone_write_granularity that depend on the prior call
> to nvme_update_disk_info.
> 
> Call nvme_update_disk_info after nvme_update_zone_info again so that the
> permission for ZNS drives are marked correctly.
> 
> Fixes: 73d90386b559 ("nvme: cleanup zone information initialization")
> Signed-off-by: Pankaj Raghav <p.raghav@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.

>  	}
>  
>  	set_bit(NVME_NS_READY, &ns->flags);


-- 
Damien Le Moal
Western Digital Research

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

[Sagi Grimberg]

On 3/11/22 22:16, Pankaj Raghav wrote:
> This commit 2f4c9ba23b88 ("nvme: export zoned namespaces without Zone
> Append support read-only") exported zoned namespaces without append support
> to be marked as ro. It does it by setting NVME_NS_FORCE_RO to the
> ns->flags in nvme_update_zone_info and later nvme_update_disk_info will
> check for this flag and set the disk as ro.
> 
> But later this commit 73d90386b559 ("nvme: cleanup zone information
> initialization") rearranged nvme_update_disk_info to be called before
> nvme_update_zone_info thereby not marking the disk as ro. The call order
> cannot be just reverted because nvme_update_zone_info sets certain queue
> parameters such as zone_write_granularity that depend on the prior call
> to nvme_update_disk_info.
> 
> Call nvme_update_disk_info after nvme_update_zone_info again so that the
> permission for ZNS drives are marked correctly.
> 
> Fixes: 73d90386b559 ("nvme: cleanup zone information initialization")
> Signed-off-by: Pankaj Raghav <p.raghav@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.
> +		 */
> +		nvme_update_disk_info(ns->disk, ns, id);

There is a call just above this, why add another call?

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

[Christoph Hellwig]

On Sun, Mar 13, 2022 at 03:15:08PM +0200, Sagi Grimberg wrote:
> There is a call just above this, why add another call?

NVME_NS_FORCE_RO is set in nvme_update_zone_info, which is called after
nvme_update_disk_info.  I think the suggestion from Damien to move the
set_disk_ro call out of nvme_update_disk_info and directly into
nvme_update_ns_info so that it can be done later is the right thing to
do here.

Re: [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@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

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

[Christoph Hellwig]

On Mon, Mar 14, 2022 at 09:36:35AM +0100, Pankaj Raghav wrote:
> 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));
>  }

This creates a race window during revalidation where the zone will be
set writable here only to be set read-only again a little later.

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

[Damien Le Moal]

On 3/14/22 17:44, Christoph Hellwig wrote:
> On Mon, Mar 14, 2022 at 09:36:35AM +0100, Pankaj Raghav wrote:
>> 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));
>>  }
> 
> This creates a race window during revalidation where the zone will be
> set writable here only to be set read-only again a little later.

Then the only good solution is to have the RO flag set before calling
device_add_disk(), no ?

-- 
Damien Le Moal
Western Digital Research

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

[Pankaj Raghav]

On 2022-03-14 09:44, Christoph Hellwig wrote:
> On Mon, Mar 14, 2022 at 09:36:35AM +0100, Pankaj Raghav wrote:
>> 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));
>>  }
> 
> This creates a race window during revalidation where the zone will be
> set writable here only to be set read-only again a little later.

We call the blk_revalidate_zone after nvme_update_zone_info. So before
we revalidate, the correct permission is already set.

	// sets the disk to ro based on nsattr
	nvme_update_disk_info(ns->disk, ns, id);

	if (ns->head->ids.csi == NVME_CSI_ZNS) {
		// Sets the disk to ro based on append
		ret = nvme_update_zone_info(ns, lbaf);
		if (ret)
			goto out_unfreeze;
	}

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

	if (blk_queue_is_zoned(ns->queue)) {
		// Revalidate is called after the permission is set
		ret = nvme_revalidate_zones(ns);
		if (ret && !nvme_first_scan(ns->disk))
			return ret;
	}

Am I missing something?

-- 
Regards,
Pankaj