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
