[Qemu-devel] [PATCH] xen_disk: fix io accounting

[Qemu-devel] [PATCH] xen_disk: fix io accounting

[Olaf Hering]

bdrv_acct_done was called unconditional. But in case the ioreq has no
segments there is no matching bdrv_acct_start call. This could lead to
bogus accounting values.

Found by code inspection.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
---
 hw/block/xen_disk.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
index 098f6c6..7f0f14a 100644
--- a/hw/block/xen_disk.c
+++ b/hw/block/xen_disk.c
@@ -483,7 +483,18 @@ static void qemu_aio_complete(void *opaque, int ret)
     ioreq->status = ioreq->aio_errors ? BLKIF_RSP_ERROR : BLKIF_RSP_OKAY;
     ioreq_unmap(ioreq);
     ioreq_finish(ioreq);
-    bdrv_acct_done(ioreq->blkdev->bs, &ioreq->acct);
+    switch (ioreq->req.operation) {
+    case BLKIF_OP_WRITE:
+    case BLKIF_OP_FLUSH_DISKCACHE:
+        if (!ioreq->req.nr_segments) {
+            break;
+        }
+    case BLKIF_OP_READ:
+        bdrv_acct_done(ioreq->blkdev->bs, &ioreq->acct);
+        break;
+    default:
+        break;
+    }
     qemu_bh_schedule(ioreq->blkdev->bh);
 }
 

Re: [Qemu-devel] [PATCH] xen_disk: fix io accounting

[Andreas Färber]

Am 06.02.2014 16:55, schrieb Olaf Hering:
> bdrv_acct_done was called unconditional. But in case the ioreq has no
> segments there is no matching bdrv_acct_start call. This could lead to
> bogus accounting values.
> 
> Found by code inspection.
> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> ---
>  hw/block/xen_disk.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
> index 098f6c6..7f0f14a 100644
> --- a/hw/block/xen_disk.c
> +++ b/hw/block/xen_disk.c
> @@ -483,7 +483,18 @@ static void qemu_aio_complete(void *opaque, int ret)
>      ioreq->status = ioreq->aio_errors ? BLKIF_RSP_ERROR : BLKIF_RSP_OKAY;
>      ioreq_unmap(ioreq);
>      ioreq_finish(ioreq);
> -    bdrv_acct_done(ioreq->blkdev->bs, &ioreq->acct);
> +    switch (ioreq->req.operation) {
> +    case BLKIF_OP_WRITE:
> +    case BLKIF_OP_FLUSH_DISKCACHE:
> +        if (!ioreq->req.nr_segments) {
> +            break;
> +        }

This will need a /* fall through */ comment for static analysis tools.

Andreas

> +    case BLKIF_OP_READ:
> +        bdrv_acct_done(ioreq->blkdev->bs, &ioreq->acct);
> +        break;
> +    default:
> +        break;
> +    }
>      qemu_bh_schedule(ioreq->blkdev->bh);
>  }
>  
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH] xen_disk: fix io accounting

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 1690 bytes --]

On Thu, 6 Feb 2014, Andreas Färber wrote:
> Am 06.02.2014 16:55, schrieb Olaf Hering:
> > bdrv_acct_done was called unconditional. But in case the ioreq has no
> > segments there is no matching bdrv_acct_start call. This could lead to
> > bogus accounting values.
> > 
> > Found by code inspection.
> > 
> > Signed-off-by: Olaf Hering <olaf@aepfle.de>
> > ---
> >  hw/block/xen_disk.c | 13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
> > index 098f6c6..7f0f14a 100644
> > --- a/hw/block/xen_disk.c
> > +++ b/hw/block/xen_disk.c
> > @@ -483,7 +483,18 @@ static void qemu_aio_complete(void *opaque, int ret)
> >      ioreq->status = ioreq->aio_errors ? BLKIF_RSP_ERROR : BLKIF_RSP_OKAY;
> >      ioreq_unmap(ioreq);
> >      ioreq_finish(ioreq);
> > -    bdrv_acct_done(ioreq->blkdev->bs, &ioreq->acct);
> > +    switch (ioreq->req.operation) {
> > +    case BLKIF_OP_WRITE:
> > +    case BLKIF_OP_FLUSH_DISKCACHE:
> > +        if (!ioreq->req.nr_segments) {
> > +            break;
> > +        }
> 
> This will need a /* fall through */ comment for static analysis tools.
> 

Aside from this, I think that the patch is OK.
Can you please resend it with the comment?

 
> > +    case BLKIF_OP_READ:
> > +        bdrv_acct_done(ioreq->blkdev->bs, &ioreq->acct);
> > +        break;
> > +    default:
> > +        break;
> > +    }
> >      qemu_bh_schedule(ioreq->blkdev->bh);
> >  }
> >  
> > 
> 
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>