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

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] vhost-blk: num-threads=0 silently accepted as one thread, no validation
--

Commit b390c3229288ae4b409cac07cdeba78f79d9b1e7
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 in vhost_blk_start() with
VHOST_SET_FORK_FROM_OWNER, VHOST_NEW_WORKER and VHOST_ATTACH_VRING_WORKER.
It creates num-threads - 1 extra workers and attaches them to the virtqueues
round-robin. 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

[ ... ]

> @@ -51,15 +97,37 @@ static int vhost_blk_start(VirtIODevice *vdev)
>          return 0;
>      }
>  
> +    nworkers = MIN(s->conf.num_threads, s->dev.nvqs);
> +    if (nworkers > 1) {

[Severity: Low]
This isn't a crash, but the new code changes how num-threads=0 and values
above the queue count are handled. Is that intended?

Before this patch, vhost_blk_start() passed nworkers to VHOST_SET_NWORKERS
whenever it was not 1. A value of 0 was therefore passed to the kernel,
and as far as I can tell start failed with "unable to set number of kernel
threads". I could not check how the kernel handled 0.

Now nworkers = MIN(num_threads, nvqs) evaluates to 0 for num-threads=0, so
"nworkers > 1" is false. The device starts silently with a single kernel
thread, as if num-threads=1 had been given.

The same happens when num-threads is larger than the number of queues. The
count is clamped to nvqs and no diagnostic is printed.

The num-threads property is DEFINE_PROP_UINT16 with a default of 1, and
vhost_blk_device_realize() only validates num_queues. Nothing checks
num_threads there either.

Should realize reject num-threads=0, and should it warn or fail when
num-threads exceeds num-queues?

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