[PATCH V4] nvmet: add buffered I/O support for file backed ns

[PATCH V4] nvmet: add buffered I/O support for file backed ns

[Chaitanya Kulkarni]

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(-)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index d3f3b3ec4d1a..3c06c517f009 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -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,
 };
 
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index a03da764ecae..2888ed6d8665 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -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;
 }
diff --git a/drivers/nvme/target/io-cmd-file.c b/drivers/nvme/target/io-cmd-file.c
index 8c42b3a8c420..09d793442833 100644
--- a/drivers/nvme/target/io-cmd-file.c
+++ b/drivers/nvme/target/io-cmd-file.c
@@ -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);
+			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));
@@ -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);
+		if (!ns->file_wq) {
+			ret = -ENOMEM;
+			goto err;
+		}
+	}
+
 	return ret;
 err:
 	ns->size = 0;
@@ -100,7 +117,7 @@ static ssize_t nvmet_file_submit_bvec(struct nvmet_req *req, loff_t pos,
 
 	iocb->ki_pos = pos;
 	iocb->ki_filp = req->ns->file;
-	iocb->ki_flags = IOCB_DIRECT | ki_flags;
+	iocb->ki_flags = ki_flags | iocb_flags(req->ns->file);
 
 	ret = call_iter(iocb, &iter);
 
@@ -189,6 +206,19 @@ static void nvmet_file_execute_rw(struct nvmet_req *req)
 	nvmet_file_submit_bvec(req, pos, bv_cnt, total_len);
 }
 
+static void nvmet_file_buffered_io_work(struct work_struct *w)
+{
+	struct nvmet_req *req = container_of(w, struct nvmet_req, f.work);
+
+	nvmet_file_execute_rw(req);
+}
+
+static void nvmet_file_execute_rw_buffered_io(struct nvmet_req *req)
+{
+	INIT_WORK(&req->f.work, nvmet_file_buffered_io_work);
+	queue_work(req->ns->file_wq, &req->f.work);
+}
+
 static void nvmet_file_flush_work(struct work_struct *w)
 {
 	struct nvmet_req *req = container_of(w, struct nvmet_req, f.work);
@@ -280,7 +310,10 @@ u16 nvmet_file_parse_io_cmd(struct nvmet_req *req)
 	switch (cmd->common.opcode) {
 	case nvme_cmd_read:
 	case nvme_cmd_write:
-		req->execute = nvmet_file_execute_rw;
+		if (!req->ns->buffered_io)
+			req->execute = nvmet_file_execute_rw;
+		else
+			req->execute = nvmet_file_execute_rw_buffered_io;
 		req->data_len = nvmet_rw_len(req);
 		return 0;
 	case nvme_cmd_flush:
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 480dfe10fad9..c4aa54d933f1 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -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;
-- 
2.17.0

[PATCH V4] nvmet: add buffered I/O support for file backed ns

[Christoph Hellwig]

> +		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.

[PATCH V4] nvmet: add buffered I/O support for file backed ns

[Sagi Grimberg]

> +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;

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;

> +}
> +
> +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,
>   };
>   
> diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
> index a03da764ecae..2888ed6d8665 100644
> --- a/drivers/nvme/target/core.c
> +++ b/drivers/nvme/target/core.c
> @@ -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;
>   }
> diff --git a/drivers/nvme/target/io-cmd-file.c b/drivers/nvme/target/io-cmd-file.c
> index 8c42b3a8c420..09d793442833 100644
> --- a/drivers/nvme/target/io-cmd-file.c
> +++ b/drivers/nvme/target/io-cmd-file.c
> @@ -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.

> +			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));
> @@ -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?

[PATCH V4] nvmet: add buffered I/O support for file backed ns

[Chaitanya Kulkarni]

From: Sagi Grimberg <sagi@grimberg.me>
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
? 
 

> +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;

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?

> +}
> +
> +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,
>?? };
>?? 
> diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
> index a03da764ecae..2888ed6d8665 100644
> --- a/drivers/nvme/target/core.c
> +++ b/drivers/nvme/target/core.c
> @@ -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;
>?? }
> diff --git a/drivers/nvme/target/io-cmd-file.c b/drivers/nvme/target/io-cmd-file.c
> index 8c42b3a8c420..09d793442833 100644
> --- a/drivers/nvme/target/io-cmd-file.c
> +++ b/drivers/nvme/target/io-cmd-file.c
> @@ -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.

> +???????????????????? 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));
> @@ -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?

[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

[PATCH V4] nvmet: add buffered I/O support for file backed ns

[Sagi Grimberg]

> [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.

>> @@ -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));
>> @@ -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?
> 
> [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...

> [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..