Patchwork [38/41] virtio-blk: use QLIST for the list of requests

login
register
mail settings
Submitter Juan Quintela
Date Dec. 2, 2009, 12:04 p.m.
Message ID <1971390dc6d0ae9014e796f8ee444dee4a90815a.1259754427.git.quintela@redhat.com>
Download mbox | patch
Permalink /patch/40045/
State New
Headers show

Comments

Juan Quintela - Dec. 2, 2009, 12:04 p.m.
viltio_blak_dma_restart_bh() was unsafe, it used req->next after having
(possible) put req in another list

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/virtio-blk.c |   29 ++++++++++++++---------------
 1 files changed, 14 insertions(+), 15 deletions(-)
Michael S. Tsirkin - Dec. 2, 2009, 5:38 p.m.
On Wed, Dec 02, 2009 at 01:04:36PM +0100, Juan Quintela wrote:
> viltio_blak_dma_restart_bh() was unsafe, it used req->next after having
> (possible) put req in another list
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>

Sounds good, but why is this part of vmstate patchset?

> ---
>  hw/virtio-blk.c |   29 ++++++++++++++---------------
>  1 files changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
> index b716a36..838ec32 100644
> --- a/hw/virtio-blk.c
> +++ b/hw/virtio-blk.c
> @@ -26,7 +26,7 @@ typedef struct VirtIOBlock
>      VirtIODevice vdev;
>      BlockDriverState *bs;
>      VirtQueue *vq;
> -    VirtIOBlockReq *rq;
> +    QLIST_HEAD (,VirtIOBlockReq) rq;
>      char serial_str[BLOCK_SERIAL_STRLEN + 1];
>      QEMUBH *bh;
>      size_t config_size;
> @@ -81,7 +81,7 @@ struct VirtIOBlockReq
>      struct virtio_blk_outhdr *out;
>      struct virtio_scsi_inhdr *scsi;
>      QEMUIOVector qiov;
> -    struct VirtIOBlockReq *next;
> +    QLIST_ENTRY(VirtIOBlockReq) next;
>  };
> 
>  static void virtio_blk_req_complete(VirtIOBlockReq *req, int status)
> @@ -105,8 +105,7 @@ static int virtio_blk_handle_write_error(VirtIOBlockReq *req, int error)
> 
>      if ((error == ENOSPC && action == BLOCK_ERR_STOP_ENOSPC)
>              || action == BLOCK_ERR_STOP_ANY) {
> -        req->next = s->rq;
> -        s->rq = req;
> +        QLIST_INSERT_HEAD(&s->rq, req, next);
>          vm_stop(0);
>      } else {
>          virtio_blk_req_complete(req, VIRTIO_BLK_S_IOERR);
> @@ -366,17 +365,19 @@ static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
>  static void virtio_blk_dma_restart_bh(void *opaque)
>  {
>      VirtIOBlock *s = opaque;
> -    VirtIOBlockReq *req = s->rq;
> +    QLIST_HEAD (, VirtIOBlockReq) rq_copy;
> +    VirtIOBlockReq *req, *next_req;
> 
>      qemu_bh_delete(s->bh);
>      s->bh = NULL;
> 
> -    s->rq = NULL;
> +    QLIST_COPY_HEAD(&rq_copy, &s->rq);
> +    QLIST_INIT(&s->rq);
> 
> -    while (req) {
> +    QLIST_FOREACH_SAFE(req, &rq_copy, next, next_req) {
> +        QLIST_REMOVE(req, next);
>          bdrv_aio_writev(req->dev->bs, req->out->sector, &req->qiov,
>              req->qiov.size / 512, virtio_blk_rw_complete, req);
> -        req = req->next;
>      }
>  }
> 
> @@ -451,14 +452,13 @@ static uint32_t virtio_blk_get_features(VirtIODevice *vdev)
>  static void virtio_blk_save(QEMUFile *f, void *opaque)
>  {
>      VirtIOBlock *s = opaque;
> -    VirtIOBlockReq *req = s->rq;
> +    VirtIOBlockReq *req;;
> 
>      virtio_save(&s->vdev, f);
> -    
> -    while (req) {
> +
> +    QLIST_FOREACH(req, &s->rq, next) {
>          qemu_put_sbyte(f, 1);
>          qemu_put_buffer(f, (unsigned char*)&req->elem, sizeof(req->elem));
> -        req = req->next;
>      }
>      qemu_put_sbyte(f, 0);
>  }
> @@ -474,8 +474,7 @@ static int virtio_blk_load(QEMUFile *f, void *opaque, int version_id)
>      while (qemu_get_sbyte(f)) {
>          VirtIOBlockReq *req = virtio_blk_alloc_request(s);
>          qemu_get_buffer(f, (unsigned char*)&req->elem, sizeof(req->elem));
> -        req->next = s->rq;
> -        s->rq = req->next;
> +        QLIST_INSERT_HEAD(&s->rq, req, next);
>      }
> 
>      return 0;
> @@ -499,7 +498,7 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, DriveInfo *dinfo)
>      s->vdev.get_features = virtio_blk_get_features;
>      s->vdev.reset = virtio_blk_reset;
>      s->bs = dinfo->bdrv;
> -    s->rq = NULL;
> +    QLIST_INIT(&s->rq);
>      if (strlen(ps))
>          strncpy(s->serial_str, ps, sizeof(s->serial_str));
>      else
> -- 
> 1.6.5.2
Juan Quintela - Dec. 2, 2009, 6:56 p.m.
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Wed, Dec 02, 2009 at 01:04:36PM +0100, Juan Quintela wrote:
>> viltio_blak_dma_restart_bh() was unsafe, it used req->next after having
>> (possible) put req in another list
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>
> Sounds good, but why is this part of vmstate patchset?

Rest of the patches depend on it. I tried to only do the cleanups needed
for vmstate.  I really forced me to not doing more cleanups.

Later, Juan.
Michael S. Tsirkin - Dec. 2, 2009, 7 p.m.
On Wed, Dec 02, 2009 at 07:56:58PM +0100, Juan Quintela wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Wed, Dec 02, 2009 at 01:04:36PM +0100, Juan Quintela wrote:
> >> viltio_blak_dma_restart_bh() was unsafe, it used req->next after having
> >> (possible) put req in another list
> >> 
> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >
> > Sounds good, but why is this part of vmstate patchset?
> 
> Rest of the patches depend on it. I tried to only do the cleanups needed
> for vmstate.  I really forced me to not doing more cleanups.
> 
> Later, Juan.

But this is bugfix, not cleanup, right?
We want bugfixes ASAP, let not mix it with cleanups etc.
You can just post 2 series, and note
that second one depends on 1'st one.

Patch

diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index b716a36..838ec32 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -26,7 +26,7 @@  typedef struct VirtIOBlock
     VirtIODevice vdev;
     BlockDriverState *bs;
     VirtQueue *vq;
-    VirtIOBlockReq *rq;
+    QLIST_HEAD (,VirtIOBlockReq) rq;
     char serial_str[BLOCK_SERIAL_STRLEN + 1];
     QEMUBH *bh;
     size_t config_size;
@@ -81,7 +81,7 @@  struct VirtIOBlockReq
     struct virtio_blk_outhdr *out;
     struct virtio_scsi_inhdr *scsi;
     QEMUIOVector qiov;
-    struct VirtIOBlockReq *next;
+    QLIST_ENTRY(VirtIOBlockReq) next;
 };

 static void virtio_blk_req_complete(VirtIOBlockReq *req, int status)
@@ -105,8 +105,7 @@  static int virtio_blk_handle_write_error(VirtIOBlockReq *req, int error)

     if ((error == ENOSPC && action == BLOCK_ERR_STOP_ENOSPC)
             || action == BLOCK_ERR_STOP_ANY) {
-        req->next = s->rq;
-        s->rq = req;
+        QLIST_INSERT_HEAD(&s->rq, req, next);
         vm_stop(0);
     } else {
         virtio_blk_req_complete(req, VIRTIO_BLK_S_IOERR);
@@ -366,17 +365,19 @@  static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
 static void virtio_blk_dma_restart_bh(void *opaque)
 {
     VirtIOBlock *s = opaque;
-    VirtIOBlockReq *req = s->rq;
+    QLIST_HEAD (, VirtIOBlockReq) rq_copy;
+    VirtIOBlockReq *req, *next_req;

     qemu_bh_delete(s->bh);
     s->bh = NULL;

-    s->rq = NULL;
+    QLIST_COPY_HEAD(&rq_copy, &s->rq);
+    QLIST_INIT(&s->rq);

-    while (req) {
+    QLIST_FOREACH_SAFE(req, &rq_copy, next, next_req) {
+        QLIST_REMOVE(req, next);
         bdrv_aio_writev(req->dev->bs, req->out->sector, &req->qiov,
             req->qiov.size / 512, virtio_blk_rw_complete, req);
-        req = req->next;
     }
 }

@@ -451,14 +452,13 @@  static uint32_t virtio_blk_get_features(VirtIODevice *vdev)
 static void virtio_blk_save(QEMUFile *f, void *opaque)
 {
     VirtIOBlock *s = opaque;
-    VirtIOBlockReq *req = s->rq;
+    VirtIOBlockReq *req;;

     virtio_save(&s->vdev, f);
-    
-    while (req) {
+
+    QLIST_FOREACH(req, &s->rq, next) {
         qemu_put_sbyte(f, 1);
         qemu_put_buffer(f, (unsigned char*)&req->elem, sizeof(req->elem));
-        req = req->next;
     }
     qemu_put_sbyte(f, 0);
 }
@@ -474,8 +474,7 @@  static int virtio_blk_load(QEMUFile *f, void *opaque, int version_id)
     while (qemu_get_sbyte(f)) {
         VirtIOBlockReq *req = virtio_blk_alloc_request(s);
         qemu_get_buffer(f, (unsigned char*)&req->elem, sizeof(req->elem));
-        req->next = s->rq;
-        s->rq = req->next;
+        QLIST_INSERT_HEAD(&s->rq, req, next);
     }

     return 0;
@@ -499,7 +498,7 @@  VirtIODevice *virtio_blk_init(DeviceState *dev, DriveInfo *dinfo)
     s->vdev.get_features = virtio_blk_get_features;
     s->vdev.reset = virtio_blk_reset;
     s->bs = dinfo->bdrv;
-    s->rq = NULL;
+    QLIST_INIT(&s->rq);
     if (strlen(ps))
         strncpy(s->serial_str, ps, sizeof(s->serial_str));
     else