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

[John Ferlan]

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