Hi Cole! Thanks for your review and suggestion. I admit that I'm not a CGroups expert. IHMO, when you allow a device (with virCgroupAllowDevice) for a specific CGroup that device would be automatically added into io_serviced monitor. But this is not happening. I tested with a stand alone container using Docker and LXC. Same results when you attach a block device. The best approach that I found was resetting throttle to include them into io_service of that specific LXC instance. It worked (and as it is an inclusion, reset does not affect block read/write stats). I'm free to accept suggestions of CGroups experts from here. I avoided to ask it on Kernel Cgroups mailing list. About aliases, there are two problems here: alias is not working for LXC (minor bug and irrelevant) and block path makes more sense to check io_serviced stats than alias. I will handle other points about your comment. Thanks again! -- Julio Cesar Faracco Em qui., 14 de nov. de 2019 às 18:34, Cole Robinson <crobinso@xxxxxxxxxx> escreveu: > > On 10/20/19 11:54 PM, jcfaracco@xxxxxxxxx wrote: > > From: Julio Faracco <jcfaracco@xxxxxxxxx> > > > > I think if you set your gitconfig correctly, you can avoid this 'From:' > showing up. I have: > > [sendemail] > from = "Cole Robinson <crobinso@xxxxxxxxxx>" > [user] > name = Cole Robinson > email = crobinso@xxxxxxxxxx > > > > > LXC was not working when you attached a new disk that points to block > > device. See: https://bugzilla.redhat.com/show_bug.cgi?id=1697115. > > Command line from virsh was showing problems with alias first (this > > feature is not supported) and after, problems to query block device. > > If you are fixing two distinct issues, this should at be at least two > separate patches. Otherwise review is more difficult among other things > > It > > was happening because extra disks were not being included into > > cgroup blkio.throttle properties and libvirt could not query this info. > > Applying those devices into 'allowed' list is not enough, libvirt should > > reset blkio.throttle as a workaround to include disks into container's > > cgroup directory. > > > > Before: > > > > virsh # domblkstat CentOS hda > > error: Failed to get block stats for domain 'CentOS' device 'hda' > > error: internal error: domain stats query failed > > > > Now: > > > > virsh # domblkstat CentOS hda > > hda rd_req 0 > > hda rd_bytes 0 > > hda wr_req 0 > > hda wr_bytes 0 > > > > Signed-off-by: Julio Faracco <jcfaracco@xxxxxxxxx> > > --- > > src/lxc/lxc_cgroup.c | 29 ++++++++++++++++++++++++++++- > > src/lxc/lxc_cgroup.h | 4 ++++ > > src/lxc/lxc_driver.c | 8 ++++---- > > 3 files changed, 36 insertions(+), 5 deletions(-) > > > > diff --git a/src/lxc/lxc_cgroup.c b/src/lxc/lxc_cgroup.c > > index 5efb495b56..de6d892521 100644 > > --- a/src/lxc/lxc_cgroup.c > > +++ b/src/lxc/lxc_cgroup.c > > @@ -302,6 +302,24 @@ virLXCTeardownHostUSBDeviceCgroup(virUSBDevicePtr dev G_GNUC_UNUSED, > > return 0; > > } > > > > +int > > +virLXCCgroupResetBlkioDeviceThrottle(virCgroupPtr cgroup, > > + const char *path) > > +{ > > + if (virCgroupSetBlkioDeviceReadBps(cgroup, path, 0) < 0) > > + return -1; > > + > > + if (virCgroupSetBlkioDeviceWriteBps(cgroup, path, 0) < 0) > > + return -1; > > + > > + if (virCgroupSetBlkioDeviceReadIops(cgroup, path, 0) < 0) > > + return -1; > > + > > + if (virCgroupSetBlkioDeviceWriteIops(cgroup, path, 0) < 0) > > + return -1; > > + > > + return 0; > > +} > > > > static int virLXCCgroupSetupDeviceACL(virDomainDefPtr def, > > virCgroupPtr cgroup) > > @@ -309,6 +327,7 @@ static int virLXCCgroupSetupDeviceACL(virDomainDefPtr def, > > int capMknod = def->caps_features[VIR_DOMAIN_CAPS_FEATURE_MKNOD]; > > int ret = -1; > > size_t i; > > + const char *src_path = NULL; > > static virLXCCgroupDevicePolicy devices[] = { > > {'c', LXC_DEV_MAJ_MEMORY, LXC_DEV_MIN_NULL}, > > {'c', LXC_DEV_MAJ_MEMORY, LXC_DEV_MIN_ZERO}, > > @@ -346,8 +365,16 @@ static int virLXCCgroupSetupDeviceACL(virDomainDefPtr def, > > !virStorageSourceIsBlockLocal(def->disks[i]->src)) > > continue; > > > > + /* Workaround to include disks into blkio.throttle. > > + * To include it, we need to reset any feature of > > + * blkio.throttle.* */ > > + src_path = virDomainDiskGetSource(def->disks[i]); > > + if (virLXCCgroupResetBlkioDeviceThrottle(cgroup, > > + src_path) < 0) > > + goto cleanup; > > + > > I'll admit I don't really know how cgroups work here. But it seems odd. > Why exactly is this needed? How does blkstats fail without this, after > the alias piece is fixed? Is something similar needed for the qemu > driver, and if not, why the difference? > > Also FWIW, not sure if you have fedora 31 installed anywhere, but seems > like the lxc driver is completely broken with cgroupv2: > > https://bugzilla.redhat.com/show_bug.cgi?id=1770763 > > The suggestion in the bug sounds simple though. If you wanted to > separately take a stab at that I'm motivated to test and review. Just a > thought > > > if (virCgroupAllowDevicePath(cgroup, > > - virDomainDiskGetSource(def->disks[i]), > > + src_path, > > (def->disks[i]->src->readonly ? > > VIR_CGROUP_DEVICE_READ : > > VIR_CGROUP_DEVICE_RW) | > > diff --git a/src/lxc/lxc_cgroup.h b/src/lxc/lxc_cgroup.h > > index 63e9e837b0..3d643a4fea 100644 > > --- a/src/lxc/lxc_cgroup.h > > +++ b/src/lxc/lxc_cgroup.h > > @@ -46,3 +46,7 @@ int > > virLXCTeardownHostUSBDeviceCgroup(virUSBDevicePtr dev, > > const char *path, > > void *opaque); > > + > > +int > > +virLXCCgroupResetBlkioDeviceThrottle(virCgroupPtr cgroup, > > + const char *path); > > diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c > > index 204a3ed522..87da55f308 100644 > > --- a/src/lxc/lxc_driver.c > > +++ b/src/lxc/lxc_driver.c > > @@ -2339,14 +2339,14 @@ lxcDomainBlockStats(virDomainPtr dom, > > goto endjob; > > } > > > > - if (!disk->info.alias) { > > + if (!disk->src->path) { > > virReportError(VIR_ERR_INTERNAL_ERROR, > > _("missing disk device alias name for %s"), disk->dst); > > goto endjob; > > } > > > > I think this whole conditional can be deleted. Earlier in the function > !path is already handled. > > > ret = virCgroupGetBlkioIoDeviceServiced(priv->cgroup, > > - disk->info.alias, > > + disk->src->path, > > &stats->rd_bytes, > > &stats->wr_bytes, > > &stats->rd_req, > > @@ -2424,14 +2424,14 @@ lxcDomainBlockStatsFlags(virDomainPtr dom, > > goto endjob; > > } > > > > - if (!disk->info.alias) { > > + if (!disk->src->path) { > > virReportError(VIR_ERR_INTERNAL_ERROR, > > _("missing disk device alias name for %s"), disk->dst); > > goto endjob; > > } > > > > Same with this block. The alias changes make sense though > > Thanks, > Cole > > > if (virCgroupGetBlkioIoDeviceServiced(priv->cgroup, > > - disk->info.alias, > > + disk->src->path, > > &rd_bytes, > > &wr_bytes, > > &rd_req, > > > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
