The property is necessary also for the disk using the source (e.g. cdrom) which needs to be kept readonly. --- src/conf/domain_conf.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 80baa090a..d660c06e0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1723,9 +1723,12 @@ void virDomainDiskEmptySource(virDomainDiskDefPtr def) { virStorageSourcePtr src = def->src; + bool readonly = src->readonly; virStorageSourceClear(src); src->type = VIR_STORAGE_TYPE_FILE; + /* readonly property is necessary for CDROMs and thus can't be cleared */ + src->readonly = readonly; } -- 2.12.2

The code that validates whether an internal snapshot is possible would reject an empty but not-readonly drive. Since floppies can have this property, add a check for emptiness. --- src/qemu/qemu_driver.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 388af4f6f..e39de625d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13825,7 +13825,8 @@ qemuDomainSnapshotPrepare(virConnectPtr conn, case VIR_DOMAIN_SNAPSHOT_LOCATION_NONE: /* Remember seeing a disk that has snapshot disabled */ - if (!dom_disk->src->readonly) + if (!virStorageSourceIsEmpty(dom_disk->src) && + !dom_disk->src->readonly) forbid_internal = true; break; -- 2.12.2

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(-) 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) + 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 && + qemuMigrateDisk(disk, nmigrate_disks, migrate_disks)) + continue; - 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); -- 2.12.2

On Wed, Apr 12, 2017 at 19:43:20 -0400, John Ferlan wrote: Well. No. This is what this function is supposed to check first. I've extracted it from qemuMigrateDisk which would basically have the same semantics if 'migrate_disks' is NULL/empty. It is semantically wrong to call that function at this place. The function returns whether a disk will be migrated when doing migration with non-shared storage. While the logic is the same, it is not correct to abuse it here. No. This is exactly what the first sentence of the commit message promises to fix. The flags contain the two bits which are checked if the user requires storage migration. In case where we migrate storage, we do not need to check that caching for the disks is disabled. This is due to the fact that the disks are migrated by qemu and thus there are no caching-coherency issues, since qemu copies all the data. This means that disks undergoing storage migration don't need to be checked for disabled caching. Yes, that's exactly what I wanted to achieve. The above addition of the same logich that qemuMigrateDisk does to determine if a disk should be migrated is precisely for the fact, that using qemuMigrateDisk at that point is semantically incorrect. So the logic is as follows: If the disk is empty, readonly, shared, or has disabled caching, it's safe to migrate. This is also for cases where shared storage is used and thus no storage migration takes place. If storage migration is used, then all disks which are migrated using the storage migration are safe to undergo migration, since the caching issue does not apply to them. All other disks need to be checked

On 04/07/2017 11:50 AM, Peter Krempa wrote: because ... IOW: For disks that are not in our migrate_disk list and this is only ever called when the flags VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC are set... So to a degree the checks made in patch 3 make a bit more sense once it's understood that there's this code.... Like I said - something subtle in how this all "magically" works... ACK for this though, John