nvmet: add buffered I/O support for file backed ns

PATCH Superseded Chaitanya Kulkarni 3 participants 5 messages Jun 20, 2018 – Jun 21, 2018

Subsystems (2)

NVMe NVMe target

lore

Share & tools

lore

Chaitanya Kulkarni

author

2018-06-20 04:01 UTC #0 lore raw

PATCH v4 1/1 nvmet: add buffered I/O support for file backed ns

This is an incremental patch based on adding support for file backed namespaces for NVMeOF target:-

http://lists.infradead.org/pipermail/linux-nvme/2018-May/017775.html/
http://lists.infradead.org/pipermail/linux-nvme/2018-May/017777.html.

We introduce "buffered_io" a new target namespace attribute. In default execution, we set "buffered_io" to false, use O_DIRECT flag when opening backend file and set the appropriate direct I/O flags when submitting I/Os. In order to switch between the modes user needs to disable namespace through configfs and then set the value of the "buffered_io". This new value will have an effect after namespace is enabled manually.

In order to handle Read/Write requests when "buffered_io" is set we introduce per namespace workqueue.

Signed-off-by Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>

---
Changes since V3 :-
1. Remove transparent mode switch and allow to switch the buffered and direct I/O mode only when ns is disabled manually.
2. Change the patch title.

Changes since V2 :-
1. Add support for transparent mode switch between buffered and direct I/O.

Changes since V1 :-
1. Rename the "vwc" namespace configfs attribute to the "buffered_io".

---
 drivers/nvme/target/configfs.c    | 30 +++++++++++++++++++++
 drivers/nvme/target/core.c        |  1 +
 drivers/nvme/target/io-cmd-file.c | 43 +++++++++++++++++++++++++++----
 drivers/nvme/target/nvmet.h       |  2 ++
 4 files changed, 71 insertions(+), 5 deletions(-)

drivers/nvme/target/configfs.c +30 -0

index d3f3b3ec4d1a..3c06c517f009 100644
@@ -407,11 +407,41 @@ static ssize_t nvmet_ns_enable_store(struct config_item *item,
 
 CONFIGFS_ATTR(nvmet_ns_, enable);
 
+static ssize_t nvmet_ns_buffered_io_show(struct config_item *item, char *page)
+{
+	return sprintf(page, "%d\n", to_nvmet_ns(item)->buffered_io);
+}
+
+static ssize_t nvmet_ns_buffered_io_store(struct config_item *item,
+		const char *page, size_t count)
+{
+	struct nvmet_ns *ns = to_nvmet_ns(item);
+	bool buffered_io;
+	int ret = 0;
+
+	if (strtobool(page, &buffered_io))
+		return -EINVAL;
+
+	mutex_unlock(&ns->subsys->lock);
+	if (ns->enabled == false)
+		ns->buffered_io = buffered_io;
+	else {
+		pr_err("disable ns before setting buffered_io value.\n");
+		ret = -EINVAL;
+	}
+	mutex_unlock(&ns->subsys->lock);
+
+	return ret ? ret : count;
+}
+
+CONFIGFS_ATTR(nvmet_ns_, buffered_io);
+
 static struct configfs_attribute *nvmet_ns_attrs[] = {
 	&nvmet_ns_attr_device_path,
 	&nvmet_ns_attr_device_nguid,
 	&nvmet_ns_attr_device_uuid,
 	&nvmet_ns_attr_enable,
+	&nvmet_ns_attr_buffered_io,
 	NULL,
 };

drivers/nvme/target/core.c +1 -0

index a03da764ecae..2888ed6d8665 100644
@@ -437,6 +437,7 @@ struct nvmet_ns *nvmet_ns_alloc(struct nvmet_subsys *subsys, u32 nsid)
 	ns->nsid = nsid;
 	ns->subsys = subsys;
 	uuid_gen(&ns->uuid);
+	ns->buffered_io = false;
 
 	return ns;
 }

drivers/nvme/target/io-cmd-file.c +38 -5

81 lines not on this page · raw message

drivers/nvme/target/nvmet.h +2 -0

index 480dfe10fad9..c4aa54d933f1 100644
@@ -65,6 +65,8 @@ struct nvmet_ns {
 	u8			nguid[16];
 	uuid_t			uuid;
 
+	bool			buffered_io;
+	struct workqueue_struct *file_wq;
 	bool			enabled;
 	struct nvmet_subsys	*subsys;
 	const char		*device_path;

Signature · 1 line

2.17.0

Christoph Hellwig

maintainer

2018-06-20 08:21 UTC #1 lore raw

PATCH v4 1/1 nvmet: add buffered I/O support for file backed ns

drivers/nvme/target/io-cmd-file.c int nvmet_file_ns_enable(struct nvmet_ns *ns) · line 75

Quoted 2 lines

drivers/nvme/target/io-cmd-file.c +2 -0

+		ns->file_wq = alloc_workqueue("nvmet-file",
+				WQ_UNBOUND_MAX_ACTIVE | WQ_MEM_RECLAIM, 0);

The WQ_UNBOUND_MAX_ACTIVE here looks bogus, as far as I can tell it isn't a flag for alloc_workqueue but needs to be passed as the last argument.

I've removed it for now, and applied the patch with a few minor cleanups to the nvme-4.19 branch.

Sagi Grimberg

maintainer

2018-06-20 17:44 UTC #2 lore raw

PATCH v4 1/1 nvmet: add buffered I/O support for file backed ns

In reply to Chaitanya Kulkarni

Quoted 11 lines

drivers/nvme/target/configfs.c +10 -0

+static ssize_t nvmet_ns_buffered_io_store(struct config_item *item,
+		const char *page, size_t count)
+{
+	struct nvmet_ns *ns = to_nvmet_ns(item);
+	bool buffered_io;
+	int ret = 0;
+
+	if (strtobool(page, &buffered_io))
+		return -EINVAL;
+

drivers/nvme/target/configfs.c static ssize_t nvmet_ns_enable_store(struct config_item *item, · line 434

Quoted 10 lines

drivers/nvme/target/configfs.c +10 -0

+	mutex_unlock(&ns->subsys->lock);
+	if (ns->enabled == false)
+		ns->buffered_io = buffered_io;
+	else {
+		pr_err("disable ns before setting buffered_io value.\n");
+		ret = -EINVAL;
+	}
+	mutex_unlock(&ns->subsys->lock);
+
+	return ret ? ret : count;

Consider changing this to:

	mutex_unlock(&ns->subsys->lock);
	if (ns->enabled) {
		pr_err("cannot modify ns while enabled\n");
		ret = -EINVAL;
		goto out_unlocked;
	}

	ns->buffered_io = buffered_io;
	ret = count;
out_unlock:
	mutex_unlock(&ns->subsys->lock);
	return ret;

Quoted 30 lines

drivers/nvme/target/configfs.c +5 -0

+}
+
+CONFIGFS_ATTR(nvmet_ns_, buffered_io);
+
  static struct configfs_attribute *nvmet_ns_attrs[] = {
  	&nvmet_ns_attr_device_path,
  	&nvmet_ns_attr_device_nguid,
  	&nvmet_ns_attr_device_uuid,
  	&nvmet_ns_attr_enable,
+	&nvmet_ns_attr_buffered_io,
  	NULL,
  };

drivers/nvme/target/core.c +1 -0

index a03da764ecae..2888ed6d8665 100644
@@ -437,6 +437,7 @@ struct nvmet_ns *nvmet_ns_alloc(struct nvmet_subsys *subsys, u32 nsid)
  	ns->nsid = nsid;
  	ns->subsys = subsys;
  	uuid_gen(&ns->uuid);
+	ns->buffered_io = false;
 
  	return ns;
  }

drivers/nvme/target/io-cmd-file.c +0 -0

index 8c42b3a8c420..09d793442833 100644

drivers/nvme/target/io-cmd-file.c line 21

Quoted 6 lines

drivers/nvme/target/io-cmd-file.c +3 -0

@@ -16,6 +16,11 @@
  void nvmet_file_ns_disable(struct nvmet_ns *ns)
  {
  	if (ns->file) {
+		if (ns->buffered_io) {
+			flush_workqueue(ns->file_wq);
+			destroy_workqueue(ns->file_wq);

destroy_workqueue flushes so no need to do that explicitly.

Quoted 24 lines

drivers/nvme/target/io-cmd-file.c +8 -3

+			ns->file_wq = NULL;
+		}
  		mempool_destroy(ns->bvec_pool);
  		ns->bvec_pool = NULL;
  		kmem_cache_destroy(ns->bvec_cache);

@@ -27,11 +32,14 @@ void nvmet_file_ns_disable(struct nvmet_ns *ns)
 
  int nvmet_file_ns_enable(struct nvmet_ns *ns)
  {
-	int ret;
+	int flags = O_RDWR | O_LARGEFILE;
  	struct kstat stat;
+	int ret;
+
+	if (!ns->buffered_io)
+		flags |= O_DIRECT;
 
-	ns->file = filp_open(ns->device_path,
-			O_RDWR | O_LARGEFILE | O_DIRECT, 0);
+	ns->file = filp_open(ns->device_path, flags, 0);
  	if (IS_ERR(ns->file)) {
  		pr_err("failed to open file %s: (%ld)\n",
  				ns->device_path, PTR_ERR(ns->file));

drivers/nvme/target/io-cmd-file.c int nvmet_file_ns_enable(struct nvmet_ns *ns) · line 75

Quoted 6 lines

drivers/nvme/target/io-cmd-file.c +3 -0

@@ -62,6 +70,15 @@ int nvmet_file_ns_enable(struct nvmet_ns *ns)
  		goto err;
  	}
 
+	if (ns->buffered_io) {
+		ns->file_wq = alloc_workqueue("nvmet-file",
+				WQ_UNBOUND_MAX_ACTIVE | WQ_MEM_RECLAIM, 0);

I seriously don't think that a wq per ns is a good idea.. What was the consideration for this?

And what's wrong with the system_unbound_wq?

Chaitanya Kulkarni

author

2018-06-20 21:44 UTC #3 lore raw

PATCH v4 1/1 nvmet: add buffered I/O support for file backed ns

From: Sagi Grimberg <[email protected]>
Sent: Wednesday, June 20, 2018 10:44 AM
To: Chaitanya Kulkarni; linux-nvme at lists.infradead.org
Cc: hch at lst.de; keith.busch at intel.com
Subject: Re: [PATCH V4] nvmet: add buffered I/O support for file backed ns ?

Quoted 10 lines

(quoted diff) +10 -0

+static ssize_t nvmet_ns_buffered_io_store(struct config_item *item,
+???????????? const char *page, size_t count)
+{
+???? struct nvmet_ns *ns = to_nvmet_ns(item);
+???? bool buffered_io;
+???? int ret = 0;
+
+???? if (strtobool(page, &buffered_io))
+???????????? return -EINVAL;
+

Review

Quoted 10 lines

(quoted diff) +10 -0

+???? mutex_unlock(&ns->subsys->lock);
+???? if (ns->enabled == false)
+???????????? ns->buffered_io = buffered_io;
+???? else {
+???????????? pr_err("disable ns before setting buffered_io value.\n");
+???????????? ret = -EINVAL;
+???? }
+???? mutex_unlock(&ns->subsys->lock);
+
+???? return ret ? ret : count;

Consider changing this to:

??????? mutex_unlock(&ns->subsys->lock);
??????? if (ns->enabled) {
??????????????? pr_err("cannot modify ns while enabled\n"); ??????????????? ret = -EINVAL;
??????????????? goto out_unlocked;
??????? }

??????? ns->buffered_io = buffered_io;
??????? ret = count;
out_unlock:
??????? mutex_unlock(&ns->subsys->lock);
??????? return ret;

[CK] I'm okay to change that if you prefer this style, can you please explain
is there anything wrong with the current code?

Quoted 30 lines

drivers/nvme/target/configfs.c +4 -0
+}
+
+CONFIGFS_ATTR(nvmet_ns_, buffered_io);
+
?? static struct configfs_attribute *nvmet_ns_attrs[] = { ??????? &nvmet_ns_attr_device_path,
??????? &nvmet_ns_attr_device_nguid,
??????? &nvmet_ns_attr_device_uuid,
??????? &nvmet_ns_attr_enable,
+???? &nvmet_ns_attr_buffered_io,
??????? NULL,
?? };
??
drivers/nvme/target/core.c +1 -0
index a03da764ecae..2888ed6d8665 100644
@@ -437,6 +437,7 @@ struct nvmet_ns *nvmet_ns_alloc(struct nvmet_subsys *subsys, u32 nsid)
 ??????? ns->nsid = nsid;
 ??????? ns->subsys = subsys;
 ??????? uuid_gen(&ns->uuid);
+???? ns->buffered_io = false;
??
??????? return ns;
?? }
drivers/nvme/target/io-cmd-file.c +0 -0
index 8c42b3a8c420..09d793442833 100644

drivers/nvme/target/io-cmd-file.c line 21

Quoted 6 lines

drivers/nvme/target/io-cmd-file.c +3 -0

@@ -16,6 +16,11 @@
 ?? void nvmet_file_ns_disable(struct nvmet_ns *ns)
 ?? {
 ??????? if (ns->file) {
+???????????? if (ns->buffered_io) {
+???????????????????? flush_workqueue(ns->file_wq);
+???????????????????? destroy_workqueue(ns->file_wq);

destroy_workqueue flushes so no need to do that explicitly. [CK] Okay.

Quoted 26 lines

drivers/nvme/target/io-cmd-file.c +2 -0
+???????????????????? ns->file_wq = NULL;
+???????????? }
 ??????????????? mempool_destroy(ns->bvec_pool);
 ??????????????? ns->bvec_pool = NULL;
 ??????????????? kmem_cache_destroy(ns->bvec_cache);
@@ -27,11 +32,14 @@ void nvmet_file_ns_disable(struct nvmet_ns *ns)
??
?? int nvmet_file_ns_enable(struct nvmet_ns *ns)
?? {
-???? int ret;
+???? int flags = O_RDWR | O_LARGEFILE;
??????? struct kstat stat;
+???? int ret;
+
+???? if (!ns->buffered_io)
+???????????? flags |= O_DIRECT;
??
-???? ns->file = filp_open(ns->device_path, -???????????????????? O_RDWR | O_LARGEFILE | O_DIRECT, 0); +???? ns->file = filp_open(ns->device_path, flags, 0); ??????? if (IS_ERR(ns->file)) {
??????????????? pr_err("failed to open file %s: (%ld)\n", ??????????????????????????????? ns->device_path, PTR_ERR(ns->file));
drivers/nvme/target/io-cmd-file.c +0 -0
@@ -62,6 +70,15 @@ int nvmet_file_ns_enable(struct nvmet_ns *ns)
 ??????????????? goto err;
 ??????? }

Quoted 4 lines

??
+???? if (ns->buffered_io) {
+???????????? ns->file_wq = alloc_workqueue("nvmet-file", +???????????????????????????? WQ_UNBOUND_MAX_ACTIVE | WQ_MEM_RECLAIM, 0);

I seriously don't think that a wq per ns is a good idea.. What was the consideration for this?

[CK] Since I/O submission is done per ns basis it is important to encapsulate workqueue and
make the property of the ns. In this way, I/Os from different namespaces will not interfere with each other.

Can you please explain the benefits of using the following model:-

1. 1: N (1 workqueue : N Namespace).
2. M: N (M workqueues : N Namespace).

[CK] Now that code is in, I'll send the configfs buffered_io parameter style change and removing
the flush wq as a separate patch.

And what's wrong with the system_unbound_wq?

Regards,
Chaitanya

Sagi Grimberg

maintainer

2018-06-21 06:51 UTC #4 lore raw

PATCH v4 1/1 nvmet: add buffered I/O support for file backed ns

Quoted 3 lines

[CK] I'm okay to change that if you prefer this style, can you please explain
is there anything wrong with the current code?

Its not wrong, its just a styling suggestion.

Quoted 29 lines

Quoted 19 lines

(quoted diff) +6 -3

@@ -27,11 +32,14 @@ void nvmet_file_ns_disable(struct nvmet_ns *ns)
 
 ?? int nvmet_file_ns_enable(struct nvmet_ns *ns)
 ?? {
-???? int ret;
+???? int flags = O_RDWR | O_LARGEFILE;
 ??????? struct kstat stat;
+???? int ret;
+
+???? if (!ns->buffered_io)
+???????????? flags |= O_DIRECT;
 
-???? ns->file = filp_open(ns->device_path,
-???????????????????? O_RDWR | O_LARGEFILE | O_DIRECT, 0);
+???? ns->file = filp_open(ns->device_path, flags, 0);
 ??????? if (IS_ERR(ns->file)) {
 ??????????????? pr_err("failed to open file %s: (%ld)\n",
 ??????????????????????????????? ns->device_path, PTR_ERR(ns->file));

Review int nvmet_file_ns_enable(struct nvmet_ns *ns) · line 75

Quoted 6 lines

(quoted diff) +3 -0

@@ -62,6 +70,15 @@ int nvmet_file_ns_enable(struct nvmet_ns *ns)
 ??????????????? goto err;
 ??????? }
 
+???? if (ns->buffered_io) {
+???????????? ns->file_wq = alloc_workqueue("nvmet-file",
+???????????????????????????? WQ_UNBOUND_MAX_ACTIVE | WQ_MEM_RECLAIM, 0);

I seriously don't think that a wq per ns is a good idea.. What was the consideration for this?

Quoted 8 lines

[CK] Since I/O submission is done per ns basis it is important to encapsulate workqueue and
make the property of the ns. In this way, I/Os from different namespaces will not interfere with each other.
Can you please explain the benefits of using the following model:-
1. 1: N (1 workqueue : N Namespace).
2. M: N (M workqueues : N Namespace).

wqs still share the same kworkers and workqueue access is rcu protected so I'm not at all sure why this should make a difference. Have you benchmarked per-ns workqueue vs. a shared one? I'd be interested in knowing if it makes a difference. It would surprise me if it did...

Quoted 2 lines

[CK] Now that code is in, I'll send the configfs buffered_io parameter style change and removing
the flush wq as a separate patch.

We have time to fix it in-place, its not sent to Jens yet..