[libvirt] [PATCH 1/3] qemuMigrationSrcIsSafe: Check local storage more thoroughly

[Daniel P. Berrangé]

On Mon, Feb 26, 2018 at 11:05:49AM +0100, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1494454
> 
> If a domain disk is stored on local filesystem (e.g. ext4) but is
> not being migrated it is very likely that domain is not able to
> run on destination. Regardless of share/cache mode.
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/qemu/qemu_migration.c | 28 ++++++++++++++++++----------
>  1 file changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index b22a327b5..e98b1e4ce 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -1215,25 +1215,26 @@ qemuMigrationSrcIsSafe(virDomainDefPtr def,
>          virDomainDiskDefPtr disk = def->disks[i];
>          const char *src = virDomainDiskGetSource(disk);
>  
> -        /* Our code elsewhere guarantees shared disks are either readonly (in
> -         * which case cache mode doesn't matter) or used with cache=none or used with cache=directsync */
> +        /* Disks without any source (i.e. floppies and CD-ROMs)
> +         * OR readonly are safe. */
>          if (virStorageSourceIsEmpty(disk->src) ||
> -            disk->src->readonly ||
> -            disk->src->shared ||
> -            disk->cachemode == VIR_DOMAIN_DISK_CACHE_DISABLE ||
> -            disk->cachemode == VIR_DOMAIN_DISK_CACHE_DIRECTSYNC)
> +            disk->src->readonly)
>              continue;
>  
> -        /* disks which are migrated by qemu are safe too */
> +        /* Disks which are migrated by qemu are safe too. */
>          if (storagemigration &&
>              qemuMigrationAnyCopyDisk(disk, nmigrate_disks, migrate_disks))
>              continue;
>  
> +        /* However, disks on local FS (e.g. ext4) are not safe. */
>          if (virDomainDiskGetType(disk) == VIR_STORAGE_TYPE_FILE) {
> -            if ((rc = virFileIsSharedFS(src)) < 0)
> +            if ((rc = virFileIsSharedFS(src)) < 0) {
>                  return false;
> -            else if (rc == 0)
> -                continue;
> +            } else if (rc == 0) {
> +                virReportError(VIR_ERR_MIGRATE_UNSAFE, "%s",
> +                               _("Migration without shared storage is unsafe"));
> +                return false;
> +            }
>              if ((rc = virStorageFileIsClusterFS(src)) < 0)
>                  return false;
>              else if (rc == 1)
> @@ -1243,6 +1244,13 @@ qemuMigrationSrcIsSafe(virDomainDefPtr def,
>              continue;
>          }
>  
> +        /* Our code elsewhere guarantees shared disks are either readonly (in
> +         * which case cache mode doesn't matter) or used with cache=none or used with cache=directsync */
> +        if (disk->src->shared ||
> +            disk->cachemode == VIR_DOMAIN_DISK_CACHE_DISABLE ||
> +            disk->cachemode == VIR_DOMAIN_DISK_CACHE_DIRECTSYNC)
> +            continue;
> +
>          virReportError(VIR_ERR_MIGRATE_UNSAFE, "%s",
>                         _("Migration may lead to data corruption if disks"
>                           " use cache != none or cache != directsync"));

A little hard to follow but the key difference is that the old code would
immediately return saying it is "safe" if the cache mode was set to a
reasonable value, and not get to checking whether disk is local or not.
We've essentially reversed that so we detect unsafe scenarios first, and
then later check safe options.

Reviewed-by: Daniel P. Berrangé <berrange at redhat.com>

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|