* Christoph Hellwig <h...@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 <ry...@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
ry...@us.ibm.com
