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