[pve-devel] [RFC qemu-server/storage 0/3] fix #5779: introduce guest hints to pass rxbounce flag to KRBD

[Fiona Ebner]

Am 24.10.25 um 2:27 PM schrieb Friedrich Weber:
> - The biggest obstacle is that we need to update all callers of
>   activate_volumes to pass guest hints where possible. This means that optimally
>   all callers should be able to generate the guest hints. Right now, there is
>   only the 'guest-ostype' hint which is taken from the VM config. Currently, this
>   is not always available to the caller of activate_volumes, sometimes
>   some extra work/refactoring would be needed to get it (e.g. see
>   PVE::QemuServer::QemuImage::convert or PVE::QemuServer::clone_disk),
>   so this needs quite some code changes, which I have not done in all cases in
>   this RFC.

See below.

> - There are also some indirect callers of activate_volumes, e.g. via
>   PVE::Storage::abs_filesystem_path or PVE::Storage::storage_migrate -- these
>   would also need to be extended to accept hints (not done in this RFC)

See below.

> - Initially, to avoid having to modify all (direct+indirect) callers of
>   activate_volumes, I thought I could pass the hints only at the few "relevant"
>   call sites (i.e., when starting a VM), but then noticed that volumes may be
>   activated by an action unrelated to a VM start (e.g. a clone), then stay
>   active, and not be re-activated by a VM start. So if e.g. we do not pass the
>   hints on clone, the KRBD volume would be mapped without rxbounce, stay active,
>   and when starting the VM, a user could run into the original problem again.
>   So we can't get away with only passing hints to the few relevant call sites,
>   and actually need to pass them everywhere (where possible).

See other mail.

> - But sometimes, it might be fine to not pass hints to activate_volumes because
>   we immediately deactivate, e.g. in QemuServer::restore_vma_archive, but only
>   because the only current hint 'guest-ostype' is not relevant in such cases -
>   might not be the case for future hints?

Ideally, if we go with the current approach, we would cover everything
so that we'd only need to adapt the generate_storage_hints() function.

But something completely different which comes to mind here. What about
the volumes that already are active immediately after allocation? For
those, we either need to:
1. deactivate them and reactivate them with hints after allocation
2. or pass along the hints during allocation
3. ensure that they are not used in situations where hints are required
before deactivation.

The alternative approach of "make sure to deactivate before using hints
in situations where actually required" would avoid that complication too
(with 1. being very similar but here it would be immediately after
allocation).

> - 'guest-ostype' might be the only hint we need for now and can be taken from
>   the guest config, but can we be sure that future hints are also going to be
>   derived from the guest config? Is it OK to have generate_storage_hints only
>   take the guest config as well?

This can easily be changed later when the need arises.

> - minor, but still: with this approach, rxbounce would also be passed for the
>   EFI disk and TPM state of Windows VMs. This is likely not needed, but probably
>   OK for a first version?

Can be okay, but see below.

> - should we already pass hints when activating volumes in pve-container too,
>   even though they are empty?

Not strictly required IMHO, but feel free to if you like.

> - is it even OK to pass the hints only to {activate,map}_volumes? What if an
>   external storage plugin needs the hint also when deactivating a volume? Should
>   we already add a $hints parameter to other plugin methods as well, to avoid
>   having to do that (and another APIVER bump) later?

I'd prefer to add it to deactivate too.

> - With hints being an argument to activate_volumes which takes a list of volumes,
>   the hint applies to all volumes in the list. Is this OK API-wise? For the
>   guest-ostype hint, this only makes sense if all passed volumes belong to the
>   same guest and not several different ones (because they may have different OS
>   types), which I think is the case for our call sites, but doesn't have to be
>   the case?

I'd prefer it to be per-volume, so that we can leave out TPM and EFI
disks and things like the $statefile when that is a volume. Can be done
by the current interface by making multiple calls of course, but that is
not ideal. Maybe one of the future hints will be precisely for TPM or
EFI, those seem like likely candidates for requiring special treatment.

We could deprecate activate_volumes() and either:

1. add a new function which takes a hash reference rather than an array
reference. The keys would be volid with values being additional options,
like snapshot name and hints.

2. Or we could have a mini-class PVE::Storage::Activate which would have
a method to activate a single volume, but keeps track of which storages
and volumes it already activated in its internal state and which could
then also have a method for deactivating what it activated (like is
needed after VM start failure).

But please note that this is independent of the plugin API and does not
need to be done at the same time as the changes there (AFAICS).