From: Dennis Maisenbacher <[email protected]>
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 <[email protected]>
Signed-off-by: Dennis Maisenbacher <[email protected]>
---
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
On 9/6/22 00:39, Dennis Maisenbacher wrote:
> From: Dennis Maisenbacher <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Dennis Maisenbacher <[email protected]>
> ---
> 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 <[email protected]>
On 9/7/22 06:53, Chaitanya Kulkarni wrote:
> On 9/6/22 00:39, Dennis Maisenbacher wrote:
>> From: Dennis Maisenbacher <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Dennis Maisenbacher <[email protected]>
>> ---
>> 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 <[email protected]>
>
--
Damien Le Moal
Western Digital Research
>>> 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
>>>> 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