[PATCH 0/3] nvmet: make model/ctrl-id configurable, configfs fix

[PATCH 0/3] nvmet: make model/ctrl-id configurable, configfs fix

[Chaitanya Kulkarni]

Hi,

This is a small patch series which allows user to configure the model
and controller ID. Also, patch #3 has a fix for missing sscanf check
in the nvmet_subsys_attr_serial_store().

The changelog is present in the respective patches.

Regards,
Chaitanya

Chaitanya Kulkarni (2):
  nvmet: make ctrl-id configurable
  nvmet: check sscanf value for subsys serial attr

Mark Ruijter (1):
  nvmet: make ctrl model configurable

 drivers/nvme/target/admin-cmd.c |  12 ++-
 drivers/nvme/target/configfs.c  | 125 +++++++++++++++++++++++++++++++-
 drivers/nvme/target/core.c      |   6 +-
 drivers/nvme/target/nvmet.h     |   6 ++
 4 files changed, 143 insertions(+), 6 deletions(-)

-- 
2.22.1


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

[PATCH V3 1/3] nvmet: make ctrl-id configurable

[Chaitanya Kulkarni]

This patch adds a new target subsys attribute which allows user to
optionally specify target controller IDs which then used in the
nvmet_execute_identify_ctrl() to fill up the nvme_id_ctrl structure.

For example, when using a cluster setup with two nodes, with a dual
ported NVMe drive and exporting the drive from both the nodes,
The connection to the host fails due to the same controller ID and
results in the following error message:-

"nvme nvmeX: Duplicate cntlid XXX with nvmeX, rejecting"

With this patch now user can partition the controller IDs for each
subsystem by setting up the cntlid_min and cntlid_max. These values
will be used at the time of the controller ID creation. By partitioning
the ctrl-ids for each subsystem results in the unique ctrl-id space
which avoids the collision.

When new attribute is not specified target will fall back to original
cntlid calculation method.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
Changes from V2:-

1. Reduce the size of the lock for sscanf for cntlid_min and cntlid_max.
2. Remove the common show and store function for cntlid.
3. Move cntlid_min and cntlid_max check into the store function. 
4. Update the patch description.
5. Move check for valid cntlid_[min|max] parameter into configfs
   respective store function.

Changes from V1:-

1. Add cntlid max and min configfs attributes.
2. Use simple if .. else statements instead of ternary operators.
---
 drivers/nvme/target/configfs.c | 72 ++++++++++++++++++++++++++++++++++
 drivers/nvme/target/core.c     |  5 ++-
 drivers/nvme/target/nvmet.h    |  2 +
 3 files changed, 77 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index 98613a45bd3b..772a74a7d969 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -862,10 +862,82 @@ static ssize_t nvmet_subsys_attr_serial_store(struct config_item *item,
 }
 CONFIGFS_ATTR(nvmet_subsys_, attr_serial);
 
+static ssize_t nvmet_subsys_attr_cntlid_min_show(struct config_item *item,
+						 char *page)
+{
+	return snprintf(page, PAGE_SIZE, "%u\n", to_subsys(item)->cntlid_min);
+}
+
+static ssize_t nvmet_subsys_attr_cntlid_min_store(struct config_item *item,
+						  const char *page, size_t cnt)
+{
+	int ret = -EINVAL;
+	u16 cntlid_min;
+
+	if (sscanf(page, "%hu\n", &cntlid_min) != 1)
+		return ret;
+
+	if (cntlid_min == 0) {
+		pr_info("specified cntlid_min = 0 is not allowed\n");
+		return ret;
+	}
+
+	down_write(&nvmet_config_sem);
+	if (cntlid_min >= to_subsys(item)->cntlid_max) {
+		pr_info("specified cntlid_min is >= current cntlid_max\n");
+		goto out;
+	}
+
+	ret = 0;
+	to_subsys(item)->cntlid_min = cntlid_min;
+out:
+	up_write(&nvmet_config_sem);
+
+	return ret ? ret : cnt;
+}
+CONFIGFS_ATTR(nvmet_subsys_, attr_cntlid_min);
+
+static ssize_t nvmet_subsys_attr_cntlid_max_show(struct config_item *item,
+						 char *page)
+{
+	return snprintf(page, PAGE_SIZE, "%u\n", to_subsys(item)->cntlid_max);
+}
+
+static ssize_t nvmet_subsys_attr_cntlid_max_store(struct config_item *item,
+						  const char *page, size_t cnt)
+{
+	int ret = -EINVAL;
+	u16 cntlid_max;
+
+	if (sscanf(page, "%hu\n", &cntlid_max) != 1)
+		return ret;
+
+	if (cntlid_max == 0) {
+		pr_info("specified cntlid_max = 0 is not allowed\n");
+		return ret;
+	}
+
+	down_write(&nvmet_config_sem);
+	if (cntlid_max <= to_subsys(item)->cntlid_min) {
+		pr_info("specified cntlid_max is <= current cntlid_min\n");
+		goto out;
+	}
+
+	ret = 0;
+	to_subsys(item)->cntlid_max = cntlid_max;
+out:
+	up_write(&nvmet_config_sem);
+
+	return ret ? ret : cnt;
+}
+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_cntlid_min,
+	&nvmet_subsys_attr_attr_cntlid_max,
 	NULL,
 };
 
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 28438b833c1b..387628ec7e37 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -1268,7 +1268,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->cntlid_min, subsys->cntlid_max,
 			     GFP_KERNEL);
 	if (ret < 0) {
 		status = NVME_SC_CONNECT_CTRL_BUSY | NVME_SC_DNR;
@@ -1416,7 +1416,8 @@ struct nvmet_subsys *nvmet_subsys_alloc(const char *subsysnqn,
 		kfree(subsys);
 		return ERR_PTR(-ENOMEM);
 	}
-
+	subsys->cntlid_min = NVME_CNTLID_MIN;
+	subsys->cntlid_max = NVME_CNTLID_MAX;
 	kref_init(&subsys->ref);
 
 	mutex_init(&subsys->lock);
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 46df45e837c9..6492d12e626a 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -211,6 +211,8 @@ struct nvmet_subsys {
 	struct list_head	namespaces;
 	unsigned int		nr_namespaces;
 	unsigned int		max_nsid;
+	u16			cntlid_min;
+	u16			cntlid_max;
 
 	struct list_head	ctrls;
 
-- 
2.22.1


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

[PATCH V4 2/3] nvmet: make ctrl model configurable

[Chaitanya Kulkarni]

From: Mark Ruijter <MRuijter@onestopsystems.com>

This patch adds a new target subsys attribute which allows user to
optionally specify model name which then used in the
nvmet_execute_identify_ctrl() to fill up the nvme_id_ctrl structure.

The default value for the model is set to "Linux" for backward
compatibility.

Signed-off-by: Mark Ruijter <MRuijter@onestopsystems.com>
[use macro for default model, coding style fixes]
Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
Changes from V3:-

1, Update commit description with additional changes on the top of
   original patch.
2. Reduce the size of the lock in nvmet_subsys_attr_model_store().
3. Don't initialize subsys->model = NULL nvmet_subsys_alloc().

Changes from V2:-

1. Use if else pattern than ternary operators.
2. Change the name nvmet_subsys_model() -> nvmet_model_number().
3. Introduce nvmet_is_ascii() to filter the model characters.
4. Change tmp -> tmp_model in nvmet_subsys_attr_model_store().

Changes from V1:-

1. Don't allocate memory for default subsys model,
2. Use helper to get the default model string from ctrl->subsys in the
   nvmet_execute_identify_ctrl() and nvmet_subsys_attr_model()_show.
   Later is needed so that nvmetcli can display default value Linux
   when the model is not set from the user.
3. Get rid of the extra variable c in the nvmet_subsys_attr_model_store()
   and replace for with while loop without loosing the code redability.
---
 drivers/nvme/target/admin-cmd.c | 12 +++++++--
 drivers/nvme/target/configfs.c  | 46 +++++++++++++++++++++++++++++++++
 drivers/nvme/target/core.c      |  1 +
 drivers/nvme/target/nvmet.h     |  4 +++
 4 files changed, 61 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 56c21b501185..613e715dc7d3 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -312,12 +312,20 @@ static void nvmet_execute_get_log_page(struct nvmet_req *req)
 	nvmet_req_complete(req, NVME_SC_INVALID_FIELD | NVME_SC_DNR);
 }
 
+const char *nvmet_model_number(struct nvmet_subsys *subsys)
+{
+	if (subsys->model)
+		return subsys->model;
+
+	return NVMET_DEFAULT_CTRL_MODEL;
+}
+
 static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
 {
+	const char *model = nvmet_model_number(req->sq->ctrl->subsys);
 	struct nvmet_ctrl *ctrl = req->sq->ctrl;
 	struct nvme_id_ctrl *id;
 	u16 status = 0;
-	const char model[] = "Linux";
 
 	id = kzalloc(sizeof(*id), GFP_KERNEL);
 	if (!id) {
@@ -332,7 +340,7 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
 	memset(id->sn, ' ', sizeof(id->sn));
 	bin2hex(id->sn, &ctrl->subsys->serial,
 		min(sizeof(ctrl->subsys->serial), sizeof(id->sn) / 2));
-	memcpy_and_pad(id->mn, sizeof(id->mn), model, sizeof(model) - 1, ' ');
+	memcpy_and_pad(id->mn, sizeof(id->mn), model, strlen(model), ' ');
 	memcpy_and_pad(id->fr, sizeof(id->fr),
 		       UTS_RELEASE, strlen(UTS_RELEASE), ' ');
 
diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index 772a74a7d969..4dea77a7da3e 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -932,12 +932,58 @@ static ssize_t nvmet_subsys_attr_cntlid_max_store(struct config_item *item,
 }
 CONFIGFS_ATTR(nvmet_subsys_, attr_cntlid_max);
 
+static ssize_t nvmet_subsys_attr_model_show(struct config_item *item,
+					    char *page)
+{
+	struct nvmet_subsys *subsys = to_subsys(item);
+
+	return snprintf(page, PAGE_SIZE, "%s\n",
+			nvmet_model_number(subsys));
+}
+
+/* See Section 1.5 of NVMe 1.4 */
+static bool nvmet_is_ascii(const char c)
+{
+	return c >= 0x20 && c <= 0x7e;
+}
+
+static ssize_t nvmet_subsys_attr_model_store(struct config_item *item,
+					     const char *page, size_t count)
+{
+	struct nvmet_subsys *subsys = to_subsys(item);
+	int pos = 0, len;
+	char *tmp_model;
+
+	len = strcspn(page, "\n");
+	if (!len)
+		return -EINVAL;
+
+	for (pos = 0; pos < len; pos++) {
+		if (!nvmet_is_ascii(page[pos]))
+			return -EINVAL;
+	}
+
+	tmp_model = kstrndup(page, len, GFP_KERNEL);
+	if (!tmp_model)
+		return -ENOMEM;
+
+	down_write(&nvmet_config_sem);
+	kfree(subsys->model);
+	subsys->model = tmp_model;
+	up_write(&nvmet_config_sem);
+
+	return count;
+}
+
+CONFIGFS_ATTR(nvmet_subsys_, attr_model);
+
 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_cntlid_min,
 	&nvmet_subsys_attr_attr_cntlid_max,
+	&nvmet_subsys_attr_attr_model,
 	NULL,
 };
 
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 387628ec7e37..c041d88ed3ea 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -1436,6 +1436,7 @@ static void nvmet_subsys_free(struct kref *ref)
 	WARN_ON_ONCE(!list_empty(&subsys->namespaces));
 
 	kfree(subsys->subsysnqn);
+	kfree(subsys->model);
 	kfree(subsys);
 }
 
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 6492d12e626a..c54ce5f66f09 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -23,6 +23,7 @@
 #define NVMET_ASYNC_EVENTS		4
 #define NVMET_ERROR_LOG_SLOTS		128
 #define NVMET_NO_ERROR_LOC		((u16)-1)
+#define NVMET_DEFAULT_CTRL_MODEL	"Linux"
 
 /*
  * Supported optional AENs:
@@ -224,6 +225,7 @@ struct nvmet_subsys {
 	u64			ver;
 	u64			serial;
 	char			*subsysnqn;
+	char			*model;
 
 	struct config_group	group;
 
@@ -489,6 +491,8 @@ u16 nvmet_bdev_flush(struct nvmet_req *req);
 u16 nvmet_file_flush(struct nvmet_req *req);
 void nvmet_ns_changed(struct nvmet_subsys *subsys, u32 nsid);
 
+const char *nvmet_model_number(struct nvmet_subsys *subsys);
+
 static inline u32 nvmet_rw_len(struct nvmet_req *req)
 {
 	return ((u32)le16_to_cpu(req->cmd->rw.length) + 1) <<
-- 
2.22.1


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

[PATCH V2 3/3] nvmet: check sscanf value for subsys serial attr

[Chaitanya Kulkarni]

For nvmet in configfs.c we check return values for all the sscanf()
calls. Add similar check into the nvmet_subsys_attr_serial_store().

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
Changes from V1:-
1. Use temp variable to scan the new serial value and retain original
   value if sscanf() fails.
---
 drivers/nvme/target/configfs.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index 4dea77a7da3e..0a19fd73979d 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -852,10 +852,13 @@ static ssize_t nvmet_subsys_attr_serial_show(struct config_item *item,
 static ssize_t nvmet_subsys_attr_serial_store(struct config_item *item,
 					      const char *page, size_t count)
 {
-	struct nvmet_subsys *subsys = to_subsys(item);
+	u64 serial;
+
+	if (sscanf(page, "%llx\n", &serial) != 1)
+		return -EINVAL;
 
 	down_write(&nvmet_config_sem);
-	sscanf(page, "%llx\n", &subsys->serial);
+	to_subsys(item)->serial = serial;
 	up_write(&nvmet_config_sem);
 
 	return count;
-- 
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: make model/ctrl-id configurable, configfs fix

[Chaitanya Kulkarni]

Ping ?
On 11/27/2019 01:40 AM, Chaitanya Kulkarni wrote:
> Hi,
>
> This is a small patch series which allows user to configure the model
> and controller ID. Also, patch #3 has a fix for missing sscanf check
> in the nvmet_subsys_attr_serial_store().
>
> The changelog is present in the respective patches.
>
> Regards,
> Chaitanya
>
> Chaitanya Kulkarni (2):
>    nvmet: make ctrl-id configurable
>    nvmet: check sscanf value for subsys serial attr
>
> Mark Ruijter (1):
>    nvmet: make ctrl model configurable
>
>   drivers/nvme/target/admin-cmd.c |  12 ++-
>   drivers/nvme/target/configfs.c  | 125 +++++++++++++++++++++++++++++++-
>   drivers/nvme/target/core.c      |   6 +-
>   drivers/nvme/target/nvmet.h     |   6 ++
>   4 files changed, 143 insertions(+), 6 deletions(-)
>


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

Re: [PATCH V3 1/3] nvmet: make ctrl-id configurable

[Sagi Grimberg]

> +static ssize_t nvmet_subsys_attr_cntlid_max_store(struct config_item *item,
> +						  const char *page, size_t cnt)
> +{
> +	int ret = -EINVAL;
> +	u16 cntlid_max;
> +
> +	if (sscanf(page, "%hu\n", &cntlid_max) != 1)
> +		return ret;
> +
> +	if (cntlid_max == 0) {
> +		pr_info("specified cntlid_max = 0 is not allowed\n");
> +		return ret;
> +	}

Check not exceeds unsigned short max value?

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

Re: [PATCH V4 2/3] nvmet: make ctrl model configurable

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

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

Re: [PATCH V2 3/3] nvmet: check sscanf value for subsys serial attr

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

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

Re: [PATCH V3 1/3] nvmet: make ctrl-id configurable

[Christoph Hellwig]

On Wed, Dec 11, 2019 at 04:45:02PM -0800, Sagi Grimberg wrote:
>
>> +static ssize_t nvmet_subsys_attr_cntlid_max_store(struct config_item *item,
>> +						  const char *page, size_t cnt)
>> +{
>> +	int ret = -EINVAL;
>> +	u16 cntlid_max;
>> +
>> +	if (sscanf(page, "%hu\n", &cntlid_max) != 1)
>> +		return ret;
>> +
>> +	if (cntlid_max == 0) {
>> +		pr_info("specified cntlid_max = 0 is not allowed\n");
>> +		return ret;
>> +	}
>
> Check not exceeds unsigned short max value?

sscanf with %hu takes care of that for you.

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

Re: [PATCH V3 1/3] nvmet: make ctrl-id configurable

[Christoph Hellwig]

On Wed, Nov 27, 2019 at 01:40:32AM -0800, Chaitanya Kulkarni wrote:
> +	int ret = -EINVAL;
> +	u16 cntlid_min;
> +
> +	if (sscanf(page, "%hu\n", &cntlid_min) != 1)
> +		return ret;
> +
> +	if (cntlid_min == 0) {
> +		pr_info("specified cntlid_min = 0 is not allowed\n");
> +		return ret;
> +	}
> +
> +	down_write(&nvmet_config_sem);
> +	if (cntlid_min >= to_subsys(item)->cntlid_max) {
> +		pr_info("specified cntlid_min is >= current cntlid_max\n");
> +		goto out;

I'm not sure we need the pr_info calls here.  The error conditions
are pretty clear when we get an -EINVAL from the write.

> +	}
> +
> +	ret = 0;
> +	to_subsys(item)->cntlid_min = cntlid_min;
> +out:
> +	up_write(&nvmet_config_sem);
> +
> +	return ret ? ret : cnt;

The tail of the function could be simplified by avoid the ret variable.
the ret variable:

	down_write(&nvmet_config_sem);
	if (cntlid_min >= to_subsys(item)->cntlid_max)
		goto out_unlock;
	to_subsys(item)->cntlid_min = cntlid_min;
	up_write(&nvmet_config_sem);
	return cnt;

out_unlock:
	up_write(&nvmet_config_sem);
	return -EINVAL;


> +	if (cntlid_max == 0) {
> +		pr_info("specified cntlid_max = 0 is not allowed\n");
> +		return ret;
> +	}
> +
> +	down_write(&nvmet_config_sem);
> +	if (cntlid_max <= to_subsys(item)->cntlid_min) {
> +		pr_info("specified cntlid_max is <= current cntlid_min\n");
> +		goto out;
> +	}
> +
> +	ret = 0;
> +	to_subsys(item)->cntlid_max = cntlid_max;
> +out:
> +	up_write(&nvmet_config_sem);
> +
> +	return ret ? ret : cnt;

Same comments here.

>  	}
> -
> +	subsys->cntlid_min = NVME_CNTLID_MIN;
> +	subsys->cntlid_max = NVME_CNTLID_MAX;
>  	kref_init(&subsys->ref);
>  
>  	mutex_init(&subsys->lock);
> diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
> index 46df45e837c9..6492d12e626a 100644
> --- a/drivers/nvme/target/nvmet.h
> +++ b/drivers/nvme/target/nvmet.h
> @@ -211,6 +211,8 @@ struct nvmet_subsys {
>  	struct list_head	namespaces;
>  	unsigned int		nr_namespaces;
>  	unsigned int		max_nsid;
> +	u16			cntlid_min;
> +	u16			cntlid_max;
>  
>  	struct list_head	ctrls;
>  
> -- 
> 2.22.1
---end quoted text---

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

Re: [PATCH V4 2/3] nvmet: make ctrl model configurable

[Christoph Hellwig]

> +static ssize_t nvmet_subsys_attr_model_show(struct config_item *item,
> +					    char *page)
> +{
> +	struct nvmet_subsys *subsys = to_subsys(item);
> +
> +	return snprintf(page, PAGE_SIZE, "%s\n",
> +			nvmet_model_number(subsys));

This whole statements easily fits onto a single line.

> +	down_write(&nvmet_config_sem);
> +	kfree(subsys->model);
> +	subsys->model = tmp_model;
> +	up_write(&nvmet_config_sem);

Without using something like RCU this means all users of ->model
now need to take nvmet_config_sem.  I guess using RCU here is simple
enough - just use rcu_swap_protected and kfree_rcu here.

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

Re: [PATCH V2 3/3] nvmet: check sscanf value for subsys serial attr

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

Re: [PATCH V4 2/3] nvmet: make ctrl model configurable

[Christoph Hellwig]

On Thu, Dec 12, 2019 at 10:28:42AM +0100, Christoph Hellwig wrote:
> Without using something like RCU this means all users of ->model
> now need to take nvmet_config_sem.  I guess using RCU here is simple
> enough - just use rcu_swap_protected and kfree_rcu here.

Actually, this should be rcu_replace_pointer instead of
rcu_swap_protected, which has been removed in for-next.

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

Re: [PATCH V3 1/3] nvmet: make ctrl-id configurable

[Chaitanya Kulkarni]

Thanks for the comments, sending next version shortly.

On 12/12/2019 01:26 AM, Christoph Hellwig wrote:
> On Wed, Nov 27, 2019 at 01:40:32AM -0800, Chaitanya Kulkarni wrote:
>> >+	int ret = -EINVAL;
>> >+	u16 cntlid_min;


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

Re: [PATCH V4 2/3] nvmet: make ctrl model configurable

[Chaitanya Kulkarni]

On 12/12/2019 01:28 AM, Christoph Hellwig wrote:
>> +static ssize_t nvmet_subsys_attr_model_show(struct config_item *item,
>> +					    char *page)
>> +{
>> +	struct nvmet_subsys *subsys = to_subsys(item);
>> +
>> +	return snprintf(page, PAGE_SIZE, "%s\n",
>> +			nvmet_model_number(subsys));
>
> This whole statements easily fits onto a single line.
>
>> +	down_write(&nvmet_config_sem);
>> +	kfree(subsys->model);
>> +	subsys->model = tmp_model;
>> +	up_write(&nvmet_config_sem);
>
> Without using something like RCU this means all users of ->model
> now need to take nvmet_config_sem.  I guess using RCU here is simple
> enough - just use rcu_swap_protected and kfree_rcu here.
>

Since this is not into the fast path subsys->lock can be used for that ?
(and in the helper nvmet_model_number() ?)

OR

Is there a specific reason we need to use rcu ?

Also similar pattern(use of nvmet_config_sem) exists for the
subsys->serial, does that mean whatever fix we do for subsys->model that 
needs to be repeated for subsys->serial ?





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