[Devel] [PATCH hci-8.1] drivers/vhost/blk: add support for worker ioctls

[Devel] [PATCH hci-8.1] drivers/vhost/blk: add support for worker ioctls

[Alexander Ivanov]

Let userspace create vhost workers and bind them to vhost-blk
virtqueues with the mainstream ioctls from c1ecd8e95007 ("vhost: allow
userspace to create workers"), the same way d74b55e65502 ("vhost_scsi:
add support for worker ioctls") does it for vhost-scsi.

VHOST_NEW_WORKER needs the device in vhost_task mode, so userspace has
to issue VHOST_SET_FORK_FROM_OWNER with VHOST_FORK_OWNER_TASK before
VHOST_SET_OWNER: our fork_from_owner_default is kthread.

https://virtuozzo.atlassian.net/browse/VSTOR-107537
Feature: vhost-blk: in-kernel accelerator for virtio-blk guests

Suggested-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
 drivers/vhost/blk.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/vhost/blk.c b/drivers/vhost/blk.c
index b170439a1806..ab3b8c4c2d5a 100644
--- a/drivers/vhost/blk.c
+++ b/drivers/vhost/blk.c
@@ -909,6 +909,14 @@ static long vhost_blk_ioctl(struct file *f, unsigned int ioctl,
 		return vhost_blk_set_features(blk, features);
 	case VHOST_RESET_OWNER:
 		return vhost_blk_reset_owner(blk);
+	case VHOST_NEW_WORKER:
+	case VHOST_FREE_WORKER:
+	case VHOST_ATTACH_VRING_WORKER:
+	case VHOST_GET_VRING_WORKER:
+		mutex_lock(&blk->dev.mutex);
+		ret = vhost_worker_ioctl(&blk->dev, ioctl, argp);
+		mutex_unlock(&blk->dev.mutex);
+		return ret;
 	default:
 		mutex_lock(&blk->dev.mutex);
 		ret = vhost_dev_ioctl(&blk->dev, ioctl, argp);
-- 
2.53.0

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1] drivers/vhost/blk: add support for worker ioctls

[no-reply]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] vhost-blk: completed bio is stranded when vq->worker is NULL, hanging vhost_blk_flush()
--

commit 813c45791bfed20701bbd685b3f3a40178259b8d
Author: Alexander Ivanov <alexander.ivanov@virtuozzo.com>

drivers/vhost/blk: add support for worker ioctls

This patch routes VHOST_NEW_WORKER, VHOST_FREE_WORKER,
VHOST_ATTACH_VRING_WORKER and VHOST_GET_VRING_WORKER to
vhost_worker_ioctl() under the device mutex in vhost_blk_ioctl(). This lets
userspace create vhost workers and bind them to vhost-blk virtqueues.

Link: https://virtuozzo.atlassian.net/browse/VSTOR-107537

> diff --git a/drivers/vhost/blk.c b/drivers/vhost/blk.c
> index b170439a1806..ab3b8c4c2d5a 100644
> --- a/drivers/vhost/blk.c
> +++ b/drivers/vhost/blk.c

[ ... ]

> @@ -909,6 +909,14 @@ static long vhost_blk_ioctl(struct file *f, unsigned int ioctl,
>  		return vhost_blk_set_features(blk, features);
>  	case VHOST_RESET_OWNER:
>  		return vhost_blk_reset_owner(blk);
> +	case VHOST_NEW_WORKER:
> +	case VHOST_FREE_WORKER:
> +	case VHOST_ATTACH_VRING_WORKER:
> +	case VHOST_GET_VRING_WORKER:
> +		mutex_lock(&blk->dev.mutex);
> +		ret = vhost_worker_ioctl(&blk->dev, ioctl, argp);
> +		mutex_unlock(&blk->dev.mutex);
> +		return ret;

[Severity: Medium]
Can a completed request be stranded here, hanging vhost_blk_flush()?

Once user-created workers can be attached to vhost-blk virtqueues, a vq
can lose its worker. When a worker dies, vhost_worker_killed() does:

    rcu_assign_pointer(vq->worker, NULL);

After that, a bio that completes goes through vhost_blk_req_done():

    if (atomic_dec_and_test(&req->bio_nr)) {
        llist_add(&req->llnode, &req->blk_vq->llhead);
        vhost_vq_work_queue(&req->blk_vq->vq, &req->blk_vq->work);
    }

vhost_vq_work_queue() finds rcu_dereference(vq->worker) == NULL, queues
nothing and returns false. vhost_blk_req_done() ignores that return value.

Only vhost_blk_handle_host_kick() calls vhost_blk_req_cleanup() and
forget_request(), and that work will never run. The request therefore
stays on llhead. Its bio, iov and bounce buffer are not freed, and
req_inflight[] is never decremented.

vhost_blk_flush() then does an untimed wait:

    wait_event(blk->flush_wait,
               !atomic_read(&blk->req_inflight[flush_bin]));

vhost_dev_flush() skips killed workers, because __vhost_worker_flush()
returns early when worker->killed is set, so nothing wakes this wait.

This wait is reachable from several paths:

    vhost_blk_release()->vhost_blk_drop_backend()->vhost_blk_flush()
    vhost_blk_reset_owner()->vhost_blk_drop_backend()->vhost_blk_flush()
    vhost_blk_set_features()->vhost_blk_flush()

The release path can hang, so the backend file reference is not dropped
and the vhost_blk is not freed.

The hang can already occur for the default worker in vhost_task mode.
This patch also exposes it to user-created workers, through
VHOST_NEW_WORKER and VHOST_ATTACH_VRING_WORKER.

Should vhost_blk_req_done() handle a false return from
vhost_vq_work_queue(), for example by cleaning up the request directly?

>  	default:
>  		mutex_lock(&blk->dev.mutex);
>  		ret = vhost_dev_ioctl(&blk->dev, ioctl, argp);

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261029110554.3315693-1-alexander.ivanov@virtuozzo.com?part=1

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1] drivers/vhost/blk: add support for worker ioctls

[Alexander Ivanov]

>> @@ -909,6 +909,14 @@ static long vhost_blk_ioctl(struct file *f, unsigned int ioctl,
>>   		return vhost_blk_set_features(blk, features);
>>   	case VHOST_RESET_OWNER:
>>   		return vhost_blk_reset_owner(blk);
>> +	case VHOST_NEW_WORKER:
>> +	case VHOST_FREE_WORKER:
>> +	case VHOST_ATTACH_VRING_WORKER:
>> +	case VHOST_GET_VRING_WORKER:
>> +		mutex_lock(&blk->dev.mutex);
>> +		ret = vhost_worker_ioctl(&blk->dev, ioctl, argp);
>> +		mutex_unlock(&blk->dev.mutex);
>> +		return ret;
> [Severity: Medium]
> Can a completed request be stranded here, hanging vhost_blk_flush()?
>
> Once user-created workers can be attached to vhost-blk virtqueues, a vq
> can lose its worker. When a worker dies, vhost_worker_killed() does:
>
>      rcu_assign_pointer(vq->worker, NULL);
>
> After that, a bio that completes goes through vhost_blk_req_done():
>
>      if (atomic_dec_and_test(&req->bio_nr)) {
>          llist_add(&req->llnode, &req->blk_vq->llhead);
>          vhost_vq_work_queue(&req->blk_vq->vq, &req->blk_vq->work);
>      }
>
> vhost_vq_work_queue() finds rcu_dereference(vq->worker) == NULL, queues
> nothing and returns false. vhost_blk_req_done() ignores that return value.
>
> Only vhost_blk_handle_host_kick() calls vhost_blk_req_cleanup() and
> forget_request(), and that work will never run. The request therefore
> stays on llhead. Its bio, iov and bounce buffer are not freed, and
> req_inflight[] is never decremented.
>
> vhost_blk_flush() then does an untimed wait:
>
>      wait_event(blk->flush_wait,
>                 !atomic_read(&blk->req_inflight[flush_bin]));
>
> vhost_dev_flush() skips killed workers, because __vhost_worker_flush()
> returns early when worker->killed is set, so nothing wakes this wait.
>
> This wait is reachable from several paths:
>
>      vhost_blk_release()->vhost_blk_drop_backend()->vhost_blk_flush()
>      vhost_blk_reset_owner()->vhost_blk_drop_backend()->vhost_blk_flush()
>      vhost_blk_set_features()->vhost_blk_flush()
>
> The release path can hang, so the backend file reference is not dropped
> and the vhost_blk is not freed.
>
> The hang can already occur for the default worker in vhost_task mode.
> This patch also exposes it to user-created workers, through
> VHOST_NEW_WORKER and VHOST_ATTACH_VRING_WORKER.
>
> Should vhost_blk_req_done() handle a false return from
> vhost_vq_work_queue(), for example by cleaning up the request directly?
This hang is pre-existing and not introduced by this patch, since the
default worker can already hit it in vhost_task mode, so we will fix it
later in a separate patch.
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1] drivers/vhost/blk: add support for worker ioctls

[Andrey Zhadchenko]

Reviewed-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>

On 10/29/26 12:05, Alexander Ivanov wrote:
> Let userspace create vhost workers and bind them to vhost-blk
> virtqueues with the mainstream ioctls from c1ecd8e95007 ("vhost: allow
> userspace to create workers"), the same way d74b55e65502 ("vhost_scsi:
> add support for worker ioctls") does it for vhost-scsi.
> 
> VHOST_NEW_WORKER needs the device in vhost_task mode, so userspace has
> to issue VHOST_SET_FORK_FROM_OWNER with VHOST_FORK_OWNER_TASK before
> VHOST_SET_OWNER: our fork_from_owner_default is kthread.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-107537
> Feature: vhost-blk: in-kernel accelerator for virtio-blk guests
> 
> Suggested-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>   drivers/vhost/blk.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/vhost/blk.c b/drivers/vhost/blk.c
> index b170439a1806..ab3b8c4c2d5a 100644
> --- a/drivers/vhost/blk.c
> +++ b/drivers/vhost/blk.c
> @@ -909,6 +909,14 @@ static long vhost_blk_ioctl(struct file *f, unsigned int ioctl,
>   		return vhost_blk_set_features(blk, features);
>   	case VHOST_RESET_OWNER:
>   		return vhost_blk_reset_owner(blk);
> +	case VHOST_NEW_WORKER:
> +	case VHOST_FREE_WORKER:
> +	case VHOST_ATTACH_VRING_WORKER:
> +	case VHOST_GET_VRING_WORKER:
> +		mutex_lock(&blk->dev.mutex);
> +		ret = vhost_worker_ioctl(&blk->dev, ioctl, argp);
> +		mutex_unlock(&blk->dev.mutex);
> +		return ret;
>   	default:
>   		mutex_lock(&blk->dev.mutex);
>   		ret = vhost_dev_ioctl(&blk->dev, ioctl, argp);

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1] drivers/vhost/blk: add support for worker ioctls

[Konstantin Khorenko]

I won't apply this version, please send v2 with the changes below.

On 10/29/26 12:05, Alexander Ivanov wrote:
> +	case VHOST_NEW_WORKER:
> +	case VHOST_FREE_WORKER:
> +	case VHOST_ATTACH_VRING_WORKER:
> +	case VHOST_GET_VRING_WORKER:
> +		mutex_lock(&blk->dev.mutex);
> +		ret = vhost_worker_ioctl(&blk->dev, ioctl, argp);
> +		mutex_unlock(&blk->dev.mutex);
> +		return ret;

- Put the VHOST_SET_NWORKERS revert (your v1 1/2) in front of this
  patch again, as one series.
- Fix the stranded-completion hang Sashiko found before this patch, in
  the same series.
- Say in the message that VHOST_SET_FORK_FROM_OWNER is needed before
  every VHOST_SET_OWNER.

VHOST_SET_NWORKERS (1491588e71daf) is still in the tree. Its workers
are written straight into vq->worker, and their attachment_cnt stays
0. So VHOST_FREE_WORKER frees a worker that a vq still uses, and the
next kick or completion writes into freed memory.

Before this patch the worker ioctls were not reachable in our kernel
at all, CONFIG_VHOST_SCSI is off. And our qemu (hci-dev) still calls
VHOST_SET_NWORKERS. In v1 the revert in 1/2 closed this hole, and it
got lost in the resend.

On the hang: the code path is old, but nobody runs vhost-blk in task
mode today, and in kthread mode a kill does not clear vq->worker. This
patch makes task mode the required setup, and killing a qemu with I/O
in flight is routine, so the fix has to land first, not later.
Mainline did the same for vhost-scsi.

> VHOST_NEW_WORKER needs the device in vhost_task mode, so userspace has
> to issue VHOST_SET_FORK_FROM_OWNER with VHOST_FORK_OWNER_TASK before
> VHOST_SET_OWNER: our fork_from_owner_default is kthread.

VHOST_RESET_OWNER puts fork_owner back to the default, and qemu resets
the owner on every device reset, so this is needed before every
VHOST_SET_OWNER, not only the first one.

-- 
Konstantin Khorenko,
Virtuozzo Linux Kernel Team

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel