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

[Peter Krempa]

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.

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

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.

> 
> > +            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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170413/936392a7/attachment-0001.sig>