[libvirt] [PATCHv3 13/19] storage: use cache to walk backing chain
Laine Stump
laine at laine.org
Thu Oct 18 18:42:13 UTC 2012
On 10/17/2012 06:30 PM, Eric Blake wrote:
> We used to walk the backing file chain at least twice per disk,
> once to set up cgroup device whitelisting, and once to set up
> security labeling. Rather than walk the chain every iteration,
> which possibly includes calls to fork() in order to open root-squashed
> NFS files, we can exploit the cache of the previous patch.
>
> * src/conf/domain_conf.h (virDomainDiskDefForeachPath): Alter
> signature.
> * src/conf/domain_conf.c (virDomainDiskDefForeachPath): Require caller
> to supply backing chain via disk, if recursion is desired.
> * src/security/security_dac.c
> (virSecurityDACSetSecurityImageLabel): Adjust caller.
> * src/security/security_selinux.c
> (virSecuritySELinuxSetSecurityImageLabel): Likewise.
> * src/security/virt-aa-helper.c (get_files): Likewise.
> * src/qemu/qemu_cgroup.c (qemuSetupDiskCgroup)
> (qemuTeardownDiskCgroup): Likewise.
> (qemuSetupCgroup): Pre-populate chain.
> ---
>
> v3: rebase to earlier changes, fix comment
including accounting for the movement of check for VIR_STORAGE_FILE_AUTO
infor virStorageFileGetMetaData()
>
> src/conf/domain_conf.c | 100 +++++++---------------------------------
> src/conf/domain_conf.h | 2 -
> src/qemu/qemu_cgroup.c | 12 ++---
> src/security/security_dac.c | 7 ---
> src/security/security_selinux.c | 11 -----
> src/security/virt-aa-helper.c | 20 ++++----
> 6 files changed, 32 insertions(+), 120 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 6487e6b..afa4cfe 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -14767,110 +14767,42 @@ done:
> }
>
>
> +/* Call iter(disk, name, depth, opaque) for each element of disk and
> + * its backing chain in the pre-populated disk->backingChain.
> + * ignoreOpenFailure determines whether to warn about a chain that
> + * mentions a backing file without also having metadata on that
> + * file. */
> int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk,
> - bool allowProbing,
> bool ignoreOpenFailure,
> - uid_t uid, gid_t gid,
> virDomainDiskDefPathIterator iter,
> void *opaque)
> {
> - virHashTablePtr paths = NULL;
> - int format;
> int ret = -1;
> size_t depth = 0;
> - char *nextpath = NULL;
> - virStorageFileMetadata *meta = NULL;
> + virStorageFileMetadata *tmp;
>
> if (!disk->src || disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK)
> return 0;
>
> - if (disk->format > 0) {
> - format = disk->format;
> - } else {
> - if (allowProbing) {
> - format = VIR_STORAGE_FILE_AUTO;
> - } else {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("no disk format for %s and probing is disabled"),
> - disk->src);
> - goto cleanup;
> - }
> - }
> -
> - paths = virHashCreate(5, NULL);
> -
> - do {
> - const char *path = nextpath ? nextpath : disk->src;
> - int fd;
> -
> - if (iter(disk, path, depth, opaque) < 0)
> - goto cleanup;
> + if (iter(disk, disk->src, 0, opaque) < 0)
> + goto cleanup;
>
> - if (virHashLookup(paths, path)) {
> + tmp = disk->backingChain;
> + while (tmp && tmp->backingStoreIsFile) {
> + if (!ignoreOpenFailure && !tmp->backingMeta) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("backing store for %s is self-referential"),
> - disk->src);
> + _("unable to visit backing chain file %s"),
> + tmp->backingStore);
> goto cleanup;
> }
> -
> - if ((fd = virFileOpenAs(path, O_RDONLY, 0, uid, gid, 0)) < 0) {
> - if (ignoreOpenFailure) {
> - char ebuf[1024];
> - VIR_WARN("Ignoring open failure on %s: %s", path,
> - virStrerror(-fd, ebuf, sizeof(ebuf)));
> - break;
> - } else {
> - virReportSystemError(-fd, _("unable to open disk path %s"),
> - path);
> - goto cleanup;
> - }
> - }
> -
> - if (!(meta = virStorageFileGetMetadataFromFD(path, fd, format))) {
> - VIR_FORCE_CLOSE(fd);
> - goto cleanup;
> - }
> -
> - if (VIR_CLOSE(fd) < 0)
> - virReportSystemError(errno,
> - _("could not close file %s"),
> - path);
> -
> - if (virHashAddEntry(paths, path, (void*)0x1) < 0)
> + if (iter(disk, tmp->backingStore, ++depth, opaque) < 0)
> goto cleanup;
> -
> - depth++;
> - VIR_FREE(nextpath);
> - nextpath = meta->backingStore;
> - meta->backingStore = NULL;
> -
> - /* Stop iterating if we reach a non-file backing store */
> - if (nextpath && !meta->backingStoreIsFile) {
> - VIR_DEBUG("Stopping iteration on non-file backing store: %s",
> - nextpath);
> - break;
> - }
> -
> - format = meta->backingStoreFormat;
> -
> - if (format == VIR_STORAGE_FILE_AUTO &&
> - !allowProbing)
> - format = VIR_STORAGE_FILE_RAW; /* Stops further recursion */
> -
> - /* Allow probing for image formats that are safe */
> - if (format == VIR_STORAGE_FILE_AUTO_SAFE)
> - format = VIR_STORAGE_FILE_AUTO;
> - virStorageFileFreeMetadata(meta);
> - meta = NULL;
> - } while (nextpath);
> + tmp = tmp->backingMeta;
> + }
>
> ret = 0;
>
> cleanup:
> - virHashFree(paths);
> - VIR_FREE(nextpath);
> - virStorageFileFreeMetadata(meta);
> -
> return ret;
> }
>
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index e7f0e9c..10ef841 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2141,9 +2141,7 @@ typedef int (*virDomainDiskDefPathIterator)(virDomainDiskDefPtr disk,
> void *opaque);
>
> int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk,
> - bool allowProbing,
> bool ignoreOpenFailure,
> - uid_t uid, gid_t gid,
> virDomainDiskDefPathIterator iter,
> void *opaque);
>
> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
> index 6b94686..428befd 100644
> --- a/src/qemu/qemu_cgroup.c
> +++ b/src/qemu/qemu_cgroup.c
> @@ -87,16 +87,14 @@ qemuSetupDiskPathAllow(virDomainDiskDefPtr disk,
> }
>
>
> -int qemuSetupDiskCgroup(struct qemud_driver *driver,
> +int qemuSetupDiskCgroup(struct qemud_driver *driver ATTRIBUTE_UNUSED,
> virDomainObjPtr vm,
> virCgroupPtr cgroup,
> virDomainDiskDefPtr disk)
> {
> qemuCgroupData data = { vm, cgroup };
> return virDomainDiskDefForeachPath(disk,
> - driver->allowDiskFormatProbing,
> true,
> - driver->user, driver->group,
> qemuSetupDiskPathAllow,
> &data);
> }
> @@ -129,16 +127,14 @@ qemuTeardownDiskPathDeny(virDomainDiskDefPtr disk ATTRIBUTE_UNUSED,
> }
>
>
> -int qemuTeardownDiskCgroup(struct qemud_driver *driver,
> +int qemuTeardownDiskCgroup(struct qemud_driver *driver ATTRIBUTE_UNUSED,
> virDomainObjPtr vm,
> virCgroupPtr cgroup,
> virDomainDiskDefPtr disk)
> {
> qemuCgroupData data = { vm, cgroup };
> return virDomainDiskDefForeachPath(disk,
> - driver->allowDiskFormatProbing,
> true,
> - driver->user, driver->group,
> qemuTeardownDiskPathDeny,
> &data);
> }
> @@ -232,7 +228,9 @@ int qemuSetupCgroup(struct qemud_driver *driver,
> }
>
> for (i = 0; i < vm->def->ndisks ; i++) {
> - if (qemuSetupDiskCgroup(driver, vm, cgroup, vm->def->disks[i]) < 0)
> + if (qemuDomainDetermineDiskChain(driver, vm->def->disks[i],
> + false) < 0 ||
> + qemuSetupDiskCgroup(driver, vm, cgroup, vm->def->disks[i]) < 0)
> goto cleanup;
> }
>
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index 9dbf95d..a67f5d6 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -358,8 +358,6 @@ virSecurityDACSetSecurityImageLabel(virSecurityManagerPtr mgr,
> virDomainDiskDefPtr disk)
>
> {
> - uid_t user;
> - gid_t group;
> void *params[2];
> virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
>
> @@ -369,15 +367,10 @@ virSecurityDACSetSecurityImageLabel(virSecurityManagerPtr mgr,
> if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK)
> return 0;
>
> - if (virSecurityDACGetImageIds(def, priv, &user, &group))
> - return -1;
> -
> params[0] = mgr;
> params[1] = def;
> return virDomainDiskDefForeachPath(disk,
> - virSecurityManagerGetAllowDiskFormatProbing(mgr),
> false,
> - user, group,
> virSecurityDACSetSecurityFileLabel,
> params);
> }
> diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
> index b5108ab..eee8d71 100644
> --- a/src/security/security_selinux.c
> +++ b/src/security/security_selinux.c
> @@ -1062,13 +1062,10 @@ virSecuritySELinuxSetSecurityImageLabel(virSecurityManagerPtr mgr,
> virDomainDiskDefPtr disk)
>
> {
> - bool allowDiskFormatProbing;
> virSecuritySELinuxCallbackData cbdata;
> cbdata.manager = mgr;
> cbdata.secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME);
>
> - allowDiskFormatProbing = virSecurityManagerGetAllowDiskFormatProbing(mgr);
> -
> if (cbdata.secdef == NULL)
> return -1;
>
> @@ -1078,16 +1075,8 @@ virSecuritySELinuxSetSecurityImageLabel(virSecurityManagerPtr mgr,
> if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK)
> return 0;
>
> - /* XXX On one hand, it would be nice to have the driver's uid:gid
> - * here so we could retry opens with it. On the other hand, it
> - * probably doesn't matter because in practice that's only useful
> - * for files on root-squashed NFS shares, and NFS doesn't properly
> - * support selinux anyway.
> - */
> return virDomainDiskDefForeachPath(disk,
> - allowDiskFormatProbing,
> true,
> - -1, -1, /* current process uid:gid */
> virSecuritySELinuxSetSecurityFileLabel,
> &cbdata);
> }
> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
> index 51daa4b..263fc92 100644
> --- a/src/security/virt-aa-helper.c
> +++ b/src/security/virt-aa-helper.c
> @@ -919,20 +919,22 @@ get_files(vahControl * ctl)
> }
>
> for (i = 0; i < ctl->def->ndisks; i++) {
> + virDomainDiskDefPtr disk = ctl->def->disks[i];
> +
> + /* XXX - if we knew the qemu user:group here we could send it in
> + * so that the open could be re-tried as that user:group.
> + */
> + disk->chain = virStorageFileGetMetadata(disk->src, disk->format,
> + -1, -1,
> + ctl->allowDiskFormatProbing,
> + NULL);
> +
> /* XXX passing ignoreOpenFailure = true to get back to the behavior
> * from before using virDomainDiskDefForeachPath. actually we should
> * be passing ignoreOpenFailure = false and handle open errors more
> * careful than just ignoring them.
> - * XXX2 - if we knew the qemu user:group here we could send it in
> - * so that the open could be re-tried as that user:group.
> */
> - int ret = virDomainDiskDefForeachPath(ctl->def->disks[i],
> - ctl->allowDiskFormatProbing,
> - true,
> - -1, -1, /* current uid:gid */
> - add_file_path,
> - &buf);
> - if (ret != 0)
> + if (virDomainDiskDefForeachPath(disk, true, add_file_path, &buf) < 0)
> goto clean;
> }
>
ACK.
More information about the libvir-list
mailing list