[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v2 4/4] qemu: Forbid migration with cache != none



On Wed, Feb 22, 2012 at 12:54:35 -0700, Eric Blake wrote:
> On 02/22/2012 07:51 AM, Jiri Denemark wrote:
> > Migrating domains with disks using cache != none is unsafe unless the
> > disk images are stored on coherent clustered filesystem. Thus we forbid
> > migrating such domains unless VIR_MIGRATE_UNSAFE flags is used.
> > ---
> > Notes:
> >     Version 2:
> >     - use virStorageFileIsClusterFS
> > 
> >  src/qemu/qemu_driver.c    |    3 ++-
> >  src/qemu/qemu_migration.c |   39 +++++++++++++++++++++++++++++++++++----
> >  src/qemu/qemu_migration.h |    6 ++++--
> >  3 files changed, 41 insertions(+), 7 deletions(-)
> > 
> 
> >  
> > +static bool
> > +qemuMigrationIsSafe(virDomainDefPtr def)
> > +{
> > +    int i;
> > +
> > +    for (i = 0 ; i < def->ndisks ; i++) {
> > +        virDomainDiskDefPtr disk = def->disks[i];
> > +
> > +        /* shared && !readonly implies cache=none */
> > +        if (disk->src &&
> > +            disk->cachemode != VIR_DOMAIN_DISK_CACHE_DISABLE &&
> > +            (disk->cachemode || !disk->shared || disk->readonly) &&
> > +            virStorageFileIsClusterFS(disk->src) == 1) {
> 
> Other than Dan's comment about the logic bug here, ACK.

Actually, I rewrote this as

+        if (disk->src &&
+            disk->cachemode != VIR_DOMAIN_DISK_CACHE_DISABLE &&
+            (disk->cachemode || !disk->shared || disk->readonly)) {
+            int cfs;
+            if ((cfs = virStorageFileIsClusterFS(disk->src)) == 1)
+                continue;
+            else if (cfs < 0)
+                return false;
+
+            qemuReportError(VIR_ERR_MIGRATE_UNSAFE, "%s",
+                            _("Migration may lead to data corruption if disks"
+                              " use cache != none"));
+            return false;
+        }

to avoid overwriting errors returned by virStorageFileIsClusterFS().

> Since the check for safety is only on the source, and the destination
> doesn't care, is there a way to add a driver feature flag, and add logic
> to libvirt.c to mask the VIR_MIGRATE_UNSAFE flag from the destination if
> it does not support the feature, similar to how we handled
> VIR_MIGRATE_CHANGE_PROTECTION via the
> VIR_DRV_FEATURE_MIGRATE_CHANGE_PROTECTION feature, as another example of
> a source-only flag?  Of course, this would be a followup patch, if we
> decide it is worth allowing an unsafe migration from 1.9.11 back to
> 1.9.10 (the upgrade migration from 1.9.10 to 1.9.11 will be unsafe
> automatically, because we weren't checking in 1.9.10).

Yeah, it would be possible to do it, however as we already added at least new
qemu capability (system_wakeup), domains running on new enough qemu will not
be migratable to 0.9.10 anyway so I guess it's just not worth it :-)

And I pushed this series, thanks for the reviews.

Jirka


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]