On 5/26/26 11:33 AM, Pavel Tikhomirov wrote:
>
>
> 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 think it's the wrong kind of error here.
The actual requests get submitted to the block layer by
vhost_blk_req_submit() ->
vhost_blk_bio_send() ->
submit_bio()
and then eventually bio_err is set by the callback vhost_blk_req_done().
What vhost_blk_handle_host_kick() is doing is notifying the guest of the
request status. The status is taken from req->bio_err.
AFAIU at this point it's pointless to modify req->bio_err. Nobody else
will see it. And vhost_blk_handle_host_kick() is a "fire and forget"
kind of callback, it returns void. So the best we can do is log the
error, which vhost_blk_set_status() already does.
> 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?
>
That's a fair point, I suppose vhost_add_used() should be called as well.
So I guess the right thing to do is just drop the if() condition
entirely and call everything unconditionally. Will do in v2.
Andrey
_______________________________________________
Devel mailing list
[email protected]
https://lists.openvz.org/mailman/listinfo/devel
