On 5/22/26 19:24, Andrey Drobyshev wrote:
> Upon request completion, vhost_blk_handle_host_kick() pops the request from
> the queue and writes one status byte into the guest's status iov via
> vhost_blk_set_status().
>
> If for whatever reason vhost_blk_set_status() fails (e.g. virtio device
> reset/disable on that queue, or QEMU-driven device state changes like
> block_resize) - we end up forgetting to call forget_request() and
> decrease the in-flight counter. Thus it remains > 0 forever.
>
> Later upon guest shutdown, when flush happens, we end up forever waiting
> in D state in vhost_blk_flush() on this operation:
>
> wait_event(blk->flush_wait, !atomic_read(&blk->req_inflight[flush_bin]));
>
> So that even SIGKILL can't reap the QEMU process. That's exactly what
> we observed in hci-volumes test after block_resize.
>
> Fix by adjusting the loop body so that forget_request() is always called
> unconditionally.
>
> https://virtuozzo.atlassian.net/browse/VSTOR-132571
> Fixes: 40a5928ec730 ("drivers/vhost: vhost-blk accelerator for virtio-blk
> guests")
> Signed-off-by: Andrey Drobyshev <[email protected]>
> ---
> drivers/vhost/blk.c | 9 ++++-----
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/vhost/blk.c b/drivers/vhost/blk.c
> index 5acdf973ac71..b11f08f878f4 100644
> --- a/drivers/vhost/blk.c
> +++ b/drivers/vhost/blk.c
> @@ -586,11 +586,10 @@ static void vhost_blk_handle_host_kick(struct
> vhost_work *work)
>
> status = req->bio_err == 0 ? VIRTIO_BLK_S_OK :
> VIRTIO_BLK_S_IOERR;
> ret = vhost_blk_set_status(req, status);
> - if (unlikely(ret))
> - continue;
> -
> - vhost_add_used(vq, req->head, req->len);
> - added = true;
> + if (likely(!ret)) {
> + vhost_add_used(vq, req->head, req->len);
> + added = true;
> + }
>
> forget_request(req);
> }
I wonder if it should instead be:
--- a/drivers/vhost/blk.c
+++ b/drivers/vhost/blk.c
@@ -585,9 +585,8 @@ static void vhost_blk_handle_host_kick(struct vhost_work
*work)
vhost_blk_req_cleanup(req);
status = req->bio_err == 0 ? VIRTIO_BLK_S_OK :
VIRTIO_BLK_S_IOERR;
- ret = vhost_blk_set_status(req, status);
- if (unlikely(ret))
- continue;
+ if (vhost_blk_set_status(req, status))
+ req->bio_err = EINVAL;
vhost_add_used(vq, req->head, req->len);
added = true;
I mean it would be nice to report some error (not sure if EINVAL or some other)
if status setting had failed, right?
I don't see why we should not "vhost_add_used" if status setting had failed, as
above we have similar handling of vhost_blk_move_bb_to_req() error and even
when request has no data in req->iov we still call vhost_add_used(), so why not
to do the same for status?
--
Best regards, Pavel Tikhomirov
Senior Software Developer, Virtuozzo.
_______________________________________________
Devel mailing list
[email protected]
https://lists.openvz.org/mailman/listinfo/devel
