On 7/1/21 4:39 AM, Michal Prívozník wrote: > 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. > IIRC it's a generic vhost-user requirement. Libvirt enforces shared memory for vhost-user-blk as well. And this blog post suggests it's a requirement for the older vhostuser net support too, though libvirt doesn't seem to enforce it: https://www.redhat.com/en/blog/hands-vhost-user-warm-welcome-dpdk >> >> 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. > Good point, we can use domcaps virtiofs reporting as a proxy for 'libvirt is new enough to allow bare access mode=shared'. Is it sufficient for qemu support? Are there qemu versions that support virtiofs but aren't new enough for libvirt's defaultRAMid determination? If we want to set this by default for new VMs I want to be as safe as possible here. If we want to start setting shared memory, possibly memfd, by default though we will still want the memoryBacking sourceType reporting as well. Thanks, Cole
