(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. 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 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> ... 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? 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. Thanks, Cole
