Patch description: 1. Bug fix for error: "Error: Device /dev/xvdp (51952, tap2) is already connected." (xenstore does not clean after DomU stopped) 2. Bug fix for error: "File ''vhd:/path/.../disk.img'' doesn''t exist." (not correct parsing) 3. Bug fix for error: "Error: Device 51952 not connected" (in config file for DomU we should be use prefix "tap2:tapdisk:xxx" (tap2:xxx) for devices from (aio, ram, qcow, vhd, remus) or "tap:tapdisk:xxx" (tap:xxx) for devices from (sync, vmdk, qcow2, ioemu)) 4. Bug fix for error: "Disk is not accessible" (if use ''tap2''-device type, then ''/dev/xpvd'' may not be accessible immediately after its creation) 5. fix regression related to the previous version of this patch.: http://xenbits.xensource.com/xen-4.0-testing.hg?rev/3544bdd56c32 Signed-off-by: eXeC001er <execooler@gmail.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2010-Jun-27 12:47 UTC
Re: [Xen-devel] [PATCH] blktap2 control function (version 2)
Is this for xen-unstable, or xen-4.0, or both? Either way, it goes via Ian Jackson and Stefano now, just fyi. I''ve cc''ed them. -- Keir On 27/06/2010 13:08, "eXeC001er" <execooler@gmail.com> wrote:> Patch description: > 1. Bug fix for error: "Error: Device /dev/xvdp (51952, tap2) is already > connected." (xenstore does not clean after DomU stopped) > 2. Bug fix for error: "File ''vhd:/path/.../disk.img'' doesn''t exist." (not > correct parsing) > 3. Bug fix for error: "Error: Device 51952 not connected" (in config file for > DomU we should be use prefix "tap2:tapdisk:xxx" (tap2:xxx) for devices from > (aio, ram, qcow, vhd, remus) or "tap:tapdisk:xxx" (tap:xxx) for devices from > (sync, vmdk, qcow2, ioemu)) > 4. Bug fix for error: "Disk is not accessible" (if use ''tap2''-device type, > then ''/dev/xpvd'' may not be accessible immediately after its creation) > 5. fix regression related to the previous version of this > patch.: http://xenbits.xensource.com/xen-4.0-testing.hg?rev/3544bdd56c32 > > Signed-off-by: eXeC001er <execooler@gmail.com> >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
eXeC001er
2010-Jun-27 13:19 UTC
Re: [Xen-devel] [PATCH] blktap2 control function (version 2)
blktap2_ctrl_func.patch - for xen-4.x-testing only (because in xen-unstable i see previous version of patch) * * *====================* attach in current e-mail: blktap2-fix-regression.patch - for xen-unstable 1. Fix regression http://xenbits.xensource.com/xen-4.0-testing.hg?rev/3544bdd56c32 2. Fix README for blktap2 * * *Signed-off-by: eXeC001er <execooler@gmail.com> * * * 2010/6/27 Keir Fraser <keir.fraser@eu.citrix.com>> Is this for xen-unstable, or xen-4.0, or both? Either way, it goes via Ian > Jackson and Stefano now, just fyi. I''ve cc''ed them. > > -- Keir > > On 27/06/2010 13:08, "eXeC001er" <execooler@gmail.com> wrote: > > > Patch description: > > 1. Bug fix for error: "Error: Device /dev/xvdp (51952, tap2) is already > > connected." (xenstore does not clean after DomU stopped) > > 2. Bug fix for error: "File ''vhd:/path/.../disk.img'' doesn''t exist." (not > > correct parsing) > > 3. Bug fix for error: "Error: Device 51952 not connected" (in config file > for > > DomU we should be use prefix "tap2:tapdisk:xxx" (tap2:xxx) for devices > from > > (aio, ram, qcow, vhd, remus) or "tap:tapdisk:xxx" (tap:xxx) for devices > from > > (sync, vmdk, qcow2, ioemu)) > > 4. Bug fix for error: "Disk is not accessible" (if use ''tap2''-device > type, > > then ''/dev/xpvd'' may not be accessible immediately after its creation) > > 5. fix regression related to the previous version of this > > patch.: http://xenbits.xensource.com/xen-4.0-testing.hg?rev/3544bdd56c32 > > > > Signed-off-by: eXeC001er <execooler@gmail.com> > > > > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Jackson
2010-Jun-28 15:45 UTC
Re: [Xen-devel] [PATCH] blktap2 control function (version 2)
eXeC001er writes ("[Xen-devel] [PATCH] blktap2 control function (version
2)"):> Patch description:
Thanks for your contribution.
> - if not os.access(disk, os.R_OK):
> - msg = "Disk isn''t accessible"
> - log.error(msg)
> - raise VmError(msg)
> + attempt = 0
> + while True:
> + if not os.access(disk, os.R_OK) and attempt > 3:
> + msg = "Disk isn''t accessible"
> + log.error(msg)
> + raise VmError(msg)
> + else:
> + break
> + attempt = attempt + 1
Um, can you explain why this is necessary or reasonable ? Why three
tries ? Why do we need to poll for this at all ? Surely if this
helps in any particular situation it means we have a race, which
should be fixed.
> diff -r 7dcfdd45bc9e tools/python/xen/xend/XendDomainInfo.py
> --- a/tools/python/xen/xend/XendDomainInfo.py Tue Jun 22 07:31:14 2010
+0100
> +++ b/tools/python/xen/xend/XendDomainInfo.py Sun Jun 27 14:12:26 2010
+0400
> @@ -3269,7 +3269,7 @@
> log.info("Unmounting %s from %s." %
> (fn, BOOTLOADER_LOOPBACK_DEVICE))
>
> - dom0.destroyDevice(''tap'',
BOOTLOADER_LOOPBACK_DEVICE)
> + dom0.destroyDevice(devtype,
BOOTLOADER_LOOPBACK_DEVICE, force = True)
I don''t understand this area of the code at all well but this change
seems plausible.
> diff -r 7dcfdd45bc9e tools/python/xen/util/blkif.py
> --- a/tools/python/xen/util/blkif.py Tue Jun 22 07:31:14 2010 +0100
> +++ b/tools/python/xen/util/blkif.py Sun Jun 27 14:12:26 2010 +0400
> @@ -87,7 +87,10 @@
> fn = "/dev/%s" %(fn,)
>
> if typ in ("tap", "tap2"):
> - (taptype, fn) = fn.split(":", 1)
> + if fn.count(":") == 1:
> + (taptype, fn) = fn.split(":", 1)
> + else:
> + (taptype, fn) = fn.split(":", 2)[1:3]
I''m not sure I understand this. Is it related to
this:> 2. Bug fix for error: "File ''vhd:/path/.../disk.img''
doesn''t exist." (not
> correct parsing)
?
> 3. Bug fix for error: "Error: Device 51952 not connected" (in
config file
> for DomU we should be use prefix "tap2:tapdisk:xxx" (tap2:xxx)
for devices
> from (aio, ram, qcow, vhd, remus) or "tap:tapdisk:xxx" (tap:xxx)
for devices
> from (sync, vmdk, qcow2, ioemu))
I haven''t tried tap2 at all myself. Is it fully supported everywhere
now ?
Later:> blktap2_ctrl_func.patch - for xen-4.x-testing only (because in xen-unstable
> i see previous version of patch)
Does that mean that xen-unstable needs fixing too ? I''d rather apply
a change to xen-unstable first and test it there.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
eXeC001er
2010-Jun-28 16:10 UTC
Re: [Xen-devel] [PATCH] blktap2 control function (version 2)
2010/6/28 Ian Jackson <Ian.Jackson@eu.citrix.com>> eXeC001er writes ("[Xen-devel] [PATCH] blktap2 control function (version > 2)"): > > Patch description: > > Thanks for your contribution. > > > - if not os.access(disk, os.R_OK): > > - msg = "Disk isn''t accessible" > > - log.error(msg) > > - raise VmError(msg) > > + attempt = 0 > > + while True: > > + if not os.access(disk, os.R_OK) and attempt > 3: > > + msg = "Disk isn''t accessible" > > + log.error(msg) > > + raise VmError(msg) > > + else: > > + break > > + attempt = attempt + 1 > > Um, can you explain why this is necessary or reasonable ? Why three > tries ? Why do we need to poll for this at all ? Surely if this > helps in any particular situation it means we have a race, which > should be fixed. >(sometimes) When i use pygrub i have error: Disk isn''t accessible.> > diff -r 7dcfdd45bc9e tools/python/xen/xend/XendDomainInfo.py > > --- a/tools/python/xen/xend/XendDomainInfo.py Tue Jun 22 07:31:14 2010 > +0100 > > +++ b/tools/python/xen/xend/XendDomainInfo.py Sun Jun 27 14:12:26 2010 > +0400 > > @@ -3269,7 +3269,7 @@ > > log.info("Unmounting %s from %s." % > > (fn, BOOTLOADER_LOOPBACK_DEVICE)) > > > > - dom0.destroyDevice(''tap'', > BOOTLOADER_LOOPBACK_DEVICE) > > + dom0.destroyDevice(devtype, > BOOTLOADER_LOOPBACK_DEVICE, force = True) > > I don''t understand this area of the code at all well but this change > seems plausible. >You can see method ''destroyDevice'' in class ''XendDomainInfo'' in tools/python/xen/xend/XendDomainInfo.py (line 1302 +- 10 lines) (related to "bug-fix 1 and 3" in my list)> > diff -r 7dcfdd45bc9e tools/python/xen/util/blkif.py > > --- a/tools/python/xen/util/blkif.py Tue Jun 22 07:31:14 2010 +0100 > > +++ b/tools/python/xen/util/blkif.py Sun Jun 27 14:12:26 2010 +0400 > > @@ -87,7 +87,10 @@ > > fn = "/dev/%s" %(fn,) > > > > if typ in ("tap", "tap2"): > > - (taptype, fn) = fn.split(":", 1) > > + if fn.count(":") == 1: > > + (taptype, fn) = fn.split(":", 1) > > + else: > > + (taptype, fn) = fn.split(":", 2)[1:3] > > I''m not sure I understand this. Is it related to this: > > 2. Bug fix for error: "File ''vhd:/path/.../disk.img'' doesn''t exist." (not > > correct parsing) > ? >yes> > 3. Bug fix for error: "Error: Device 51952 not connected" (in config file > > for DomU we should be use prefix "tap2:tapdisk:xxx" (tap2:xxx) for > devices > > from (aio, ram, qcow, vhd, remus) or "tap:tapdisk:xxx" (tap:xxx) for > devices > > from (sync, vmdk, qcow2, ioemu)) > > I haven''t tried tap2 at all myself. Is it fully supported everywhere > now ? >see above> > Later: > > blktap2_ctrl_func.patch - for xen-4.x-testing only (because in > xen-unstable > > i see previous version of patch) > > Does that mean that xen-unstable needs fixing too ? I''d rather apply > a change to xen-unstable first and test it there. >xen-unstable have my previous patch, but it can lead to regression (if use ''tap:xxx'' instead ''tap:tapdisk:xxx'')> > Ian. >Thanks. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2010-Jun-29 12:44 UTC
Re: [Xen-devel] [PATCH] blktap2 control function (version 2)
FYI this remains unacked afaik. K. On 27/06/2010 14:19, "eXeC001er" <execooler@gmail.com> wrote:> blktap2_ctrl_func.patch - for xen-4.x-testing only (because in xen-unstable i > see previous version of patch) > > ===================> attach in current e-mail: > blktap2-fix-regression.patch - for xen-unstable > > 1. Fix > regression http://xenbits.xensource.com/xen-4.0-testing.hg?rev/3544bdd56c32 > 2. Fix README for blktap2 > > Signed-off-by: eXeC001er <execooler@gmail.com> > > 2010/6/27 Keir Fraser <keir.fraser@eu.citrix.com> >> Is this for xen-unstable, or xen-4.0, or both? Either way, it goes via Ian >> Jackson and Stefano now, just fyi. I''ve cc''ed them. >> >> -- Keir >> >> On 27/06/2010 13:08, "eXeC001er" <execooler@gmail.com> wrote: >> >>> Patch description: >>> 1. Bug fix for error: "Error: Device /dev/xvdp (51952, tap2) is already >>> connected." (xenstore does not clean after DomU stopped) >>> 2. Bug fix for error: "File ''vhd:/path/.../disk.img'' doesn''t exist." (not >>> correct parsing) >>> 3. Bug fix for error: "Error: Device 51952 not connected" (in config file >>> for >>> DomU we should be use prefix "tap2:tapdisk:xxx" (tap2:xxx) for devices from >>> (aio, ram, qcow, vhd, remus) or "tap:tapdisk:xxx" (tap:xxx) for devices from >>> (sync, vmdk, qcow2, ioemu)) >>> 4. Bug fix for error: "Disk is not accessible" (if use ''tap2''-device type, >>> then ''/dev/xpvd'' may not be accessible immediately after its creation) >>> 5. fix regression related to the previous version of this >>> patch.: http://xenbits.xensource.com/xen-4.0-testing.hg?rev/3544bdd56c32 >>> >>> Signed-off-by: eXeC001er <execooler@gmail.com> >>> >> >> > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Jackson
2010-Jun-29 13:59 UTC
Re: [Xen-devel] [PATCH] blktap2 control function (version 2)
Thanks for your responses. Can I ask you to try to (a) separate out
these patches, and (b) explain the reasoning behind them in more
detail ? If you can provide a separate message with a separate patch
for each change, with an explanation at greater length, it will be
much easier for us to evaluate them. Thanks.
There are a few things I can comment on as is:
eXeC001er writes ("Re: [Xen-devel] [PATCH] blktap2 control function
(version 2)"):> [Ian:]
> > Um, can you explain why this is necessary or reasonable ? Why three
> > tries ? Why do we need to poll for this at all ? Surely if this
> > helps in any particular situation it means we have a race, which
> > should be fixed.
>
> (sometimes) When i use pygrub i have error: Disk isn''t accessible.
I''m afraid that reply doesn''t address my comments. What is
the race
and why is your fix correct ?
> > 3. Bug fix for error: "Error: Device 51952 not connected"
(in config file
> > for DomU we should be use prefix "tap2:tapdisk:xxx"
(tap2:xxx) for devices
> > from (aio, ram, qcow, vhd, remus) or "tap:tapdisk:xxx"
(tap:xxx) for devices
> > from (sync, vmdk, qcow2, ioemu))
After discussing this with my colleagues, it''s not clear to me that we
should be exposing the difference between blktap vs blktap2 in domain
config files. xl tries blktap2 first and uses it if available, and
then uses blktap if not. Is that not the correct behaviour ?
> Does that mean that xen-unstable needs fixing too ? I''d rather
apply
> a change to xen-unstable first and test it there.
>
> xen-unstable have my previous patch, but it can lead to regression (if use
''tap:xxx'' instead ''tap:tapdisk:xxx'')
Should we revert your previous patch while we discuss it ?
Thanks,
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
eXeC001er
2010-Jun-29 14:30 UTC
Re: [Xen-devel] [PATCH] blktap2 control function (version 2)
2010/6/29 Ian Jackson <Ian.Jackson@eu.citrix.com>> Thanks for your responses. Can I ask you to try to (a) separate out > these patches, and (b) explain the reasoning behind them in more > detail ? If you can provide a separate message with a separate patch > for each change, with an explanation at greater length, it will be > much easier for us to evaluate them. Thanks. >OK.> There are a few things I can comment on as is: > > eXeC001er writes ("Re: [Xen-devel] [PATCH] blktap2 control function > (version 2)"): > > [Ian:] > > > Um, can you explain why this is necessary or reasonable ? Why three > > > tries ? Why do we need to poll for this at all ? Surely if this > > > helps in any particular situation it means we have a race, which > > > should be fixed. > > > > (sometimes) When i use pygrub i have error: Disk isn''t accessible. > > I''m afraid that reply doesn''t address my comments. What is the race > and why is your fix correct ? >may be it is not correct, but i can not find other way.> > > > 3. Bug fix for error: "Error: Device 51952 not connected" (in config > file > > > for DomU we should be use prefix "tap2:tapdisk:xxx" (tap2:xxx) for > devices > > > from (aio, ram, qcow, vhd, remus) or "tap:tapdisk:xxx" (tap:xxx) for > devices > > > from (sync, vmdk, qcow2, ioemu)) > > After discussing this with my colleagues, it''s not clear to me that we > should be exposing the difference between blktap vs blktap2 in domain > config files. xl tries blktap2 first and uses it if available, and > then uses blktap if not. Is that not the correct behaviour ? >yes, right. (I need to find more info about ''tap2'' and ''tap'', maybe I''m wrong.).> > > Does that mean that xen-unstable needs fixing too ? I''d rather apply > > a change to xen-unstable first and test it there. > > > > xen-unstable have my previous patch, but it can lead to regression (if > use ''tap:xxx'' instead ''tap:tapdisk:xxx'') > > Should we revert your previous patch while we discuss it ? >I think ''YES''.> > Thanks, > Ian. >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2010-Jun-29 14:30 UTC
Re: [Xen-devel] [PATCH] blktap2 control function (version 2)
On 29/06/2010 14:59, "Ian Jackson" <Ian.Jackson@eu.citrix.com> wrote:>> Does that mean that xen-unstable needs fixing too ? I''d rather apply >> a change to xen-unstable first and test it there. >> >> xen-unstable have my previous patch, but it can lead to regression (if use >> ''tap:xxx'' instead ''tap:tapdisk:xxx'') > > Should we revert your previous patch while we discuss it ?I reverted it from xen-4.0-testing already for obvious reasons. I think it makes sense to do so in xen-unstable too (via your tree) and then re-start from a clean slate in all trees. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
eXeC001er
2010-Jun-30 13:47 UTC
Re: [Xen-devel] [PATCH] blktap2 control function (version 2)
When it rolled back the changes? 2010/6/29 Keir Fraser <keir.fraser@eu.citrix.com>> On 29/06/2010 14:59, "Ian Jackson" <Ian.Jackson@eu.citrix.com> wrote: > > >> Does that mean that xen-unstable needs fixing too ? I''d rather apply > >> a change to xen-unstable first and test it there. > >> > >> xen-unstable have my previous patch, but it can lead to regression (if > use > >> ''tap:xxx'' instead ''tap:tapdisk:xxx'') > > > > Should we revert your previous patch while we discuss it ? > > I reverted it from xen-4.0-testing already for obvious reasons. I think it > makes sense to do so in xen-unstable too (via your tree) and then re-start > from a clean slate in all trees. > > -- Keir > > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel