Re: [libvirt] [PATCH] virfile: relax checks for hugetlbfs

On 12/04/2017 04:21 PM, David Vrabel wrote: > There are use cases where it is useful to use the support in libvirt > for file-backed guest memory, but without using hugetlbfs but tmpfs > instead (for example, to run tests on hosts that do not have hugepages > configured, or to use Linux's idle page tracking to monitor guest > memory accesses at a 4k page granularity.). > > Drop the check for hugetlbfs when querying the huge page size, but > move it to the loop that's searching for a suitable mount to use. > > Change-Id: I2c9589191e14653724725b944171689553ee6bae > --- > src/util/virfile.c | 21 ++++++++++++++------- > 1 file changed, 14 insertions(+), 7 deletions(-) >> diff --git a/src/util/virfile.c b/src/util/virfile.c > index 82cb36dbc..24ff5e208 100644 > --- a/src/util/virfile.c > +++ b/src/util/virfile.c > @@ -3438,19 +3438,23 @@ virFileGetHugepageSize(const char *path, > goto cleanup; > } > > - if (fs.f_type != HUGETLBFS_MAGIC) { > - virReportError(VIR_ERR_INTERNAL_ERROR, > - _("not a hugetlbfs mount: '%s'"), > - path); > - goto cleanup; > - } > - > *size = fs.f_bsize / 1024; /* we are storing size in KiB */ > ret = 0; > cleanup: > return ret; > } > > +static bool > +virFileIsHugeTLBFS(const char *path) > +{ > + struct statfs fs; > + > + if (statfs(path, &fs) < 0) { > + return false; > + } Note that we don't put curly braces around one line bodies. > + return fs.f_type == HUGETLBFS_MAGIC; > +} > + > # define PROC_MEMINFO "/proc/meminfo" > # define HUGEPAGESIZE_STR "Hugepagesize:" > > @@ -3517,6 +3521,9 @@ virFileFindHugeTLBFS(virHugeTLBFSPtr *ret_fs, > if (STRNEQ(mb.mnt_type, "hugetlbfs")) > continue; > > + if (!virFileIsHugeTLBFS(mb.mnt_dir)) > + continue; > + > if (VIR_EXPAND_N(fs, nfs, 1) < 0) > goto cleanup; > > Frankly, I'm not a fan of this patch. Currently, when people have <hugepages/> in their domain XML we guarantee that hugepages are used to back the guest memory. With this patch users might be mislead as it enables *any* other filesystem. All that's needed is to put mount points into qemu.conf. I know you want it for testing, but is it an upstream material? IOW, I'm not nacking this, I just want some conversation to happen first.