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

[PATCH v2] 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.

Add a helper nvme_set_disk_ro that can be called in nvme_update_zone_info
to set the permission for ZNS drives correctly.

Fixes: 73d90386b559 ("nvme: cleanup zone information initialization")
Signed-off-by: Pankaj Raghav <p.raghav@samsung.com>
---
Changes since v1:
- Add a new helper to set permission directly from nvme_update_zone_info
  instead of calling nvme_update_disk_info again (Damien)

Note:
Christoph made a point that there is a race window where the disk will
be marked as writable during revalidate zones.
I already responded to the comment in the email thread as it will not be
the case as we mark the disk as ro before we start revalidating the
disk.
https://lore.kernel.org/all/1bbc81a0-19f0-433b-28c2-b22d28176e37@grimberg.me/T/#m0e19022babda81339188ee334551a5fb867abf4c

 drivers/nvme/host/core.c | 3 +--
 drivers/nvme/host/nvme.h | 5 +++++
 drivers/nvme/host/zns.c  | 1 +
 3 files changed, 7 insertions(+), 2 deletions(-)

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;
-- 
2.25.1

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

[Chaitanya Kulkarni]

> +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));
> +}
> +

Do you have a second caller for above function ?

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

[Chaitanya Kulkarni]

On 3/16/22 02:54, Chaitanya Kulkarni wrote:
> 
>> +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));
>> +}
>> +
> 
> Do you have a second caller for above function ?
> 
> 

Is this function in the fast path ?

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

[Pankaj Raghav]

On 2022-03-16 10:56, Chaitanya Kulkarni wrote:
> On 3/16/22 02:54, Chaitanya Kulkarni wrote:
>>
>>> +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));
>>> +}
>>> +
>>
>> Do you have a second caller for above function ?
>>
ATM, no. Open coding them is an option but I don't know if it will be
nice sprinkling the call to set_disk_ro in nvme_update_zone_info.
>>
> 
> Is this function in the fast path ?
> 
No, this is called during init.

-- 
Regards,
Pankaj

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

[Chaitanya Kulkarni]

On 3/16/22 03:21, Pankaj Raghav wrote:
> 
> 
> On 2022-03-16 10:56, Chaitanya Kulkarni wrote:
>> On 3/16/22 02:54, Chaitanya Kulkarni wrote:
>>>
>>>> +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));
>>>> +}
>>>> +
>>>
>>> Do you have a second caller for above function ?
>>>
> ATM, no. Open coding them is an option but I don't know if it will be
> nice sprinkling the call to set_disk_ro in nvme_update_zone_info.
>>>

why are you bloating header file with the function that has no
other caller ?

>>
>> Is this function in the fast path ?
>>
> No, this is called during init.

I don't see any point of making it inline n bloating header file.

> 

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

[Pankaj Raghav]

On 2022-03-16 17:50, Chaitanya Kulkarni wrote:
> On 3/16/22 03:21, Pankaj Raghav wrote:
> why are you bloating header file with the function that has no
> other caller ?
> 
>>>
>>> Is this function in the fast path ?
>>>
>> No, this is called during init.
> 
> I don't see any point of making it inline n bloating header file.
> 
Got it. I will open code it in the next version. I am still waiting for
Christoph to reply on the next proposed solution as this patch has other
issues as he pointed out in his reply.

-- 
Regards,
Pankaj

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

[Christoph Hellwig]

On Wed, Mar 16, 2022 at 10:34:23AM +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 will now set a namespace that was read-only due to unsupported ZNS
features writable during revalidation.

>   * 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);

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.

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

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

[Christoph Hellwig]

On Wed, Mar 16, 2022 at 03:28:30PM +0100, Pankaj Raghav wrote:
> 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:

I don't think we really need the nvme_set_disk_mode helper,  but we
also need this call for the multipath disk.

Otherwise the approach looks good to me.

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

[Chaitanya Kulkarni]

On 3/16/22 02:34, 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.
> 
> Add a helper nvme_set_disk_ro that can be called in nvme_update_zone_info
> to set the permission for ZNS drives correctly.
> 
> Fixes: 73d90386b559 ("nvme: cleanup zone information initialization")
> Signed-off-by: Pankaj Raghav <p.raghav@samsung.com>
> ---


what kind of testing you have done on this patch ?
do you have blktests which addressed this behavior ?

-ck

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

[Pankaj Raghav]

On 2022-03-16 17:52, Chaitanya Kulkarni wrote:
> On 3/16/22 02:34, Pankaj Raghav wrote:
> 
> what kind of testing you have done on this patch ?
It was tested on a real HW. QEMU also does not expose any option to
disable append to this path. The only way is either have a real HW that
doesn't support append or patch QEMU.

> do you have blktests which addressed this behavior ?

I thought about adding a blktest but there is no way AFAIK to disable
append on a HW via any block interface. We probably need some sort of HW
specific handling for different zoned devices. Let me know if you have
some ideas there.

-- 
Regards,
Pankaj