[PATCH 1/2] nvmet: don't check iosqes, iocqes for discovery controllers

[PATCH 1/2] nvmet: don't check iosqes, iocqes for discovery controllers

[Sagi Grimberg]

From the base spec, Figure 78:
"Controller Configuration, these fields are defined as parameters to
configure an "I/O Controller (IOC)" and not to configure a "Discovery
Controller (DC)".
...
"If the controller does not support I/O queues, then this field shall
be read-only with a value of 0h"

Just have this check for I/O controllers.

Fixes: a07b4970f464 ("nvmet: add a generic NVMe target")
Reported-by: Belanger, Martin <Martin.Belanger@dell.com>
Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
---
 drivers/nvme/target/core.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 8ce4d59cc9e7..baa26a65d83d 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -1107,9 +1107,14 @@ static void nvmet_start_ctrl(struct nvmet_ctrl *ctrl)
 {
 	lockdep_assert_held(&ctrl->lock);
 
-	if (nvmet_cc_iosqes(ctrl->cc) != NVME_NVM_IOSQES ||
-	    nvmet_cc_iocqes(ctrl->cc) != NVME_NVM_IOCQES ||
-	    nvmet_cc_mps(ctrl->cc) != 0 ||
+	if (!ctrl->subsys->type == NVME_NQN_DISC &&
+	    (nvmet_cc_iosqes(ctrl->cc) != NVME_NVM_IOSQES ||
+	     nvmet_cc_iocqes(ctrl->cc) != NVME_NVM_IOCQES)) {
+		ctrl->csts = NVME_CSTS_CFS;
+		return;
+	}
+
+	if (nvmet_cc_mps(ctrl->cc) != 0 ||
 	    nvmet_cc_ams(ctrl->cc) != 0 ||
 	    nvmet_cc_css(ctrl->cc) != 0) {
 		ctrl->csts = NVME_CSTS_CFS;
-- 
2.27.0


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

[PATCH 2/2] nvme: don't set iosqes,iocqes for discovery controllers

[Sagi Grimberg]

From the base spec, Figure 78:
"Controller Configuration, these fields are defined as parameters to
configure an "I/O Controller (IOC)" and not to configure a "Discovery
Controller (DC)".
...
"If the controller does not support I/O queues, then this field shall
be read-only with a value of 0h"

Set these only for I/O controllers.

Reported-by: Belanger, Martin <Martin.Belanger@dell.com>
Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
---
 drivers/nvme/host/core.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 546a10407385..a7578c601727 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2443,6 +2443,11 @@ int nvme_disable_ctrl(struct nvme_ctrl *ctrl)
 }
 EXPORT_SYMBOL_GPL(nvme_disable_ctrl);
 
+static inline bool nvme_discovery_ctrl(struct nvme_ctrl *ctrl)
+{
+	return ctrl->opts && ctrl->opts->discovery_nqn;
+}
+
 int nvme_enable_ctrl(struct nvme_ctrl *ctrl)
 {
 	unsigned dev_page_min;
@@ -2468,7 +2473,8 @@ int nvme_enable_ctrl(struct nvme_ctrl *ctrl)
 		ctrl->ctrl_config = NVME_CC_CSS_NVM;
 	ctrl->ctrl_config |= (NVME_CTRL_PAGE_SHIFT - 12) << NVME_CC_MPS_SHIFT;
 	ctrl->ctrl_config |= NVME_CC_AMS_RR | NVME_CC_SHN_NONE;
-	ctrl->ctrl_config |= NVME_CC_IOSQES | NVME_CC_IOCQES;
+	if (!nvme_discovery_ctrl(ctrl))
+		ctrl->ctrl_config |= NVME_CC_IOSQES | NVME_CC_IOCQES;
 	ctrl->ctrl_config |= NVME_CC_ENABLE;
 
 	ret = ctrl->ops->reg_write32(ctrl, NVME_REG_CC, ctrl->ctrl_config);
@@ -2888,11 +2894,6 @@ static const struct attribute_group *nvme_subsys_attrs_groups[] = {
 	NULL,
 };
 
-static inline bool nvme_discovery_ctrl(struct nvme_ctrl *ctrl)
-{
-	return ctrl->opts && ctrl->opts->discovery_nqn;
-}
-
 static bool nvme_validate_cntlid(struct nvme_subsystem *subsys,
 		struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
 {
-- 
2.27.0


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

Re: [PATCH 2/2] nvme: don't set iosqes,iocqes for discovery controllers

[Christoph Hellwig]

So if we change this we break interaction with all old Linux (and maybe
other?) targets.

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

Re: [PATCH 2/2] nvme: don't set iosqes,iocqes for discovery controllers

[Sagi Grimberg]

> So if we change this we break interaction with all old Linux (and maybe
> other?) targets.

Well yes.. Do you think that we should do a retry in case it fails for
backward compatibility?

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

Re: [PATCH 2/2] nvme: don't set iosqes,iocqes for discovery controllers

[Christoph Hellwig]

On Wed, Feb 10, 2021 at 02:37:09PM -0800, Sagi Grimberg wrote:
>> So if we change this we break interaction with all old Linux (and maybe
>> other?) targets.
>
> Well yes.. Do you think that we should do a retry in case it fails for
> backward compatibility?

I think we have to.

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

Re: [PATCH 2/2] nvme: don't set iosqes,iocqes for discovery controllers

[Sagi Grimberg]

>>> So if we change this we break interaction with all old Linux (and maybe
>>> other?) targets.
>>
>> Well yes.. Do you think that we should do a retry in case it fails for
>> backward compatibility?
> 
> I think we have to.

The annoying thing here is that this issue will manifest in the host
waiting on nvme_wait_ready for a long 7.5 seconds to understand that
its maybe incompatible and re-attempt (nvme_enable_ctrl writes to cap
and then polls CSTS)...

What I have now looks like:
--
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 02579f4f776c..ee65c89a991c 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2457,8 +2457,14 @@ int nvme_disable_ctrl(struct nvme_ctrl *ctrl)
  }
  EXPORT_SYMBOL_GPL(nvme_disable_ctrl);

+static inline bool nvme_discovery_ctrl(struct nvme_ctrl *ctrl)
+{
+       return ctrl->opts && ctrl->opts->discovery_nqn;
+}
+
  int nvme_enable_ctrl(struct nvme_ctrl *ctrl)
  {
+       bool compat_disc_ctrl_config = false;
         unsigned dev_page_min;
         int ret;

@@ -2482,13 +2488,33 @@ int nvme_enable_ctrl(struct nvme_ctrl *ctrl)
                 ctrl->ctrl_config = NVME_CC_CSS_NVM;
         ctrl->ctrl_config |= (NVME_CTRL_PAGE_SHIFT - 12) << 
NVME_CC_MPS_SHIFT;
         ctrl->ctrl_config |= NVME_CC_AMS_RR | NVME_CC_SHN_NONE;
-       ctrl->ctrl_config |= NVME_CC_IOSQES | NVME_CC_IOCQES;
+again:
+       if (!nvme_discovery_ctrl(ctrl) || compat_disc_ctrl_config)
+               ctrl->ctrl_config |= NVME_CC_IOSQES | NVME_CC_IOCQES;
         ctrl->ctrl_config |= NVME_CC_ENABLE;

         ret = ctrl->ops->reg_write32(ctrl, NVME_REG_CC, ctrl->ctrl_config);
         if (ret)
                 return ret;
-       return nvme_wait_ready(ctrl, ctrl->cap, true);
+
+       ret = nvme_wait_ready(ctrl, ctrl->cap, true);
+       if (ret) {
+               if (!compat_disc_ctrl_config) {
+                       pr_err("retrying...\n");
+                       /*
+                        * Backward compatibility work-around: some targets
+                        * (i.e. older Linux targets) may incorrectly verify
+                        * iosqes,iocqes are non-zero for discovery
+                        * controllers. So in order not to break them, we
+                        * attempt once again with the incorrect settings.
+                        */
+                       if (nvme_disable_ctrl(ctrl))
+                               return ret;
+                       compat_disc_ctrl_config = true;
+                       goto again;
+               }
+       }
+       return ret;
  }
  EXPORT_SYMBOL_GPL(nvme_enable_ctrl);

@@ -2902,11 +2928,6 @@ static const struct attribute_group 
*nvme_subsys_attrs_groups[] = {
         NULL,
  };

-static inline bool nvme_discovery_ctrl(struct nvme_ctrl *ctrl)
-{
-       return ctrl->opts && ctrl->opts->discovery_nqn;
-}
-
  static bool nvme_validate_cntlid(struct nvme_subsystem *subsys,
                 struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
  {
--

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

Re: [PATCH 2/2] nvme: don't set iosqes,iocqes for discovery controllers

[Christoph Hellwig]

On Thu, Feb 11, 2021 at 02:43:27AM -0800, Sagi Grimberg wrote:
> The annoying thing here is that this issue will manifest in the host
> waiting on nvme_wait_ready for a long 7.5 seconds to understand that
> its maybe incompatible and re-attempt (nvme_enable_ctrl writes to cap
> and then polls CSTS)...
>
> What I have now looks like:

In terms of what to do about the retry this is what we'll have to
do.  That being said I think for existing controllers we should just
stick to the existing wrong value, and only set the correct one
for versions > 1.3.  This isn't exactly correct, but probably causes
the least harm.  It will need an extensive comment, and maybe we need
to add an option to override it if controllers show up that do not
like the wrong value.

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

Re: [PATCH 2/2] nvme: don't set iosqes,iocqes for discovery controllers

[Sagi Grimberg]

>> The annoying thing here is that this issue will manifest in the host
>> waiting on nvme_wait_ready for a long 7.5 seconds to understand that
>> its maybe incompatible and re-attempt (nvme_enable_ctrl writes to cap
>> and then polls CSTS)...
>>
>> What I have now looks like:
> 
> In terms of what to do about the retry this is what we'll have to
> do.  That being said I think for existing controllers we should just
> stick to the existing wrong value, and only set the correct one
> for versions > 1.3.  This isn't exactly correct, but probably causes
> the least harm.  It will need an extensive comment, and maybe we need
> to add an option to override it if controllers show up that do not
> like the wrong value.
So you suggest that we keep the retry w.a. but continue to send the
wrong value in 1.3 or lower?

I personally think that this would be extremely confusing to users, if
they do trip on this, but I agree that they are less likely to hit
it because existing controllers obviously accept wrong values...

As for option to override. I really hate it.. What is the least exposure
we can get for this but still allow the user to control this?

Wish we had a good and reliable quirk mechanism for fabrics...

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

Re: [PATCH 2/2] nvme: don't set iosqes,iocqes for discovery controllers

[Christoph Hellwig]

On Thu, Feb 11, 2021 at 11:44:44AM -0800, Sagi Grimberg wrote:
> So you suggest that we keep the retry w.a. but continue to send the
> wrong value in 1.3 or lower?
>
> I personally think that this would be extremely confusing to users, if
> they do trip on this, but I agree that they are less likely to hit
> it because existing controllers obviously accept wrong values...

Yes.  While I'd like to move to standards compliance as much as possible,
introducing a long delay for every existing working setup is 
very counter productive.

>
> As for option to override. I really hate it.. What is the least exposure
> we can get for this but still allow the user to control this?

We can of course still override it using an option.

> Wish we had a good and reliable quirk mechanism for fabrics...

We do, using the serial number.  But that does not help for an issue
during connection establishment.

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

Re: [PATCH 1/2] nvmet: don't check iosqes,iocqes for discovery controllers

[Christoph Hellwig]

On Tue, Feb 09, 2021 at 02:12:34AM -0800, Sagi Grimberg wrote:
> +	if (!ctrl->subsys->type == NVME_NQN_DISC &&

I think the normal way to write this would be a !=.

> +	    (nvmet_cc_iosqes(ctrl->cc) != NVME_NVM_IOSQES ||
> +	     nvmet_cc_iocqes(ctrl->cc) != NVME_NVM_IOCQES)) {

By the spec we'd need to check these are 0, but of course we'd break
the existing Linux host.  Maybe throw in a comment?

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

Re: [PATCH 1/2] nvmet: don't check iosqes,iocqes for discovery controllers

[Sagi Grimberg]

>> +	if (!ctrl->subsys->type == NVME_NQN_DISC &&
> 
> I think the normal way to write this would be a !=.

:)

>> +	    (nvmet_cc_iosqes(ctrl->cc) != NVME_NVM_IOSQES ||
>> +	     nvmet_cc_iocqes(ctrl->cc) != NVME_NVM_IOCQES)) {
> 
> By the spec we'd need to check these are 0, but of course we'd break
> the existing Linux host.  Maybe throw in a comment?

Sure, I'll add it.

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