[pve-devel] [PATCH storage 1/2] fix #5779: rbd: allow to pass custom krbd map options

[Thomas Lamprecht]

Am 19.12.24 um 09:05 schrieb Friedrich Weber:
> On 30/10/2024 17:49, Friedrich Weber wrote:
>> [...]
>>
>> Yeah, I see the point.
>>
>> Of course, another alternative is enabling `rxbounce` unconditionally,
>> as initially requested in [1]. I'm a hesitant to do that because from
>> reading its description I'd expect it could have a performance impact --
>> it's probably small, if any, but this should probably be checked before
>> changing the default.
>>
> 
> I took another look at this: When rxbounce was first introduced, there
> was a discussion whether krbd could enabled automatically switch to
> "rxbounce mode" if needed [0]. I asked upstream whether this seems
> realistic [1], and they responded it's very unlikely to happen.
> 
> So the cleanest solution from a user point of view might be if PVE
> automatically passes rxbounce only when mapping disks of Windows VMs.
> But as Fabian points out [2], this would require some way to pass this
> information to the storage layer.

One option here might be to add a small base module in pve-storage for
guest hints that then qemu-server and potentially pve-container can
implement and pass along as instance through a new parameter to
respective calls to the storage layer.
Or, instead of a whole module and all that just an optional callback,
that would be quite similar just less boilerplate.

Parameters and how hints are returned would still need some careful
thought out design to avoid adding some annoying coupling here.
>From top of my head the hints could be constants provided by each
pve-storage plugin and the callback would then receive the available
hints as hash plus potentially some other info, like the storage type
ore (some) storage options' values. As then the callback can check if
it even makes sense to return some hint and one avoids the need for
bumping version dependencies constantly (if this even gets used that
much).

> Of course always passing rxbounce is still an option. Upstream confirmed
> there is a theoretical performance impact, but it might be negligible in
> practice [0]. So if benchmarks show the impact is indeed negligible, we
> could go for that route.
> 
> But even with benchmarks done carefully, there is a chance that they
> fail to catch a performance impact on some types of workloads. So in
> order to not disturb setups that currently work fine without rxbounce, I
> have a slight preference for only passing rxbounce when needed, even if
> that requires some architectural changes.

I agree with your sentiment. IMO if it would be OK to enable in general
then our work should probably be to make upstream enable it by default.