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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

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

On Thu, Apr 13, 2017 at 10:00:11 -0400, John Ferlan wrote:
> 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).

So to enable storage migration, you need to set one of the
VIR_MIGRATE_NON_SHARED_* flags. This will migrate any non shared, non
readonly, and non empty disk. Since this was not optimal, a new
migration parameter was added so that the list of discs which should be
migrated can be explicitly set when migration is started. And
qemuMigrateDisk is just a helper which tells whether a given disk is
supposed to be migrated: either it is specified in the explicitly passed
list of disks, or when no list is passed it uses the default rule.

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list