Jim Fehlig
2007-Aug-02 19:01 UTC
[Xen-devel] [PATCH][xend] Fix/cleanup destoryDevice code path
More subtle problems with destroyDevice code path in xend. See attached patch Regards, Jim _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Mats Petersson
2007-Aug-02 19:25 UTC
Re: [Xen-devel] [PATCH][xend] Fix/cleanup destoryDevice code path
Jim Can you check if this removes the device nodes within xenstore when doing xm save [1] - as I modified this section of code to fix up a problem with just that not so long ago. Unfortunately, I''m not able to test it myself, as I''m "without a xen-capable machine at the moment". I have no particular reason to believe it DOESN''T do that, I''m just asking to make sure that it''s destroyed properly, because I found that leaving lots of device info in xenstore bloats the database in xenstore, and eventually Dom0 runs out of physical memory.... :-( The easiest way to do this is to repeatedly save and restore a domain, whilst whatching the size of xenstore, e.g. "xenstore-ls | wc" -- Mats At 20:01 02/08/2007, Jim Fehlig wrote:>More subtle problems with destroyDevice code path in xend. See attached >patch > >Regards, >Jim > ># HG changeset patch ># User Jim Fehlig <jfehlig@novell.com> ># Date 1186081049 21600 ># Node ID 430ae0d3a333ff4d212df7c2313caa03e8f4dd51 ># Parent 88bb0d305308a2cab31fd8559a6a2719db1ea55a >Fix/cleanup destroyDevice code path in xend. > >When calling destroyDevice code path (e.g. xm block-detach dom devid), >allow specifying an integer device id or a device name such as xvdN or >/dev/xvdN. Allowing the /dev/xvdN form is useful when detaching devices >from dom0. Bootloaders may do this to unmount a disk previously >mounted in dom0. > >Move examination of device ID format into the DevController, permitting >device controllers to determine a valid device ID instead of higher >level code. > >Signed-off-by: Jim Fehlig <jfehlig@novell.com> > >diff -r 88bb0d305308 -r 430ae0d3a333 tools/python/xen/xend/XendDomainInfo.py >--- a/tools/python/xen/xend/XendDomainInfo.py Wed Aug 01 15:47:54 2007 +0100 >+++ b/tools/python/xen/xend/XendDomainInfo.py Thu Aug 02 12:57:29 2007 -0600 >@@ -559,18 +559,8 @@ class XendDomainInfo: > self.getDeviceController(devclass).waitForDevices() > > def destroyDevice(self, deviceClass, devid, force = False): >- try: >- dev = int(devid) >- except ValueError: >- # devid is not a number but a string containing either device >- # name (e.g. xvda) or device_type/device_id (e.g. vbd/51728) >- dev = type(devid) is str and devid.split(''/'')[-1] or None >- if dev == None: >- log.debug("Could not find the device %s", devid) >- return None >- >- log.debug("dev = %s", dev) >- return >self.getDeviceController(deviceClass).destroyDevice(dev, force) >+ log.debug("dev = %s", devid) >+ return >self.getDeviceController(deviceClass).destroyDevice(devid, force) > > def getDeviceSxprs(self, deviceClass): > if self._stateGet() in (DOM_STATE_RUNNING, DOM_STATE_PAUSED): >diff -r 88bb0d305308 -r 430ae0d3a333 >tools/python/xen/xend/server/DevController.py >--- a/tools/python/xen/xend/server/DevController.py Wed Aug 01 >15:47:54 2007 +0100 >+++ b/tools/python/xen/xend/server/DevController.py Thu Aug 02 >12:57:29 2007 -0600 >@@ -203,27 +203,32 @@ class DevController: > > The implementation here simply deletes the appropriate > paths from the > store. This may be overridden by subclasses who need to > perform other >- tasks on destruction. Further, the implementation here can only >- accept integer device IDs, or values that can be converted to >- integers. Subclasses may accept other values and convert them to >- integers before passing them here. >- """ >- >- devid = int(devid) >+ tasks on destruction. The implementation here accepts integer device >+ IDs or paths containg integer deviceIDs, e.g. vfb/0. Subclasses may >+ accept other values and convert them to integers before passing them >+ here. >+ """ >+ >+ try: >+ dev = int(devid) >+ except ValueError: >+ # Does devid contain devicetype/deviceid? >+ # Propogate exception if unable to find an integer devid >+ dev = int(type(devid) is str and devid.split(''/'')[-1] or None) > > # Modify online status /before/ updating state (latter is watched by > # drivers, so this ordering avoids a race). >- self.writeBackend(devid, ''online'', "0") >- self.writeBackend(devid, ''state'', str(xenbusState[''Closing''])) >+ self.writeBackend(dev, ''online'', "0") >+ self.writeBackend(dev, ''state'', str(xenbusState[''Closing''])) > > if force: >- frontpath = self.frontendPath(devid) >+ frontpath = self.frontendPath(dev) > backpath = xstransact.Read(frontpath, "backend") > if backpath: > xstransact.Remove(backpath) > xstransact.Remove(frontpath) > >- self.vm._removeVm("device/%s/%d" % (self.deviceClass, devid)) >+ self.vm._removeVm("device/%s/%d" % (self.deviceClass, dev)) > > def configurations(self): > return map(self.configuration, self.deviceIDs()) >diff -r 88bb0d305308 -r 430ae0d3a333 tools/python/xen/xend/server/blkif.py >--- a/tools/python/xen/xend/server/blkif.py Wed Aug 01 15:47:54 2007 +0100 >+++ b/tools/python/xen/xend/server/blkif.py Thu Aug 02 12:57:29 2007 -0600 >@@ -149,13 +149,16 @@ class BlkifController(DevController): > def destroyDevice(self, devid, force): > """@see DevController.destroyDevice""" > >- # If we are given a device name, then look up the device ID from it, >- # and destroy that ID instead. If what we are given is an integer, >- # then assume it''s a device ID and pass it straight through to our >- # superclass''s method. >- >+ # vbd device IDs can be either string or integer. Further, the >+ # following string values are possible: >+ # - devicetype/deviceid (vbd/51728) >+ # - devicetype/devicename (/dev/xvdb) >+ # - devicename (xvdb) >+ # Let our superclass handle integer or devicetype/deviceid forms. >+ # If we are given a device name form, then look up the device ID >+ # from it, and destroy that ID instead. > try: >- DevController.destroyDevice(self, int(devid), force) >+ DevController.destroyDevice(self, devid, force) > except ValueError: > devid_end = type(devid) is str and devid.split(''/'')[-1] or None > > >_______________________________________________ >Xen-devel mailing list >Xen-devel@lists.xensource.com >http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jim Fehlig
2007-Aug-03 15:53 UTC
Re: [Xen-devel] [PATCH][xend] Fix/cleanup destoryDevice code path
Mats Petersson wrote:> Jim > > Can you check if this removes the device nodes within xenstore when > doing xm save [1] - as I modified this section of code to fix up a > problem with just that not so long ago. Unfortunately, I''m not able to > test it myself, as I''m "without a xen-capable machine at the moment".Yes, everything gets nuked from xenstore as expected on save.> I have no particular reason to believe it DOESN''T do that, I''m just > asking to make sure that it''s destroyed properly, because I found that > leaving lots of device info in xenstore bloats the database in > xenstore, and eventually Dom0 runs out of physical memory.... :-(Yes, I recall this behavior. I recently received a large database file (~34M) from a 3.0.4-based system that includes your patches. This system had well over 25000 vms started/[stopped|destroyed|failed] and the database had *many* /local/domain/<vmid>/device/console/0 entries. So some types of devices (console in particular) are still orphaned under some yet undetermined vm lifeclycle event. Trying to reproduce ... BTW, any ideas on how to read a xenstore tdb file provided via a bug report? :-) Regards, Jim _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel