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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
