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

[Cole Robinson]

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)
>>>>
>>>> On 6/30/21 8:10 AM, Lin Ma wrote:
>>>>> So far, virt-manager only supports virtio-9p, The patchset adds
>>>>> virtiofs
>>>>> which offering better performance.
>>>>>
>>>>> We know that the virtiofs needs 'shared' access mode of memory backing
>>>>> or 'shared' access mode of virtual numa node, But virt-manager doesn't
>>>>> provide UI to configure memory backing or virtual numa node because
>>>>> they
>>>>> are advanced features and can be configured by raw XML editor.
>>>>>
>>>>> This patchset introduces basic virtiofs support and offers an easier
>>>>> way
>>>>> to configure virtiofs by adjusting access mode to 'shared' if
>>>>> necessary.
>>>>>
>>>>> I don't intend to introduce memory backing UI or numa UI, That means I
>>>>> need to modify the access mode attribue which belongs memorybacking or
>>>>> numa in filesystem code, This perhaps looks not good, Any comments are
>>>>> appreciated.
>>>>>
>>>>
>>>> Thanks for the patches. Regarding virtio-fs I've recorded my thoughts in
>>>> this issue: https://github.com/virt-manager/virt-manager/issues/127
>>>>
>>>> Basically I don't want to add this to virt-manager until we can make it
>>>> closer to 'just work' without pitfalls. IMO that means adjusting libvirt
>>>> to report via domcapabilities when it is safe and supported to
>>>> unconditionally specify shared memory, without hugepages or numa config.
>>>> Then we set that by default for new VMs, and _maybe_ do something like
>>>> what your patches do (set it automatically when user requests virtiofs
>>>> via addhw).
>>>>
>>>> Until that's done, it's a pain in the ass to try and figure out, outside
>>>> of libvirt, whether the domain XML has suitable setup to make virtio-fs
>>>> work, and what is the simplest memory XML adjustment to make virtiofs
>>>> work. We basically have to reimplement the libvirt
>>>> qemuValidateDomainDefVhostUserRequireSharedMemory function from here
>>>> https://gitlab.com/libvirt/libvirt/-/blob/master/src/qemu/qemu_validate.c#L1427
>>>>
>>>>
>>>> Your code attempts to implement the numa_nodes check, but it doesn't
>>>> account for the defaultRAMID bit.
>>>
>>> Right. IIRC the shared memory is needed for DAX. I wonder if there's a
>>> way to turn off DAX in virtiofsd. Then the <filesystem/> could be added
>>> just like any other device.
>>
>> Because vhost-user needs shared memory, virtio-fs can't be enabled in
>> isolation.
>>
>>>>
>>>> The specific <memoryBacking><access mode='shared></memoryBacking> config
>>>> is only accepted on libvirt 7.0.0+ AFAICT:
>>>> https://gitlab.com/libvirt/libvirt/-/commit/bff2ad5d6b1f25da02802273934d2a519159fec7
>>>>
>>>>
>>>> And even then we probably want libvirt 7.1.0 at least before we set it
>>>> unconditionally for new VMs:
>>>> https://gitlab.com/libvirt/libvirt/-/commit/677c90cc1d1fcb3aba09b5d4f0f8f83099911775
>>>>
>>>>
>>>
>>> This could be avoided if domcapabilities were checked for before adding
>>> virtiofsd. I mean, support for virtiofsd was added in 6.2.0; later, some
>>> requirements were refined (e.g. NUMA nodes no longer needed in
>>> v6.9.0-rc1~161). yada yada yada and only recently (v7.4.0-rc1~117)
>>> virtiofs is announced in domcapabilities.
>>>
>>>> So if you want to help move this forward in a sustainable way, please
>>>> look into extending libvirt domcapabilities. One related bit would be
>>>> reporting valid memory source type values, so that we know if memfd is
>>>> an option (it can be compiled out of qemu). We may prefer to use that
>>>> over type='file' memory, if it simplifies things. I think the schema
>>>> would be:
>>>>
>>>> <domainCapabilities>
>>>>   <memoryBacking supported='yes'>
>>>>     <enum name='sourceType'>
>>>>       <value>file</value>
>>>>       <value>memfd</value>
>>>>       ...
>>>>
>>>
>>> Yes, this looks sane and could be valuable for other use cases too.
>>>
>>>> The 7.1.0 check, when access mode=shared can be used without numa or
>>>> hugepages, we probably need some arbitrary boolean to report. It
>>>> could be:
>>>>
>>>> <domainCapabilities>
>>>>   <memoryBacking>
>>>>     <bareAccessMode supported='yes'>
>>>>
>>>> Or maybe something under <features>. There isn't a clear precedent for
>>>> exposing something like this in the XML. CCing mprivozn, any
>>>> suggestions?
>>>
>>> I think we can rely on <filesystem/> from domcaps AND newly added
>>> <memoryBacking/> as described above. Yes, this will leave behind some
>>> versions where virtiofs would work and yet virt-manager won't be able to
>>> configure it, but I think that's acceptable.
>>>
>>>>
>>>> Lin if you get those into libvirt I will be happy to help you land
>>>> virtio-fs support in virt-manager, writing code coverage tests etc.
>>>
>>> And I can help with domcaps, let me know if you want to post patches
>>> yourself or whether I should do it.
>>
>> You know much about domcaps, I lean towards to you, So, please.
> 
> Will do. But just do double check: we need domcaps to report:
> 
>   <memoryBacking>
>     <source type="file|anonymous|memfd"/>
>   </memoryBacking>
> 
> Do we need something else too? I mean, for virtiofs we will need to set
> <access mode="shared"/> in the domain XML, but that doesn't seem to be
> conditional. I mean, nothing in libvirt checks whether qemu/kernel
> supports "shared" or not.
> 

IMO just source type= is enough.

Thanks,
Cole