Re: Re: [Xen-devel] Re: Writing a tool for Shared Persistent Windows Boot Image

["Daniel P. Berrange" <berrange@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3325 bytes --]

On Fri, Jun 29, 2007 at 09:18:50AM -0700, Andrew Warfield wrote:
> If you can send some more details on the crash we should be able to
> sort this out -- it's certainly something that has worked in the past.

Ok, so here's the scenario. Traditionally with HVM I have a disk

   file:/xen/rhel4i386.img,hda,w

Having seen the changeset 13827:6524e02edbeb I tried

  tap:aio:/xen/rhel4i386.img,hda,w

And

  tap:aio:/xen/rhel4i386.img,xvda,w

The latter is the preferred, since paravirt drivers should not be hijacking
the IDE devices inside the guest. However, the changeset 13827 doesn't seem
to support xvd* since QEMU filters out any devices with such a name.

With vanilla  Xen 3.1.0 qemu goes defunct when starting the guest
logging

 qemu: could not open hard disk image 'aio:/xen/rhel4i386.img'

After a little investigation I found that in BlktapController

       try:
           imagetype = self.vm.info['image']['type']
       except:
           imagetype = ""

Has long ago been broken and should instead be

        try:
            imagetype = self.vm.info.image_type()
        except:
            imagetype = ""


Once I made that change I can see a phantom device being created, but QEMU
still crashes & burns with  qemu-dm-XXXX.log showing

  qemu: could not open hard disk image '/dev/xvdc1'

I started to debug this, but looking at changeset 13827:6524e02edbeb I rapidly
came to the conclusion that the whole idea of phantom devices is complete
overkill & the entire problem could be addressed with a couple of lines in
ioemu/xenstore.c.  QEMU already knows how to handle all the different 
types of file format blktap does, so there's no need to setup extra phantom
devices. All thats needed is for QEMU to a) strip the aio: (or equivalent)
prefix and b) convert xvdN -> hdN if required.

So I'm attaching two patches. The first reverts 13827:6524e02edbeb
and the second tweaks ioemu/xenstore.c so it can handle blktap devices
directly. With these applied ontop of Xen 3.1.0 I can successfully
start HVM guests using  the two example tap:aio lines I show earlier
in this mail. The patch also adds a couple of logging lines which 
end up in qemu-dm-XXX.log as they'll be useful if ever debugging QEMU
boot issues - it is far too silent when things go wrong which makes
diagnosis hard.

    Signed-off-by: Daniel Berrange <berrange@redhat.com>


The patch being reverted:
 
 $ diffstat xen-revert-phantom.patch 
 ioemu/xenstore.c                           |   46 ---------------------
 python/xen/xend/XendConfig.py              |   41 -------------------
 python/xen/xend/XendDomainInfo.py          |   58 ---------------------------
 python/xen/xend/server/BlktapController.py |   62 -----------------------------
 python/xen/xend/server/DevController.py    |   13 ------
 5 files changed, 3 insertions(+), 217 deletions(-)

The new patch:

 $ diffstat xen-qemu-blktap.patch 
 xenstore.c |   28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

Regards,
Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

[-- Attachment #2: xen-revert-phantom.patch --]
[-- Type: text/plain, Size: 12359 bytes --]

diff -r c0b0974fb055 tools/ioemu/xenstore.c
--- a/tools/ioemu/xenstore.c	Fri May 18 16:59:32 2007 +0100
+++ b/tools/ioemu/xenstore.c	Fri Jun 29 15:48:53 2007 -0400
@@ -10,7 +10,6 @@
 
 #include "vl.h"
 #include "block_int.h"
-#include <unistd.h>
 #include <sys/ipc.h>
 #include <sys/shm.h>
 #include <sys/types.h>
@@ -61,28 +60,11 @@ void xenstore_check_new_media_present(in
     qemu_mod_timer(insert_timer, qemu_get_clock(rt_clock) + timeout);
 }
 
-static void waitForDevice(char *fn)
-{ 
-    struct stat sbuf;
-    int status;
-    int uwait = UWAIT_MAX;
-
-    do {
-        status = stat(fn, &sbuf);
-        if (!status) break;
-        usleep(UWAIT);
-        uwait -= UWAIT;
-    } while (uwait > 0);
-
-    return;
-}
-
 void xenstore_parse_domain_config(int domid)
 {
     char **e = NULL;
     char *buf = NULL, *path;
-    char *fpath = NULL, *bpath = NULL,
-        *dev = NULL, *params = NULL, *type = NULL;
+    char *bpath = NULL, *dev = NULL, *params = NULL, *type = NULL;
     int i, is_scsi;
     unsigned int len, num, hd_index;
 
@@ -140,36 +122,12 @@ void xenstore_parse_domain_config(int do
         params = xs_read(xsh, XBT_NULL, buf, &len);
         if (params == NULL)
             continue;
-        /* 
-         * check if device has a phantom vbd; the phantom is hooked
-         * to the frontend device (for ease of cleanup), so lookup 
-         * the frontend device, and see if there is a phantom_vbd
-         * if there is, we will use resolution as the filename
-         */
-        if (pasprintf(&buf, "%s/device/vbd/%s/phantom_vbd", path, e[i]) == -1)
-            continue;
-        free(fpath);
-        fpath = xs_read(xsh, XBT_NULL, buf, &len);
-        if (fpath) {
-            if (pasprintf(&buf, "%s/dev", fpath) == -1)
-                continue;
-            free(params);
-            params = xs_read(xsh, XBT_NULL, buf , &len);
-            if (params) {
-                /* 
-                 * wait for device, on timeout silently fail because we will 
-                 * fail to open below
-                 */
-                waitForDevice(params);
-            }
-        }
 
         bs_table[hd_index + (is_scsi ? MAX_DISKS : 0)] = bdrv_new(dev);
         /* check if it is a cdrom */
         if (type && !strcmp(type, "cdrom")) {
             bdrv_set_type_hint(bs_table[hd_index], BDRV_TYPE_CDROM);
-            if (pasprintf(&buf, "%s/params", bpath) != -1)
-                xs_watch(xsh, buf, dev);
+	    xs_watch(xsh, buf, dev);
         }
         /* open device now if media present */
         if (params[0]) {
diff -r c0b0974fb055 tools/python/xen/xend/XendConfig.py
--- a/tools/python/xen/xend/XendConfig.py	Fri May 18 16:59:32 2007 +0100
+++ b/tools/python/xen/xend/XendConfig.py	Fri Jun 29 15:37:43 2007 -0400
@@ -1126,47 +1126,6 @@ class XendConfig(dict):
         # no valid device to add
         return ''
 
-    def phantom_device_add(self, dev_type, cfg_xenapi = None,
-                   target = None):
-        """Add a phantom tap device configuration in XenAPI struct format.
-        """
-
-        if target == None:
-            target = self
-        
-        if dev_type not in XendDevices.valid_devices() and \
-           dev_type not in XendDevices.pseudo_devices():        
-            raise XendConfigError("XendConfig: %s not a valid device type" %
-                            dev_type)
-
-        if cfg_xenapi == None:
-            raise XendConfigError("XendConfig: device_add requires some "
-                                  "config.")
-
-        if cfg_xenapi:
-            log.debug("XendConfig.phantom_device_add: %s" % str(cfg_xenapi))
- 
-        if cfg_xenapi:
-            dev_info = {}            
-            if dev_type in ('vbd', 'tap'):
-                if dev_type == 'vbd':
-                    dev_info['uname'] = cfg_xenapi.get('image', '')
-                    dev_info['dev'] = '%s:disk' % cfg_xenapi.get('device')
-                elif dev_type == 'tap':
-                    if cfg_xenapi.get('image').find('tap:') == -1:
-                        dev_info['uname'] = 'tap:qcow:%s' % cfg_xenapi.get('image')
-                    dev_info['dev'] =  '/dev/%s' % cfg_xenapi.get('device')
-                    dev_info['uname'] = cfg_xenapi.get('image')
-                dev_info['mode'] = cfg_xenapi.get('mode')
-                dev_info['backend'] = '0'
-                dev_uuid = cfg_xenapi.get('uuid', uuid.createString())
-                dev_info['uuid'] = dev_uuid
-                self['devices'][dev_uuid] = (dev_type, dev_info)
-                self['vbd_refs'].append(dev_uuid)
-                return dev_uuid
-
-        return ''
-
     def console_add(self, protocol, location, other_config = {}):
         dev_uuid = uuid.createString()
         if protocol == 'vt100':
diff -r c0b0974fb055 tools/python/xen/xend/XendDomainInfo.py
--- a/tools/python/xen/xend/XendDomainInfo.py	Fri May 18 16:59:32 2007 +0100
+++ b/tools/python/xen/xend/XendDomainInfo.py	Fri Jun 29 15:37:43 2007 -0400
@@ -1623,49 +1623,14 @@ class XendDomainInfo:
     # VM Destroy
     # 
 
-    def _prepare_phantom_paths(self):
-        # get associated devices to destroy
-        # build list of phantom devices to be removed after normal devices
-        plist = []
-        if self.domid is not None:
-            from xen.xend.xenstore.xstransact import xstransact
-            t = xstransact("%s/device/vbd" % GetDomainPath(self.domid))
-            for dev in t.list():
-                backend_phantom_vbd = xstransact.Read("%s/device/vbd/%s/phantom_vbd" \
-                                      % (self.dompath, dev))
-                if backend_phantom_vbd is not None:
-                    frontend_phantom_vbd =  xstransact.Read("%s/frontend" \
-                                      % backend_phantom_vbd)
-                    plist.append(backend_phantom_vbd)
-                    plist.append(frontend_phantom_vbd)
-        return plist
-
-    def _cleanup_phantom_devs(self, plist):
-        # remove phantom devices
-        if not plist == []:
-            time.sleep(2)
-        for paths in plist:
-            if paths.find('backend') != -1:
-                from xen.xend.server import DevController
-                # Modify online status /before/ updating state (latter is watched by
-                # drivers, so this ordering avoids a race).
-                xstransact.Write(paths, 'online', "0")
-                xstransact.Write(paths, 'state', str(DevController.xenbusState['Closing']))
-            # force
-            xstransact.Remove(paths)
-
     def destroy(self):
         """Cleanup VM and destroy domain.  Nothrow guarantee."""
 
         log.debug("XendDomainInfo.destroy: domid=%s", str(self.domid))
-
-        paths = self._prepare_phantom_paths()
 
         self._cleanupVm()
         if self.dompath is not None:
             self.destroyDomain()
-
-        self._cleanup_phantom_devs(paths)
 
         if "transient" in self.info["other_config"] \
            and bool(self.info["other_config"]["transient"]):
@@ -1675,8 +1640,6 @@ class XendDomainInfo:
 
     def destroyDomain(self):
         log.debug("XendDomainInfo.destroyDomain(%s)", str(self.domid))
-
-        paths = self._prepare_phantom_paths()
 
         try:
             if self.domid is not None:
@@ -1692,7 +1655,7 @@ class XendDomainInfo:
         XendDomain.instance().remove_domain(self)
 
         self.cleanupDomain()
-        self._cleanup_phantom_devs(paths)
+
 
 
     def resumeDomain(self):
@@ -2403,25 +2366,6 @@ class XendDomainInfo:
             
         return dev_uuid
 
-    def create_phantom_vbd_with_vdi(self, xenapi_vbd, vdi_image_path):
-        """Create a VBD using a VDI from XendStorageRepository.
-
-        @param xenapi_vbd: vbd struct from the Xen API
-        @param vdi_image_path: VDI UUID
-        @rtype: string
-        @return: uuid of the device
-        """
-        xenapi_vbd['image'] = vdi_image_path
-        dev_uuid = self.info.phantom_device_add('tap', cfg_xenapi = xenapi_vbd)
-        if not dev_uuid:
-            raise XendError('Failed to create device')
-
-        if self._stateGet() == XEN_API_VM_POWER_STATE_RUNNING:
-            _, config = self.info['devices'][dev_uuid]
-            config['devid'] = self.getDeviceController('tap').createDevice(config)
-
-        return config['devid']
-
     def create_vif(self, xenapi_vif):
         """Create VIF device from the passed struct in Xen API format.
 
diff -r c0b0974fb055 tools/python/xen/xend/server/BlktapController.py
--- a/tools/python/xen/xend/server/BlktapController.py	Fri May 18 16:59:32 2007 +0100
+++ b/tools/python/xen/xend/server/BlktapController.py	Fri Jun 29 15:37:43 2007 -0400
@@ -2,10 +2,7 @@
 
 
 from xen.xend.server.blkif import BlkifController
-from xen.xend.XendLogging import log
 
-phantomDev = 0;
-phantomId = 0;
 
 class BlktapController(BlkifController):
     def __init__(self, vm):
@@ -15,62 +12,3 @@ class BlktapController(BlkifController):
         """@see DevController#frontendRoot"""
         
         return "%s/device/vbd" % self.vm.getDomainPath()
-
-    def getDeviceDetails(self, config):
-        (devid, back, front) = BlkifController.getDeviceDetails(self, config)
-
-        phantomDevid = 0
-        wrapped = False
-
-        try:
-            imagetype = self.vm.info['image']['type']
-        except:
-            imagetype = ""
-
-        if imagetype == 'hvm':
-            tdevname = back['dev']
-            index = ['c', 'd', 'e', 'f', 'g', 'h', 'i', \
-                     'j', 'l', 'm', 'n', 'o', 'p']
-            while True:
-                global phantomDev
-                global phantomId
-                import os, stat
-
-                phantomId = phantomId + 1
-                if phantomId == 16:
-                    if index[phantomDev] == index[-1]:
-                        if wrapped:
-                            raise VmError(" No loopback block \
-                                       devices are available. ")
-                        wrapped = True
-                        phantomDev = 0
-                    else:
-                        phantomDev = phantomDev + 1
-                    phantomId = 1
-                devname = 'xvd%s%d' % (index[phantomDev], phantomId)
-                try:
-                    info = os.stat('/dev/%s' % devname)
-                except:
-                    break
-
-            vbd = { 'mode': 'w', 'device': devname }
-            fn = 'tap:%s' % back['params']
-
-            # recurse ... by creating the vbd, then fallthrough
-            # and finish creating the original device
-
-            from xen.xend import XendDomain
-            dom0 = XendDomain.instance().privilegedDomain()
-            phantomDevid = dom0.create_phantom_vbd_with_vdi(vbd, fn)
-            # we need to wait for this device at a higher level
-            # the vbd that gets created will have a link to us
-            # and will let them do it there
-
-        # add a hook to point to the phantom device,
-        # root path is always the same (dom0 tap)
-        if phantomDevid != 0:
-            front['phantom_vbd'] = '/local/domain/0/backend/tap/0/%s' \
-                                   % str(phantomDevid)
-
-        return (devid, back, front)
-
diff -r c0b0974fb055 tools/python/xen/xend/server/DevController.py
--- a/tools/python/xen/xend/server/DevController.py	Fri May 18 16:59:32 2007 +0100
+++ b/tools/python/xen/xend/server/DevController.py	Fri Jun 29 15:37:43 2007 -0400
@@ -474,19 +474,6 @@ class DevController:
     def waitForBackend(self, devid):
 
         frontpath = self.frontendPath(devid)
-        # lookup a phantom 
-        phantomPath = xstransact.Read(frontpath, 'phantom_vbd')
-        if phantomPath is not None:
-            log.debug("Waiting for %s's phantom %s.", devid, phantomPath)
-            statusPath = phantomPath + '/' + HOTPLUG_STATUS_NODE
-            ev = Event()
-            result = { 'status': Timeout }
-            xswatch(statusPath, hotplugStatusCallback, ev, result)
-            ev.wait(DEVICE_CREATE_TIMEOUT)
-            err = xstransact.Read(statusPath, HOTPLUG_ERROR_NODE)
-            if result['status'] != 'Connected':
-                return (result['status'], err)
-            
         backpath = xstransact.Read(frontpath, "backend")
 
 

[-- Attachment #3: xen-qemu-blktap.patch --]
[-- Type: text/plain, Size: 2846 bytes --]

diff -r ff1a3395024c tools/ioemu/xenstore.c
--- a/tools/ioemu/xenstore.c	Fri Jun 29 15:37:48 2007 -0400
+++ b/tools/ioemu/xenstore.c	Fri Jun 29 16:06:13 2007 -0400
@@ -64,7 +64,7 @@ void xenstore_parse_domain_config(int do
 {
     char **e = NULL;
     char *buf = NULL, *path;
-    char *bpath = NULL, *dev = NULL, *params = NULL, *type = NULL;
+    char *bpath = NULL, *dev = NULL, *params = NULL, *type = NULL, *drv = NULL;
     int i, is_scsi;
     unsigned int len, num, hd_index;
 
@@ -98,6 +98,13 @@ void xenstore_parse_domain_config(int do
         bpath = xs_read(xsh, XBT_NULL, buf, &len);
         if (bpath == NULL)
             continue;
+        /* read the driver type of the device */
+        if (pasprintf(&buf, "%s/type", bpath) == -1)
+            continue;
+        free(drv);
+        drv = xs_read(xsh, XBT_NULL, buf, &len);
+        if (drv == NULL)
+            continue;
         /* read the name of the device */
         if (pasprintf(&buf, "%s/dev", bpath) == -1)
             continue;
@@ -105,6 +112,13 @@ void xenstore_parse_domain_config(int do
         dev = xs_read(xsh, XBT_NULL, buf, &len);
         if (dev == NULL)
             continue;
+	/* Force xvdN to look like  hdN */
+	if (!strncmp(dev, "xvd", 3)) {
+	    fprintf(logfile, "Converting device type '%s'\n", dev);
+	    memmove(dev, dev+1, strlen(dev));
+	    dev[0] = 'h';
+	    dev[1] = 'd';
+	}
         is_scsi = !strncmp(dev, "sd", 2);
         if ((strncmp(dev, "hd", 2) && !is_scsi) || strlen(dev) != 3 )
             continue;
@@ -122,7 +136,15 @@ void xenstore_parse_domain_config(int do
         params = xs_read(xsh, XBT_NULL, buf, &len);
         if (params == NULL)
             continue;
-
+	/* Strip off blktap sub-type prefix aio: - QEMU can autodetect this */
+	if (!strcmp(drv, "tap") && params[0]) {
+	    char *offset = strchr(params, ':');
+	    if (!offset)
+	        continue;
+	    fprintf(logfile, "Stripping blktap sub-type prefix from %s\n", params);
+	    memmove(params, offset+1, strlen(offset+1)+1);
+	}
+	fprintf(logfile, "Creating disk '%s' with driver '%s'\n", dev, drv);
         bs_table[hd_index + (is_scsi ? MAX_DISKS : 0)] = bdrv_new(dev);
         /* check if it is a cdrom */
         if (type && !strcmp(type, "cdrom")) {
@@ -131,6 +153,7 @@ void xenstore_parse_domain_config(int do
         }
         /* open device now if media present */
         if (params[0]) {
+            fprintf(logfile, "Initializing disk '%s' with media '%s'\n", dev, params);
             if (bdrv_open(bs_table[hd_index + (is_scsi ? MAX_DISKS : 0)],
                           params, 0 /* snapshot */) < 0)
                 fprintf(stderr, "qemu: could not open hard disk image '%s'\n",
@@ -146,6 +169,7 @@ void xenstore_parse_domain_config(int do
 
 
  out:
+    free(drv);
     free(type);
     free(params);
     free(dev);

[-- Attachment #4: Type: text/plain, Size: 137 bytes --]

_______________________________________________
Xen-users mailing list
Xen-users@lists.xensource.com
http://lists.xensource.com/xen-users