Re: [PATCH hci-8.1 v2 1/2] vhost-blk: create kernel threads with the mainstream worker ioctls

[Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>]

On 10/27/26 2:47 PM, Alexander Ivanov wrote:
> num-threads was passed to our own VHOST_SET_NWORKERS kernel ioctl.
> The kernel is dropping it in favour of the mainstream worker ioctls,
> which vhost-blk now supports, so use those: create num-threads - 1
> workers with VHOST_NEW_WORKER and attach them to the virtqueues
> round-robin with VHOST_ATTACH_VRING_WORKER, the default worker being
> the first one. This is the layout VHOST_SET_NWORKERS produced.
> 
> VHOST_NEW_WORKER works only in vhost_task mode, while our kernel,
> unlike mainline, defaults to kthreads, so switch the device to
> vhost_task mode with VHOST_SET_FORK_FROM_OWNER before VHOST_SET_OWNER.
> VHOST_RESET_OWNER restores the default, hence this is done on every
> start. With num-threads=1 nothing changes: no worker ioctls, kthread
> mode.
> 
> On a kernel without these ioctls warn and run a single thread rather
> than fail to start vhost-blk.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-107537
> 
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>  [...]
The overall logic is fine, but I'd like patch to be simplified.

First, there're dedicated vhost_ops->vhost_new_worker() and
vhost_ops->vhost_attach_vring_worker() calls.  We should reuse them.
For consistency, let's add another patch "vhost-backend: add
vhost_set_fork_from_owner op".  Smth like:

>   /* hw/virtio/vhost-backend.c */
>   static int vhost_kernel_set_fork_from_owner(struct vhost_dev *dev,
>                                               uint8_t fork_owner)
>   {
>       return vhost_kernel_call(dev, VHOST_SET_FORK_FROM_OWNER, &fork_owner);
>   }

Other issues (by Claude):

1. Dead fallback in vhost_blk_set_workers(). The ENOTTY && i == 1 branch
handles a kernel that has VHOST_SET_FORK_FROM_OWNER (6.17 / vz10) but
not VHOST_NEW_WORKER (6.4) — no such kernel exists. Once the fork-mode
ioctl succeeded, a worker-ioctl failure is a real error.  Drop, do a
single fallback point in start().

2. Fork-mode block in vhost_blk_start() is hard to read. A declaration
inside the if, the ENOTTY test nested under 'ret != -ENOTTY', the
'nworkers = 1' fall-through at the bottom. Switch to flat three-way if
(ret == -ENOTTY) … else if (ret < 0) … at the call site, ioctl plumbing
in the backend.

3. Redundant clamp. MIN(nworkers, VHOST_BLK_MAX_QUEUES) is already
guaranteed by nworkers ≤ nvqs ≤ VHOST_BLK_MAX_QUEUES (realize, :666); a
clamp hides an invariant. Switch to assert(nworkers <= ARRAY_SIZE(ids)).

4. memset() + field stores for two structs per iteration. Switch to
designated initializers, and the two loops merged into one: thread w is
created by its first user vq (i < nworkers), then attached — same ioctl
count, one error path per ioctl, loop body reads as "which thread,
create if first, attach".

5. s->dev. repeated throughout. Use struct vhost_dev *hdev = &s->dev;
once, as vhost-scsi does; vhost_blk_set_workers() takes hdev and no
longer needs VHostBlk.

6. Commit message nits (1/2). "The kernel is dropping it" needs the
kernel change/ticket; say that VHOST_NEW_WORKER returns -EFAULT (not
ENOTTY) in kthread mode, which is why the mode switch — not the worker
ioctl — is the capability probe; "default worker being the first one"
belongs in the code comment, where it already is.

Here's what the changed version might look like:

> /*
>    * Spread the virtqueues round-robin over nworkers kernel threads. Thread 0
>    * is the default worker VHOST_SET_OWNER created; vq 0 and every nworkers-th
>    * vq after it stay on it.
>    */
>   static int vhost_blk_set_workers(struct vhost_dev *hdev, int nworkers)
>   {
>       uint32_t ids[VHOST_BLK_MAX_QUEUES];     /* ids[0]: default worker, unused */
>       int i, ret;
> 
>       assert(nworkers <= ARRAY_SIZE(ids));
> 
>       for (i = 1; i < hdev->nvqs; i++) {
>           int w = i % nworkers;
>           struct vhost_vring_worker vq_worker = { .index = i };
> 
>           if (w == 0) {
>               continue;
>           }
> 
>           /* vq i < nworkers is the first user of thread i: create it here */
>           if (i < nworkers) {
>               struct vhost_worker_state worker = { };
> 
>               ret = hdev->vhost_ops->vhost_new_worker(hdev, &worker);
>               if (ret < 0) {
>                   error_report("vhost-blk: unable to create kernel thread: %s",
>                                strerror(-ret));
>                   return ret;
>               }
>               ids[w] = worker.worker_id;
>           }
> 
>           vq_worker.worker_id = ids[w];
>           ret = hdev->vhost_ops->vhost_attach_vring_worker(hdev, &vq_worker);
>           if (ret < 0) {
>               error_report("vhost-blk: unable to attach kernel thread to "
>                            "queue %d: %s", i, strerror(-ret));
>               return ret;
>           }
>       }
> 
>       return 0;
>   }
> 
>   static int vhost_blk_start(VirtIODevice *vdev)
>   {
>       VHostBlk *s = VHOST_BLK(vdev);
>       struct vhost_dev *hdev = &s->dev;
>       ...
>       int ret, i, nworkers;
>       ...
>       if (s->vhost_started) {
>           return 0;
>       }
> 
>       nworkers = MIN(s->conf.num_threads, hdev->nvqs);
>       if (nworkers > 1) {
>           /*
>            * VHOST_NEW_WORKER needs vhost_task mode; our kernel defaults to
>            * kthreads and VHOST_RESET_OWNER brings that default back, so ask
>            * on every start. Must precede VHOST_SET_OWNER.
>            */
>           ret = hdev->vhost_ops->vhost_set_fork_from_owner(hdev,
>                                                            VHOST_FORK_OWNER_TASK);
>           if (ret == -ENOTTY) {
>               warn_report_once("vhost-blk: kernel does not support vhost tasks, "
>                                "using a single kernel thread");
>               nworkers = 1;
>           } else if (ret < 0) {
>               error_report("vhost-blk: unable to set vhost task mode: %s",
>                            strerror(-ret));
>               return ret;
>           }
>       }
> 
>       if (ioctl(s->vhostfd, VHOST_SET_OWNER, NULL)) {
>           error_report("vhost-blk: unable to set owner");
>           return -ENOSYS;
>       }
> 
>       if (nworkers > 1) {
>           ret = vhost_blk_set_workers(hdev, nworkers);
>           if (ret < 0) {
>               return ret;
>           }
>       }
> 
>       if (s->conf.serial) {
>       ...

Andrey