On 6/1/26 18:46, 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]>
> ---
> v1 -> v2:
> * Don't skip calling vhost_add_used(). Just remove if(...) clause and
> call vhost_blk_set_status(), vhost_add_used() and forget_request()
> unconditionally.
>
> drivers/vhost/blk.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/vhost/blk.c b/drivers/vhost/blk.c
> index f8bedc3e0ee8..e048a7cf07a6 100644
> --- a/drivers/vhost/blk.c
> +++ b/drivers/vhost/blk.c
> @@ -565,7 +565,6 @@ static void vhost_blk_handle_host_kick(struct vhost_work
> *work)
> struct vhost_blk *blk = NULL;
> bool added;
> u8 status;
> - int ret;
>
> blk_vq = container_of(work, struct vhost_blk_vq, work);
> vq = &blk_vq->vq;
> @@ -585,9 +584,7 @@ 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;
> + vhost_blk_set_status(req, status);
Looks good.
nit: Probably we would like to suppress possible unused return value
warnings here with something like:
(void) vhost_blk_set_status(req, status);
>
> vhost_add_used(vq, req->head, req->len);
> added = true;
--
Best regards, Pavel Tikhomirov
Senior Software Developer, Virtuozzo.
_______________________________________________
Devel mailing list
[email protected]
https://lists.openvz.org/mailman/listinfo/devel
