From patchwork Tue Nov 9 19:35:48 2010 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Alex Williamson X-Patchwork-Id: 70572 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [199.232.76.165]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 4A8E7B711B for ; Wed, 10 Nov 2010 06:37:20 +1100 (EST) Received: from localhost ([127.0.0.1]:41705 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PFu0L-0001UP-1i for incoming@patchwork.ozlabs.org; Tue, 09 Nov 2010 14:37:17 -0500 Received: from [140.186.70.92] (port=52497 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PFtz2-0000yI-QD for qemu-devel@nongnu.org; Tue, 09 Nov 2010 14:35:59 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PFtz1-0001o2-6e for qemu-devel@nongnu.org; Tue, 09 Nov 2010 14:35:56 -0500 Received: from mx1.redhat.com ([209.132.183.28]:6510) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PFtz0-0001nw-Rm for qemu-devel@nongnu.org; Tue, 09 Nov 2010 14:35:55 -0500 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id oA9JZoMo032062 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Tue, 9 Nov 2010 14:35:50 -0500 Received: from [10.3.113.74] (ovpn-113-74.phx2.redhat.com [10.3.113.74]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id oA9JZnaJ013617; Tue, 9 Nov 2010 14:35:49 -0500 From: Alex Williamson To: "Michael S. Tsirkin" In-Reply-To: <1289324646.14321.39.camel@x201> References: <20101108205901.GB10777@redhat.com> <1289251417.28165.37.camel@x201> <20101109120020.GC22705@redhat.com> <1289314703.28165.53.camel@x201> <20101109150708.GA25725@redhat.com> <1289316894.14321.15.camel@x201> <20101109154217.GA26326@redhat.com> <1289317620.14321.19.camel@x201> <20101109161525.GA26897@redhat.com> <1289320245.14321.28.camel@x201> <20101109164917.GA27287@redhat.com> <1289324646.14321.39.camel@x201> Date: Tue, 09 Nov 2010 12:35:48 -0700 Message-ID: <1289331348.14321.58.camel@x201> Mime-Version: 1.0 X-Scanned-By: MIMEDefang 2.68 on 10.5.11.23 X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. Cc: cam@cs.ualberta.ca, qemu-devel@nongnu.org, kvm@vger.kernel.org, quintela@redhat.com Subject: [Qemu-devel] Re: [PATCH 0/6] Save state error handling (kill off no_migrate) X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org On Tue, 2010-11-09 at 10:44 -0700, Alex Williamson wrote: > On Tue, 2010-11-09 at 18:49 +0200, Michael S. Tsirkin wrote: > > On Tue, Nov 09, 2010 at 09:30:45AM -0700, Alex Williamson wrote: > > > On Tue, 2010-11-09 at 18:15 +0200, Michael S. Tsirkin wrote: > > > > On Tue, Nov 09, 2010 at 08:47:00AM -0700, Alex Williamson wrote: > > > > > > > But it could. What if ivshmem is acting in a peer role, but has no > > > > > > > clients, could it migrate? What if ivshmem is migratable when the > > > > > > > migration begins, but while the migration continues, a connection is > > > > > > > setup and it becomes unmigratable. > > > > > > > > > > > > Sounds like something we should work to prevent, not support :) > > > > > > > > > > s/:)/:(/ why? > > > > > > > > It will just confuse everyone. Also if it happens after sending > > > > all of memory, it's pretty painful. > > > > > > It happens after sending all of memory with no_migrate, and I think > > > pushing that earlier might introduce some races around when > > > register_device_unmigratable() can be called. > > > > Good point. I guess we could check it twice just to speed things up. > > > > > > > > > Using this series, ivshmem would > > > > > > > have multiple options how to support this. It could a) NAK the > > > > > > > migration, b) drop connections and prevent new connections until the > > > > > > > migration finishes, c) detect that new connections have happened since > > > > > > > the migration started and cancel. And probably more. no_migrate can > > > > > > > only do a). And in fact, we can only test no_migrate after the VM is > > > > > > > stopped (after all memory is migrated) because otherwise it could race > > > > > > > with devices setting no_migrate during migration. > > > > > > > > > > > > We really want no_migrate to be static. changing it is abusing > > > > > > the infrastructure. > > > > > > > > > > You call it abusing, I call it making use of the infrastructure. Why > > > > > unnecessarily restrict ourselves? Is return 0/-1 really that scary, > > > > > unmaintainable, undebuggable? I don't understand the resistance. > > > > > > > > > > Alex > > > > > > > > management really does not know how to handle unexpected > > > > migration failures. They must be avoided. > > > > > > > > There are some very special cases that fail migration. They are > > > > currently easy to find with grep register_device_unmigratable. > > > > I prefer to keep it that way. > > > > > > How can management tools be improved to better handle unexpected > > > migration failures when the only way for qemu to fail is an abort? > > > We need the infrastructure to at least return an error first. Do we just > > > need to add some fprintfs to the save core to print the id string of the > > > device that failed to save? I just can't buy the "code is easier to > > > grep" as an argument against adding better error handling to the save > > > code path. > > > > I just don't buy the 'we'll return meaningless error codes at random > > point in time and management will figure it out' as an argument :) > > Why is the error code meaningless? The error code stops the migration > in qemu and hopefully prints an error message (we could easily add an > fprintf to the core save code to ensure we know the device responsible > for the NAK). From there we can figure out how to return the error to > monitors and management tools, but we need to have a way to know there's > an error first. > > > > Anyone else want to chime in? > > > > > > Alex > > > > Maybe try coding up some user using the new infrastructure to do > > something useful, that register_device_unmigratable can't do. > > With the number of people I hear complaining about how qemu has too many > aborts, no error checking, and no way to return errors, I'm a little > dumbfounded that there's such a roadblock to actually add some simple > error handling. Is it the error handling you're opposed to, or the way > I'm using it to NAK a migration? Would something like this in place of 6/6 ease your grep'ability and debug'ability concerns? Unfortunately it adds an assert, but if we stomp on the vmsd, then the save state entry can't be unregistered. Signed-off-by: Alex Williamson diff --git a/savevm.c b/savevm.c index 521edc8..b0aaa46 100644 --- a/savevm.c +++ b/savevm.c @@ -1039,7 +1039,6 @@ typedef struct SaveStateEntry { const VMStateDescription *vmsd; void *opaque; CompatEntry *compat; - int no_migrate; } SaveStateEntry; @@ -1103,7 +1102,6 @@ int register_savevm_live(DeviceState *dev, se->load_state = load_state; se->opaque = opaque; se->vmsd = NULL; - se->no_migrate = 0; if (dev && dev->parent_bus && dev->parent_bus->info->get_dev_path) { char *id = dev->parent_bus->info->get_dev_path(dev); @@ -1170,6 +1168,22 @@ void unregister_savevm(DeviceState *dev, const char *idstr, void *opaque) } } +static int nomigrate_set_params(int blk_enable, int shared, void *opaque) +{ + return -EFAULT; +} + +static int nomigrate_save_live_state(Monitor *mon, QEMUFile *f, int stage, + void *opaque) +{ + return -EFAULT; +} + +static int nomigrate_save_state(QEMUFile *f, void *opaque) +{ + return -EFAULT; +} + /* mark a device as not to be migrated, that is the device should be unplugged before migration */ void register_device_unmigratable(DeviceState *dev, const char *idstr, @@ -1190,7 +1204,10 @@ void register_device_unmigratable(DeviceState *dev, const char *idstr, QTAILQ_FOREACH(se, &savevm_handlers, entry) { if (strcmp(se->idstr, id) == 0 && se->opaque == opaque) { - se->no_migrate = 1; + se->set_params = nomigrate_set_params; + se->save_live_state = nomigrate_save_live_state; + se->save_state = nomigrate_save_state; + assert(!se->vmsd); } } } @@ -1410,10 +1427,6 @@ static int vmstate_load(QEMUFile *f, SaveStateEntry *se, int version_id) static int vmstate_save(QEMUFile *f, SaveStateEntry *se) { - if (se->no_migrate) { - return -1; - } - if (!se->vmsd) { /* Old style */ return se->save_state(f, se->opaque); } @@ -1443,6 +1456,9 @@ int qemu_savevm_state_begin(Monitor *mon, QEMUFile *f, int blk_enable, } ret = se->set_params(blk_enable, shared, se->opaque); if (ret < 0) { + monitor_printf(mon, + "Save state begin blocked by device '%s', error:" + " %s\n", se->idstr, strerror(-ret)); return ret; } } @@ -1470,6 +1486,9 @@ int qemu_savevm_state_begin(Monitor *mon, QEMUFile *f, int blk_enable, ret = se->save_live_state(mon, f, QEMU_VM_SECTION_START, se->opaque); if (ret < 0) { + monitor_printf(mon, + "Save state begin blocked by device '%s', error:" + " %s\n", se->idstr, strerror(-ret)); return ret; } } @@ -1503,6 +1522,9 @@ int qemu_savevm_state_iterate(Monitor *mon, QEMUFile *f) synchronized over and over again. */ break; } else if (ret < 0) { + monitor_printf(mon, + "Save state iterate blocked by device '%s', error:" + " %s\n", se->idstr, strerror(-ret)); return ret; } } @@ -1535,6 +1557,9 @@ int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f) r = se->save_live_state(mon, f, QEMU_VM_SECTION_END, se->opaque); if (r < 0) { + monitor_printf(mon, + "Save state complete blocked by device '%s', error:" + " %s\n", se->idstr, strerror(-r)); return r; } } @@ -1559,7 +1584,9 @@ int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f) r = vmstate_save(f, se); if (r < 0) { - monitor_printf(mon, "cannot migrate with device '%s'\n", se->idstr); + monitor_printf(mon, + "Save state complete blocked by device '%s', error:" + " %s\n", se->idstr, strerror(-r)); return r; } }