Re: [PATCH] nvmet: convert from kmap to nvmet_copy_from_sgl

[Max Gurtovoy]

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@xxxxxxxxxxxx>
> > > Cc: Christoph Hellwig <hch@xxxxxx>
> > > Cc: Sagi Grimberg <sagi@xxxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxxxx>

>