[virt-tools-list] [virt-manager 0/8] filesystem: Add support for virtiofs

[Cole Robinson]

On 7/29/21 6:37 AM, lma wrote:
> 在 2021-07-29 02:46，Cole Robinson 写道：
>> On 7/15/21 9:17 AM, Michal Prívozník wrote:
>>> On 7/7/21 3:31 PM, Cole Robinson wrote:
>>>> On 7/7/21 4:50 AM, Michal Prívozník wrote:
>>>>> On 7/5/21 5:20 AM, lma wrote:
>>>>>> 在 2021-07-01 08:39，Michal Prívozník 写道：
>>>>>>> On 7/1/21 12:52 AM, Cole Robinson wrote:
>>>>>>>> (ccing mprivozn with a domaincapabilities design question below)
>>>
>>> <snip/>
>>>
>>>>
>>>> IMO just source type= is enough.
>>>>
>>>
>>> FYI, I just merged patches that implement this in libvirt:
>>>
>>> 133d05a15e qemu: capabilities: fill in domcaps <memoryBacking>
>>> e27e22585a conf: domcaps: Report <memoryBacking>
>>>
>>> # virsh domcapabilities | xmllint --xpath
>>> /domainCapabilities/memoryBacking -
>>>   <memoryBacking supported="yes">
>>>     <enum name="sourceType">
>>>       <value>file</value>
>>>       <value>anonymous</value>
>>>       <value>memfd</value>
>>>     </enum>
>>>   </memoryBacking>
>>>
>>>
>>
>> Thanks Michal!
>>
>> We had some internal discussion about using memfd by default, but
>> there's some caveats: memfd implies shared memory, and shared memory
>> does things like prevent KSM from working, add some hiccups to memory
>> ballooning, and causes VM to appear to own all its memory after
>> save/restore. So I'm not sure we should set this as the default any time
>> soon.
>>
>> So this puts us back at the beginning. Do we make addhardware try to
>> transparently set up shared memory when virtiofs is requested? That
>> still makes me leery TBH.
> 
> IMO transparently setting up shared memory in this case is not a good idea.
> 
>> I'm thinking we should add a UI checkbox in the memory screen for
>> enabling/disabling shared memory. This way the logic is testable for an
>> existing VM outside of the addhardware wizard. If our logic is incorrect
>> and screws up their VM like making it unbootable, there's an explicit
>> way to back out that change. We should prefer memfd over file shm memory
>> if memfd is available in domcaps.
>>
>> Then when adding virtiofs device in addhardware, we try to make the
>> default behavior work, but let the user opt out. So in the standard
>> case, where VM has no shared memory config, when selecting virtio-fs, we
>> show a new checkbox 'Enable shared memory for the whole VM' with an info
>> icon + tooltip explaining this is necessary for virtio-fs. We still show
>> it even when the host already has shared memory configured, but disable
>> the checkbox and add a tooltip saying 'VM is already configured with
>> shared memory'. I think that should be enough to make this safe to
>> maintain, and more informative to the user about what is happening
> 
> Based on the above valuable information, What I intend to do are:
> - If virtiofs is available in domcaps: Add a UI checkbox in the memory
> screen
>   of details for enabling/disabling shared memory, meanwhile set 'memfd' as
>   backend if memfd is available in domcaps, otherwise set 'file' as
> backend.
> 

Sounds good

> - This UI checkbox will be invisible if virtiofs is unavailable in domcaps.
> 

I prefer not to hide things, that can confuse users wondering why they
see the UI on one setup but not the other. So for this case, show the
checkbox, but disable the entire thing and add a tooltip explaining
'Libvirt may not be new enough to support shared memory'

> - This UI checkbox will be disabled in the memory screen if existing
> virtio-fs
>   device(s) are detected in VM config.

IMO we don't need to go that far. If the user disables the checkbox,
their VM won't start on next boot, which IMO is enough to inform users
'don't do that'. And it's easy enough for them to undo that change. This
will save us some code and reduce the test matrix.

> 
> - Don't add such UI checkbox in new VM wizard.
> 

Agreed.

> - Add a checkbox "Enable shared memory for the whole VM" with an info
> icon +
>   tooltip when selecting virtio-fs in the addhardware wizard or in the
> details.
> 

Sounds good

> - Still show this checkbox even when the VM already has shared memory
> configured,
>   but disable the checkbox and add a tooltip saying 'VM is already
> configured with shared memory'
> 

Yes I think so, same principal I mentioned above for not hiding the
checkbox.

> BTW, About the "Are there qemu versions that support virtiofs but aren't
> new
> enough for libvirt's defaultRAMid determination?":
> We know that qemu exposes default_ram_id through 'query-machines' since
> v5.2.
> Libvirt uses memory-backend-* for regular VM memory if default RAM ID is
> detected.
> If the default RAM ID isn't detected, Libvirt falls back to the
> deprecated -mem-path,
> (Unfortunately, default RAM ID isn't exposed in domcapabilities)
> So it's harmless setting shared memory via the above checkbox in VM memory
> screen even if the underlying qemu is v5.0 or v5.1.
> 

Yup sounds good.

I suggest to do the memory details checkbox first, since that can go in
on its own, and we can make sure it behaves correctly and the behavior
is tested. Then the virtiofs addhardware code is added and uses the
already tested pieces.

> Thanks Michal!
> Thanks Cole!
> 

Thank you as well!

FYI I will be disappearing on paternity leave within the next few days
and I may be offline for up to 8 weeks. Depending on down time I may pop
in to catch up on reviews but no guarantees, sorry.

Thanks,
Cole