[libvirt] [PATCH 3/5] qemu: migration: Skip cache=none check for disks which are storage-migrated

[John Ferlan]

On 04/07/2017 11:50 AM, Peter Krempa wrote:
> Since the disks are copied by qemu, there's no need to enforce
> cache=none. Thankfully the code that added qemuMigrateDisk did not break
> existing configs, since if you don't select any disk to migrate
> explicitly the code behaves sanely.
> 
> The logic for determining whether a disk should be migrated is
> open-coded since using qemuMigrateDisk twice would be semantically
> incorrect.
> ---
>  src/qemu/qemu_migration.c | 57 ++++++++++++++++++++++++++++-------------------
>  1 file changed, 34 insertions(+), 23 deletions(-)
> 

Thanks for altering the multiple non-negative checks logic to the easier
to read positive log...

Still it feels like there's "two" things going on here w/ that
storagemigration bool though.

> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index d8222fe3b..5bd45137c 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -1126,9 +1126,14 @@ qemuMigrationIsAllowed(virQEMUDriverPtr driver,
>  static bool
>  qemuMigrationIsSafe(virDomainDefPtr def,
>                      size_t nmigrate_disks,
> -                    const char **migrate_disks)
> +                    const char **migrate_disks,
> +                    unsigned int flags)
> +
>  {
> +    bool storagemigration = flags & (VIR_MIGRATE_NON_SHARED_DISK |
> +                                     VIR_MIGRATE_NON_SHARED_INC);
>      size_t i;
> +    int rc;
> 
>      for (i = 0; i < def->ndisks; i++) {
>          virDomainDiskDefPtr disk = def->disks[i];
> @@ -1136,29 +1141,35 @@ qemuMigrationIsSafe(virDomainDefPtr def,
> 
>          /* Our code elsewhere guarantees shared disks are either readonly (in
>           * which case cache mode doesn't matter) or used with cache=none */
> -        if (qemuMigrateDisk(disk, nmigrate_disks, migrate_disks) &&
> -            disk->cachemode != VIR_DOMAIN_DISK_CACHE_DISABLE) {
> -            int rc;
> +        if (virStorageSourceIsEmpty(disk->src) ||
> +            disk->src->readonly ||
> +            disk->src->shared ||
> +            disk->cachemode == VIR_DOMAIN_DISK_CACHE_DISABLE)

So this check is being done first to avoid the chance that a disk that
was defined in the domain, but wasn't in some supplied migrate_disks
doesn't erroneously cause a failure because qemuMigrateDisk would never
do that last check which after patch 4 does the Empty, Read, Shared check.

> +            continue;
> 
> -            if (virDomainDiskGetType(disk) == VIR_STORAGE_TYPE_FILE) {
> -                if ((rc = virFileIsSharedFS(src)) < 0)
> -                    return false;
> -                else if (rc == 0)
> -                    continue;
> -                if ((rc = virStorageFileIsClusterFS(src)) < 0)
> -                    return false;
> -                else if (rc == 1)
> -                    continue;
> -            } else if (disk->src->type == VIR_STORAGE_TYPE_NETWORK &&
> -                       disk->src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD) {
> -                continue;
> -            }
> +        /* disks which are migrated by qemu are safe too */
> +        if (storagemigration &&

This is the part that seems to be 'extra' and not related to the commit
message. IIUC the flags in question aren't necessarily supplied - so if
they weren't supplied the subsequent check won't be made? Is that what
is desired?  Perhaps I'm missing something subtle or that's a "given"
with those flags.

> +            qemuMigrateDisk(disk, nmigrate_disks, migrate_disks))
> +            continue;

Also previously, if the qemuMigrateDisk was true (and cache != DISABLE),
then the SharedFS/ClusterFS and RBD checks would be made. With this
change as long as both MigrateDisk and those flags are true, the code
would just go to the next disk and not check those flags. Again perhaps
something subtle I'm missing.

John

> 
> -            virReportError(VIR_ERR_MIGRATE_UNSAFE, "%s",
> -                           _("Migration may lead to data corruption if disks"
> -                             " use cache != none"));
> -            return false;
> +        if (virDomainDiskGetType(disk) == VIR_STORAGE_TYPE_FILE) {
> +            if ((rc = virFileIsSharedFS(src)) < 0)
> +                return false;
> +            else if (rc == 0)
> +                continue;
> +            if ((rc = virStorageFileIsClusterFS(src)) < 0)
> +                return false;
> +            else if (rc == 1)
> +                continue;
> +        } else if (disk->src->type == VIR_STORAGE_TYPE_NETWORK &&
> +                   disk->src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD) {
> +            continue;
>          }
> +
> +        virReportError(VIR_ERR_MIGRATE_UNSAFE, "%s",
> +                       _("Migration may lead to data corruption if disks"
> +                         " use cache != none"));
> +        return false;
>      }
> 
>      return true;
> @@ -1915,7 +1926,7 @@ qemuMigrationBeginPhase(virQEMUDriverPtr driver,
>          goto cleanup;
> 
>      if (!(flags & VIR_MIGRATE_UNSAFE) &&
> -        !qemuMigrationIsSafe(vm->def, nmigrate_disks, migrate_disks))
> +        !qemuMigrationIsSafe(vm->def, nmigrate_disks, migrate_disks, flags))
>          goto cleanup;
> 
>      if (flags & VIR_MIGRATE_POSTCOPY &&
> @@ -4773,7 +4784,7 @@ qemuMigrationPerformJob(virQEMUDriverPtr driver,
>          goto endjob;
> 
>      if (!(flags & VIR_MIGRATE_UNSAFE) &&
> -        !qemuMigrationIsSafe(vm->def, nmigrate_disks, migrate_disks))
> +        !qemuMigrationIsSafe(vm->def, nmigrate_disks, migrate_disks, flags))
>          goto endjob;
> 
>      qemuMigrationStoreDomainState(vm);
>