virtio-blk: rerror/werror fixes (including corruption fix)
cover virtio-blk: rerror/werror fixes (including corruption fix)
This fixes two bugs in the handling of rerror/werror in virtio-blk. First is a fix for read requests morphing into write requests after the VM was stopped by a read error. Second is to consider rerror/werror in places where virtio-blk used to directly report errors back.
This series should be applied to stable as well.
Kevin Wolf (3):
virtio_blk: Factor virtio_blk_handle_request out
virtio-blk: Fix restart after read error
virtio-blk: Fix error cases which ignored rerror/werror
hw/virtio-blk.c | 93 +++++++++++++++++++++++++++++++++--------------------- 1 files changed, 57 insertions(+), 36 deletions(-)
PATCH 1/3 virtio_blk: Factor virtio_blk_handle_request out
We need a function that handles a single request. Create one by splitting out code from virtio_blk_handle_output.
--- hw/virtio-blk.c | 78 ++++++++++++++++++++++++++++++++---------------------- 1 files changed, 46 insertions(+), 32 deletions(-)
hw/virtio-blk.c +46 -32
@@ -317,46 +317,60 @@ static void virtio_blk_handle_read(VirtIOBlockReq *req) } } +typedef struct MultiReqBuffer { +BlockRequest blkreq[32 ]; +int num_writes; +BlockDriverState *old_bs; +}MultiReqBuffer ; + +static void virtio_blk_handle_request (VirtIOBlockReq *req, +MultiReqBuffer *mrb) +{ +if (req->elem.out_num <1 || req->elem.in_num <1 ) { +fprintf (stderr,"virtio-blk missing headers\n" ); +exit (1 ); + } + +if (req->elem.out_sg[0 ].iov_len <sizeof (*req->out) || + req->elem.in_sg[req->elem.in_num -1 ].iov_len <sizeof (*req->in)) { +fprintf (stderr,"virtio-blk header not in correct element\n" ); +exit (1 ); + } + + req->out = (void *)req->elem.out_sg[0 ].iov_base; + req->in = (void *)req->elem.in_sg[req->elem.in_num -1 ].iov_base; + +if (req->out->type &VIRTIO_BLK_T_FLUSH ) { +virtio_blk_handle_flush (req); + }else if (req->out->type &VIRTIO_BLK_T_SCSI_CMD ) { +virtio_blk_handle_scsi (req); + }else if (req->out->type &VIRTIO_BLK_T_OUT ) { +qemu_iovec_init_external (&req->qiov, &req->elem.out_sg[1 ], + req->elem.out_num -1 ); +virtio_blk_handle_write (mrb->blkreq, &mrb->num_writes, + req, &mrb->old_bs); + }else { +qemu_iovec_init_external (&req->qiov, &req->elem.in_sg[0 ], + req->elem.in_num -1 ); +virtio_blk_handle_read (req); + } +} +static void virtio_blk_handle_output (VirtIODevice *vdev,VirtQueue *vq) {VirtIOBlock *s =to_virtio_blk (vdev);VirtIOBlockReq *req; -BlockRequest blkreq[32 ]; -int num_writes =0 ; -BlockDriverState *old_bs =NULL ; +MultiReqBuffer mrb = { + .num_writes =0 , + .old_bs =NULL , + };while ((req =virtio_blk_get_request (s))) { -if (req->elem.out_num <1 || req->elem.in_num <1 ) { -fprintf (stderr,"virtio-blk missing headers\n" ); -exit (1 ); - } - -if (req->elem.out_sg[0 ].iov_len <sizeof (*req->out) || - req->elem.in_sg[req->elem.in_num -1 ].iov_len <sizeof (*req->in)) { -fprintf (stderr,"virtio-blk header not in correct element\n" ); -exit (1 ); - } - - req->out = (void *)req->elem.out_sg[0 ].iov_base; - req->in = (void *)req->elem.in_sg[req->elem.in_num -1 ].iov_base; - -if (req->out->type &VIRTIO_BLK_T_FLUSH ) { -virtio_blk_handle_flush (req); - }else if (req->out->type &VIRTIO_BLK_T_SCSI_CMD ) { -virtio_blk_handle_scsi (req); - }else if (req->out->type &VIRTIO_BLK_T_OUT ) { -qemu_iovec_init_external (&req->qiov, &req->elem.out_sg[1 ], - req->elem.out_num -1 ); -virtio_blk_handle_write (blkreq, &num_writes, req, &old_bs); - }else { -qemu_iovec_init_external (&req->qiov, &req->elem.in_sg[0 ], - req->elem.in_num -1 ); -virtio_blk_handle_read (req); - } +virtio_blk_handle_request (req, &mrb); } -if (num_writes >0 ) { -do_multiwrite (old_bs, blkreq, num_writes); +if (mrb.num_writes >0 ) { +do_multiwrite (mrb.old_bs, mrb.blkreq, mrb.num_writes); }/*
Signature · 1 line
1.6.5.2
[Qemu-devel] Re: [PATCH 1/3] virtio_blk: Factor virtio_blk_handle_request out
Quoted 2 lines · Kevin Wolf
We need a function that handles a single request. Create one by splitting out code from
virtio_blk_handle_output.
Looks good,
Anthony Liguori
AppliedRe: [Qemu-devel] [PATCH 1/3] virtio_blk: Factor virtio_blk_handle_request out
In reply to Kevin WolfQuoted 5 lines · Kevin Wolf
We need a function that handles a single request. Create one by splitting out code from
virtio_blk_handle_output.Signed-off-by Kevin Wolf<[email protected]>
Applied all. Thanks.
Regards,
Anthony Liguori
Quoted 102 lines
--- hw/virtio-blk.c | 78 ++++++++++++++++++++++++++++++++---------------------- 1 files changed, 46 insertions(+), 32 deletions(-)hw/virtio-blk.c +46
-32@@ -317,46 +317,60 @@ static void virtio_blk_handle_read(VirtIOBlockReq *req) } } +typedef struct MultiReqBuffer { +BlockRequest blkreq[32 ]; +int num_writes; +BlockDriverState *old_bs; +}MultiReqBuffer ; + +static void virtio_blk_handle_request (VirtIOBlockReq *req, +MultiReqBuffer *mrb) +{ +if (req->elem.out_num<1 || req->elem.in_num<1 ) { +fprintf (stderr,"virtio-blk missing headers\n" ); +exit (1 ); + } + +if (req->elem.out_sg[0 ].iov_len<sizeof (*req->out) || + req->elem.in_sg[req->elem.in_num -1 ].iov_len<sizeof (*req->in)) { +fprintf (stderr,"virtio-blk header not in correct element\n" ); +exit (1 ); + } + + req->out = (void *)req->elem.out_sg[0 ].iov_base; + req->in = (void *)req->elem.in_sg[req->elem.in_num -1 ].iov_base; + +if (req->out->type&VIRTIO_BLK_T_FLUSH ) { +virtio_blk_handle_flush (req); + }else if (req->out->type&VIRTIO_BLK_T_SCSI_CMD ) { +virtio_blk_handle_scsi (req); + }else if (req->out->type&VIRTIO_BLK_T_OUT ) { +qemu_iovec_init_external (&req->qiov,&req->elem.out_sg[1 ], + req->elem.out_num -1 ); +virtio_blk_handle_write (mrb->blkreq,&mrb->num_writes, + req,&mrb->old_bs); + }else { +qemu_iovec_init_external (&req->qiov,&req->elem.in_sg[0 ], + req->elem.in_num -1 ); +virtio_blk_handle_read (req); + } +} +static void virtio_blk_handle_output (VirtIODevice *vdev,VirtQueue *vq) {VirtIOBlock *s =to_virtio_blk (vdev);VirtIOBlockReq *req; -BlockRequest blkreq[32 ]; -int num_writes =0 ; -BlockDriverState *old_bs =NULL ; +MultiReqBuffer mrb = { + .num_writes =0 , + .old_bs =NULL , + };while ((req =virtio_blk_get_request (s))) { -if (req->elem.out_num<1 || req->elem.in_num<1 ) { -fprintf (stderr,"virtio-blk missing headers\n" ); -exit (1 ); - } - -if (req->elem.out_sg[0 ].iov_len<sizeof (*req->out) || - req->elem.in_sg[req->elem.in_num -1 ].iov_len<sizeof (*req->in)) { -fprintf (stderr,"virtio-blk header not in correct element\n" ); -exit (1 ); - } - - req->out = (void *)req->elem.out_sg[0 ].iov_base; - req->in = (void *)req->elem.in_sg[req->elem.in_num -1 ].iov_base; - -if (req->out->type&VIRTIO_BLK_T_FLUSH ) { -virtio_blk_handle_flush (req); - }else if (req->out->type&VIRTIO_BLK_T_SCSI_CMD ) { -virtio_blk_handle_scsi (req); - }else if (req->out->type&VIRTIO_BLK_T_OUT ) { -qemu_iovec_init_external (&req->qiov,&req->elem.out_sg[1 ], - req->elem.out_num -1 ); -virtio_blk_handle_write (blkreq,&num_writes, req,&old_bs); - }else { -qemu_iovec_init_external (&req->qiov,&req->elem.in_sg[0 ], - req->elem.in_num -1 ); -virtio_blk_handle_read (req); - } +virtio_blk_handle_request (req,&mrb); } -if (num_writes>0 ) { -do_multiwrite (old_bs, blkreq, num_writes); +if (mrb.num_writes>0 ) { +do_multiwrite (mrb.old_bs, mrb.blkreq, mrb.num_writes); }/*
PATCH 2/3 virtio-blk: Fix restart after read error
In reply to Kevin WolfCurrent code assumes that only write requests are ever going to be restarted. This is wrong since rerror=stop exists. Instead of directly starting writes, use the same request processing as used for new requests.
--- hw/virtio-blk.c | 11 +++++++++-- 1 files changed, 9 insertions(+), 2 deletions(-)
hw/virtio-blk.c +9 -2
@@ -384,6 +384,10 @@ static void virtio_blk_dma_restart_bh(void *opaque) {VirtIOBlock *s = opaque;VirtIOBlockReq *req = s->rq; +MultiReqBuffer mrb = { + .num_writes =0 , + .old_bs =NULL , + };qemu_bh_delete (s->bh); s->bh =NULL ;
@@ -391,10 +395,13 @@ static void virtio_blk_dma_restart_bh(void *opaque) s->rq =NULL ;while (req) { -bdrv_aio_writev (req->dev->bs, req->out->sector, &req->qiov, - req->qiov.size /512 , virtio_blk_rw_complete, req); +virtio_blk_handle_request (req, &mrb); req = req->next; } + +if (mrb.num_writes >0 ) { +do_multiwrite (mrb.old_bs, mrb.blkreq, mrb.num_writes); + } }static void virtio_blk_dma_restart_cb (void *opaque,int running,int reason)
Signature · 1 line
1.6.5.2
[Qemu-devel] Re: [PATCH 2/3] virtio-blk: Fix restart after read error
Quoted 3 lines · Kevin Wolf
Current code assumes that only write requests are ever going to be restarted. This is wrong since rerror=stop exists. Instead of directly starting writes, use the same request processing as used for new requests.
Looks good,
PATCH 3/3 virtio-blk: Fix error cases which ignored rerror/werror
In reply to Kevin WolfIf an I/O request fails right away instead of getting an error only in the callback, we still need to consider rerror/werror.
--- hw/virtio-blk.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
hw/virtio-blk.c +2 -2
@@ -269,7 +269,7 @@ static void do_multiwrite(BlockDriverState *bs, BlockRequest *blkreq,if (ret !=0 ) {for (i =0 ; i < num_writes; i++) {if (blkreq[i].error) { -virtio_blk_req_complete (blkreq[i].opaque,VIRTIO_BLK_S_IOERR ); +virtio_blk_rw_complete (blkreq[i].opaque, -EIO ); } } }
@@ -313,7 +313,7 @@ static void virtio_blk_handle_read(VirtIOBlockReq *req) acb =bdrv_aio_readv (req->dev->bs, req->out->sector, &req->qiov, req->qiov.size /512 , virtio_blk_rw_complete, req);if (!acb) { -virtio_blk_req_complete (req,VIRTIO_BLK_S_IOERR ); +virtio_blk_rw_complete (req, -EIO ); } }
Signature · 1 line
1.6.5.2
[Qemu-devel] Re: [PATCH 3/3] virtio-blk: Fix error cases which ignored rerror/werror
Quoted 2 lines · Kevin Wolf
If an I/O request fails right away instead of getting an error only in the callback, we still need to consider rerror/werror.
Looks good,