linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] nvmet: fix mar and mor off-by-one errors
@ 2022-09-06  7:39 Dennis Maisenbacher
  2022-09-06 21:53 ` Chaitanya Kulkarni
  2022-09-07  6:39 ` Christoph Hellwig
  0 siblings, 2 replies; 6+ messages in thread
From: Dennis Maisenbacher @ 2022-09-06  7:39 UTC (permalink / raw)
  To: linux-nvme
  Cc: Dennis Maisenbacher, Niklas Cassel, Christoph Hellwig,
	Sagi Grimberg, Chaitanya Kulkarni, Damien Le Moal, linux-kernel

From: Dennis Maisenbacher <dennis.maisenbacher@wdc.com>

Maximum Active Resources (MAR) and Maximum Open Resources (MOR) are 0's
based vales where a value of 0xffffffff indicates that there is no limit.

Decrement the values that are returned by bdev_max_open_zones and
bdev_max_active_zones as the block layer helpers are not 0's based.
A 0 returned by the block layer helpers indicates no limit, thus convert
it to 0xffffffff (U32_MAX).

Fixes: aaf2e048af27 ("nvmet: add ZBD over ZNS backend support")
Suggested-by: Niklas Cassel <niklas.cassel@wdc.com>
Signed-off-by: Dennis Maisenbacher <dennis.maisenbacher@wdc.com>
---
Changes in v2:
    - Add explicit check if block layer helpers return a 0 and if so
    convert it to U32_MAX.
    - Add Fixes tag.

 drivers/nvme/target/zns.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/target/zns.c b/drivers/nvme/target/zns.c
index c7ef69f29fe4..eae81f939067 100644
--- a/drivers/nvme/target/zns.c
+++ b/drivers/nvme/target/zns.c
@@ -100,6 +100,7 @@ void nvmet_execute_identify_cns_cs_ns(struct nvmet_req *req)
 	struct nvme_id_ns_zns *id_zns;
 	u64 zsze;
 	u16 status;
+	u32 mar, mor;
 
 	if (le32_to_cpu(req->cmd->identify.nsid) == NVME_NSID_ALL) {
 		req->error_loc = offsetof(struct nvme_identify, nsid);
@@ -130,8 +131,20 @@ void nvmet_execute_identify_cns_cs_ns(struct nvmet_req *req)
 	zsze = (bdev_zone_sectors(req->ns->bdev) << 9) >>
 					req->ns->blksize_shift;
 	id_zns->lbafe[0].zsze = cpu_to_le64(zsze);
-	id_zns->mor = cpu_to_le32(bdev_max_open_zones(req->ns->bdev));
-	id_zns->mar = cpu_to_le32(bdev_max_active_zones(req->ns->bdev));
+
+	mor = bdev_max_open_zones(req->ns->bdev);
+	if (!mor)
+		mor = U32_MAX;
+	else
+		--mor;
+	id_zns->mor = cpu_to_le32(mor);
+
+	mar = bdev_max_active_zones(req->ns->bdev);
+	if (!mar)
+		mar = U32_MAX;
+	else
+		--mar;
+	id_zns->mar = cpu_to_le32(mar);
 
 done:
 	status = nvmet_copy_to_sgl(req, 0, id_zns, sizeof(*id_zns));
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH v2] nvmet: fix mar and mor off-by-one errors
  2022-09-06  7:39 [PATCH v2] nvmet: fix mar and mor off-by-one errors Dennis Maisenbacher
@ 2022-09-06 21:53 ` Chaitanya Kulkarni
  2022-09-06 22:35   ` Damien Le Moal
  2022-09-07  6:39 ` Christoph Hellwig
  1 sibling, 1 reply; 6+ messages in thread
From: Chaitanya Kulkarni @ 2022-09-06 21:53 UTC (permalink / raw)
  To: Dennis Maisenbacher, linux-nvme
  Cc: Niklas Cassel, Christoph Hellwig, Sagi Grimberg,
	Chaitanya Kulkarni, Damien Le Moal, linux-kernel

On 9/6/22 00:39, Dennis Maisenbacher wrote:
> From: Dennis Maisenbacher <dennis.maisenbacher@wdc.com>
> 
> Maximum Active Resources (MAR) and Maximum Open Resources (MOR) are 0's
> based vales where a value of 0xffffffff indicates that there is no limit.
> 
> Decrement the values that are returned by bdev_max_open_zones and
> bdev_max_active_zones as the block layer helpers are not 0's based.
> A 0 returned by the block layer helpers indicates no limit, thus convert
> it to 0xffffffff (U32_MAX).
> 
> Fixes: aaf2e048af27 ("nvmet: add ZBD over ZNS backend support")
> Suggested-by: Niklas Cassel <niklas.cassel@wdc.com>
> Signed-off-by: Dennis Maisenbacher <dennis.maisenbacher@wdc.com>
> ---
> Changes in v2:
>      - Add explicit check if block layer helpers return a 0 and if so
>      convert it to U32_MAX.
>      - Add Fixes tag.
> 
>   drivers/nvme/target/zns.c | 17 +++++++++++++++--
>   1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvme/target/zns.c b/drivers/nvme/target/zns.c
> index c7ef69f29fe4..eae81f939067 100644
> --- a/drivers/nvme/target/zns.c
> +++ b/drivers/nvme/target/zns.c
> @@ -100,6 +100,7 @@ void nvmet_execute_identify_cns_cs_ns(struct nvmet_req *req)
>   	struct nvme_id_ns_zns *id_zns;
>   	u64 zsze;
>   	u16 status;
> +	u32 mar, mor;

consider :-
  +	u32 mar, mor;
    	u64 zsze;
    	u16 status;

>   
>   	if (le32_to_cpu(req->cmd->identify.nsid) == NVME_NSID_ALL) {
>   		req->error_loc = offsetof(struct nvme_identify, nsid);
> @@ -130,8 +131,20 @@ void nvmet_execute_identify_cns_cs_ns(struct nvmet_req *req)
>   	zsze = (bdev_zone_sectors(req->ns->bdev) << 9) >>
>   					req->ns->blksize_shift;
>   	id_zns->lbafe[0].zsze = cpu_to_le64(zsze);
> -	id_zns->mor = cpu_to_le32(bdev_max_open_zones(req->ns->bdev));
> -	id_zns->mar = cpu_to_le32(bdev_max_active_zones(req->ns->bdev));
> +
> +	mor = bdev_max_open_zones(req->ns->bdev);
> +	if (!mor)
> +		mor = U32_MAX;
> +	else
> +		--mor;
> +	id_zns->mor = cpu_to_le32(mor);
> +
> +	mar = bdev_max_active_zones(req->ns->bdev);
> +	if (!mar)
> +		mar = U32_MAX;
> +	else
> +		--mar;
> +	id_zns->mar = cpu_to_le32(mar);
>   

above 14 lines of code can be simplified as in 4-5 lines :-

mor = bdev_max_open_zones(req->ns->bdev);
id->zns->mor = cpu_tp_le32(mor ? mor - 1 : U32_MAX);

mar = bdev_max_active_zones(req->ns->bdev);
id->zns->mar = cpu_tp_le32(mar ? mar - 1 : U32_MAX);

>   done:
>   	status = nvmet_copy_to_sgl(req, 0, id_zns, sizeof(*id_zns));


either way,

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH v2] nvmet: fix mar and mor off-by-one errors
  2022-09-06 21:53 ` Chaitanya Kulkarni
@ 2022-09-06 22:35   ` Damien Le Moal
  2022-09-07  2:43     ` Chaitanya Kulkarni
  0 siblings, 1 reply; 6+ messages in thread
From: Damien Le Moal @ 2022-09-06 22:35 UTC (permalink / raw)
  To: Chaitanya Kulkarni, Dennis Maisenbacher, linux-nvme
  Cc: Niklas Cassel, Christoph Hellwig, Sagi Grimberg, Damien Le Moal,
	linux-kernel

On 9/7/22 06:53, Chaitanya Kulkarni wrote:
> On 9/6/22 00:39, Dennis Maisenbacher wrote:
>> From: Dennis Maisenbacher <dennis.maisenbacher@wdc.com>
>>
>> Maximum Active Resources (MAR) and Maximum Open Resources (MOR) are 0's
>> based vales where a value of 0xffffffff indicates that there is no limit.
>>
>> Decrement the values that are returned by bdev_max_open_zones and
>> bdev_max_active_zones as the block layer helpers are not 0's based.
>> A 0 returned by the block layer helpers indicates no limit, thus convert
>> it to 0xffffffff (U32_MAX).
>>
>> Fixes: aaf2e048af27 ("nvmet: add ZBD over ZNS backend support")
>> Suggested-by: Niklas Cassel <niklas.cassel@wdc.com>
>> Signed-off-by: Dennis Maisenbacher <dennis.maisenbacher@wdc.com>
>> ---
>> Changes in v2:
>>      - Add explicit check if block layer helpers return a 0 and if so
>>      convert it to U32_MAX.
>>      - Add Fixes tag.
>>
>>   drivers/nvme/target/zns.c | 17 +++++++++++++++--
>>   1 file changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/nvme/target/zns.c b/drivers/nvme/target/zns.c
>> index c7ef69f29fe4..eae81f939067 100644
>> --- a/drivers/nvme/target/zns.c
>> +++ b/drivers/nvme/target/zns.c
>> @@ -100,6 +100,7 @@ void nvmet_execute_identify_cns_cs_ns(struct nvmet_req *req)
>>   	struct nvme_id_ns_zns *id_zns;
>>   	u64 zsze;
>>   	u16 status;
>> +	u32 mar, mor;
> 
> consider :-
>   +	u32 mar, mor;
>     	u64 zsze;
>     	u16 status;
> 
>>   
>>   	if (le32_to_cpu(req->cmd->identify.nsid) == NVME_NSID_ALL) {
>>   		req->error_loc = offsetof(struct nvme_identify, nsid);
>> @@ -130,8 +131,20 @@ void nvmet_execute_identify_cns_cs_ns(struct nvmet_req *req)
>>   	zsze = (bdev_zone_sectors(req->ns->bdev) << 9) >>
>>   					req->ns->blksize_shift;
>>   	id_zns->lbafe[0].zsze = cpu_to_le64(zsze);
>> -	id_zns->mor = cpu_to_le32(bdev_max_open_zones(req->ns->bdev));
>> -	id_zns->mar = cpu_to_le32(bdev_max_active_zones(req->ns->bdev));
>> +
>> +	mor = bdev_max_open_zones(req->ns->bdev);
>> +	if (!mor)
>> +		mor = U32_MAX;
>> +	else
>> +		--mor;
>> +	id_zns->mor = cpu_to_le32(mor);
>> +
>> +	mar = bdev_max_active_zones(req->ns->bdev);
>> +	if (!mar)
>> +		mar = U32_MAX;
>> +	else
>> +		--mar;
>> +	id_zns->mar = cpu_to_le32(mar);
>>   
> 
> above 14 lines of code can be simplified as in 4-5 lines :-

Simplified ? It is much harder to read in my opinion...

> 
> mor = bdev_max_open_zones(req->ns->bdev);
> id->zns->mor = cpu_tp_le32(mor ? mor - 1 : U32_MAX);
> 
> mar = bdev_max_active_zones(req->ns->bdev);
> id->zns->mar = cpu_tp_le32(mar ? mar - 1 : U32_MAX);
> 
>>   done:
>>   	status = nvmet_copy_to_sgl(req, 0, id_zns, sizeof(*id_zns));
> 
> 
> either way,
> 
> Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
> 

-- 
Damien Le Moal
Western Digital Research


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH v2] nvmet: fix mar and mor off-by-one errors
  2022-09-06 22:35   ` Damien Le Moal
@ 2022-09-07  2:43     ` Chaitanya Kulkarni
  2022-09-07  7:04       ` Dennis Maisenbacher
  0 siblings, 1 reply; 6+ messages in thread
From: Chaitanya Kulkarni @ 2022-09-07  2:43 UTC (permalink / raw)
  To: Damien Le Moal, Dennis Maisenbacher, linux-nvme
  Cc: Niklas Cassel, Christoph Hellwig, Sagi Grimberg, Damien Le Moal,
	linux-kernel


>>>    	if (le32_to_cpu(req->cmd->identify.nsid) == NVME_NSID_ALL) {
>>>    		req->error_loc = offsetof(struct nvme_identify, nsid);
>>> @@ -130,8 +131,20 @@ void nvmet_execute_identify_cns_cs_ns(struct nvmet_req *req)
>>>    	zsze = (bdev_zone_sectors(req->ns->bdev) << 9) >>
>>>    					req->ns->blksize_shift;
>>>    	id_zns->lbafe[0].zsze = cpu_to_le64(zsze);
>>> -	id_zns->mor = cpu_to_le32(bdev_max_open_zones(req->ns->bdev));
>>> -	id_zns->mar = cpu_to_le32(bdev_max_active_zones(req->ns->bdev));
>>> +
>>> +	mor = bdev_max_open_zones(req->ns->bdev);
>>> +	if (!mor)
>>> +		mor = U32_MAX;
>>> +	else
>>> +		--mor;
>>> +	id_zns->mor = cpu_to_le32(mor);
>>> +
>>> +	mar = bdev_max_active_zones(req->ns->bdev);
>>> +	if (!mar)
>>> +		mar = U32_MAX;
>>> +	else
>>> +		--mar;
>>> +	id_zns->mar = cpu_to_le32(mar);
>>>    
>>
>> above 14 lines of code can be simplified as in 4-5 lines :-
> 
> Simplified ? It is much harder to read in my opinion...
> 
>>

there are two if ... else ... doing identical things on same data
type u32 and its return type is also same le32,
if my suggestion is hard to read then common code needs
to be moved to the helper as it is not clear the need for
code duplication from commit message.

-ck


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH v2] nvmet: fix mar and mor off-by-one errors
  2022-09-06  7:39 [PATCH v2] nvmet: fix mar and mor off-by-one errors Dennis Maisenbacher
  2022-09-06 21:53 ` Chaitanya Kulkarni
@ 2022-09-07  6:39 ` Christoph Hellwig
  1 sibling, 0 replies; 6+ messages in thread
From: Christoph Hellwig @ 2022-09-07  6:39 UTC (permalink / raw)
  To: Dennis Maisenbacher
  Cc: linux-nvme, Niklas Cassel, Christoph Hellwig, Sagi Grimberg,
	Chaitanya Kulkarni, Damien Le Moal, linux-kernel

Thanks,

applied to nvme-6.0.

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH v2] nvmet: fix mar and mor off-by-one errors
  2022-09-07  2:43     ` Chaitanya Kulkarni
@ 2022-09-07  7:04       ` Dennis Maisenbacher
  0 siblings, 0 replies; 6+ messages in thread
From: Dennis Maisenbacher @ 2022-09-07  7:04 UTC (permalink / raw)
  To: Chaitanya Kulkarni, Damien Le Moal, linux-nvme
  Cc: Niklas Cassel, Christoph Hellwig, Sagi Grimberg, Damien Le Moal,
	linux-kernel

>>>>     if (le32_to_cpu(req->cmd->identify.nsid) == NVME_NSID_ALL) {
>>>>             req->error_loc = offsetof(struct nvme_identify, nsid);
>>>> @@ -130,8 +131,20 @@ void nvmet_execute_identify_cns_cs_ns(struct nvmet_req *req)
>>>>     zsze = (bdev_zone_sectors(req->ns->bdev) << 9) >>
>>>>                                     req->ns->blksize_shift;
>>>>     id_zns->lbafe[0].zsze = cpu_to_le64(zsze);
>>>> -   id_zns->mor = cpu_to_le32(bdev_max_open_zones(req->ns->bdev));
>>>> -   id_zns->mar = cpu_to_le32(bdev_max_active_zones(req->ns->bdev));
>>>> +
>>>> +   mor = bdev_max_open_zones(req->ns->bdev);
>>>> +   if (!mor)
>>>> +           mor = U32_MAX;
>>>> +   else
>>>> +           --mor;
>>>> +   id_zns->mor = cpu_to_le32(mor);
>>>> +
>>>> +   mar = bdev_max_active_zones(req->ns->bdev);
>>>> +   if (!mar)
>>>> +           mar = U32_MAX;
>>>> +   else
>>>> +           --mar;
>>>> +   id_zns->mar = cpu_to_le32(mar);
>>>>
>>>
>>> above 14 lines of code can be simplified as in 4-5 lines :-
>>
>> Simplified ? It is much harder to read in my opinion...
>>
>>>
>
>there are two if ... else ... doing identical things on same data
>type u32 and its return type is also same le32,
>if my suggestion is hard to read then common code needs
>to be moved to the helper as it is not clear the need for
>code duplication from commit message.

For my taste the conditional operator would have looked like a good
tradeoff in this case. It condenses the code and verbosely converts a 0 to
U32_MAX. A new helper would have read like the conditional operator.

Christoph already applied the patch. In any case thanks for your comments! 

Dennis


^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-09-07  7:04 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-06  7:39 [PATCH v2] nvmet: fix mar and mor off-by-one errors Dennis Maisenbacher
2022-09-06 21:53 ` Chaitanya Kulkarni
2022-09-06 22:35   ` Damien Le Moal
2022-09-07  2:43     ` Chaitanya Kulkarni
2022-09-07  7:04       ` Dennis Maisenbacher
2022-09-07  6:39 ` Christoph Hellwig

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).