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

[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@redhat.com>
Fixes: 3a85a5de29ea ("nvme-loop: add a NVMe loopback host driver")
Cc: Yi Zhang <yi.zhang@redhat.com>
Cc: Sagi Grimberg <sagi@grimberg.me>
Cc: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: Ming Lei <ming.lei@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

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

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

[Ming Lei]

On Tue, Mar 26, 2019 at 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

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

[Ming Lei]

On Tue, Mar 26, 2019 at 3:37 PM Ming Lei <tom.leiming@gmail.com> wrote:
>
> On Tue, Mar 26, 2019 at 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

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

[Christoph Hellwig]

On Tue, Mar 26, 2019 at 03:37:36PM +0800, Ming Lei wrote:
> On Tue, Mar 26, 2019 at 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.

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

[Ming Lei]

On Wed, Mar 27, 2019 at 4:15 PM Christoph Hellwig <hch@lst.de> wrote:
>
> On Tue, Mar 26, 2019 at 03:37:36PM +0800, Ming Lei wrote:
> > On Tue, Mar 26, 2019 at 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

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

[Christoph Hellwig]

On Wed, Mar 27, 2019 at 04:26:26PM +0800, Ming Lei wrote:
> On Wed, Mar 27, 2019 at 4:15 PM Christoph Hellwig <hch@lst.de> wrote:
> >
> > On Tue, Mar 26, 2019 at 03:37:36PM +0800, Ming Lei wrote:
> > > On Tue, Mar 26, 2019 at 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@redhat.com>
Fixes: d5eff33ee6f8("nvmet: add simple file backed ns support")
Cc: Yi Zhang <yi.zhang@redhat.com>
Cc: Sagi Grimberg <sagi@grimberg.me>
Cc: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: Ming Lei <ming.lei@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

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

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

[Ming Lei]

On Tue, Mar 26, 2019 at 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

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

[Christoph Hellwig]

On Tue, Mar 26, 2019 at 03:50:01PM +0800, Ming Lei wrote:
> On Tue, Mar 26, 2019 at 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@redhat.com>
Fixes: d5eff33ee6f8("nvmet: add simple file backed ns support")
Cc: Yi Zhang <yi.zhang@redhat.com>
Cc: Sagi Grimberg <sagi@grimberg.me>
Cc: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: Ming Lei <ming.lei@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