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

[Michal Prívozník]

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.

Michal