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
