James (song wei)
2010-Apr-22 08:08 UTC
[Xen-devel] earlier remove the backend of tapdisk device in xenstore to release the resource allocated in backend driver lies in dom0''kernel
Blktapctl thread will use qemu-dm connection instead of tapdisk-ioemu in the case of FV VM. We found the resource like memory allocated for this Guest can''t be free for backend driver couldn''t be closed in qemu-dm. This patch would remove the backend of tapdisk device earlier in xenstore to triger qemu-dm to notify the backend driver to release the resource allocated. I have tested this patch at the case of 1, save && restore 2, destory && shutdown 3, snapshot regards, -James (Song Wei) Signed-off-by: James ( Song Wei ) <jsong@novell.com> diff -r fadf63ab49e7 tools/python/xen/xend/XendDomainInfo.py --- a/tools/python/xen/xend/XendDomainInfo.py Mon Apr 19 17:57:28 2010 +0100 +++ b/tools/python/xen/xend/XendDomainInfo.py Thu Apr 22 15:54:01 2010 +0800 @@ -2406,8 +2406,13 @@ def _releaseDevices(self, suspend = False): """Release all domain''s devices. Nothrow guarantee.""" + t = xstransact("%s/device" % self.vmpath) if self.image: try: + for dev in t.list(''tap''): + log.debug("Early removing %s", dev); + self.getDeviceController(''tap'').destroyDevice(dev, True) + time.sleep(0.1) log.debug("Destroying device model") self.image.destroyDeviceModel() except Exception, e: @@ -2416,9 +2421,10 @@ log.debug("No device model") log.debug("Releasing devices") - t = xstransact("%s/device" % self.vmpath) try: for devclass in XendDevices.valid_devices(): + if devclass is ''tap'': + continue for dev in t.list(devclass): try: log.debug("Removing %s", dev); http://old.nabble.com/file/p28325456/tapdisk-close.patch tapdisk-close.patch -- View this message in context: http://old.nabble.com/earlier-remove-the-backend-of-tapdisk-device-in-xenstore-to-release-the-resource-allocated-in-backend-driver-lies-in-dom0%27kernel-tp28325456p28325456.html Sent from the Xen - Dev mailing list archive at Nabble.com. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2010-Apr-22 11:13 UTC
Re: [Xen-devel] earlier remove the backend of tapdisk device in xenstore to release the resource allocated in backend driver lies in dom0''kernel
>>> "James (song wei)" <jsong@novell.com> 22.04.10 10:08 >>> >--- a/tools/python/xen/xend/XendDomainInfo.py Mon Apr 19 17:57:28 2010 +0100 >+++ b/tools/python/xen/xend/XendDomainInfo.py Thu Apr 22 15:54:01 2010 +0800 >@@ -2406,8 +2406,13 @@ > > def _releaseDevices(self, suspend = False): > """Release all domain''s devices. Nothrow guarantee.""" >+ t = xstransact("%s/device" % self.vmpath) > if self.image: > try: >+ for dev in t.list(''tap''): >+ log.debug("Early removing %s", dev); >+ self.getDeviceController(''tap'').destroyDevice(dev, True) >+ time.sleep(0.1) > log.debug("Destroying device model") > self.image.destroyDeviceModel() > except Exception, e: >@@ -2416,9 +2421,10 @@ > log.debug("No device model") > > log.debug("Releasing devices") >- t = xstransact("%s/device" % self.vmpath) > try: > for devclass in XendDevices.valid_devices(): >+ if devclass is ''tap'': >+ continue > for dev in t.list(devclass): > try: > log.debug("Removing %s", dev);This seems more like a hack than a solution: Surely qemu-dm gets sent some sort of signal to shut down and clean up. The question thus really is why that cleanup doesn''t include cleaning up blktap related resources. That is, I would expect the fix to be in qemu-dm, or at most in the xend code that reaps qemu-dm. More generally any solution should be generic, or it should be explained properly why the device class needs treatment different from any of the other ones (and, for this specific case, why moving the device destruction a little ahead will now *guarantee* that the cleanup can happen as expected). Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jim Fehlig
2010-Apr-22 17:14 UTC
Re: [Xen-devel] earlier remove the backend of tapdisk device in xenstore to release the resource allocated in backend driver lies in dom0''kernel
Jan Beulich wrote:>>>> "James (song wei)" <jsong@novell.com> 22.04.10 10:08 >>> >>>> >> --- a/tools/python/xen/xend/XendDomainInfo.py Mon Apr 19 17:57:28 2010 +0100 >> +++ b/tools/python/xen/xend/XendDomainInfo.py Thu Apr 22 15:54:01 2010 +0800 >> @@ -2406,8 +2406,13 @@ >> >> def _releaseDevices(self, suspend = False): >> """Release all domain''s devices. Nothrow guarantee.""" >> + t = xstransact("%s/device" % self.vmpath) >> if self.image: >> try: >> + for dev in t.list(''tap''): >> + log.debug("Early removing %s", dev); >> + self.getDeviceController(''tap'').destroyDevice(dev, True) >> + time.sleep(0.1) >> log.debug("Destroying device model") >> self.image.destroyDeviceModel() >> except Exception, e: >> @@ -2416,9 +2421,10 @@ >> log.debug("No device model") >> >> log.debug("Releasing devices") >> - t = xstransact("%s/device" % self.vmpath) >> try: >> for devclass in XendDevices.valid_devices(): >> + if devclass is ''tap'': >> + continue >> for dev in t.list(devclass): >> try: >> log.debug("Removing %s", dev); >> > > This seems more like a hack than a solution: Surely qemu-dm gets > sent some sort of signal to shut down and clean up. The question > thus really is why that cleanup doesn''t include cleaning up blktap > related resources. That is, I would expect the fix to be in qemu-dm, > or at most in the xend code that reaps qemu-dm. >Agreed. qemu-dm should clean up these resources on shutdown. AFAICT, it currently relies on receiving CTRMSG_CLOSE from blktapctrl (see ioemu-dir/hw/xen_blktap.c), which it may never receive before exiting. Regards, Jim _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
James (song wei)
2010-Apr-23 01:48 UTC
Re: [Xen-devel] earlier remove the backend of tapdisk device in xenstore to release the resource allocated in backend driver lies in dom0''kernel
Jim Fehlig wrote:> >>Agreed. qemu-dm should clean up these resources on shutdown. AFAICT, >>it currently relies on receiving CTRMSG_CLOSE from blktapctrl (see >>ioemu-dir/hw/xen_blktap.c), which it may never receive before exiting. > > No, I don''t think so. The root cause in this issue is qemu-dm exit too > earlier. > Qemu-dm has been destroyed before blktapctrl send "CTRMSG_CLOSE", > moreover, removing the backend node in xenstore trigers the callback in > blktapctrl to send "CTRMSG_CLOSE". > It''s xend to decide who ought to be occured earlier between sending SIGHUP > to qemu-dm and removing the backend node in xenstore. Obviously, xend is > the role of manager. So I think fix this issue in xend is appropriate. > In addition, If we really want to fix it in qemu, we have to intercept > the exit signal sent by kill() in xend. I don''t think intercept this > singal in qemu for this point is proper. > > -Jame (Song Wei) > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel > >-- View this message in context: http://old.nabble.com/earlier-remove-the-backend-of-tapdisk-device-in-xenstore-to-release-the-resource-allocated-in-backend-driver-lies-in-dom0%27kernel-tp28325456p28336546.html Sent from the Xen - Dev mailing list archive at Nabble.com. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
James (song wei)
2010-Apr-23 03:24 UTC
Re: [Xen-devel] earlier remove the backend of tapdisk device in xenstore to release the resource allocated in backend driver lies in dom0''kernel
Jan Beulich wrote:> > > > >> More generally any solution should be generic, or it should be >> explained properly why the device class needs treatment >> different from any of the other ones (and, for this specific >> case, why moving the device destruction a little ahead will >> now *guarantee* that the cleanup can happen as expected). > > > Moving the device destruction a little ahead of killing qemu-dm would > triger blktapctl thread send CTRMSG_CLOSE to "qemu-dm" before it exit. > And then, qemu-dm would notify backend driver to release resouce by > calling release() of driver through closing the opened device file of > "/dev/xen/blktapN" > > thanks Jim and Jan, > > -James > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel > >-- View this message in context: http://old.nabble.com/earlier-remove-the-backend-of-tapdisk-device-in-xenstore-to-release-the-resource-allocated-in-backend-driver-lies-in-dom0%27kernel-tp28325456p28337027.html Sent from the Xen - Dev mailing list archive at Nabble.com. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2010-Apr-23 08:13 UTC
Re: [Xen-devel] earlier remove the backend of tapdisk device in xenstore to release the resource allocated in backend driver lies in dom0''kernel
>>> "James (song wei)" <jsong@novell.com> 23.04.10 05:24 >>> >Jan Beulich wrote: >> More generally any solution should be generic, or it should be >> explained properly why the device class needs treatment >> different from any of the other ones (and, for this specific >> case, why moving the device destruction a little ahead will >> now *guarantee* that the cleanup can happen as expected). > > > Moving the device destruction a little ahead of killing qemu-dm would > triger blktapctl thread send CTRMSG_CLOSE to "qemu-dm" before it exit. > And then, qemu-dm would notify backend driver to release resouce by > calling release() of driver through closing the opened device file of > "/dev/xen/blktapN"Even though Keir already applied your patch, I continue to disagree: The only function you modified is XendDomainInfo._releaseDevices(), which itself doesn''t kill qemu-dm afaict (I didn''t actually spot where it gets killed). Hence the only effect you achieve is that the window in time for the CTRLMSG_CLOSE to arrive gets enlarged (the insertion of time.sleep(0.1) is also a sign of just using heuristics instead of proper sequencing of events). You still can''t guarantee that the message will arrive in time (i.e. if qemu-dm doesn''t get scheduled soon enough), and hence you just decreased the likelihood of the original issue. Imo, there''s no way around doing proper cleanup from qemu-dm''s signal handler, or there needs to be better handshaking between xend and qemu-dm. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2010-Apr-23 08:54 UTC
Re: [Xen-devel] earlier remove the backend of tapdisk device in xenstore to release the resource allocated in backend driver lies in dom0''kernel
On 23/04/2010 09:13, "Jan Beulich" <JBeulich@novell.com> wrote:>> Moving the device destruction a little ahead of killing qemu-dm would >> triger blktapctl thread send CTRMSG_CLOSE to "qemu-dm" before it exit. >> And then, qemu-dm would notify backend driver to release resouce by >> calling release() of driver through closing the opened device file of >> "/dev/xen/blktapN" > > Even though Keir already applied your patch, I continue to disagree:I agreed with you and Jim, and subsequently reverted it. K. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
James Song
2010-Apr-23 09:09 UTC
Re: [Xen-devel] earlier remove the backend of tapdisk device in xenstore to release the resource allocated in backend driver lies in dom0''kernel
>>> Keir Fraser <keir.fraser@eu.citrix.com> 2010-4-23 16:54 >>> >> Moving the device destruction a little ahead of killing qemu-dm would >> triger blktapctl thread send CTRMSG_CLOSE to "qemu-dm" before it exit. >> And then, qemu-dm would notify backend driver to release resouce by >> calling release() of driver through closing the opened device file of >> "/dev/xen/blktapN" > > Even though Keir already applied your patch, I continue to disagree:>>I agreed with you and Jim, and subsequently reverted it.>> K.Ok, fine, I''ll rewrite the patch and send again soon. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel