[PATCH] Revert "nvme: verify MNAN value if ANA is enabled"

[PATCH] Revert "nvme: verify MNAN value if ANA is enabled"

[Chaitanya Kulkarni]

This reverts commit 8872c159c7a83daf633768cee7a7ef7154010341. This is
needed to move forward with the blktests for now, without this patch
all the testcases result in the error :-

[ 3502.072798] nvme nvme1: Invalid MNAN value 1024

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/nvme/host/multipath.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index 23573fe3fc7d..a501917d4c6e 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -813,13 +813,6 @@ int nvme_mpath_init_identify(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
 	    !(ctrl->subsys->cmic & NVME_CTRL_CMIC_ANA))
 		return 0;
 
-	if (!ctrl->max_namespaces ||
-	    ctrl->max_namespaces > le32_to_cpu(id->nn)) {
-		dev_err(ctrl->device,
-			"Invalid MNAN value %u\n", ctrl->max_namespaces);
-		return -EINVAL;
-	}
-
 	ctrl->anacap = id->anacap;
 	ctrl->anatt = id->anatt;
 	ctrl->nanagrpid = le32_to_cpu(id->nanagrpid);
-- 
2.22.1


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] Revert "nvme: verify MNAN value if ANA is enabled"

[Chaitanya Kulkarni]

Daniel,

On 6/9/21 19:45, Chaitanya Kulkarni wrote:
> This reverts commit 8872c159c7a83daf633768cee7a7ef7154010341. This is
> needed to move forward with the blktests for now, without this patch
> all the testcases result in the error :-
>
> [ 3502.072798] nvme nvme1: Invalid MNAN value 1024
>
> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

Thinking about the code again I think following should work :-

diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index 23573fe3fc7d..4277f1554bd5 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -813,7 +813,7 @@ int nvme_mpath_init_identify(struct nvme_ctrl *ctrl,
struct nvme_id_ctrl *id)
            !(ctrl->subsys->cmic & NVME_CTRL_CMIC_ANA))
                return 0;
 
-       if (!ctrl->max_namespaces ||
+       if (ctrl->max_namespaces &&
            ctrl->max_namespaces > le32_to_cpu(id->nn)) {
                dev_err(ctrl->device,
                        "Invalid MNAN value %u\n", ctrl->max_namespaces);
diff --git a/drivers/nvme/target/admin-cmd.c
b/drivers/nvme/target/admin-cmd.c
index cd60a8184d04..a8ec377bb68d 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -394,7 +394,7 @@ static void nvmet_execute_identify_ctrl(struct
nvmet_req *req)
        id->maxcmd = cpu_to_le16(NVMET_MAX_CMD);
 
        id->nn = cpu_to_le32(ctrl->subsys->max_nsid);
-       id->mnan = cpu_to_le32(NVMET_MAX_NAMESPACES);
+       id->mnan = cpu_to_le32(ctrl->subsys->max_nsid);
        id->oncs = cpu_to_le16(NVME_CTRL_ONCS_DSM |
                        NVME_CTRL_ONCS_WRITE_ZEROES);
 

If it does, then we can drop the this revert and I'll send above 2 fixes.

Daniel, can you please confirm ?



_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] Revert "nvme: verify MNAN value if ANA is enabled"

[Daniel Wagner]

Hi Chaitanya,

On Thu, Jun 10, 2021 at 02:55:08AM +0000, Chaitanya Kulkarni wrote:
> On 6/9/21 19:45, Chaitanya Kulkarni wrote:
> > This reverts commit 8872c159c7a83daf633768cee7a7ef7154010341. This is
> > needed to move forward with the blktests for now, without this patch
> > all the testcases result in the error :-
> >
> > [ 3502.072798] nvme nvme1: Invalid MNAN value 1024
> 
> Thinking about the code again I think following should work :-
> 
> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
> index 23573fe3fc7d..4277f1554bd5 100644
> --- a/drivers/nvme/host/multipath.c
> +++ b/drivers/nvme/host/multipath.c
> @@ -813,7 +813,7 @@ int nvme_mpath_init_identify(struct nvme_ctrl *ctrl,
> struct nvme_id_ctrl *id)
>             !(ctrl->subsys->cmic & NVME_CTRL_CMIC_ANA))
>                 return 0;
>  
> -       if (!ctrl->max_namespaces ||
> +       if (ctrl->max_namespaces &&
>             ctrl->max_namespaces > le32_to_cpu(id->nn)) {
>                 dev_err(ctrl->device,
>                         "Invalid MNAN value %u\n", ctrl->max_namespaces);

'!ctrl->max_namespace' could also be written as
'ctrl->max_namespace != 0' which makes it more obvious what the intend
is here:

  If the controller supports Asymmetric Namespace Access Reporting, then
  this field shall be set to a non-zero value that is less than or equal
  to the NN value.


> diff --git a/drivers/nvme/target/admin-cmd.c
> b/drivers/nvme/target/admin-cmd.c
> index cd60a8184d04..a8ec377bb68d 100644
> --- a/drivers/nvme/target/admin-cmd.c
> +++ b/drivers/nvme/target/admin-cmd.c
> @@ -394,7 +394,7 @@ static void nvmet_execute_identify_ctrl(struct
> nvmet_req *req)
>         id->maxcmd = cpu_to_le16(NVMET_MAX_CMD);
>  
>         id->nn = cpu_to_le32(ctrl->subsys->max_nsid);
> -       id->mnan = cpu_to_le32(NVMET_MAX_NAMESPACES);
> +       id->mnan = cpu_to_le32(ctrl->subsys->max_nsid);
>         id->oncs = cpu_to_le16(NVME_CTRL_ONCS_DSM |
>                         NVME_CTRL_ONCS_WRITE_ZEROES);

This looks like the right fix for the upper limit problem.
I can see the tests are still failing with

  nvme nvme0: Invalid MNAN value 0

which indicates there is another problem in the nvmet core.

I try to find it.x

Thanks,
Daniel

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] Revert "nvme: verify MNAN value if ANA is enabled"

[Daniel Wagner]

On Thu, Jun 10, 2021 at 09:45:46AM +0200, Daniel Wagner wrote:
> >         id->nn = cpu_to_le32(ctrl->subsys->max_nsid);
> > -       id->mnan = cpu_to_le32(NVMET_MAX_NAMESPACES);
> > +       id->mnan = cpu_to_le32(ctrl->subsys->max_nsid);
> >         id->oncs = cpu_to_le16(NVME_CTRL_ONCS_DSM |
> >                         NVME_CTRL_ONCS_WRITE_ZEROES);
> 

I am a bit confused how the target code indexes the namespaces. In 6.1
Namespaces the valid range for NSID is defined from 0 to namespace
-1. In 6.1.2 it says 'Any NSID is valid, except if that NSID is 0h or
greater than the Number of Namespaces'. And the figure 348 shows clearly
NSID 0 is not valid.

Looking through the code I think the nsid range used starts at 0 and not
at 1. Which would explain why the above change is not enough. The valid
range for max_nsid should start at 1 and not 0.

Thoughts?

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] Revert "nvme: verify MNAN value if ANA is enabled"

[Chaitanya Kulkarni]

On 6/10/21 00:45, Daniel Wagner wrote:
> Hi Chaitanya,
>
> On Thu, Jun 10, 2021 at 02:55:08AM +0000, Chaitanya Kulkarni wrote:
>> On 6/9/21 19:45, Chaitanya Kulkarni wrote:
>>> This reverts commit 8872c159c7a83daf633768cee7a7ef7154010341. This is
>>> needed to move forward with the blktests for now, without this patch
>>> all the testcases result in the error :-
>>>
>>> [ 3502.072798] nvme nvme1: Invalid MNAN value 1024
>> Thinking about the code again I think following should work :-
>>
>> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
>> index 23573fe3fc7d..4277f1554bd5 100644
>> --- a/drivers/nvme/host/multipath.c
>> +++ b/drivers/nvme/host/multipath.c
>> @@ -813,7 +813,7 @@ int nvme_mpath_init_identify(struct nvme_ctrl *ctrl,
>> struct nvme_id_ctrl *id)
>>             !(ctrl->subsys->cmic & NVME_CTRL_CMIC_ANA))
>>                 return 0;
>>  
>> -       if (!ctrl->max_namespaces ||
>> +       if (ctrl->max_namespaces &&
>>             ctrl->max_namespaces > le32_to_cpu(id->nn)) {
>>                 dev_err(ctrl->device,
>>                         "Invalid MNAN value %u\n", ctrl->max_namespaces);
> '!ctrl->max_namespace' could also be written as
> 'ctrl->max_namespace != 0' which makes it more obvious what the intend
> is here:
>
>   If the controller supports Asymmetric Namespace Access Reporting, then
>   this field shall be set to a non-zero value that is less than or equal
>   to the NN value.
>
>

Let us look into the host side issue first then we can move to target side.

Consider a scenario where ANA enabled subsys with 0 namespaces on the target
side. When host issues connect command to such a controller ctrl->mnan
should be 0 and ctrl->nn should be 0 which should be valid.

With original check in the code :-

if (!ctrl->max_namespaces ||
    ctrl->max_namespaces > le32_to_cpu(id->nn))

!ctrl->max_namespaces will return in 1 and due to || condition will be
true and code will report the error. The change proposed in this patch
with above mentioned scenario :- if (ctrl->max_namespaces &&
ctrl->max_namespaces > le32_to_cpu(id->nn)) { ctrl->max_namespaces will
return 0 and due to && condition will be false and new code will not
report zero. So I think above suggested patch is needed irrespective of
the target fix.


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] Revert "nvme: verify MNAN value if ANA is enabled"

[Chaitanya Kulkarni]

On 6/10/21 00:45, Daniel Wagner wrote:
>> diff --git a/drivers/nvme/target/admin-cmd.c
>> b/drivers/nvme/target/admin-cmd.c
>> index cd60a8184d04..a8ec377bb68d 100644
>> --- a/drivers/nvme/target/admin-cmd.c
>> +++ b/drivers/nvme/target/admin-cmd.c
>> @@ -394,7 +394,7 @@ static void nvmet_execute_identify_ctrl(struct
>> nvmet_req *req)
>>         id->maxcmd = cpu_to_le16(NVMET_MAX_CMD);
>>  
>>         id->nn = cpu_to_le32(ctrl->subsys->max_nsid);
>> -       id->mnan = cpu_to_le32(NVMET_MAX_NAMESPACES);
>> +       id->mnan = cpu_to_le32(ctrl->subsys->max_nsid);
>>         id->oncs = cpu_to_le16(NVME_CTRL_ONCS_DSM |
>>                         NVME_CTRL_ONCS_WRITE_ZEROES);
> This looks like the right fix for the upper limit problem.
> I can see the tests are still failing with
>
>   nvme nvme0: Invalid MNAN value 0
>
> which indicates there is another problem in the nvmet core.
>
> I try to find it.x
>
> Thanks,
> Daniel
>


With following two patches blktests test-cases are passing for me.

root@vm nvme (nvme-5.14) # git log -2
commit cd5d6d55a405f97ad5c31a738976eea0e16ebe20 (HEAD -> nvme-5.14)
Author: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
Date:   Wed Jun 9 20:07:56 2021 -0700

    nvmet: set the mnan value to nn
   
    From spec :-
    "MNAN : If the controller supports Asymmetric Namespace Access
Reporting,
    then this field shall be set to a non-zero value that is less than or
    equal tothe NN value".
   
    In nvmet_execute_identify_ctrl() when building the identify controller
    data structure set the mnan value to nn to follow the spec.
   
    Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

commit 1368a1a5e7566d726bf74234d05895c3f0d54690
Author: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
Date:   Wed Jun 9 20:07:00 2021 -0700

    nvme: fix the comparison in the mnan check
   
    The existing check for the valid mnan value will result in the error
    when ctrl->max_namespaces are set to the 1024 from NVMeOF target since
    !1024 == 0 so it will lead to next comparison 1024 > is->nn which will
    be always true untill target has 1024 namespaces.
   
    This patch fixes the comparison.
   
    Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
root@vm nvme (nvme-5.14) # cat
0001-nvme-fix-the-comparison-in-the-mnan-check.patch
From f1f0947ca495b7d9b8721411e407ebf9efad0df9 Mon Sep 17 00:00:00 2001
From: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
Date: Wed, 9 Jun 2021 20:07:00 -0700
Subject: [PATCH 1/2] nvme: fix the comparison in the mnan check

The existing check for the valid mnan value will result in the error
when ctrl->max_namespaces are set to the 1024 from NVMeOF target since
!1024 == 0 so it will lead to next comparison 1024 > is->nn which will
be always true untill target has 1024 namespaces.

This patch fixes the comparison.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/nvme/host/multipath.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index 23573fe3fc7d..4277f1554bd5 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -813,7 +813,7 @@ int nvme_mpath_init_identify(struct nvme_ctrl *ctrl,
struct nvme_id_ctrl *id)
         !(ctrl->subsys->cmic & NVME_CTRL_CMIC_ANA))
         return 0;
 
-    if (!ctrl->max_namespaces ||
+    if (ctrl->max_namespaces &&
         ctrl->max_namespaces > le32_to_cpu(id->nn)) {
         dev_err(ctrl->device,
             "Invalid MNAN value %u\n", ctrl->max_namespaces);
-- 
2.22.1

root@vm nvme (nvme-5.14) # cat 0002-nvmet-set-the-mnan-value-to-nn.patch
From d18c2c851686c4e5fa8ddb457e6726cafb86b85f Mon Sep 17 00:00:00 2001
From: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
Date: Wed, 9 Jun 2021 20:07:56 -0700
Subject: [PATCH 2/2] nvmet: set the mnan value to nn

From spec :-
"MNAN : If the controller supports Asymmetric Namespace Access Reporting,
then this field shall be set to a non-zero value that is less than or
equal tothe NN value".

In nvmet_execute_identify_ctrl() when building the identify controller
data structure set the mnan value to nn to follow the spec.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/nvme/target/admin-cmd.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/target/admin-cmd.c
b/drivers/nvme/target/admin-cmd.c
index cd60a8184d04..88e85d9a5be8 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -394,7 +394,11 @@ static void nvmet_execute_identify_ctrl(struct
nvmet_req *req)
     id->maxcmd = cpu_to_le16(NVMET_MAX_CMD);
 
     id->nn = cpu_to_le32(ctrl->subsys->max_nsid);
-    id->mnan = cpu_to_le32(NVMET_MAX_NAMESPACES);
+    if (IS_ENABLED(CONFIG_NVME_MULTIPATH))
+        id->mnan = cpu_to_le32(ctrl->subsys->max_nsid);
+    else
+        id->mnan = cpu_to_le32(NVMET_MAX_NAMESPACES);
+
     id->oncs = cpu_to_le16(NVME_CTRL_ONCS_DSM |
             NVME_CTRL_ONCS_WRITE_ZEROES);
 
-- 
2.22.1

root@vm nvme (nvme-5.14) # ./compile_nvme.sh
+ umount /mnt/nvme0n1
+ clear_dmesg
umount: /mnt/nvme0n1: not mounted
+ modprobe -r nvme-fabrics
+ modprobe -r nvme_loop
+ modprobe -r nvmet
+ modprobe -r nvme
+ sleep 1
+ modprobe -r nvme-core
+ lsmod
+ grep nvme
+ git apply ./all-fixes.diff
+ sleep 1
++ nproc
+ make -j 64 M=drivers/nvme/ modules
  CC [M]  drivers/nvme//target/loop.o
  LD [M]  drivers/nvme//target/nvme-loop.o
  MODPOST drivers/nvme//Module.symvers
  LD [M]  drivers/nvme//target/nvme-loop.ko
+ HOST=drivers/nvme/host
+ TARGET=drivers/nvme/target
++ uname -r
+ HOST_DEST=/lib/modules/5.13.0-rc3nvme+/kernel/drivers/nvme/host/
++ uname -r
+ TARGET_DEST=/lib/modules/5.13.0-rc3nvme+/kernel/drivers/nvme/target/
+ cp drivers/nvme/host/nvme-core.ko drivers/nvme/host/nvme-fabrics.ko
drivers/nvme/host/nvme-fc.ko drivers/nvme/host/nvme.ko
drivers/nvme/host/nvme-rdma.ko drivers/nvme/host/nvme-tcp.ko
/lib/modules/5.13.0-rc3nvme+/kernel/drivers/nvme/host//
+ cp drivers/nvme/target/nvme-fcloop.ko drivers/nvme/target/nvme-loop.ko
drivers/nvme/target/nvmet-fc.ko drivers/nvme/target/nvmet.ko
drivers/nvme/target/nvmet-rdma.ko drivers/nvme/target/nvmet-tcp.ko
/lib/modules/5.13.0-rc3nvme+/kernel/drivers/nvme/target//
+ ls -lrth /lib/modules/5.13.0-rc3nvme+/kernel/drivers/nvme/host/
/lib/modules/5.13.0-rc3nvme+/kernel/drivers/nvme/target//
/lib/modules/5.13.0-rc3nvme+/kernel/drivers/nvme/host/:
total 9.1M
-rw-r--r--. 1 root root 4.0M Jun 10 13:50 nvme-core.ko
-rw-r--r--. 1 root root 639K Jun 10 13:50 nvme-fabrics.ko
-rw-r--r--. 1 root root 1.2M Jun 10 13:50 nvme-fc.ko
-rw-r--r--. 1 root root 1.1M Jun 10 13:50 nvme.ko
-rw-r--r--. 1 root root 1.2M Jun 10 13:50 nvme-rdma.ko
-rw-r--r--. 1 root root 1.1M Jun 10 13:50 nvme-tcp.ko

/lib/modules/5.13.0-rc3nvme+/kernel/drivers/nvme/target//:
total 8.3M
-rw-r--r--. 1 root root 718K Jun 10 13:50 nvme-fcloop.ko
-rw-r--r--. 1 root root 623K Jun 10 13:50 nvme-loop.ko
-rw-r--r--. 1 root root 1.1M Jun 10 13:50 nvmet-fc.ko
-rw-r--r--. 1 root root 4.0M Jun 10 13:50 nvmet.ko
-rw-r--r--. 1 root root 1.1M Jun 10 13:50 nvmet-rdma.ko
-rw-r--r--. 1 root root 868K Jun 10 13:50 nvmet-tcp.ko
+ modprobe nvme
+ git co drivers/nvme/target/loop.c
Updated 1 path from the index
root@vm nvme (nvme-5.14) # cdblktests
root@vm blktests (master) # ./check nvme
nvme/002 (create many subsystems and test discovery)         [passed]
    runtime  40.408s  ...  40.579s
nvme/003 (test if we're sending keep-alives to a discovery controller)
[passed]
    runtime  10.179s  ...  10.180s
nvme/004 (test nvme and nvmet UUID NS descriptors)           [passed]
    runtime  1.786s  ...  1.794s
nvme/005 (reset local loopback target)                       [passed]
    runtime  2.241s  ...  2.233s
nvme/006 (create an NVMeOF target with a block device-backed ns) [passed]
    runtime  0.147s  ...  0.152s
nvme/007 (create an NVMeOF target with a file-backed ns)     [passed]
    runtime  0.097s  ...  0.102s
nvme/008 (create an NVMeOF host with a block device-backed ns) [passed]
    runtime  1.793s  ...  1.792s
nvme/009 (create an NVMeOF host with a file-backed ns)       [passed]
    runtime  1.749s  ...  1.751s
nvme/010 (run data verification fio job on NVMeOF block device-backed
ns) [passed]
    runtime  38.113s  ...  31.199s
nvme/011 (run data verification fio job on NVMeOF file-backed ns) [passed]
    runtime  296.910s  ...  302.248s
nvme/012 (run mkfs and data verification fio job on NVMeOF block
device-backed ns) [passed]
    runtime  8.823s  ...  9.221s
nvme/013 (run mkfs and data verification fio job on NVMeOF file-backed
ns) [passed]
    runtime  23.517s  ...  24.288s
nvme/014 (flush a NVMeOF block device-backed ns)             [passed]
    runtime  22.787s  ...  20.386s
nvme/015 (unit test for NVMe flush for file backed ns)       [passed]
    runtime  20.599s  ...  23.212s
nvme/016 (create/delete many NVMeOF block device-backed ns and test
discovery) [passed]
    runtime  21.616s  ...  21.016s
nvme/017 (create/delete many file-ns and test discovery)     [passed]
    runtime    ...  20.721s
nvme/018 (unit test NVMe-oF out of range access on a file backend) [passed]
    runtime  1.777s  ...  1.752s
nvme/019 (test NVMe DSM Discard command on NVMeOF block-device ns) [passed]
    runtime  1.795s  ...  1.805s
nvme/020 (test NVMe DSM Discard command on NVMeOF file-backed ns) [passed]
    runtime  1.756s  ...  1.746s
nvme/021 (test NVMe list command on NVMeOF file-backed ns)   [passed]
    runtime  1.734s  ...  1.745s
nvme/022 (test NVMe reset command on NVMeOF file-backed ns)  [passed]
    runtime  2.168s  ...  2.181s
nvme/023 (test NVMe smart-log command on NVMeOF block-device ns) [passed]
    runtime  1.782s  ...  1.776s
nvme/024 (test NVMe smart-log command on NVMeOF file-backed ns) [passed]
    runtime  1.730s  ...  1.743s
nvme/025 (test NVMe effects-log command on NVMeOF file-backed ns) [passed]
    runtime  1.719s  ...  1.767s
nvme/026 (test NVMe ns-descs command on NVMeOF file-backed ns) [passed]
    runtime  1.718s  ...  1.732s
nvme/027 (test NVMe ns-rescan command on NVMeOF file-backed ns) [passed]
    runtime  1.754s  ...  1.748s
nvme/028 (test NVMe list-subsys command on NVMeOF file-backed ns) [passed]
    runtime  1.753s  ...  1.776s
nvme/029 (test userspace IO via nvme-cli read/write interface) [passed]
    runtime  2.280s  ...  2.244s
nvme/030 (ensure the discovery generation counter is updated
appropriately) [passed]
    runtime  0.425s  ...  0.401s
nvme/031 (test deletion of NVMeOF controllers immediately after setup)
[passed]
    runtime  6.134s  ...  5.996s
nvme/038 (test deletion of NVMeOF subsystem without enabling) [passed]
    runtime  0.075s  ...  0.059s
root@vm blktests (master) #



_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] Revert "nvme: verify MNAN value if ANA is enabled"

[Daniel Wagner]

Hi Chaitanay,

On Thu, Jun 10, 2021 at 08:32:56PM +0000, Chaitanya Kulkarni wrote:
> Let us look into the host side issue first then we can move to target side.

Sure

> Consider a scenario where ANA enabled subsys with 0 namespaces on the target
> side. When host issues connect command to such a controller ctrl->mnan
> should be 0 and ctrl->nn should be 0 which should be valid.
> 
> With original check in the code :-
> 
> if (!ctrl->max_namespaces ||
>     ctrl->max_namespaces > le32_to_cpu(id->nn))

In the scenario you describe this is obvious wrong. I suspect we can't
decide if there are namespaces on the target side enabled or not at this
point. Maybe later in the discovery path we could check max_namespace
again. But I don't know if this is the trouble worth.

So yes I agree with your proposed change. In the end it's not important
anyway.

Thanks,
Daniel

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] Revert "nvme: verify MNAN value if ANA is enabled"

[Daniel Wagner]

On Thu, Jun 10, 2021 at 09:01:14PM +0000, Chaitanya Kulkarni wrote:
> commit 1368a1a5e7566d726bf74234d05895c3f0d54690
> Author: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
> Date:   Wed Jun 9 20:07:00 2021 -0700
> 
>     nvme: fix the comparison in the mnan check
>    
>     The existing check for the valid mnan value will result in the error
>     when ctrl->max_namespaces are set to the 1024 from NVMeOF target since
>     !1024 == 0 so it will lead to next comparison 1024 > is->nn which will
>     be always true untill target has 1024 namespaces.

The commit message doesn't make sense to me. NSID is not limited to
1024.

From the discussion in the other mail, I though the argument is, if
there are no namespaces on the target the MNAN is allowed to be
zero. The original check assumed there are always namespaces. The
proposed fix drops the first half of the specs statement:

  ... then this field shall be set to a non-zero value that is less than
  or equal to the NN value.


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] Revert "nvme: verify MNAN value if ANA is enabled"

[Chaitanya Kulkarni]

On 6/11/21 02:17, Daniel Wagner wrote:
> On Thu, Jun 10, 2021 at 09:01:14PM +0000, Chaitanya Kulkarni wrote:
>> commit 1368a1a5e7566d726bf74234d05895c3f0d54690
>> Author: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
>> Date:   Wed Jun 9 20:07:00 2021 -0700
>>
>>     nvme: fix the comparison in the mnan check
>>    
>>     The existing check for the valid mnan value will result in the error
>>     when ctrl->max_namespaces are set to the 1024 from NVMeOF target since
>>     !1024 == 0 so it will lead to next comparison 1024 > is->nn which will
>>     be always true untill target has 1024 namespaces.
> The commit message doesn't make sense to me. NSID is not limited to
> 1024.
>
> From the discussion in the other mail, I though the argument is, if
> there are no namespaces on the target the MNAN is allowed to be
> zero. The original check assumed there are always namespaces. The
> proposed fix drops the first half of the specs statement:
>
>   ... then this field shall be set to a non-zero value that is less than
>   or equal to the NN value.
>
>

But that will require any controller to have at least one namespace, with
current implementation we don't enforce that behavior, maybe then we should
check that in the discovery or id controller path if ANA log page is
supported ?



_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] Revert "nvme: verify MNAN value if ANA is enabled"

[Chaitanya Kulkarni]

On 6/11/21 02:17, Daniel Wagner wrote:
> On Thu, Jun 10, 2021 at 09:01:14PM +0000, Chaitanya Kulkarni wrote:
>> commit 1368a1a5e7566d726bf74234d05895c3f0d54690
>> Author: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
>> Date:   Wed Jun 9 20:07:00 2021 -0700
>>
>>     nvme: fix the comparison in the mnan check
>>    
>>     The existing check for the valid mnan value will result in the error
>>     when ctrl->max_namespaces are set to the 1024 from NVMeOF target since
>>     !1024 == 0 so it will lead to next comparison 1024 > is->nn which will
>>     be always true untill target has 1024 namespaces.
> The commit message doesn't make sense to me. NSID is not limited to
> 1024.
>
> From the discussion in the other mail, I though the argument is, if
> there are no namespaces on the target the MNAN is allowed to be
> zero. The original check assumed there are always namespaces. The
> proposed fix drops the first half of the specs statement:
>
>   ... then this field shall be set to a non-zero value that is less than
>   or equal to the NN value.
>
>


I think we can keep the code as it is on the host side and fix the
target side.

I've sent out a small series to fix that, see if that makes sense.



_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme