Re: [Qemu-devel] [PATCH 0/3] better I/O accounting V2

[Ryan Harper <ryanh@us.ibm.com>]

* Christoph Hellwig <hch@lst.de> [2011-08-24 13:46]:
> On Mon, Aug 22, 2011 at 11:46:00AM -0500, Ryan Harper wrote:
> > So, I believe this is how it's happening.
> > 
> > we start accounting on a write which is turned into a multiwrite via
> > virtio_blk_handle_write() which calls virtio_submit_multiwrite().
> > 
> > Then when the multiwrite completes, we invoke virtio_blk_rw_complete()
> > on each part of the multiwrite.  None of these requests have had their
> > acct structure initialized since there was just *one* initial write.
> > We could do a bdrv_acct_start() on each req, but that would break the
> > concept of hiding the additional writes under the initial request.
> 
> We do initialize all write fields correctly.  Where we fail right now
> is for non-read/write requests.  I can reproduce your issue by reading
> the serial attribute in sysfs - any chance Fedora does that during boot?

Indeed.  F15 has udev hooks to support /dev/disk/by-id for virtio-blk
devices which will issue non-read/write request (get drive serial).

> 
> The patch below should take care of these cases:

Tested with this patch, F15 boots fine, mkfs.ext4 and dd all work as
expected.

Tested-by: Ryan Harper <ryanh@us.ibm.com>

> 
> 
> Index: qemu/hw/virtio-blk.c
> ===================================================================
> --- qemu.orig/hw/virtio-blk.c	2011-08-24 20:32:03.764503179 +0200
> +++ qemu/hw/virtio-blk.c	2011-08-24 20:35:55.716579922 +0200
> @@ -59,9 +59,6 @@ static void virtio_blk_req_complete(Virt
>      stb_p(&req->in->status, status);
>      virtqueue_push(s->vq, &req->elem, req->qiov.size + sizeof(*req->in));
>      virtio_notify(&s->vdev, s->vq);
> -
> -    bdrv_acct_done(s->bs, &req->acct);
> -    g_free(req);
>  }
> 
>  static int virtio_blk_handle_rw_error(VirtIOBlockReq *req, int error,
> @@ -83,6 +80,8 @@ static int virtio_blk_handle_rw_error(Vi
>          vm_stop(VMSTOP_DISKFULL);
>      } else {
>          virtio_blk_req_complete(req, VIRTIO_BLK_S_IOERR);
> +        bdrv_acct_done(s->bs, &req->acct);
> +        g_free(req);
>          bdrv_mon_event(s->bs, BDRV_ACTION_REPORT, is_read);
>      }
> 
> @@ -102,6 +101,8 @@ static void virtio_blk_rw_complete(void
>      }
> 
>      virtio_blk_req_complete(req, VIRTIO_BLK_S_OK);
> +    bdrv_acct_done(req->dev->bs, &req->acct);
> +    g_free(req);
>  }
> 
>  static void virtio_blk_flush_complete(void *opaque, int ret)
> @@ -115,6 +116,8 @@ static void virtio_blk_flush_complete(vo
>      }
> 
>      virtio_blk_req_complete(req, VIRTIO_BLK_S_OK);
> +    bdrv_acct_done(req->dev->bs, &req->acct);
> +    g_free(req);
>  }
> 
>  static VirtIOBlockReq *virtio_blk_alloc_request(VirtIOBlock *s)
> @@ -157,6 +160,7 @@ static void virtio_blk_handle_scsi(VirtI
>       */
>      if (req->elem.out_num < 2 || req->elem.in_num < 3) {
>          virtio_blk_req_complete(req, VIRTIO_BLK_S_IOERR);
> +        g_free(req);
>          return;
>      }
> 
> @@ -165,6 +169,7 @@ static void virtio_blk_handle_scsi(VirtI
>       */
>      if (req->elem.out_num > 2 && req->elem.in_num > 3) {
>          virtio_blk_req_complete(req, VIRTIO_BLK_S_UNSUPP);
> +        g_free(req);
>          return;
>      }
> 
> @@ -231,11 +236,13 @@ static void virtio_blk_handle_scsi(VirtI
>      stl_p(&req->scsi->data_len, hdr.dxfer_len);
> 
>      virtio_blk_req_complete(req, status);
> +    g_free(req);
>  }
>  #else
>  static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
>  {
>      virtio_blk_req_complete(req, VIRTIO_BLK_S_UNSUPP);
> +    g_free(req);
>  }
>  #endif /* __linux__ */
> 
> @@ -378,6 +385,7 @@ static void virtio_blk_handle_request(Vi
>                  s->serial ? s->serial : "",
>                  MIN(req->elem.in_sg[0].iov_len, VIRTIO_BLK_ID_BYTES));
>          virtio_blk_req_complete(req, VIRTIO_BLK_S_OK);
> +        g_free(req);
>      } else if (type & VIRTIO_BLK_T_OUT) {
>          qemu_iovec_init_external(&req->qiov, &req->elem.out_sg[1],
>                                   req->elem.out_num - 1);

-- 
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
ryanh@us.ibm.com