[PATCH 0/2] nvmet: add some more configfs attributes

[PATCH 0/2] nvmet: add some more configfs attributes

[Hannes Reinecke]

Hi all,

here are two small patches to make the 'MDTS' value and the 'CNTLID'
range configurable. The first is primarily for debugging, but the other
is required if the nvme target is deployed in a cluster.

As usual, comments and reviews are welcome.

Hannes Reinecke (2):
  nvmet: make MDTS value configurable
  nvmet: make CNTLID range configurable

 drivers/nvme/target/admin-cmd.c |  3 +-
 drivers/nvme/target/configfs.c  | 87 +++++++++++++++++++++++++++++++++++++++++
 drivers/nvme/target/core.c      |  4 +-
 drivers/nvme/target/nvmet.h     |  4 ++
 4 files changed, 95 insertions(+), 3 deletions(-)

-- 
2.16.4

[PATCH 1/2] nvmet: make MDTS value configurable

[Hannes Reinecke]

For testing the bio splitting algorithm it's useful to be able to
modify the mdts value.

Signed-off-by: Hannes Reinecke <hare at suse.com>
---
 drivers/nvme/target/admin-cmd.c |  3 +--
 drivers/nvme/target/configfs.c  | 27 +++++++++++++++++++++++++++
 drivers/nvme/target/nvmet.h     |  2 ++
 3 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 76250181fee0..1c4cb371fac8 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -311,8 +311,7 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
 	/* we support multiple ports, multiples hosts and ANA: */
 	id->cmic = (1 << 0) | (1 << 1) | (1 << 3);
 
-	/* no limit on data transfer sizes for now */
-	id->mdts = 0;
+	id->mdts = ctrl->subsys->mdts;
 	id->cntlid = cpu_to_le16(ctrl->cntlid);
 	id->ver = cpu_to_le32(ctrl->subsys->ver);
 
diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index adb79545cdd7..63b9991de79e 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -859,10 +859,37 @@ static ssize_t nvmet_subsys_attr_serial_store(struct config_item *item,
 }
 CONFIGFS_ATTR(nvmet_subsys_, attr_serial);
 
+static ssize_t nvmet_subsys_attr_mdts_show(struct config_item *item,
+					   char *page)
+{
+	struct nvmet_subsys *subsys = to_subsys(item);
+
+	return snprintf(page, PAGE_SIZE, "%u\n", subsys->mdts);
+}
+
+static ssize_t nvmet_subsys_attr_mdts_store(struct config_item *item,
+					    const char *page, size_t count)
+{
+	struct nvmet_subsys *subsys = to_subsys(item);
+	unsigned int mdts;
+	int ret;
+
+	ret = kstrtou32(page, 0, &mdts);
+	if (ret)
+		return ret;
+	down_write(&nvmet_config_sem);
+	subsys->mdts = mdts;
+	up_write(&nvmet_config_sem);
+
+	return count;
+}
+CONFIGFS_ATTR(nvmet_subsys_, attr_mdts);
+
 static struct configfs_attribute *nvmet_subsys_attrs[] = {
 	&nvmet_subsys_attr_attr_allow_any_host,
 	&nvmet_subsys_attr_attr_version,
 	&nvmet_subsys_attr_attr_serial,
+	&nvmet_subsys_attr_attr_mdts,
 	NULL,
 };
 
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 51e49efd7849..f05f069f83ea 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -217,6 +217,8 @@ struct nvmet_subsys {
 	bool			allow_any_host;
 
 	u16			max_qid;
+	u8			mdts;
+	u8			mpsmin;
 
 	u64			ver;
 	u64			serial;
-- 
2.16.4

[PATCH 2/2] nvmet: make CNTLID range configurable

[Hannes Reinecke]

When trying to run in a clustered environment we need to keep the
controller ID values unique. So this patch add to additional config
attributes 'cntlid_min' and 'cntlid_max' to only allow specific ranges
for the controller ID.

Signed-off-by: Hannes Reinecke <hare at suse.com>
---
 drivers/nvme/target/configfs.c | 60 ++++++++++++++++++++++++++++++++++++++++++
 drivers/nvme/target/core.c     |  4 ++-
 drivers/nvme/target/nvmet.h    |  2 ++
 3 files changed, 65 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index 63b9991de79e..07f837abea62 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -885,11 +885,71 @@ static ssize_t nvmet_subsys_attr_mdts_store(struct config_item *item,
 }
 CONFIGFS_ATTR(nvmet_subsys_, attr_mdts);
 
+static ssize_t nvmet_subsys_attr_cntlid_min_show(struct config_item *item,
+						 char *page)
+{
+	struct nvmet_subsys *subsys = to_subsys(item);
+
+	return snprintf(page, PAGE_SIZE, "%u\n", subsys->min_cntlid);
+}
+
+static ssize_t nvmet_subsys_attr_cntlid_min_store(struct config_item *item,
+						  const char *page, size_t count)
+{
+	struct nvmet_subsys *subsys = to_subsys(item);
+	unsigned int min;
+	int ret;
+
+	ret = kstrtou32(page, 0, &min);
+	if (ret)
+		return ret;
+	down_write(&nvmet_config_sem);
+	if (min >= subsys->max_cntlid || min < NVME_CNTLID_MIN)
+		ret = -EINVAL;
+	else
+		subsys->min_cntlid = min;
+	up_write(&nvmet_config_sem);
+
+	return ret ? ret : count;
+}
+CONFIGFS_ATTR(nvmet_subsys_, attr_cntlid_min);
+
+static ssize_t nvmet_subsys_attr_cntlid_max_show(struct config_item *item,
+						 char *page)
+{
+	struct nvmet_subsys *subsys = to_subsys(item);
+
+	return snprintf(page, PAGE_SIZE, "%u\n", subsys->max_cntlid);
+}
+
+static ssize_t nvmet_subsys_attr_cntlid_max_store(struct config_item *item,
+						  const char *page, size_t count)
+{
+	struct nvmet_subsys *subsys = to_subsys(item);
+	unsigned int max;
+	int ret;
+
+	ret = kstrtou32(page, 0, &max);
+	if (ret)
+		return ret;
+	down_write(&nvmet_config_sem);
+	if (max >= NVME_CNTLID_MAX || max < subsys->min_cntlid)
+		ret = -EINVAL;
+	else
+		subsys->max_cntlid = max;
+	up_write(&nvmet_config_sem);
+
+	return ret ? ret : count;
+}
+CONFIGFS_ATTR(nvmet_subsys_, attr_cntlid_max);
+
 static struct configfs_attribute *nvmet_subsys_attrs[] = {
 	&nvmet_subsys_attr_attr_allow_any_host,
 	&nvmet_subsys_attr_attr_version,
 	&nvmet_subsys_attr_attr_serial,
 	&nvmet_subsys_attr_attr_mdts,
+	&nvmet_subsys_attr_attr_cntlid_min,
+	&nvmet_subsys_attr_attr_cntlid_max,
 	NULL,
 };
 
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 2d73b66e3686..fe60734b421d 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -1241,7 +1241,7 @@ u16 nvmet_alloc_ctrl(const char *subsysnqn, const char *hostnqn,
 		goto out_free_cqs;
 
 	ret = ida_simple_get(&cntlid_ida,
-			     NVME_CNTLID_MIN, NVME_CNTLID_MAX,
+			     subsys->min_cntlid, subsys->max_cntlid,
 			     GFP_KERNEL);
 	if (ret < 0) {
 		status = NVME_SC_CONNECT_CTRL_BUSY | NVME_SC_DNR;
@@ -1382,6 +1382,8 @@ struct nvmet_subsys *nvmet_subsys_alloc(const char *subsysnqn,
 		kfree(subsys);
 		return NULL;
 	}
+	subsys->min_cntlid = NVME_CNTLID_MIN;
+	subsys->max_cntlid = NVME_CNTLID_MAX;
 	subsys->type = type;
 	subsys->subsysnqn = kstrndup(subsysnqn, NVMF_NQN_SIZE,
 			GFP_KERNEL);
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index f05f069f83ea..7bc81bf7d45e 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -212,6 +212,8 @@ struct nvmet_subsys {
 	unsigned int		max_nsid;
 
 	struct list_head	ctrls;
+	unsigned int		min_cntlid;
+	unsigned int		max_cntlid;
 
 	struct list_head	hosts;
 	bool			allow_any_host;
-- 
2.16.4

[PATCH 1/2] nvmet: make MDTS value configurable

[Heitke, Kenneth]

On 3/29/2019 6:04 AM, Hannes Reinecke wrote:
> For testing the bio splitting algorithm it's useful to be able to
> modify the mdts value.
> 
> Signed-off-by: Hannes Reinecke <hare at suse.com>
> ---
>   drivers/nvme/target/admin-cmd.c |  3 +--
>   drivers/nvme/target/configfs.c  | 27 +++++++++++++++++++++++++++
>   drivers/nvme/target/nvmet.h     |  2 ++
>   3 files changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
> index 76250181fee0..1c4cb371fac8 100644
> --- a/drivers/nvme/target/admin-cmd.c
> +++ b/drivers/nvme/target/admin-cmd.c
> @@ -311,8 +311,7 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
>   	/* we support multiple ports, multiples hosts and ANA: */
>   	id->cmic = (1 << 0) | (1 << 1) | (1 << 3);
>   
> -	/* no limit on data transfer sizes for now */
> -	id->mdts = 0;
> +	id->mdts = ctrl->subsys->mdts;
>   	id->cntlid = cpu_to_le16(ctrl->cntlid);
>   	id->ver = cpu_to_le32(ctrl->subsys->ver);
>   
> diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
> index adb79545cdd7..63b9991de79e 100644
> --- a/drivers/nvme/target/configfs.c
> +++ b/drivers/nvme/target/configfs.c
> @@ -859,10 +859,37 @@ static ssize_t nvmet_subsys_attr_serial_store(struct config_item *item,
>   }
>   CONFIGFS_ATTR(nvmet_subsys_, attr_serial);
>   
> +static ssize_t nvmet_subsys_attr_mdts_show(struct config_item *item,
> +					   char *page)
> +{
> +	struct nvmet_subsys *subsys = to_subsys(item);
> +
> +	return snprintf(page, PAGE_SIZE, "%u\n", subsys->mdts);
> +}
> +
> +static ssize_t nvmet_subsys_attr_mdts_store(struct config_item *item,
> +					    const char *page, size_t count)
> +{
> +	struct nvmet_subsys *subsys = to_subsys(item);
> +	unsigned int mdts;
> +	int ret;
> +
> +	ret = kstrtou32(page, 0, &mdts);
> +	if (ret)
> +		return ret;
> +	down_write(&nvmet_config_sem);
> +	subsys->mdts = mdts;

What is the convention, if any, for taking a possible 32-bit value and
putting it into an 8-bit variable? Should there be a warning if the
value is greater than the max allowed?

> +	up_write(&nvmet_config_sem);
> +
> +	return count;
> +}
> +CONFIGFS_ATTR(nvmet_subsys_, attr_mdts);
> +
>   static struct configfs_attribute *nvmet_subsys_attrs[] = {
>   	&nvmet_subsys_attr_attr_allow_any_host,
>   	&nvmet_subsys_attr_attr_version,
>   	&nvmet_subsys_attr_attr_serial,
> +	&nvmet_subsys_attr_attr_mdts,
>   	NULL,
>   };
>   
> diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
> index 51e49efd7849..f05f069f83ea 100644
> --- a/drivers/nvme/target/nvmet.h
> +++ b/drivers/nvme/target/nvmet.h
> @@ -217,6 +217,8 @@ struct nvmet_subsys {
>   	bool			allow_any_host;
>   
>   	u16			max_qid;
> +	u8			mdts;
> +	u8			mpsmin;
>   
>   	u64			ver;
>   	u64			serial;
> 

[PATCH 1/2] nvmet: make MDTS value configurable

[Max Gurtovoy]

On 3/29/2019 3:04 PM, Hannes Reinecke wrote:
> For testing the bio splitting algorithm it's useful to be able to
> modify the mdts value.
>
> Signed-off-by: Hannes Reinecke <hare at suse.com>
> ---
>   drivers/nvme/target/admin-cmd.c |  3 +--
>   drivers/nvme/target/configfs.c  | 27 +++++++++++++++++++++++++++
>   drivers/nvme/target/nvmet.h     |  2 ++
>   3 files changed, 30 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
> index 76250181fee0..1c4cb371fac8 100644
> --- a/drivers/nvme/target/admin-cmd.c
> +++ b/drivers/nvme/target/admin-cmd.c
> @@ -311,8 +311,7 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
>   	/* we support multiple ports, multiples hosts and ANA: */
>   	id->cmic = (1 << 0) | (1 << 1) | (1 << 3);
>   
> -	/* no limit on data transfer sizes for now */
> -	id->mdts = 0;
> +	id->mdts = ctrl->subsys->mdts;

I think it's more of a ctrl/nvmet_port configuration than a subsystem.

For example if you have 2 HCAs, one is super strong and can transfer 
upto 8MB and the second is old model and can transfer upto 64KiB.

do

id->mdts = ctrl->ops->port_mdts(req->port);

and inside the implementation you can do

min_not_zero(max_port_hca_cap_in_mdts_units, mdts_configfs_val);

and it will fix the comment that is written above (we're not really 
unlimited).

this way we're using the low level device characteristics and also cover 
the bio split test.

thoughts ?


>   	id->cntlid = cpu_to_le16(ctrl->cntlid);
>   	id->ver = cpu_to_le32(ctrl->subsys->ver);
>   
> diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
> index adb79545cdd7..63b9991de79e 100644
> --- a/drivers/nvme/target/configfs.c
> +++ b/drivers/nvme/target/configfs.c
> @@ -859,10 +859,37 @@ static ssize_t nvmet_subsys_attr_serial_store(struct config_item *item,
>   }
>   CONFIGFS_ATTR(nvmet_subsys_, attr_serial);
>   
> +static ssize_t nvmet_subsys_attr_mdts_show(struct config_item *item,
> +					   char *page)
> +{
> +	struct nvmet_subsys *subsys = to_subsys(item);
> +
> +	return snprintf(page, PAGE_SIZE, "%u\n", subsys->mdts);
> +}
> +
> +static ssize_t nvmet_subsys_attr_mdts_store(struct config_item *item,
> +					    const char *page, size_t count)
> +{
> +	struct nvmet_subsys *subsys = to_subsys(item);
> +	unsigned int mdts;
> +	int ret;
> +
> +	ret = kstrtou32(page, 0, &mdts);
> +	if (ret)
> +		return ret;
> +	down_write(&nvmet_config_sem);
> +	subsys->mdts = mdts;
> +	up_write(&nvmet_config_sem);
> +
> +	return count;
> +}
> +CONFIGFS_ATTR(nvmet_subsys_, attr_mdts);
> +
>   static struct configfs_attribute *nvmet_subsys_attrs[] = {
>   	&nvmet_subsys_attr_attr_allow_any_host,
>   	&nvmet_subsys_attr_attr_version,
>   	&nvmet_subsys_attr_attr_serial,
> +	&nvmet_subsys_attr_attr_mdts,
>   	NULL,
>   };
>   
> diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
> index 51e49efd7849..f05f069f83ea 100644
> --- a/drivers/nvme/target/nvmet.h
> +++ b/drivers/nvme/target/nvmet.h
> @@ -217,6 +217,8 @@ struct nvmet_subsys {
>   	bool			allow_any_host;
>   
>   	u16			max_qid;
> +	u8			mdts;
> +	u8			mpsmin;


are you using mpsmin ?


>   
>   	u64			ver;
>   	u64			serial;

[PATCH 1/2] nvmet: make MDTS value configurable

[Hannes Reinecke]

On 3/31/19 5:04 PM, Max Gurtovoy wrote:
> 
> On 3/29/2019 3:04 PM, Hannes Reinecke wrote:
>> For testing the bio splitting algorithm it's useful to be able to
>> modify the mdts value.
>>
>> Signed-off-by: Hannes Reinecke <hare at suse.com>
>> ---
>> ? drivers/nvme/target/admin-cmd.c |? 3 +--
>> ? drivers/nvme/target/configfs.c? | 27 +++++++++++++++++++++++++++
>> ? drivers/nvme/target/nvmet.h???? |? 2 ++
>> ? 3 files changed, 30 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/nvme/target/admin-cmd.c 
>> b/drivers/nvme/target/admin-cmd.c
>> index 76250181fee0..1c4cb371fac8 100644
>> --- a/drivers/nvme/target/admin-cmd.c
>> +++ b/drivers/nvme/target/admin-cmd.c
>> @@ -311,8 +311,7 @@ static void nvmet_execute_identify_ctrl(struct 
>> nvmet_req *req)
>> ????? /* we support multiple ports, multiples hosts and ANA: */
>> ????? id->cmic = (1 << 0) | (1 << 1) | (1 << 3);
>> -??? /* no limit on data transfer sizes for now */
>> -??? id->mdts = 0;
>> +??? id->mdts = ctrl->subsys->mdts;
> 
> I think it's more of a ctrl/nvmet_port configuration than a subsystem.
> 
> For example if you have 2 HCAs, one is super strong and can transfer 
> upto 8MB and the second is old model and can transfer upto 64KiB.
> 
> do
> 
> id->mdts = ctrl->ops->port_mdts(req->port);
> 
> and inside the implementation you can do
> 
> min_not_zero(max_port_hca_cap_in_mdts_units, mdts_configfs_val);
> 
> and it will fix the comment that is written above (we're not really 
> unlimited).
> 
> this way we're using the low level device characteristics and also cover 
> the bio split test.
> 
> thoughts ?
> 
> 
Yeah, true, it should be a port configuration.

I'll be updating the patch.

Cheers,

Hannes

[PATCH 1/2] nvmet: make MDTS value configurable

[Keith Busch]

On Sun, Mar 31, 2019@07:02:40PM -0700, Hannes Reinecke wrote:
> > I think it's more of a ctrl/nvmet_port configuration than a subsystem.
> > 
> > For example if you have 2 HCAs, one is super strong and can transfer 
> > upto 8MB and the second is old model and can transfer upto 64KiB.
> > 
> > do
> > 
> > id->mdts = ctrl->ops->port_mdts(req->port);
> > 
> > and inside the implementation you can do
> > 
> > min_not_zero(max_port_hca_cap_in_mdts_units, mdts_configfs_val);
> > 
> > and it will fix the comment that is written above (we're not really 
> > unlimited).
> > 
> > this way we're using the low level device characteristics and also cover 
> > the bio split test.
> > 
> > thoughts ?
> > 
> > 
> Yeah, true, it should be a port configuration.
> 
> I'll be updating the patch.

Wouldn't this indicate MDTS should be driven from hardware constraints
rather than user tunable knobs? I don't think we should have user
parameters if their only purpose is to test how host drivers react
to them. It's okay if they've a functional purpose, but there are so
many adjustable nvme parameters, this is a bit of a slippery slope if we
want to turn the nvme target driver into a host driver test vehicle.

[PATCH 1/2] nvmet: make MDTS value configurable

[Hannes Reinecke]

On 4/1/19 5:10 AM, Keith Busch wrote:
> On Sun, Mar 31, 2019@07:02:40PM -0700, Hannes Reinecke wrote:
>>> I think it's more of a ctrl/nvmet_port configuration than a subsystem.
>>>
>>> For example if you have 2 HCAs, one is super strong and can transfer
>>> upto 8MB and the second is old model and can transfer upto 64KiB.
>>>
>>> do
>>>
>>> id->mdts = ctrl->ops->port_mdts(req->port);
>>>
>>> and inside the implementation you can do
>>>
>>> min_not_zero(max_port_hca_cap_in_mdts_units, mdts_configfs_val);
>>>
>>> and it will fix the comment that is written above (we're not really
>>> unlimited).
>>>
>>> this way we're using the low level device characteristics and also cover
>>> the bio split test.
>>>
>>> thoughts ?
>>>
>>>
>> Yeah, true, it should be a port configuration.
>>
>> I'll be updating the patch.
> 
> Wouldn't this indicate MDTS should be driven from hardware constraints
> rather than user tunable knobs? I don't think we should have user
> parameters if their only purpose is to test how host drivers react
> to them. It's okay if they've a functional purpose, but there are so
> many adjustable nvme parameters, this is a bit of a slippery slope if we
> want to turn the nvme target driver into a host driver test vehicle.
> 
I would love to indicate hardware constraints to nvme target; there's a 
similar problem I'm facing in that the hardware is using only a limited 
set of queues, but I cannot express this with nvme target.
However, in the absense of such an interface having a manual 
configuration allows us to at least _map_ onto the hardware capabilities.

But I don't really buy the 'too many parameters' objection.
What _is_ the point of the software target if we cannot tweak it to test 
out certain corner cases in the spec?

Cheers,

Hannes
-- 
r. Hannes Reinecke            Teamlead Storage & Networking
hare at suse.de                              +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG N?rnberg)

[PATCH 1/2] nvmet: make MDTS value configurable

[Max Gurtovoy]

On 4/1/2019 2:48 PM, Hannes Reinecke wrote:
> On 4/1/19 5:10 AM, Keith Busch wrote:
>> On Sun, Mar 31, 2019@07:02:40PM -0700, Hannes Reinecke wrote:
>>>> I think it's more of a ctrl/nvmet_port configuration than a subsystem.
>>>>
>>>> For example if you have 2 HCAs, one is super strong and can transfer
>>>> upto 8MB and the second is old model and can transfer upto 64KiB.
>>>>
>>>> do
>>>>
>>>> id->mdts = ctrl->ops->port_mdts(req->port);
>>>>
>>>> and inside the implementation you can do
>>>>
>>>> min_not_zero(max_port_hca_cap_in_mdts_units, mdts_configfs_val);
>>>>
>>>> and it will fix the comment that is written above (we're not really
>>>> unlimited).
>>>>
>>>> this way we're using the low level device characteristics and also 
>>>> cover
>>>> the bio split test.
>>>>
>>>> thoughts ?
>>>>
>>>>
>>> Yeah, true, it should be a port configuration.
>>>
>>> I'll be updating the patch.
>>
>> Wouldn't this indicate MDTS should be driven from hardware constraints
>> rather than user tunable knobs? I don't think we should have user
>> parameters if their only purpose is to test how host drivers react
>> to them. It's okay if they've a functional purpose, but there are so
>> many adjustable nvme parameters, this is a bit of a slippery slope if we
>> want to turn the nvme target driver into a host driver test vehicle.
>>
> I would love to indicate hardware constraints to nvme target; there's 
> a similar problem I'm facing in that the hardware is using only a 
> limited set of queues, but I cannot express this with nvme target.
> However, in the absense of such an interface having a manual 
> configuration allows us to at least _map_ onto the hardware capabilities.

my suggestion exactly check the HW constraint.

My suggestion also makes sure that a greedy user will get a clamp and an 
economical user can save resources at the target.

Let's say I have a very "weak" target server with super HCA 
(ConnectX-5), so in that case I would like to set mdts=4 (or so) and let 
the initiator block layer to deal with 1MB IO splits.

Keith,

although NVMe has many params, I guess we can make some of the most 
vital ones to be tunable.

I agree regarding code adaptation for test-cases (we shouldn't change 
driver in order to write tests), but in this case it's a real world 
scenario.

>
> But I don't really buy the 'too many parameters' objection.

we need to take that into consideration as well and expose only vital 
params (we don't want to have hundreds of configfs entries as iSCSI)


> What _is_ the point of the software target if we cannot tweak it to 
> test out certain corner cases in the spec?
>
> Cheers,
>
> Hannes


-Max.

[PATCH 1/2] nvmet: make MDTS value configurable

[Keith Busch]

On Tue, Apr 02, 2019@03:06:02PM +0300, Max Gurtovoy wrote:
> On 4/1/2019 2:48 PM, Hannes Reinecke wrote:
> > On 4/1/19 5:10 AM, Keith Busch wrote:
> > > 
> > > Wouldn't this indicate MDTS should be driven from hardware constraints
> > > rather than user tunable knobs? I don't think we should have user
> > > parameters if their only purpose is to test how host drivers react
> > > to them. It's okay if they've a functional purpose, but there are so
> > > many adjustable nvme parameters, this is a bit of a slippery slope if we
> > > want to turn the nvme target driver into a host driver test vehicle.
> > > 
> > I would love to indicate hardware constraints to nvme target; there's a
> > similar problem I'm facing in that the hardware is using only a limited
> > set of queues, but I cannot express this with nvme target.
> > However, in the absense of such an interface having a manual
> > configuration allows us to at least _map_ onto the hardware
> > capabilities.
> 
> my suggestion exactly check the HW constraint.
> 
> My suggestion also makes sure that a greedy user will get a clamp and an
> economical user can save resources at the target.
> 
> Let's say I have a very "weak" target server with super HCA (ConnectX-5), so
> in that case I would like to set mdts=4 (or so) and let the initiator block
> layer to deal with 1MB IO splits.
> 
> Keith,
> 
> although NVMe has many params, I guess we can make some of the most vital
> ones to be tunable.
> 
> I agree regarding code adaptation for test-cases (we shouldn't change driver
> in order to write tests), but in this case it's a real world scenario.

Fair enough, I am all for config options that serve to help the nvme
target work better.

[PATCH 1/2] nvmet: make MDTS value configurable

[Christoph Hellwig]

On Sun, Mar 31, 2019@09:10:37PM -0600, Keith Busch wrote:
> Wouldn't this indicate MDTS should be driven from hardware constraints
> rather than user tunable knobs? I don't think we should have user
> parameters if their only purpose is to test how host drivers react
> to them. It's okay if they've a functional purpose, but there are so
> many adjustable nvme parameters, this is a bit of a slippery slope if we
> want to turn the nvme target driver into a host driver test vehicle.

Agreed.  In that past we've not merged patches that just include
tunables for testing purposes, and I'm inclined to keep that stance.

[PATCH 2/2] nvmet: make CNTLID range configurable

[Christoph Hellwig]

On Fri, Mar 29, 2019@01:04:04PM +0100, Hannes Reinecke wrote:
> When trying to run in a clustered environment we need to keep the
> controller ID values unique. So this patch add to additional config
> attributes 'cntlid_min' and 'cntlid_max' to only allow specific ranges
> for the controller ID.

Err, can you explain the use case a little better?  This sounds like
running two different nvmet instances and preting they are a single
subsystem.  Which sounds pretty dangerous without additional
communication layers.

[PATCH 2/2] nvmet: make CNTLID range configurable

[Hannes Reinecke]

On 4/3/19 8:11 PM, Christoph Hellwig wrote:
> On Fri, Mar 29, 2019@01:04:04PM +0100, Hannes Reinecke wrote:
>> When trying to run in a clustered environment we need to keep the
>> controller ID values unique. So this patch add to additional config
>> attributes 'cntlid_min' and 'cntlid_max' to only allow specific ranges
>> for the controller ID.
> 
> Err, can you explain the use case a little better?  This sounds like
> running two different nvmet instances and preting they are a single
> subsystem.  Which sounds pretty dangerous without additional
> communication layers.
> 
I'm running three qemu instances with RDMA HCAs via vfio.
Two instances are accessing the same backing device and exporting that 
via NVME-oF to the third; one exports the device as 'optimized' and the 
other as 'non-optimized'.
Idea it to power-cycle/reset/crash the first instance and test failover.

Cheers,

Hannes