[Qemu-devel] Xen patches - status summary

[Qemu-devel] Xen patches - status summary

[Ian Jackson]

I don't know about you but I'm starting to lose track of all the
patches we're submitting from the Xen tree.  So here's a summary of
recent activity:

Patches from Stefano Stabellini awaiting commit to qemu:

 [1/3] vnc dynamic resolution	} minor discussion, all issues resolved
 [2/3] WMVi extension support	}  so should IMO be applied (updated version
 [3/3] vga shared buffer	}  in case of vga shared buffer patch)
 sdl shared buffer support	  no discussion, should be applied IMO
 opengl rendering in the sdl ...  no discussion, should be applied IMO

Patches from me awaiting commit to qemu:

 Use fd signal trick to ... select	discussed, should be applied[1]
 Introduce #define QEMU_ASYNC_EVENTLOOP	discussed, should be applied[1]
 usb-linux.c: #define __user ...	discussed, should be applied[2]
 IDE error checking (2 patches)		2nd repost, should be applied[3]

Patches from me currently in discussion / awaiting rework:

 wrap up use of dangerous ctype.h ...	rework suggested by Anthony
 IDE write cache (3 patches)		2nd repost, under discussion[4]

Recent patches from me no longer needing attention:

 suppress ... warnings in scsi-generic	applied, thanks
 Do not try to use -net user as ...	applied, thanks
 Check that ... submission succeeds	superseded by `IDE error ...'

Looking at my git logs here there are quite a few more which will be
forthcoming but perhaps it would be good to digest these first ?

Thanks,
Ian.

Discussion:

[1] Re: Use fd signal trick to break us out of select
    Re: Introduce #define QEMU_ASYNC_EVENTLOOP

Anthony Ligouri had a different patch based on a helper thread to
provide an emulation of signalfd rather than use of a signal handler.
However (as confirmed by Jamie Lokier) some versions of glibc have
a bug in the handling of the glibc internal aio helper thread(s)
which can make efforts to block signals ineffective.  This means
that Anthony's patch will not work properly on those libcs and as
I wrote already this means that my patch should be applied.

The QEMU_ASYNC_EVENTLOOP change is a tidying up of the NBD feature to
make qemu-nbd share more commonality with qemu-img.  As discussed
there are perhaps even more cleanups that could be done to improve
this but as I say this change is a good start and should be applied.

[2] Re: usb-linux.c: #define __user to work around broken Linux headers

Thiemo Seufer complained that `distros should fix their broken
headers instead', which as I said is I think a ridiculous argument.
The headers have indeed been fixed in most upstreams but we would like
to be portable to older unfixed systems.  No-one else has commented.

[3] Re: IDE error checking

I summarised the discussion in my message `[REPOST] [PATCH 0/2]'.
There has been no recent response.  For the reasons I explain, I think
this patch should be applied now without waiting for any
ENOSPC-auto-pause feature.  Daniel Berrange appeared to agree last
time this was discussed.

This has been previously discussed in February 2008; a less complete
version of the same patch was posted by Samuel Thibault in early
August 2008 and withdrawn in favour of this patchset.

[4] Re: IDE write cache

This is tangentially related to Andrea Arcangeli's comments about IDE
write cacheing; a similar discussion was had about this patch
previously.

This patch has been previously submitted in February 2008 and again in
late March and early April 2008.

If Andrea's comments are justified then a change to qemu is needed
anyway and that could most fruitfully be made on top of this patchset.
So for this reason, and the others I have given, I think this should
be applied now but I appreciate that people might want to wait for the
discussion to settle.

-- 

Re: [Qemu-devel] Xen patches - status summary

[Gerd Hoffmann]

Ian Jackson wrote:
> I don't know about you but I'm starting to lose track of all the
> patches we're submitting from the Xen tree.  So here's a summary of
> recent activity:
> 
> Patches from Stefano Stabellini awaiting commit to qemu:
> 
>  [1/3] vnc dynamic resolution	} minor discussion, all issues resolved
>  [2/3] WMVi extension support	}  so should IMO be applied (updated version
>  [3/3] vga shared buffer	}  in case of vga shared buffer patch)
>  sdl shared buffer support	  no discussion, should be applied IMO
>  opengl rendering in the sdl ...  no discussion, should be applied IMO

I think they still sitting in anthonys inbox waiting for review ...

> Looking at my git logs here there are quite a few more which will be
> forthcoming but perhaps it would be good to digest these first ?

In case you don't know what to do with your time meanwhile:
/me waits for qemu-xen catching up with upstream, so I can update and
respin the patch series ;)

> [1] Re: Use fd signal trick to break us out of select
>     Re: Introduce #define QEMU_ASYNC_EVENTLOOP
> 
> Anthony Ligouri had a different patch based on a helper thread to
> provide an emulation of signalfd rather than use of a signal handler.
> However (as confirmed by Jamie Lokier) some versions of glibc have
> a bug in the handling of the glibc internal aio helper thread(s)
> which can make efforts to block signals ineffective.  This means
> that Anthony's patch will not work properly on those libcs and as
> I wrote already this means that my patch should be applied.

I also tend to dislike os-specific stuff due to the portability issues
it brings.  Sometimes there are good reasons to use it nevertheless.

> The QEMU_ASYNC_EVENTLOOP change is a tidying up of the NBD feature to
> make qemu-nbd share more commonality with qemu-img.  As discussed
> there are perhaps even more cleanups that could be done to improve
> this but as I say this change is a good start and should be applied.

Cleanups is, well, not the correct word IMHO.  The complete block device
handling needs a major redesign.  That this ifdef is needed in the first
place is a blatant layering violation.  Also we should be able to
support async I/O in some form (be it libaio, threads or whatever)
without hacking support for it into each and every file format handler.

> [2] Re: usb-linux.c: #define __user to work around broken Linux headers
> 
> Thiemo Seufer complained that `distros should fix their broken
> headers instead', which as I said is I think a ridiculous argument.
> The headers have indeed been fixed in most upstreams but we would like
> to be portable to older unfixed systems.  No-one else has commented.

It's fixed for quite a while.  Any *supported* distros where this is
still an issue?  I saw you mention Fedora Core 6.  There are no
(security) updates any more for this one, so nobody should seriously use
it these days ...

As those workarounds tend to hand around even when the reason to
introduce them is long gone I'd agree to better not apply this one.

> [3] Re: IDE error checking

Yep, this certainly needs to be fixed.
/me votes for applying them, although I don't know IDE good enougth to
comment on the patches themself.

cheers,
  Gerd

Re: [Qemu-devel] Xen patches - status summary

[Anthony Liguori]

Gerd Hoffmann wrote:
> Ian Jackson wrote:
>   
>> I don't know about you but I'm starting to lose track of all the
>> patches we're submitting from the Xen tree.  So here's a summary of
>> recent activity:
>>
>> Patches from Stefano Stabellini awaiting commit to qemu:
>>
>>  [1/3] vnc dynamic resolution	} minor discussion, all issues resolved
>>  [2/3] WMVi extension support	}  so should IMO be applied (updated version
>>  [3/3] vga shared buffer	}  in case of vga shared buffer patch)
>>  sdl shared buffer support	  no discussion, should be applied IMO
>>  opengl rendering in the sdl ...  no discussion, should be applied IMO
>>     
>
> I think they still sitting in anthonys inbox waiting for review ...
>   

Yeah, sorry, I'm on vacation this week and my network access is much 
worse than I expected it to be.  I'm queuing it all up and will go 
through it this weekend when I get back.

>> Looking at my git logs here there are quite a few more which will be
>> forthcoming but perhaps it would be good to digest these first ?
>>     
>
> In case you don't know what to do with your time meanwhile:
> /me waits for qemu-xen catching up with upstream, so I can update and
> respin the patch series ;)
>
>   
>> [1] Re: Use fd signal trick to break us out of select
>>     Re: Introduce #define QEMU_ASYNC_EVENTLOOP
>>
>> Anthony Ligouri had a different patch based on a helper thread to
>> provide an emulation of signalfd rather than use of a signal handler.
>> However (as confirmed by Jamie Lokier) some versions of glibc have
>> a bug in the handling of the glibc internal aio helper thread(s)
>> which can make efforts to block signals ineffective.  This means
>> that Anthony's patch will not work properly on those libcs and as
>> I wrote already this means that my patch should be applied.
>>     
>
> I also tend to dislike os-specific stuff due to the portability issues
> it brings.  Sometimes there are good reasons to use it nevertheless.
>   

I haven't made up my mind yet about which approach is better.  I'll 
commit something this weekend though.

>> The QEMU_ASYNC_EVENTLOOP change is a tidying up of the NBD feature to
>> make qemu-nbd share more commonality with qemu-img.  As discussed
>> there are perhaps even more cleanups that could be done to improve
>> this but as I say this change is a good start and should be applied.
>>     
>
> Cleanups is, well, not the correct word IMHO.  The complete block device
> handling needs a major redesign.  That this ifdef is needed in the first
> place is a blatant layering violation.  Also we should be able to
> support async I/O in some form (be it libaio, threads or whatever)
> without hacking support for it into each and every file format handler.
>   

I have something queued up to help us get closer to this.

>> [3] Re: IDE error checking
>>     
>
> Yep, this certainly needs to be fixed.
> /me votes for applying them, although I don't know IDE good enougth to
> comment on the patches themself.
>   

I think I explained what I didn't like about this patch--that all IO 
errors were being reported to the guest as ECC errors.  I don't feel 
this is correct, in particular for ENOSPC.  When ENOSPC occurs, the 
guest is likely to really mess itself up.  Since ENOSPC is almost 
certainly going to be the most common error, it needs to be handled 
specially.

Regards,

Anthony Liguori

> cheers,
>   Gerd
>
>
>
>
>   

Re: [Qemu-devel] Xen patches - status summary

[Anthony Liguori]

Ian Jackson wrote
> [3] Re: IDE error checking
>
> I summarised the discussion in my message `[REPOST] [PATCH 0/2]'.
> There has been no recent response.  For the reasons I explain, I think
> this patch should be applied now without waiting for any
> ENOSPC-auto-pause feature.

For the vast majority of users, ENOSPC is the only realistic error that 
will occur.  I don't think a patch that doesn't handle ENOSPC in a sane 
way is really that useful.

> [4] Re: IDE write cache
>   

I have to review these patches but I do agree with the theory behind them.

Regards,

Anthony Liguori

Re: [Qemu-devel] Xen patches - status summary

[Ian Jackson]

Anthony Liguori writes ("Re: [Qemu-devel] Xen patches - status summary"):
> Ian Jackson wrote
> > I summarised the discussion in my message `[REPOST] [PATCH 0/2]'.
> > There has been no recent response.  For the reasons I explain, I think
> > this patch should be applied now without waiting for any
> > ENOSPC-auto-pause feature.
> 
> For the vast majority of users, ENOSPC is the only realistic error that 
> will occur.  I don't think a patch that doesn't handle ENOSPC in a sane 
> way is really that useful.

I find it surprising that you're taking this line.

Are you really saying that it is better for the guest's data to be
silently corrupted, by pretending that the write happened when in fact
it didn't, than it is to give the guest an error indication ?  The
current code in qemu silently discards many error indications and
allows the guest to continue in blissful ignorance.

If that's not what you're saying then surely my patch is an
improvement ?  The fact that it doesn't make the situation as good as
you think it could be doesn't mean that it isn't correct or useful.

Even if you think my patch is pointless it makes no harmful change to
the functionality and lays the groundwork for a more sophisticated
treatment: it adds the missing return values to bdrv_ functions, and
the capability to return non-ENOSPC errors to guests.

It may be the case that for many of qemu's users errors are largely a
result of COW ENOSPC, and that this differs from the situation with
Xen where more of the users are doing RAID or NAS with more
sophisticated guests which can be expected to handle errors properly.
That might explain a difference in emphasis and lead to different
conclusions about whether (for example) the default should be to pause
a VM when ENOSPC is detected.

But _even if_ you think the right default behaviour is actually to
pause a VM when ENOSPC is detected, there are certainly users and
contexts who will want a different configuration (in the Xen case it's
very tricky for qemu to unilaterally pause the VM).

Ian.

Re: [Qemu-devel] Xen patches - status summary

[Ian Jackson]

Anthony Liguori writes ("Re: [Qemu-devel] Xen patches - status summary"):
> Yeah, sorry, I'm on vacation this week and my network access is much 
> worse than I expected it to be.  I'm queuing it all up and will go 
> through it this weekend when I get back.

Fair enough, that's great, thanks.  There are of course more potential
patches awaiting preparation.  I'll post another summary some time
next week, when it seems like things have changed enough.

> > I also tend to dislike os-specific stuff due to the portability issues
> > it brings.  Sometimes there are good reasons to use it nevertheless.
> 
> I haven't made up my mind yet about which approach is better.  I'll 
> commit something this weekend though.

Thanks.  Did you see Jamie's comments about the (quite alarming)
glibc 2.3.1 aio thread signal mask bug ?  If it weren't for that I
would say there's not that much to choose between our approaches but
under the circumstances I think the signal handler rather than helper
thread approach is essential.

If you are dead set on using a signalfd emulation, I could rework my
patch so that it contains an interface roughly equivalent to signalfd,
so that signalfd can easily be used where available.

> >> The QEMU_ASYNC_EVENTLOOP change is a tidying up of the NBD feature to
> >> make qemu-nbd share more commonality with qemu-img.  As discussed
> >> there are perhaps even more cleanups that could be done to improve
> >> this but as I say this change is a good start and should be applied.
> >
> > Cleanups is, well, not the correct word IMHO.  The complete block device
> > handling needs a major redesign.  That this ifdef is needed in the first
> > place is a blatant layering violation.  Also we should be able to
> > support async I/O in some form (be it libaio, threads or whatever)
> > without hacking support for it into each and every file format handler.
> 
> I have something queued up to help us get closer to this.

Err.  All that my QEMU_ASYNC_EVENTLOOP patch does is to replace
   #if !defined(QEMU_IMG) && !defined(QEMU_NBD)
with
   #ifdef QEMU_ASYNC_EVENTLOOP
which really doesn't seem controversial.  If there was some objection
to the structure of the current code, it should have been raised
before qemu-img and qemu-nbd were committed, surely ?

I think my main point in this context is that we should not make the
best be the enemy of the good.  I am carefully preparing my patches so
that they are always strictly improving, and so that any controversial
or major changes are separated out.

It is frustrating to have a trivial but clear improvement blocked by a
discussion which essentially says that it doesn't solve the entire
problem because what is really needed is a complete restructuring.

Am I supposed to hold off, and allow my trivial improvments to rot
again, for an indefinite time while awaiting the Glorious Republic to
Last a Thousand Years ?

I think the rule should generally be that a patch should be applied if
it (a) makes some improvement, no matter how minor and (b) does not
make any future desirable restructuring harder.  The fact that a
restructuring is desirable does not mean that minor improvements
should be blocked in the meantime.

Ian.

Re: [Qemu-devel] Xen patches - status summary

[Anthony Liguori]

Ian Jackson wrote:
>>>> The QEMU_ASYNC_EVENTLOOP change is a tidying up of the NBD feature to
>>>> make qemu-nbd share more commonality with qemu-img.  As discussed
>>>> there are perhaps even more cleanups that could be done to improve
>>>> this but as I say this change is a good start and should be applied.
>>>>         
>>> Cleanups is, well, not the correct word IMHO.  The complete block device
>>> handling needs a major redesign.  That this ifdef is needed in the first
>>> place is a blatant layering violation.  Also we should be able to
>>> support async I/O in some form (be it libaio, threads or whatever)
>>> without hacking support for it into each and every file format handler.
>>>       
>> I have something queued up to help us get closer to this.

That was an FYI, it wasn't about your patch.  I agree your patch is a 
step in the right direction.

Regards,

Anthony Liguori

Re: [Qemu-devel] Xen patches - status summary

[Anthony Liguori]

Ian Jackson wrote:
> Anthony Liguori writes ("Re: [Qemu-devel] Xen patches - status summary"):
>
> I think the rule should generally be that a patch should be applied if
> it (a) makes some improvement, no matter how minor and (b) does not
> make any future desirable restructuring harder.  The fact that a
> restructuring is desirable does not mean that minor improvements
> should be blocked in the meantime.
>   

There's a balance between band-aid fixing and making forward progress in 
improving things.  Honestly, you've spent more time writing emails 
trying to avoid adding special handling for ENOSPC than it would have 
taken to just update the original patch.

Reporting an IDE error on ENOSPC is just plain wrong.  It doesn't matter 
if already broken and you're just changing how it's broken.  It's not 
like it's a massive effort to update the patch.  I'll eventually get to 
it myself but it's a whole lot easier for you to just do it and then we 
can move on to more important things :-)

Regards,

Anthony Liguori

> Ian.
>
>
>