[PATCH 0/3] nvmet: use right MNAN value for controllers

[PATCH 0/3] nvmet: use right MNAN value for controllers

[Chaitanya Kulkarni]

Hi,

In current implementation we report MNAN value as NVMET_MAX_NAMESPACES.

For Spec regarding MNAN value:-

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.

The first patch sets identify controller MNAN value to subsys->max_nsid
that is always less than or equal to the NN.

The second patch prevents creating the controller from subsys if subsys
has 0 namespaces since it will report 0 (invalid) MNAN value.

The third patch prevents reporting such subsys in the dicovery log page.

-ck

Chaitanya Kulkarni (3):
  nvmet: use subsys->max_nsid to report MNAN
  nvmet: prevent creating ctrl with no namespace
  nvmet: don't report subsys with 0 namespaces

 drivers/nvme/target/admin-cmd.c |  2 +-
 drivers/nvme/target/core.c      |  9 +++++++++
 drivers/nvme/target/discovery.c | 13 +++++++++++++
 3 files changed, 23 insertions(+), 1 deletion(-)

-- 
2.22.1


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

[PATCH 1/3] nvmet: use subsys->max_nsid to report MNAN

[Chaitanya Kulkarni]

For Spec regarding MNAN value:-

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.

Instead of using NVMET_MAX_NAMESPACES use subsys->max_nsid to set the
MNAN value.

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

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


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

Re: [PATCH 1/3] nvmet: use subsys->max_nsid to report MNAN

[Christoph Hellwig]

On Sun, Jun 13, 2021 at 01:02:50PM -0700, Chaitanya Kulkarni wrote:
> For Spec regarding MNAN value:-
> 
> 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.
> 
> Instead of using NVMET_MAX_NAMESPACES use subsys->max_nsid to set the
> MNAN value.

I think this is a rather bad idea as it is supposed to be a relatively
static number.  I think the right fix is to stop playing games with a
dyanmic NN and always report UINT_MAX in nn.

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

Re: [PATCH 1/3] nvmet: use subsys->max_nsid to report MNAN

[Chaitanya Kulkarni]

On 6/13/21 23:36, Christoph Hellwig wrote:
> On Sun, Jun 13, 2021 at 01:02:50PM -0700, Chaitanya Kulkarni wrote:
>> For Spec regarding MNAN value:-
>>
>> 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.
>>
>> Instead of using NVMET_MAX_NAMESPACES use subsys->max_nsid to set the
>> MNAN value.
> I think this is a rather bad idea as it is supposed to be a relatively
> static number.  I think the right fix is to stop playing games with a
> dyanmic NN and always report UINT_MAX in nn.
>

Sent out the patch with NN set to U32_MAX.


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

[PATCH 2/3] nvmet: prevent creating ctrl with no namespace

[Chaitanya Kulkarni]

From Spec :-

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

In current implementation if subsys has 0 namespaces then we report
wrong MNAN value for the controller if we allow controller creation.

Prevent new controller creation if the subsys associated with the new
controller has no namespaces.

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

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 146909486b8f..26d93ac9bd2e 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -1325,6 +1325,15 @@ u16 nvmet_alloc_ctrl(const char *subsysnqn, const char *hostnqn,
 		goto out;
 	}
 
+	mutex_lock(&subsys->lock);
+	if (!subsys->max_nsid) {
+		mutex_unlock(&subsys->lock);
+		pr_err("invalid mnan value for the ana enabled controller\n");
+		status = NVME_SC_INTERNAL;
+		goto out_put_subsystem;
+	}
+	mutex_unlock(&subsys->lock);
+
 	down_read(&nvmet_config_sem);
 	if (!nvmet_host_allowed(subsys, hostnqn)) {
 		pr_info("connect by host %s for subsystem %s not allowed\n",
-- 
2.22.1


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

Re: [PATCH 2/3] nvmet: prevent creating ctrl with no namespace

[Sagi Grimberg]

>  From Spec :-
> 
> If the controller supports Asymmetric Namespace Access Reporting, then
> MNAN shall be set to a non-zero value that is less than or equal
> to the NN value.
> 
> In current implementation if subsys has 0 namespaces then we report
> wrong MNAN value for the controller if we allow controller creation.
> 
> Prevent new controller creation if the subsys associated with the new
> controller has no namespaces.

This is a perfectly valid thing to do, preventing this is not the
solution here.

Why not just set both nn and mnan to NVMET_MAX_NAMESPACES and be done
with it?

> 
> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
> ---
>   drivers/nvme/target/core.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
> index 146909486b8f..26d93ac9bd2e 100644
> --- a/drivers/nvme/target/core.c
> +++ b/drivers/nvme/target/core.c
> @@ -1325,6 +1325,15 @@ u16 nvmet_alloc_ctrl(const char *subsysnqn, const char *hostnqn,
>   		goto out;
>   	}
>   
> +	mutex_lock(&subsys->lock);
> +	if (!subsys->max_nsid) {
> +		mutex_unlock(&subsys->lock);
> +		pr_err("invalid mnan value for the ana enabled controller\n");
> +		status = NVME_SC_INTERNAL;
> +		goto out_put_subsystem;
> +	}
> +	mutex_unlock(&subsys->lock);
> +
>   	down_read(&nvmet_config_sem);
>   	if (!nvmet_host_allowed(subsys, hostnqn)) {
>   		pr_info("connect by host %s for subsystem %s not allowed\n",
> 

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

Re: [PATCH 2/3] nvmet: prevent creating ctrl with no namespace

[Christoph Hellwig]

On Thu, Jun 17, 2021 at 12:06:11PM -0700, Sagi Grimberg wrote:
> Why not just set both nn and mnan to NVMET_MAX_NAMESPACES and be done
> with it?

Yes, I'm pretty sure I suggested that before somewhere..

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

Re: [PATCH 2/3] nvmet: prevent creating ctrl with no namespace

[Chaitanya Kulkarni]

On 6/17/21 21:28, Christoph Hellwig wrote:
> On Thu, Jun 17, 2021 at 12:06:11PM -0700, Sagi Grimberg wrote:
>> Why not just set both nn and mnan to NVMET_MAX_NAMESPACES and be done
>> with it?
> Yes, I'm pretty sure I suggested that before somewhere..
>

Sent out the patch with NVMET_MAX_NAMESPACES, please have a look.



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

[PATCH 3/3] nvmet: don't report subsys with 0 namespaces

[Chaitanya Kulkarni]

Skip reporting the subsys from the discovery log page which has no
namespaces.

This is needed because we don't allow creating a controller from such
subsystem from previous patch.

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

diff --git a/drivers/nvme/target/discovery.c b/drivers/nvme/target/discovery.c
index 7aa62bc6ae84..03a34ad3d59b 100644
--- a/drivers/nvme/target/discovery.c
+++ b/drivers/nvme/target/discovery.c
@@ -158,6 +158,17 @@ static size_t discovery_log_entries(struct nvmet_req *req)
 	return entries;
 }
 
+static bool nvmet_disc_get_log_report_subsys(struct nvmet_subsys *s)
+{
+	bool skip;
+
+	mutex_lock(&s->lock);
+	/* skip reporting the subsys with 0 MNAN */
+	skip = s->max_nsid ? true : false;
+	mutex_unlock(&s->lock);
+	return skip;
+}
+
 static void nvmet_execute_disc_get_log_page(struct nvmet_req *req)
 {
 	const int entry_size = sizeof(struct nvmf_disc_rsp_page_entry);
@@ -210,6 +221,8 @@ static void nvmet_execute_disc_get_log_page(struct nvmet_req *req)
 
 		if (!nvmet_host_allowed(p->subsys, ctrl->hostnqn))
 			continue;
+		if (!nvmet_disc_get_log_report_subsys(p->subsys))
+			continue;
 
 		nvmet_set_disc_traddr(req, req->port, traddr);
 		nvmet_format_discovery_entry(hdr, req->port,
-- 
2.22.1


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

Re: [PATCH 0/3] nvmet: use right MNAN value for controllers

[Chaitanya Kulkarni]

On 6/13/21 13:03, Chaitanya Kulkarni wrote:
> Hi,
>
> In current implementation we report MNAN value as NVMET_MAX_NAMESPACES.
>
> For Spec regarding MNAN value:-
>
> 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.
>
> The first patch sets identify controller MNAN value to subsys->max_nsid
> that is always less than or equal to the NN.
>
> The second patch prevents creating the controller from subsys if subsys
> has 0 namespaces since it will report 0 (invalid) MNAN value.
>
> The third patch prevents reporting such subsys in the dicovery log page.
>
> -ck


Please ignore this as some of the block tests are failing I need to figure
out whether any of those needs a fix or something is wrong in this series,
sorry for the noise.



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