[PATCH 0/4] nvmet: fix file & block device backed ns

[PATCH 0/4] nvmet: fix file & block device backed ns

[Ming Lei]

Hi,

The 1st patch sets loop queue's segment boundary mask as PAGE_SIZE - 1
because nvme target only accepts single-page sg list.

The 2nd & 3rd patches fix computation io vector.

The last patch builds bvec from sg directly.

Ming Lei (4):
  nvmet: set loop queue's segment boundary mask as PAGE_SIZE - 1
  nvmet: fix computation of io vector number
  nvmet: fix computation of bvec->bv_len
  nvmet: build bvec from sg directly

 drivers/nvme/target/io-cmd-file.c | 20 ++++++++++----------
 drivers/nvme/target/loop.c        |  3 +++
 2 files changed, 13 insertions(+), 10 deletions(-)

Cc: Yi Zhang <yi.zhang at redhat.com>
Cc: Sagi Grimberg <sagi at grimberg.me>
Cc: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>

-- 
2.9.5

[PATCH 1/4] nvmet: set loop queue's segment boundary mask as PAGE_SIZE - 1

[Ming Lei]

NVMe target only accepts single-page sg list, either file or block
device backed target code follows this assumption.

However, loop target is one exception, given the sg list is from
the host queue directly.

This patch sets loop queue's segment boundary mask as PAGE_SIZE - 1
for folowoing NVMe target assumption.

Reported-by: Yi Zhang <yi.zhang at redhat.com>
Fixes: 3a85a5de29ea ("nvme-loop: add a NVMe loopback host driver")
Cc: Yi Zhang <yi.zhang at redhat.com>
Cc: Sagi Grimberg <sagi at grimberg.me>
Cc: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>
Cc: <stable at vger.kernel.org>
Signed-off-by: Ming Lei <ming.lei at redhat.com>
---
 drivers/nvme/target/loop.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/nvme/target/loop.c b/drivers/nvme/target/loop.c
index b9f623ab01f3..7194f86b9dac 100644
--- a/drivers/nvme/target/loop.c
+++ b/drivers/nvme/target/loop.c
@@ -549,6 +549,9 @@ static int nvme_loop_create_io_queues(struct nvme_loop_ctrl *ctrl)
 	if (ret)
 		goto out_cleanup_connect_q;
 
+	/* target only accepts single-page sg list */
+	blk_queue_segment_boundary(ctrl->ctrl.connect_q, PAGE_SIZE - 1);
+
 	return 0;
 
 out_cleanup_connect_q:
-- 
2.9.5

[PATCH 1/4] nvmet: set loop queue's segment boundary mask as PAGE_SIZE - 1

[Christoph Hellwig]

Note that in many cases allocating larger pages in the other targets
would be useful.  Given that you fix the one places where we made a page
size assumption I don't see a great need to limit outselves here.

[PATCH 1/4] nvmet: set loop queue's segment boundary mask as PAGE_SIZE - 1

[Ming Lei]

On Tue, Mar 26, 2019@3:36 PM Christoph Hellwig <hch@lst.de> wrote:
>
> Note that in many cases allocating larger pages in the other targets
> would be useful.  Given that you fix the one places where we made a page
> size assumption I don't see a great need to limit outselves here.

block device backed ns still needs this patch.


thanks,
Ming Lei

[PATCH 1/4] nvmet: set loop queue's segment boundary mask as PAGE_SIZE - 1

[Ming Lei]

On Tue, Mar 26, 2019@3:37 PM Ming Lei <tom.leiming@gmail.com> wrote:
>
> On Tue, Mar 26, 2019@3:36 PM Christoph Hellwig <hch@lst.de> wrote:
> >
> > Note that in many cases allocating larger pages in the other targets
> > would be useful.  Given that you fix the one places where we made a page
> > size assumption I don't see a great need to limit outselves here.
>
> block device backed ns still needs this patch.

Also both the 2nd & 3rd fixes depend on this patch too.


Thanks,
Ming Lei

[PATCH 1/4] nvmet: set loop queue's segment boundary mask as PAGE_SIZE - 1

[Christoph Hellwig]

On Tue, Mar 26, 2019@03:37:36PM +0800, Ming Lei wrote:
> On Tue, Mar 26, 2019@3:36 PM Christoph Hellwig <hch@lst.de> wrote:
> >
> > Note that in many cases allocating larger pages in the other targets
> > would be useful.  Given that you fix the one places where we made a page
> > size assumption I don't see a great need to limit outselves here.
> 
> block device backed ns still needs this patch.

Then we need to fix that code as well.  Or even better make bio_add_page
handle the larger than page case fine if it really doesn't do that yet.

[PATCH 1/4] nvmet: set loop queue's segment boundary mask as PAGE_SIZE - 1

[Ming Lei]

On Wed, Mar 27, 2019@4:15 PM Christoph Hellwig <hch@lst.de> wrote:
>
> On Tue, Mar 26, 2019@03:37:36PM +0800, Ming Lei wrote:
> > On Tue, Mar 26, 2019@3:36 PM Christoph Hellwig <hch@lst.de> wrote:
> > >
> > > Note that in many cases allocating larger pages in the other targets
> > > would be useful.  Given that you fix the one places where we made a page
> > > size assumption I don't see a great need to limit outselves here.
> >
> > block device backed ns still needs this patch.
>
> Then we need to fix that code as well.  Or even better make bio_add_page
> handle the larger than page case fine if it really doesn't do that yet.

Given loop is the only exception, this patch is exactly the fix, right?

Thanks,
Ming Lei

[PATCH 1/4] nvmet: set loop queue's segment boundary mask as PAGE_SIZE - 1

[Christoph Hellwig]

On Wed, Mar 27, 2019@04:26:26PM +0800, Ming Lei wrote:
> On Wed, Mar 27, 2019@4:15 PM Christoph Hellwig <hch@lst.de> wrote:
> >
> > On Tue, Mar 26, 2019@03:37:36PM +0800, Ming Lei wrote:
> > > On Tue, Mar 26, 2019@3:36 PM Christoph Hellwig <hch@lst.de> wrote:
> > > >
> > > > Note that in many cases allocating larger pages in the other targets
> > > > would be useful.  Given that you fix the one places where we made a page
> > > > size assumption I don't see a great need to limit outselves here.
> > >
> > > block device backed ns still needs this patch.
> >
> > Then we need to fix that code as well.  Or even better make bio_add_page
> > handle the larger than page case fine if it really doesn't do that yet.
> 
> Given loop is the only exception, this patch is exactly the fix, right?

Well, we want to be able to pass multi-page biovecs everywhere, so I think
we'd just solve this based on loop so that we can later take advantage of
it everywhere.

[PATCH 2/4] nvmet: fix computation of io vector number

[Ming Lei]

Current file backed ns code uses request data length to compute
number of io vector.

This way is actually wrong, the warning in nvmet_file_execute_io()
may be triggered because the sg->offset isn't considered.

Given the sg list is only single-page for nvme target, simply use
the req->sg_cnt as number of io vector.

Reported-by: Yi Zhang <yi.zhang at redhat.com>
Fixes: d5eff33ee6f8("nvmet: add simple file backed ns support")
Cc: Yi Zhang <yi.zhang at redhat.com>
Cc: Sagi Grimberg <sagi at grimberg.me>
Cc: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>
Cc: <stable at vger.kernel.org>
Signed-off-by: Ming Lei <ming.lei at redhat.com>
---
 drivers/nvme/target/io-cmd-file.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/target/io-cmd-file.c b/drivers/nvme/target/io-cmd-file.c
index 3e43212d3c1c..d67a43832cb1 100644
--- a/drivers/nvme/target/io-cmd-file.c
+++ b/drivers/nvme/target/io-cmd-file.c
@@ -128,7 +128,7 @@ static void nvmet_file_io_done(struct kiocb *iocb, long ret, long ret2)
 
 static bool nvmet_file_execute_io(struct nvmet_req *req, int ki_flags)
 {
-	ssize_t nr_bvec = DIV_ROUND_UP(req->data_len, PAGE_SIZE);
+	ssize_t nr_bvec = req->sg_cnt;
 	struct sg_page_iter sg_pg_iter;
 	unsigned long bv_cnt = 0;
 	bool is_sync = false;
@@ -225,7 +225,7 @@ static void nvmet_file_submit_buffered_io(struct nvmet_req *req)
 
 static void nvmet_file_execute_rw(struct nvmet_req *req)
 {
-	ssize_t nr_bvec = DIV_ROUND_UP(req->data_len, PAGE_SIZE);
+	ssize_t nr_bvec = req->sg_cnt;
 
 	if (!req->sg_cnt || !nr_bvec) {
 		nvmet_req_complete(req, 0);
-- 
2.9.5

[PATCH 2/4] nvmet: fix computation of io vector number

[Christoph Hellwig]

I'd rather see this merged with the last one (and patch 3 dropped as
it just gets removed entirely in 4) - that fixes all the issues together
and doesn't require your patch 1 as as a workaround first.

[PATCH 2/4] nvmet: fix computation of io vector number

[Ming Lei]

On Tue, Mar 26, 2019@3:38 PM Christoph Hellwig <hch@lst.de> wrote:
>
> I'd rather see this merged with the last one (and patch 3 dropped as
> it just gets removed entirely in 4) - that fixes all the issues together
> and doesn't require your patch 1 as as a workaround first.

Patch 1 isn't a workaround, which is needed for stable tree, and
the other fixes won't work without patch 1.

However, we may revert patch 1 for 5.1 in which multi-page bvec
is supported.

I am fine to merge the last three in one patch since the backporting
isn't too hard for me, :-) However I guess our stable guys may prefer to
simpler fix.

Thanks,
Ming Lei

[PATCH 2/4] nvmet: fix computation of io vector number

[Christoph Hellwig]

On Tue, Mar 26, 2019@03:50:01PM +0800, Ming Lei wrote:
> On Tue, Mar 26, 2019@3:38 PM Christoph Hellwig <hch@lst.de> wrote:
> >
> > I'd rather see this merged with the last one (and patch 3 dropped as
> > it just gets removed entirely in 4) - that fixes all the issues together
> > and doesn't require your patch 1 as as a workaround first.
> 
> Patch 1 isn't a workaround, which is needed for stable tree, and
> the other fixes won't work without patch 1.
> 
> However, we may revert patch 1 for 5.1 in which multi-page bvec
> is supported.

Lets do it another way around - put the proper fix into 5.1-rc and
only apply patch 1 for stable.  No need to put something in and
immediately revert it.

[PATCH 3/4] nvmet: fix computation of bvec->bv_len

[Ming Lei]

PAGE_SIZE - sg->offset may be bigger than sg->length, so we have
to cap it.

Reported-by: Yi Zhang <yi.zhang at redhat.com>
Fixes: d5eff33ee6f8("nvmet: add simple file backed ns support")
Cc: Yi Zhang <yi.zhang at redhat.com>
Cc: Sagi Grimberg <sagi at grimberg.me>
Cc: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>
Cc: <stable at vger.kernel.org>
Signed-off-by: Ming Lei <ming.lei at redhat.com>
---
 drivers/nvme/target/io-cmd-file.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/target/io-cmd-file.c b/drivers/nvme/target/io-cmd-file.c
index d67a43832cb1..149e17e699e5 100644
--- a/drivers/nvme/target/io-cmd-file.c
+++ b/drivers/nvme/target/io-cmd-file.c
@@ -79,7 +79,8 @@ static void nvmet_file_init_bvec(struct bio_vec *bv, struct sg_page_iter *iter)
 {
 	bv->bv_page = sg_page_iter_page(iter);
 	bv->bv_offset = iter->sg->offset;
-	bv->bv_len = PAGE_SIZE - iter->sg->offset;
+	bv->bv_len = min_t(unsigned, PAGE_SIZE - iter->sg->offset,
+			   iter->sg->length);
 }
 
 static ssize_t nvmet_file_submit_bvec(struct nvmet_req *req, loff_t pos,
-- 
2.9.5

[PATCH 4/4] nvmet: build bvec from sg directly

[Ming Lei]

NVMe target only accepts single-page sg list, so build bvec
from sg directly.

Cc: Yi Zhang <yi.zhang at redhat.com>
Cc: Sagi Grimberg <sagi at grimberg.me>
Cc: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>
Signed-off-by: Ming Lei <ming.lei at redhat.com>
---
 drivers/nvme/target/io-cmd-file.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/nvme/target/io-cmd-file.c b/drivers/nvme/target/io-cmd-file.c
index 149e17e699e5..bc6ebb51b0bf 100644
--- a/drivers/nvme/target/io-cmd-file.c
+++ b/drivers/nvme/target/io-cmd-file.c
@@ -75,12 +75,11 @@ int nvmet_file_ns_enable(struct nvmet_ns *ns)
 	return ret;
 }
 
-static void nvmet_file_init_bvec(struct bio_vec *bv, struct sg_page_iter *iter)
+static void nvmet_file_init_bvec(struct bio_vec *bv, struct scatterlist *sg)
 {
-	bv->bv_page = sg_page_iter_page(iter);
-	bv->bv_offset = iter->sg->offset;
-	bv->bv_len = min_t(unsigned, PAGE_SIZE - iter->sg->offset,
-			   iter->sg->length);
+	bv->bv_page = sg_page(sg);
+	bv->bv_offset = sg->offset;
+	bv->bv_len = sg->length;
 }
 
 static ssize_t nvmet_file_submit_bvec(struct nvmet_req *req, loff_t pos,
@@ -130,13 +129,13 @@ static void nvmet_file_io_done(struct kiocb *iocb, long ret, long ret2)
 static bool nvmet_file_execute_io(struct nvmet_req *req, int ki_flags)
 {
 	ssize_t nr_bvec = req->sg_cnt;
-	struct sg_page_iter sg_pg_iter;
 	unsigned long bv_cnt = 0;
 	bool is_sync = false;
 	size_t len = 0, total_len = 0;
 	ssize_t ret = 0;
 	loff_t pos;
-
+	int i;
+	struct scatterlist *sg;
 
 	if (req->f.mpool_alloc && nr_bvec > NVMET_MAX_MPOOL_BVEC)
 		is_sync = true;
@@ -148,8 +147,8 @@ static bool nvmet_file_execute_io(struct nvmet_req *req, int ki_flags)
 	}
 
 	memset(&req->f.iocb, 0, sizeof(struct kiocb));
-	for_each_sg_page(req->sg, &sg_pg_iter, req->sg_cnt, 0) {
-		nvmet_file_init_bvec(&req->f.bvec[bv_cnt], &sg_pg_iter);
+	for_each_sg(req->sg, sg, req->sg_cnt, i) {
+		nvmet_file_init_bvec(&req->f.bvec[bv_cnt], sg);
 		len += req->f.bvec[bv_cnt].bv_len;
 		total_len += req->f.bvec[bv_cnt].bv_len;
 		bv_cnt++;
-- 
2.9.5