nvmet: implement flush all support

PATCH Superseded Chaitanya Kulkarni 3 participants 7 messages Jun 26, 2018 – Jul 2, 2018

Subsystems (2)

NVMe NVMe target

lore

Share & tools

lore

Chaitanya Kulkarni

author

2018-06-26 22:17 UTC #0 lore raw

v4 cover nvmet: implement flush all support

Hi,

This implements the flush command behavior described in "NVMe 1.3a TP 4035" for NSID value of FFFFFFFFh. Now flush command applies to all the namespaces attached to the controller processing the Flush command when the value 0xFFFFFFFF is set in the NSID field.

Regards,
Chaitanya

Changes since V3 :-
1. Restructure the nvmet_flush_all_work() and incorporate changes suggested by Sagi.
2. Execute the flush command for bdev in worker thread context.
3. Remove the nvmet_bdev_init_flush_bio() introduced in earlier patch and call nvmet_bdev_execute_flush() directly from nvmet_bdev_flush_work().

Changes since V2 :-
1. Remove the nvmet_flush_work struct and use the nvmet_req directly.
2. Small cleanups.
3. Use system workqueue instead of high priority system workqueue.
4. Flush each work as iterate.
5. Don't use worker thred context for block device flush (submit_bio()).
6. Add error message if flush fails.
7. Remove the space/tab formatting in the preparation patch.

Changes since V1 :-
1. Use system workqueues and parallelize the flush command submission.
2. Incorporate the coding style changes suggested by Sagi.
3. Add a new struct nvmet_subsys member to keep track of the active namespace belongs to a subsystem.

Chaitanya Kulkarni (2):
nvmet: move struct work_struct out of union
nvmet: add support for flush all

 drivers/nvme/target/admin-cmd.c   |  2 +-
 drivers/nvme/target/core.c        | 89 +++++++++++++++++++++++++++++++
 drivers/nvme/target/io-cmd-bdev.c | 17 +++++-
 drivers/nvme/target/io-cmd-file.c | 33 +++++++-----
 drivers/nvme/target/nvmet.h       | 12 ++++-
 5 files changed, 135 insertions(+), 18 deletions(-)

Signature · 1 line

2.17.0

Chaitanya Kulkarni

author

2018-06-26 22:17 UTC #1 lore raw

PATCH v4 1/2 nvmet: move struct work_struct out of union

This is a preparation patch which moves the struct work_struct the parameter out of the anonymous union present in the request structure so that it can be used to submit the generic work.

Reviewed-by Sagi Grimberg <sagi at grimberg.me>

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

---
 drivers/nvme/target/io-cmd-file.c | 24 ++++++++++++------------
 drivers/nvme/target/nvmet.h       |  2 +-
 2 files changed, 13 insertions(+), 13 deletions(-)

drivers/nvme/target/io-cmd-file.c +12 ~~-12~~

index 16502018fbfd..4e6e6c255a58 100644
@@ -207,20 +207,20 @@ static void nvmet_file_execute_rw(struct nvmet_req *req)
 
 static void nvmet_file_buffered_io_work(struct work_struct *w)
 {
-	struct nvmet_req *req = container_of(w, struct nvmet_req, f.work);
+	struct nvmet_req *req = container_of(w, struct nvmet_req, 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);
+	INIT_WORK(&req->work, nvmet_file_buffered_io_work);
+	queue_work(req->ns->file_wq, &req->work);
 }
 
 static void nvmet_file_flush_work(struct work_struct *w)
 {
-	struct nvmet_req *req = container_of(w, struct nvmet_req, f.work);
+	struct nvmet_req *req = container_of(w, struct nvmet_req, work);
 	int ret;
 
 	ret = vfs_fsync(req->ns->file, 1);

@@ -230,8 +230,8 @@ static void nvmet_file_flush_work(struct work_struct *w)
 
 static void nvmet_file_execute_flush(struct nvmet_req *req)
 {
-	INIT_WORK(&req->f.work, nvmet_file_flush_work);
-	schedule_work(&req->f.work);
+	INIT_WORK(&req->work, nvmet_file_flush_work);
+	schedule_work(&req->work);
 }
 
 static void nvmet_file_execute_discard(struct nvmet_req *req)

@@ -258,7 +258,7 @@ static void nvmet_file_execute_discard(struct nvmet_req *req)
 
 static void nvmet_file_dsm_work(struct work_struct *w)
 {
-	struct nvmet_req *req = container_of(w, struct nvmet_req, f.work);
+	struct nvmet_req *req = container_of(w, struct nvmet_req, work);
 
 	switch (le32_to_cpu(req->cmd->dsm.attributes)) {
 	case NVME_DSMGMT_AD:

@@ -275,13 +275,13 @@ static void nvmet_file_dsm_work(struct work_struct *w)
 
 static void nvmet_file_execute_dsm(struct nvmet_req *req)
 {
-	INIT_WORK(&req->f.work, nvmet_file_dsm_work);
-	schedule_work(&req->f.work);
+	INIT_WORK(&req->work, nvmet_file_dsm_work);
+	schedule_work(&req->work);
 }
 
 static void nvmet_file_write_zeroes_work(struct work_struct *w)
 {
-	struct nvmet_req *req = container_of(w, struct nvmet_req, f.work);
+	struct nvmet_req *req = container_of(w, struct nvmet_req, work);
 	struct nvme_write_zeroes_cmd *write_zeroes = &req->cmd->write_zeroes;
 	int mode = FALLOC_FL_ZERO_RANGE | FALLOC_FL_KEEP_SIZE;
 	loff_t offset;

@@ -298,8 +298,8 @@ static void nvmet_file_write_zeroes_work(struct work_struct *w)
 
 static void nvmet_file_execute_write_zeroes(struct nvmet_req *req)
 {
-	INIT_WORK(&req->f.work, nvmet_file_write_zeroes_work);
-	schedule_work(&req->f.work);
+	INIT_WORK(&req->work, nvmet_file_write_zeroes_work);
+	schedule_work(&req->work);
 }
 
 u16 nvmet_file_parse_io_cmd(struct nvmet_req *req)

drivers/nvme/target/nvmet.h +1 -1

index c4aa54d933f1..b1cd8eb6717f 100644
@@ -256,9 +256,9 @@ struct nvmet_req {
 			bool			mpool_alloc;
 			struct kiocb            iocb;
 			struct bio_vec          *bvec;
-			struct work_struct      work;
 		} f;
 	};
+	struct work_struct      work;
 	int			sg_cnt;
 	/* data length as parsed from the command: */
 	size_t			data_len;

Signature · 1 line

2.17.0

Chaitanya Kulkarni

author

2018-06-26 22:17 UTC #2 lore raw

PATCH v4 2/2 nvmet: add support for flush all

In reply to Chaitanya Kulkarni

This patch implements the flush command behavior described in "NVMe 1.3a TP 4035" for NSID value of FFFFFFFFh. Now flush command applies to all the namespaces attached to the controller processing the Flush command when the value 0xFFFFFFFF is set in the NSID field.

For block device and file-backed ns we execute flush in the separate worker thread context (nvmet_bdev_flush_work() and nvmet_file_flush_work() respectively).

We also add a new struct nvmet_subsys member to keep the track of number of currently active namespaces "active_ns". We update this "active_ns" member in nvmet_ns_enable()/nvmet_ns_disable().

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

---
 drivers/nvme/target/admin-cmd.c   |  2 +-
 drivers/nvme/target/core.c        | 89 +++++++++++++++++++++++++++++++
 drivers/nvme/target/io-cmd-bdev.c | 17 +++++-
 drivers/nvme/target/io-cmd-file.c |  9 +++-
 drivers/nvme/target/nvmet.h       | 10 ++++
 5 files changed, 122 insertions(+), 5 deletions(-)

drivers/nvme/target/admin-cmd.c +1 -1

index e2c6f8b39388..2ad52e5cdaed 100644
@@ -256,7 +256,7 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
 			NVME_CTRL_ONCS_WRITE_ZEROES);
 
 	/* XXX: don't report vwc if the underlying device is write through */
-	id->vwc = NVME_CTRL_VWC_PRESENT;
+	id->vwc = NVME_CTRL_VWC_PRESENT | 1 << 2 | 1 << 1;
 
 	/*
 	 * We can't support atomic writes bigger than a LBA without support

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

119 lines not on this page · raw message

drivers/nvme/target/io-cmd-bdev.c +15 -2

index e0b0f7df70c2..a08275c70f73 100644
@@ -46,10 +46,16 @@ void nvmet_bdev_ns_disable(struct nvmet_ns *ns)
 
 static void nvmet_bio_done(struct bio *bio)
 {
+	u16 status = NVME_SC_INTERNAL | NVME_SC_DNR;
 	struct nvmet_req *req = bio->bi_private;
 
-	nvmet_req_complete(req,
-		bio->bi_status ? NVME_SC_INTERNAL | NVME_SC_DNR : 0);
+	if (unlikely(nvmet_cmd_flush_all(req))) {
+		if (req->b.inline_bio.bi_status)
+			req->rsp->status = cpu_to_le16(status);
+		return;
+	}
+
+	nvmet_req_complete(req, bio->bi_status ? status : 0);
 
 	if (bio != &req->b.inline_bio)
 		bio_put(bio);

@@ -124,6 +130,13 @@ static void nvmet_bdev_execute_flush(struct nvmet_req *req)
 	submit_bio(bio);
 }
 
+void nvmet_bdev_flush_work(struct work_struct *w)
+{
+	struct nvmet_req *req = container_of(w, struct nvmet_req, work);
+
+	nvmet_bdev_execute_flush(req);
+}
+
 static u16 nvmet_bdev_discard_range(struct nvmet_ns *ns,
 		struct nvme_dsm_range *range, struct bio **bio)
 {

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

index 4e6e6c255a58..d1c79a3c4f9b 100644
@@ -218,14 +218,19 @@ static void nvmet_file_execute_rw_buffered_io(struct nvmet_req *req)
 	queue_work(req->ns->file_wq, &req->work);
 }
 
-static void nvmet_file_flush_work(struct work_struct *w)
+void nvmet_file_flush_work(struct work_struct *w)
 {
 	struct nvmet_req *req = container_of(w, struct nvmet_req, work);
+	u16 status;
 	int ret;
 
 	ret = vfs_fsync(req->ns->file, 1);
+	status = ret < 0 ? NVME_SC_INTERNAL | NVME_SC_DNR : NVME_SC_SUCCESS;
 
-	nvmet_req_complete(req, ret < 0 ? NVME_SC_INTERNAL | NVME_SC_DNR : 0);
+	if (nvmet_cmd_flush_all(req))
+		req->rsp->status = cpu_to_le16(status);
+	else
+		nvmet_req_complete(req, status);
 }
 
 static void nvmet_file_execute_flush(struct nvmet_req *req)

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

index b1cd8eb6717f..48928b1acded 100644
@@ -169,6 +169,7 @@ struct nvmet_subsys {
 
 	struct list_head	namespaces;
 	unsigned int		max_nsid;
+	unsigned int		active_ns;
 
 	struct list_head	ctrls;

@@ -380,9 +381,18 @@ int nvmet_file_ns_enable(struct nvmet_ns *ns);
 void nvmet_bdev_ns_disable(struct nvmet_ns *ns);
 void nvmet_file_ns_disable(struct nvmet_ns *ns);
 
+void nvmet_bdev_flush_work(struct work_struct *w);
+void nvmet_file_flush_work(struct work_struct *w);
+
 static inline u32 nvmet_rw_len(struct nvmet_req *req)
 {
 	return ((u32)le16_to_cpu(req->cmd->rw.length) + 1) <<
 			req->ns->blksize_shift;
 }
+
+static inline int nvmet_cmd_flush_all(struct nvmet_req *req)
+{
+	return (req->cmd->common.opcode == nvme_cmd_flush) &&
+		(le32_to_cpu(req->cmd->rw.nsid) == 0xFFFFFFFF);
+}
 #endif /* _NVMET_H */

Signature · 1 line

2.17.0

Sagi Grimberg

maintainer

2018-06-27 08:04 UTC #3 lore raw

PATCH v4 2/2 nvmet: add support for flush all

Looks much better,

Reviewed-by Sagi Grimberg <sagi at grimberg.me>

Christoph Hellwig

maintainer

2018-06-28 14:42 UTC #4 lore raw

v4 cover nvmet: implement flush all support

In reply to Chaitanya Kulkarni

Quoted 6 lines · Chaitanya Kulkarni

Hi,
This implements the flush command behavior described in "NVMe 1.3a TP 4035" for NSID value of FFFFFFFFh. Now flush command applies to all the namespaces attached to the controller processing the Flush command when the value 0xFFFFFFFF is set in the NSID field.

Why? No other I/O command supports 0xFFFFFFFF, and sending an all namespaces flush is entirely unnatural. Unless we have a very strong use case I don't want to carry this code around.

Chaitanya Kulkarni

author

2018-07-02 05:43 UTC #5 lore raw

v4 cover nvmet: implement flush all support

Hi Christoph,

It allows NVMeOF host to just send one command and complete the flush per-ctrl basis as opposed to per-ns basis.
In the current implementation, the host needs to execute nvme-list-ns or a variant of the nvme-cli command per-ctrl and iterate through the ns-list, whereas we can make user's life easier by providing the option present in the NVMe spec.

This is especially useful when you have many NVMeOF controllers configured on the host with multiple namespaces.

Also given that NVMeOF ctrl can have different types of namespaces
(bdev, file, and file-buffered-io and each type can have different behavior for flush) this feature will not require host to keep track of each ns type in order to execute flush depending on the ns type.

I didn't understand the "unnatural" comment, are you implying that this feature is not that desirable for NVMeOF ctrls as compare to other types of ctrls?

If you still think this is not a target kernel code's responsibility and should be done in the management tool like nvme-cli on the host side we can drop this patch.

From: Christoph Hellwig <[email protected]>
Sent: Thursday, June 28, 2018 7:42 AM
To: Chaitanya Kulkarni
Cc: linux-nvme at lists.infradead.org; hch at lst.de; keith.busch at intel.com; sagi at grimberg.me
Subject: Re: [PATCH V4 0/2] nvmet: implement flush all support
?

Quoted 6 lines · Chaitanya Kulkarni

Hi,
This implements the flush command behavior described in "NVMe 1.3a? TP 4035" for NSID value of FFFFFFFFh. Now flush command applies to all the namespaces attached to the controller processing the Flush command when the value 0xFFFFFFFF is set in the NSID field.

Why?? No other I/O command supports 0xFFFFFFFF, and sending an all namespaces flush is entirely unnatural.? Unless we have a very strong use case I don't want to carry this code around.

Christoph Hellwig

maintainer

2018-07-02 12:16 UTC #6 lore raw

v4 cover nvmet: implement flush all support

Quoted 4 lines · Chaitanya Kulkarni

Hi Christoph,
It allows NVMeOF host to just send one command and complete the flush per-ctrl basis as opposed to per-ns basis.

Yes, but why would you ever do that? Even when we discussed the issue in the working group no one could actually come up with a practical use case.