Re: [PATCH 04/16] hw/block/nvme: remove redundant has_sg member
> -Original Message-
> From: Qemu-devel On
> Behalf Of Klaus Jensen
> Sent: Thursday, July 30, 2020 3:29 AM
> To: Minwoo Im
> Cc: Kevin Wolf ; qemu-block@nongnu.org; Klaus Jensen
> ; qemu-de...@nongnu.org; Max Reitz ;
> Keith Busch
> 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
> > >
> > > 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
> > > Reviewed-by: Maxim Levitsky
> > > ---
> > > 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), >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
>
> > > qemu_sglist_destroy(>qsg);
> > > }
> > > +if (req->iov.nalloc) {
> > > +qemu_iovec_destroy(>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 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
> >
> > 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
> > Reviewed-by: Maxim Levitsky
> > ---
> > 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), >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.
> > qemu_sglist_destroy(>qsg);
> > }
> > +if (req->iov.nalloc) {
> > +qemu_iovec_destroy(>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 04/16] hw/block/nvme: remove redundant has_sg member
Klaus,
On 20-07-20 13:37:36, Klaus Jensen wrote:
> From: Klaus Jensen
>
> 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
> Reviewed-by: Maxim Levitsky
> ---
> 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), >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.
> qemu_sglist_destroy(>qsg);
> }
> +if (req->iov.nalloc) {
> +qemu_iovec_destroy(>iov);
> +}
> +
Maybe this can be in a separated commit?
Otherwise, It looks good to me.
Thanks,
[PATCH 04/16] hw/block/nvme: remove redundant has_sg member
From: Klaus Jensen
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
Reviewed-by: Maxim Levitsky
---
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), >acct);
req->status = NVME_INTERNAL_DEV_ERROR;
}
-if (req->has_sg) {
+
+if (req->qsg.nalloc) {
qemu_sglist_destroy(>qsg);
}
+if (req->iov.nalloc) {
+qemu_iovec_destroy(>iov);
+}
+
nvme_enqueue_req_completion(cq, req);
}
static uint16_t nvme_flush(NvmeCtrl *n, NvmeNamespace *ns, NvmeCmd *cmd,
NvmeRequest *req)
{
-req->has_sg = false;
block_acct_start(blk_get_stats(n->conf.blk), >acct, 0,
BLOCK_ACCT_FLUSH);
req->aiocb = blk_aio_flush(n->conf.blk, nvme_rw_cb, req);
@@ -583,7 +587,6 @@ static uint16_t nvme_write_zeros(NvmeCtrl *n, NvmeNamespace
*ns, NvmeCmd *cmd,
return NVME_LBA_RANGE | NVME_DNR;
}
-req->has_sg = false;
block_acct_start(blk_get_stats(n->conf.blk), >acct, 0,
BLOCK_ACCT_WRITE);
req->aiocb = blk_aio_pwrite_zeroes(n->conf.blk, offset, count,
@@ -621,7 +624,6 @@ static uint16_t nvme_rw(NvmeCtrl *n, NvmeNamespace *ns,
NvmeCmd *cmd,
}
if (req->qsg.nsg > 0) {
-req->has_sg = true;
block_acct_start(blk_get_stats(n->conf.blk), >acct, req->qsg.size,
acct);
req->aiocb = is_write ?
@@ -630,7 +632,6 @@ static uint16_t nvme_rw(NvmeCtrl *n, NvmeNamespace *ns,
NvmeCmd *cmd,
dma_blk_read(n->conf.blk, >qsg, data_offset, BDRV_SECTOR_SIZE,
nvme_rw_cb, req);
} else {
-req->has_sg = false;
block_acct_start(blk_get_stats(n->conf.blk), >acct, req->iov.size,
acct);
req->aiocb = is_write ?
diff --git a/hw/block/nvme.h b/hw/block/nvme.h
index 0b6a8ae66559..5519b5cc7686 100644
--- a/hw/block/nvme.h
+++ b/hw/block/nvme.h
@@ -22,7 +22,6 @@ typedef struct NvmeRequest {
struct NvmeSQueue *sq;
BlockAIOCB *aiocb;
uint16_tstatus;
-boolhas_sg;
NvmeCqe cqe;
BlockAcctCookie acct;
QEMUSGList qsg;
--
2.27.0
