On 04/07/2017 11:50 AM, Peter Krempa wrote:

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(+)

Wouldn't restoring readonly only be necessary "if (def->device == VIR_DOMAIN_DISK_DEVICE_CDROM)" since that's the primary reason it was set by libvirt? Of course it's only ever set at XML parse time, so would inserting a source path necessarily reset it for non CDROM's. Also know it's possible to check git history and all, could the commit message be adjusted to provide a few more shreds of information... Such as adding in "commit id '462c4b66' was a bit too aggressive and missed restoring the readonly flag for the storage source that is set by ParseXML for CDROM's" Of course this all makes me wonder if something else was too aggressively cleared/reset, but at least this much is perfectly understandable. ACK for what's here w/ a bit more details for the commit message... John As an aside, the API could have been called DiskSetEmptySource or DiskSetSourceEmpty... DiskEmptySource almost looks like a boolean function. Oh well different issue ;-)

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; }

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);

On 04/13/2017 06:28 AM, Peter Krempa wrote:

On Wed, Apr 12, 2017 at 19:43:20 -0400, John Ferlan wrote:

...

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.

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.

OK... Right I understand this - although my wording wasn't as precise.

...

...

+ 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

No. This is exactly what the first sentence of the commit message promises to fix.

...

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.

The flags contain the two bits which are checked if the user requires storage migration.

When you say "storage migration" - I'm assuming NBD, is that assumption true? Or can the non shared, non readonly, non empty, or cache!=none disks be migrated by some other means?

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.

What's confusing to me about the message is that qemuMigrateDisk doesn't check caching... The way the code is changed - it doesn't matter about storage migrated since *any* disk with cache=none is deemed OK.

...

...

+ 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.

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

Perhaps if qemuMigrateDisk were documented it'd be easier to understand (multiple options for storage transfer - via command line that fills in migrate_disks and the nbd server setup). In any case, thanks for the extra details and this does make (more) sense now... So ACK for the patch... John