Re: [PATCH hci-8.1 v3 3/4] 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:
- [Medium] vhost-blk: single-thread fallback only handles -ENOTTY from unknown vhost ioctls
- [Low] vhost-blk: ENOTTY worker fallback leaves device in vhost_task mode
- [Low] vhost-blk: num-threads=0 silently accepted as a single kernel thread
--

commit bce7b559a829ea8e18f3184e167eeedc9027e77f
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 creates num-threads - 1 workers and attaches
them to the virtqueues round-robin. It also switches the device to
vhost_task mode with VHOST_SET_FORK_FROM_OWNER before VHOST_SET_OWNER. If
the kernel lacks these ioctls, it warns and runs a single thread.

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 int vhost_blk_set_workers(struct vhost_dev *hdev, int nworkers)
> +{

[ ... ]

> +            ret = hdev->vhost_ops->vhost_new_worker(hdev, &worker);
> +            if (ret == -ENOTTY) {
> +                /* vhost-blk predating worker ioctls, fails on vq 1 only */
> +                warn_report_once("vhost-blk: kernel does not support worker "
> +                                 "ioctls, using a single kernel thread");
> +                return 0;
> +            } else if (ret < 0) {
> +                error_report("vhost-blk: unable to create kernel thread: %s",
> +                             strerror(-ret));
> +                return ret;
> +            }

[Severity: Medium]
Is -ENOTTY the only errno an older kernel can return here? The fallback to
a single thread is taken only for an exact -ENOTTY from
vhost_blk_set_workers()->vhost_new_worker(). Any other errno goes through
error_report() and fails vhost_blk_start().

The same applies in vhost_blk_start() to the result of
vhost_set_fork_from_owner():

    ret = hdev->vhost_ops->vhost_set_fork_from_owner(hdev,
                                                     VHOST_FORK_OWNER_TASK);
    if (ret == -ENOTTY) {
        ...
        nworkers = 1;
    } else if (ret < 0) {
        error_report("vhost-blk: unable to set vhost task mode: %s", ...);
        return ret;

Both go through vhost_kernel_call(), which returns the raw -errno from
ioctl().

The comment in vhost_kernel_set_features() says that for unknown vhost
ioctls the kernel interprets the argument as a virtio queue id and most
likely errors out validating that id, instead of reporting an unknown
operation.

If VHOST_SET_FORK_FROM_OWNER falls through to the vring ioctl handler on a
kernel without it, could the result be -ENOBUFS instead of -ENOTTY? In that
case the commit message promise to "warn and run a single thread rather than
fail to start vhost-blk" would not hold when num-threads > 1.

The commit message also notes that VHOST_NEW_WORKER fails with -EFAULT in
kthread mode, so the errno is not uniform across kernel configurations.

I couldn't check the kernel side, so this is based on the in-tree comment.

> +    }
> +
> +    return 0;
> +}

[ ... ]

> +    nworkers = MIN(s->conf.num_threads, hdev->nvqs);
> +    if (nworkers > 1) {

[Severity: Low]
If num-threads is 0, nworkers becomes 0 here, so both nworkers > 1 blocks
are skipped. Doesn't vhost_blk_start() then silently run a single kthread
with no diagnostic?

The old code passed num_threads to VHOST_SET_NWORKERS whenever it was not 1,
and failed with -EINVAL if that ioctl failed. vhost_blk_device_realize()
validates num_queues but not num_threads, and the num-threads property is a
DEFINE_PROP_UINT16 with a default of 1 and no range check.

Should num-threads=0 be rejected at realize time, or is the silent fallback
intended? The commit message doesn't mention this change in behaviour.

> +        /*
> +         * 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);

[Severity: Low]
On a released vz10 kernel that has VHOST_SET_FORK_FROM_OWNER but whose
vhost-blk lacks the worker ioctls, this call succeeds and switches the
device to vhost_task mode. Later, vhost_blk_set_workers() gets -ENOTTY from
vhost_new_worker() on vq 1, warns, and returns 0.

Does anything restore VHOST_FORK_OWNER_KTHREAD on that path? The fallback
then runs one thread in vhost_task mode, while num-threads=1 runs one thread
in kthread mode.

The commit message says "With num-threads=1 nothing changes: no worker
ioctls, kthread mode", but the fallback case isn't described as differing
from it. Is that difference intended?

I couldn't confirm from this tree whether task mode matters for vhost-blk on
those kernels.

> +        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) {

[ ... ]

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