Re: [PATCH hci-8.1 v4 4/5] vhost-blk: create kernel threads with the mainstream worker ioctls

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Low] vhost-blk: vhost_blk_set_workers() leaves vq on default thread on NEW_WORKER failure
- [Low] vhost-blk: num-threads=0 is silently accepted instead of rejected
- [Low] vhost-blk: workers from vhost_blk_set_workers() not released on vhost_blk_start() error paths
--

commit 23b6bc10b68c2d486e285cf836111089db385f0f
Author: Alexander Ivanov <alexander.ivanov@virtuozzo.com>

vhost-blk: create kernel threads with the mainstream worker ioctls

This patch replaces the VHOST_SET_NWORKERS ioctl with VHOST_NEW_WORKER and VHOST_ATTACH_VRING_WORKER. It spreads the virtqueues round-robin over num-threads - 1 extra workers. It also switches the device to vhost_task mode with VHOST_SET_FORK_FROM_OWNER before VHOST_SET_OWNER.

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

> diff --git a/hw/block/vhost-blk.c b/hw/block/vhost-blk.c
> --- a/hw/block/vhost-blk.c
> +++ b/hw/block/vhost-blk.c

[ ... ]

> +static void vhost_blk_set_workers(struct vhost_dev *hdev, int nworkers)
> +{
> +    uint32_t ids[VHOST_BLK_MAX_QUEUES];
> +    int i, ret, err = 0, nfailed = 0;
> +
> +    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;
> +        }
> +
> +        if (i < nworkers) {
> +            struct vhost_worker_state worker = { };
> +
> +            ret = hdev->vhost_ops->vhost_new_worker(hdev, &worker);
> +            if (ret < 0) {
> +                warn_report("vhost-blk: unable to create kernel thread: %s",
> +                            strerror(-ret));
> +                nworkers = i;
> +                continue;
> +            }

[Severity: Low]
When vhost_new_worker() fails for queue i, the code sets nworkers = i and
does continue.  Doesn't that skip the attach step for queue i itself?

Queue i then stays on the default thread, even though workers 1..i-1
already exist and could take it.  Later queues are spread correctly,
because w = i % nworkers uses the reduced nworkers.

The commit message says "if VHOST_NEW_WORKER fails partway, spread the
remaining virtqueues over the threads created so far".  Should the failing
queue also be attached to one of the existing workers, for example by
recomputing w and falling through to the attach step?

> +            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) {
> +            err = ret;
> +            nfailed++;
> +        }
> +    }

[ ... ]

> @@ -52,15 +99,24 @@ static int vhost_blk_start(VirtIODevice *vdev)
>          return 0;
>      }
>  
> +    nworkers = MIN(s->conf.num_threads, hdev->nvqs);
> +    if (nworkers > 1) {

[Severity: Low]
With -device vhost-blk,num-threads=0, nworkers = MIN(0, nvqs) = 0, so
every "nworkers > 1" branch is skipped and no warning is printed.

The old code did this:

    nworkers = s->conf.num_threads;
    if (nworkers != 1 && ioctl(s->vhostfd, VHOST_SET_NWORKERS, &nworkers)) {
        error_report("vhost-blk: unable to set number of kernel threads");
        return -EINVAL;
    }

So 0 was passed to the kernel, and the device failed to start if the ioctl
rejected it.  I could not check the kernel handler.

The num-threads property is a plain DEFINE_PROP_UINT16 with default 1, and
I found no realize-time check that rejects 0.  Is silently running
num-threads=0 on the single default thread intended?  The commit message
only says that num-threads=1 is unchanged.

> +        ret = hdev->vhost_ops->vhost_set_fork_from_owner(hdev,
> +                                                         VHOST_FORK_OWNER_TASK);

[ ... ]

> +    if (nworkers > 1) {
> +        vhost_blk_set_workers(hdev, nworkers);
>      }
>  
>      if (s->conf.serial) {

[Severity: Low]
Can the extra workers created by vhost_blk_set_workers() outlive a failed
start?

Once vhost_blk_set_workers() has run, later failures in vhost_blk_start()
do not issue VHOST_RESET_OWNER or VHOST_FREE_WORKER.  These are the
VHOST_BLK_SET_SERIAL failure, set_guest_notifiers, and the err_set,
err_disable and err_stop paths.

The only place that does the teardown is vhost_blk_reset():

    ret = ioctl(s->vhostfd, VHOST_RESET_OWNER, NULL);

So after a failed start with num-threads > 1 and at least 2 virtqueues, the
owner and the extra worker threads seem to stay alive until a later reset
or until the vhost fd is closed.

The missing owner reset on these error paths was there before this patch.
But this patch adds worker threads to what stays alive on them.  I could not
verify how the vz kernel handles a retried VHOST_SET_OWNER.  Should the
error paths release the workers?

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