[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 04/16] hw/block/nvme: remove redundant has_sg member
From: |
Minwoo Im |
Subject: |
Re: [PATCH 04/16] hw/block/nvme: remove redundant has_sg member |
Date: |
Thu, 30 Jul 2020 09:34:42 +0900 |
> -----Original Message-----
> From: Qemu-devel <qemu-devel-bounces+minwoo.im=samsung.com@nongnu.org> On
> Behalf Of Klaus Jensen
> Sent: Thursday, July 30, 2020 3:29 AM
> To: Minwoo Im <minwoo.im.dev@gmail.com>
> Cc: Kevin Wolf <kwolf@redhat.com>; qemu-block@nongnu.org; Klaus Jensen
> <k.jensen@samsung.com>; qemu-devel@nongnu.org; Max Reitz <mreitz@redhat.com>;
> Keith Busch <kbusch@kernel.org>
> Subject: Re: [PATCH 04/16] hw/block/nvme: remove redundant has_sg member
>
> On Jul 30 00:29, Minwoo Im wrote:
> > Klaus,
> >
>
> Hi Minwoo,
>
> Thanks for the reviews and welcome to the party! :)
>
> > On 20-07-20 13:37:36, Klaus Jensen wrote:
> > > From: Klaus Jensen <k.jensen@samsung.com>
> > >
> > > Remove the has_sg member from NvmeRequest since it's redundant.
> > >
> > > Also, make sure the request iov is destroyed at completion time.
> > >
> > > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > > Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
> > > ---
> > > hw/block/nvme.c | 11 ++++++-----
> > > hw/block/nvme.h | 1 -
> > > 2 files changed, 6 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > > index cb236d1c8c46..6a1a1626b87b 100644
> > > --- a/hw/block/nvme.c
> > > +++ b/hw/block/nvme.c
> > > @@ -548,16 +548,20 @@ static void nvme_rw_cb(void *opaque, int ret)
> > > block_acct_failed(blk_get_stats(n->conf.blk), &req->acct);
> > > req->status = NVME_INTERNAL_DEV_ERROR;
> > > }
> > > - if (req->has_sg) {
> > > +
> > > + if (req->qsg.nalloc) {
> >
> > Personally, I prefer has_xxx or is_xxx to check whether the request is
> > based on sg or iov as an inline function, but 'nalloc' is also fine to
> > figure out the meaning of purpose here.
> >
>
> What I really want to do is get rid of this duality with qsg and iovs at
> some point. I kinda wanna get rid of the dma helpers and the qsg
> entirely and do the DMA handling directly.
>
> Maybe an `int flags` member in NvmeRequest would be better for this,
> such as NVME_REQ_DMA etc.
That looks better, but it looks like this is out of scope of this series.
Anyway, Please take my tag for this patch.
Reviewed-by: Minwoo Im <minwoo.im.dev@gmail.com>
>
> > > qemu_sglist_destroy(&req->qsg);
> > > }
> > > + if (req->iov.nalloc) {
> > > + qemu_iovec_destroy(&req->iov);
> > > + }
> > > +
> >
> > Maybe this can be in a separated commit?
> >
>
> Yeah. I guess whenever a commit message includes "Also, ..." you really
> should factor the change out ;)
>
> I'll split it.
>
> > Otherwise, It looks good to me.
> >
> > Thanks,
> >
>
> Does that mean I can add your R-b? :)
- Re: [PATCH 02/16] hw/block/nvme: add mapping helpers, (continued)
[PATCH 04/16] hw/block/nvme: remove redundant has_sg member, Klaus Jensen, 2020/07/20
[PATCH 06/16] hw/block/nvme: pass request along for tracing, Klaus Jensen, 2020/07/20
[PATCH 10/16] hw/block/nvme: add check for mdts, Klaus Jensen, 2020/07/20
[PATCH 07/16] hw/block/nvme: add request mapping helper, Klaus Jensen, 2020/07/20