* [PATCH] nvmet: convert from kmap to nvmet_copy_from_sgl
@ 2017-04-18 23:32 Logan Gunthorpe
2017-04-19 9:33 ` Max Gurtovoy
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Logan Gunthorpe @ 2017-04-18 23:32 UTC (permalink / raw)
To: linux-nvme, linux-kernel
Cc: Logan Gunthorpe, Christoph Hellwig, Sagi Grimberg
This is safer as it doesn't rely on the data being stored in
a single page in an sgl.
It also aids our effort to start phasing out users of sg_page. See [1].
For this we kmalloc some memory, copy to it and free at the end. Note:
we can't allocate this memory on the stack as the kbuild test robot
reports some frame size overflows on i386.
[1] https://lwn.net/Articles/720053/
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Sagi Grimberg <sagi@grimberg.me>
---
drivers/nvme/target/fabrics-cmd.c | 32 +++++++++++++++++++++++++-------
1 file changed, 25 insertions(+), 7 deletions(-)
diff --git a/drivers/nvme/target/fabrics-cmd.c b/drivers/nvme/target/fabrics-cmd.c
index 8bd022af..2e0ab10 100644
--- a/drivers/nvme/target/fabrics-cmd.c
+++ b/drivers/nvme/target/fabrics-cmd.c
@@ -122,7 +122,15 @@ static void nvmet_execute_admin_connect(struct nvmet_req *req)
struct nvmet_ctrl *ctrl = NULL;
u16 status = 0;
- d = kmap(sg_page(req->sg)) + req->sg->offset;
+ d = kmalloc(sizeof(*d), GFP_KERNEL);
+ if (!d) {
+ status = NVME_SC_INTERNAL;
+ goto complete;
+ }
+
+ status = nvmet_copy_from_sgl(req, 0, d, sizeof(*d));
+ if (status)
+ goto out;
/* zero out initial completion result, assign values as needed */
req->rsp->result.u32 = 0;
@@ -143,7 +151,7 @@ static void nvmet_execute_admin_connect(struct nvmet_req *req)
}
status = nvmet_alloc_ctrl(d->subsysnqn, d->hostnqn, req,
- le32_to_cpu(c->kato), &ctrl);
+ le32_to_cpu(c->kato), &ctrl);
if (status)
goto out;
@@ -158,7 +166,8 @@ static void nvmet_execute_admin_connect(struct nvmet_req *req)
req->rsp->result.u16 = cpu_to_le16(ctrl->cntlid);
out:
- kunmap(sg_page(req->sg));
+ kfree(d);
+complete:
nvmet_req_complete(req, status);
}
@@ -170,7 +179,15 @@ static void nvmet_execute_io_connect(struct nvmet_req *req)
u16 qid = le16_to_cpu(c->qid);
u16 status = 0;
- d = kmap(sg_page(req->sg)) + req->sg->offset;
+ d = kmalloc(sizeof(*d), GFP_KERNEL);
+ if (!d) {
+ status = NVME_SC_INTERNAL;
+ goto complete;
+ }
+
+ status = nvmet_copy_from_sgl(req, 0, d, sizeof(*d));
+ if (status)
+ goto out;
/* zero out initial completion result, assign values as needed */
req->rsp->result.u32 = 0;
@@ -183,8 +200,8 @@ static void nvmet_execute_io_connect(struct nvmet_req *req)
}
status = nvmet_ctrl_find_get(d->subsysnqn, d->hostnqn,
- le16_to_cpu(d->cntlid),
- req, &ctrl);
+ le16_to_cpu(d->cntlid),
+ req, &ctrl);
if (status)
goto out;
@@ -205,7 +222,8 @@ static void nvmet_execute_io_connect(struct nvmet_req *req)
pr_info("adding queue %d to ctrl %d.\n", qid, ctrl->cntlid);
out:
- kunmap(sg_page(req->sg));
+ kfree(d);
+complete:
nvmet_req_complete(req, status);
return;
--
2.1.4
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] nvmet: convert from kmap to nvmet_copy_from_sgl
2017-04-18 23:32 [PATCH] nvmet: convert from kmap to nvmet_copy_from_sgl Logan Gunthorpe
@ 2017-04-19 9:33 ` Max Gurtovoy
2017-04-19 9:38 ` Johannes Thumshirn
2017-04-19 19:32 ` Christoph Hellwig
2017-04-20 11:31 ` Sagi Grimberg
2 siblings, 1 reply; 7+ messages in thread
From: Max Gurtovoy @ 2017-04-19 9:33 UTC (permalink / raw)
To: Logan Gunthorpe, linux-nvme, linux-kernel
Cc: Christoph Hellwig, Sagi Grimberg
Hi Logan,
On 4/19/2017 2:32 AM, Logan Gunthorpe wrote:
> This is safer as it doesn't rely on the data being stored in
> a single page in an sgl.
>
> It also aids our effort to start phasing out users of sg_page. See [1].
>
> For this we kmalloc some memory, copy to it and free at the end. Note:
> we can't allocate this memory on the stack as the kbuild test robot
> reports some frame size overflows on i386.
>
> [1] https://lwn.net/Articles/720053/
>
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Sagi Grimberg <sagi@grimberg.me>
> ---
> drivers/nvme/target/fabrics-cmd.c | 32 +++++++++++++++++++++++++-------
> 1 file changed, 25 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/nvme/target/fabrics-cmd.c b/drivers/nvme/target/fabrics-cmd.c
> index 8bd022af..2e0ab10 100644
> --- a/drivers/nvme/target/fabrics-cmd.c
> +++ b/drivers/nvme/target/fabrics-cmd.c
> @@ -122,7 +122,15 @@ static void nvmet_execute_admin_connect(struct nvmet_req *req)
> struct nvmet_ctrl *ctrl = NULL;
> u16 status = 0;
>
> - d = kmap(sg_page(req->sg)) + req->sg->offset;
> + d = kmalloc(sizeof(*d), GFP_KERNEL);
I'd prefer removing the dynamic allocation and use d on the stack to
simplify the code.
Any thoughts ?
> + if (!d) {
> + status = NVME_SC_INTERNAL;
> + goto complete;
> + }
> +
> + status = nvmet_copy_from_sgl(req, 0, d, sizeof(*d));
> + if (status)
> + goto out;
>
> /* zero out initial completion result, assign values as needed */
> req->rsp->result.u32 = 0;
> @@ -143,7 +151,7 @@ static void nvmet_execute_admin_connect(struct nvmet_req *req)
> }
>
> status = nvmet_alloc_ctrl(d->subsysnqn, d->hostnqn, req,
> - le32_to_cpu(c->kato), &ctrl);
> + le32_to_cpu(c->kato), &ctrl);
> if (status)
> goto out;
>
> @@ -158,7 +166,8 @@ static void nvmet_execute_admin_connect(struct nvmet_req *req)
> req->rsp->result.u16 = cpu_to_le16(ctrl->cntlid);
>
> out:
> - kunmap(sg_page(req->sg));
> + kfree(d);
> +complete:
> nvmet_req_complete(req, status);
> }
>
> @@ -170,7 +179,15 @@ static void nvmet_execute_io_connect(struct nvmet_req *req)
> u16 qid = le16_to_cpu(c->qid);
> u16 status = 0;
>
> - d = kmap(sg_page(req->sg)) + req->sg->offset;
> + d = kmalloc(sizeof(*d), GFP_KERNEL);
here too.
> + if (!d) {
> + status = NVME_SC_INTERNAL;
> + goto complete;
> + }
> +
> + status = nvmet_copy_from_sgl(req, 0, d, sizeof(*d));
> + if (status)
> + goto out;
>
> /* zero out initial completion result, assign values as needed */
> req->rsp->result.u32 = 0;
> @@ -183,8 +200,8 @@ static void nvmet_execute_io_connect(struct nvmet_req *req)
> }
>
> status = nvmet_ctrl_find_get(d->subsysnqn, d->hostnqn,
> - le16_to_cpu(d->cntlid),
> - req, &ctrl);
> + le16_to_cpu(d->cntlid),
> + req, &ctrl);
> if (status)
> goto out;
>
> @@ -205,7 +222,8 @@ static void nvmet_execute_io_connect(struct nvmet_req *req)
> pr_info("adding queue %d to ctrl %d.\n", qid, ctrl->cntlid);
>
> out:
> - kunmap(sg_page(req->sg));
> + kfree(d);
> +complete:
> nvmet_req_complete(req, status);
> return;
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] nvmet: convert from kmap to nvmet_copy_from_sgl
2017-04-19 9:33 ` Max Gurtovoy
@ 2017-04-19 9:38 ` Johannes Thumshirn
2017-04-19 9:44 ` Max Gurtovoy
0 siblings, 1 reply; 7+ messages in thread
From: Johannes Thumshirn @ 2017-04-19 9:38 UTC (permalink / raw)
To: Max Gurtovoy
Cc: Logan Gunthorpe, linux-nvme, linux-kernel, Christoph Hellwig,
Sagi Grimberg
On Wed, Apr 19, 2017 at 12:33:09PM +0300, Max Gurtovoy wrote:
> Hi Logan,
>
> On 4/19/2017 2:32 AM, Logan Gunthorpe wrote:
> >This is safer as it doesn't rely on the data being stored in
> >a single page in an sgl.
> >
> >It also aids our effort to start phasing out users of sg_page. See [1].
> >
> >For this we kmalloc some memory, copy to it and free at the end. Note:
> >we can't allocate this memory on the stack as the kbuild test robot
> >reports some frame size overflows on i386.
> >
> >[1] https://lwn.net/Articles/720053/
> >
> >Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
> >Cc: Christoph Hellwig <hch@lst.de>
> >Cc: Sagi Grimberg <sagi@grimberg.me>
> >---
> > drivers/nvme/target/fabrics-cmd.c | 32 +++++++++++++++++++++++++-------
> > 1 file changed, 25 insertions(+), 7 deletions(-)
> >
> >diff --git a/drivers/nvme/target/fabrics-cmd.c b/drivers/nvme/target/fabrics-cmd.c
> >index 8bd022af..2e0ab10 100644
> >--- a/drivers/nvme/target/fabrics-cmd.c
> >+++ b/drivers/nvme/target/fabrics-cmd.c
> >@@ -122,7 +122,15 @@ static void nvmet_execute_admin_connect(struct nvmet_req *req)
> > struct nvmet_ctrl *ctrl = NULL;
> > u16 status = 0;
> >
> >- d = kmap(sg_page(req->sg)) + req->sg->offset;
> >+ d = kmalloc(sizeof(*d), GFP_KERNEL);
>
> I'd prefer removing the dynamic allocation and use d on the stack to
> simplify the code.
> Any thoughts ?
Hi Max,
Pasting from above:
> >we can't allocate this memory on the stack as the kbuild test robot
> >reports some frame size overflows on i386.
--
Johannes Thumshirn Storage
jthumshirn@suse.de +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] nvmet: convert from kmap to nvmet_copy_from_sgl
2017-04-19 9:38 ` Johannes Thumshirn
@ 2017-04-19 9:44 ` Max Gurtovoy
2017-04-19 15:40 ` Logan Gunthorpe
0 siblings, 1 reply; 7+ messages in thread
From: Max Gurtovoy @ 2017-04-19 9:44 UTC (permalink / raw)
To: Johannes Thumshirn
Cc: Logan Gunthorpe, linux-nvme, linux-kernel, Christoph Hellwig,
Sagi Grimberg
On 4/19/2017 12:38 PM, Johannes Thumshirn wrote:
> On Wed, Apr 19, 2017 at 12:33:09PM +0300, Max Gurtovoy wrote:
>> Hi Logan,
>>
>> On 4/19/2017 2:32 AM, Logan Gunthorpe wrote:
>>> This is safer as it doesn't rely on the data being stored in
>>> a single page in an sgl.
>>>
>>> It also aids our effort to start phasing out users of sg_page. See [1].
>>>
>>> For this we kmalloc some memory, copy to it and free at the end. Note:
>>> we can't allocate this memory on the stack as the kbuild test robot
>>> reports some frame size overflows on i386.
>>>
>>> [1] https://lwn.net/Articles/720053/
>>>
>>> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
>>> Cc: Christoph Hellwig <hch@lst.de>
>>> Cc: Sagi Grimberg <sagi@grimberg.me>
>>> ---
>>> drivers/nvme/target/fabrics-cmd.c | 32 +++++++++++++++++++++++++-------
>>> 1 file changed, 25 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/nvme/target/fabrics-cmd.c b/drivers/nvme/target/fabrics-cmd.c
>>> index 8bd022af..2e0ab10 100644
>>> --- a/drivers/nvme/target/fabrics-cmd.c
>>> +++ b/drivers/nvme/target/fabrics-cmd.c
>>> @@ -122,7 +122,15 @@ static void nvmet_execute_admin_connect(struct nvmet_req *req)
>>> struct nvmet_ctrl *ctrl = NULL;
>>> u16 status = 0;
>>>
>>> - d = kmap(sg_page(req->sg)) + req->sg->offset;
>>> + d = kmalloc(sizeof(*d), GFP_KERNEL);
>>
>> I'd prefer removing the dynamic allocation and use d on the stack to
>> simplify the code.
>> Any thoughts ?
>
> Hi Max,
>
> Pasting from above:
>
>>> we can't allocate this memory on the stack as the kbuild test robot
>>> reports some frame size overflows on i386.
Thanks Johannes, I missed that comment.
Looks good,
Reviewed-by: Max Gurtovoy <maxg@mellanox.com>
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] nvmet: convert from kmap to nvmet_copy_from_sgl
2017-04-18 23:32 [PATCH] nvmet: convert from kmap to nvmet_copy_from_sgl Logan Gunthorpe
2017-04-19 9:33 ` Max Gurtovoy
@ 2017-04-19 19:32 ` Christoph Hellwig
2017-04-20 11:31 ` Sagi Grimberg
2 siblings, 0 replies; 7+ messages in thread
From: Christoph Hellwig @ 2017-04-19 19:32 UTC (permalink / raw)
To: Logan Gunthorpe
Cc: linux-nvme, linux-kernel, Christoph Hellwig, Sagi Grimberg
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] nvmet: convert from kmap to nvmet_copy_from_sgl
2017-04-18 23:32 [PATCH] nvmet: convert from kmap to nvmet_copy_from_sgl Logan Gunthorpe
2017-04-19 9:33 ` Max Gurtovoy
2017-04-19 19:32 ` Christoph Hellwig
@ 2017-04-20 11:31 ` Sagi Grimberg
2 siblings, 0 replies; 7+ messages in thread
From: Sagi Grimberg @ 2017-04-20 11:31 UTC (permalink / raw)
To: Logan Gunthorpe, linux-nvme, linux-kernel; +Cc: Christoph Hellwig
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-04-20 11:31 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-18 23:32 [PATCH] nvmet: convert from kmap to nvmet_copy_from_sgl Logan Gunthorpe
2017-04-19 9:33 ` Max Gurtovoy
2017-04-19 9:38 ` Johannes Thumshirn
2017-04-19 9:44 ` Max Gurtovoy
2017-04-19 15:40 ` Logan Gunthorpe
2017-04-19 19:32 ` Christoph Hellwig
2017-04-20 11:31 ` Sagi Grimberg
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).