Re: [Qemu-devel] [PATCH] [RFC] aio/async: Add timed bottom-halves

[Paolo Bonzini]

Il 15/07/2013 22:15, Alex Bligh ha scritto:
> Paolo,
> 
> --On 15 July 2013 16:25:01 +0200 Paolo Bonzini <address@hidden> wrote:
> 
> Thanks for the review.
> 
>> Il 06/07/2013 18:24, Alex Bligh ha scritto:
>>> Add timed bottom halves. A timed bottom half is a bottom half that
>>> will not execute until a given time has passed (qemu_bh_schedule_at)
>>> or a given interval has passed (qemu_bh_schedule_in).
>>
>> ... and may be delayed arbitrarily past that given interval if you are
>> running in qemu-img or in other synchronous I/O APIs.
> 
> That's true. However, the problem with timers is worse, in that we
> poll for timers even less frequently as far as I can tell.

Right, we poll for bottom halves during qemu_aio_wait().  We don't poll
for timers.

>> I'm especially
>> worried that this will not have any effect if bdrv_aio_cancel is calling
>> qemu_aio_wait.  bdrv_aio_cancel is presumably one place where you want
>> timeout/reconnect functionality to trigger.
> 
> Well, I'm a newbie here, so may well be wrong but I thought qemu_aio_wait
> /did/ call bottom halves (but didn't call QemuTimers). Provided time
> does actually advance (which inspection suggests it does), then
> these bh's should be called just like any other bh's. I may have missed
> the point here entirely.

So far you are right.

But this only happens if qemu_aio_wait() actually returns, so that on
the next call we poll for timers.  If QEMU is stuck in qemu_aio_wait()'s
infinite-timeout poll(), it will never advance and process the timed
bottom halves.

This goes to the question of having aio_notify() or not.  If you have
it, you will immediately process timed BHs that are "born expired".  For
other bottom halves, there will be no difference if you add it or not.

>> I would really prefer to have a TimeEventNotifier or something like
>> that, which is API-compatibile with EventNotifier (so you can use the
>> regular aio-*.c APIs) but triggers when a given time has passed.
>> Basically an "heavyweight" QEMUTimer; that would be a timerfd on Linux,
>> and a queue timer on Windows.  No idea on other POSIX systems,
>> unfortunately.
> 
> I was trying to use the bh API because that's what the existing block
> drivers use, and what I really wanted was a bh that wouldn't execute
> for a while. Do EventNotifiers run whilst AIO is polling for completion?

Yes, and they can actually interrupt qemu_aio_wait().  See
aio_set_event_notifier.

>> Even better would be to remove the whole timer stuff (POSIX timers,
>> setitimer, and the Win32 equivalents), and just make the timers use a
>> shorter timeout for the main loop.  If you do this, I suspect adding
>> timer support to AioContext would be much simpler.
> 
> In discussion with Stefan H on IRC, I originally suggested moving the
> QemuTimer poll to the AIO loop (or adding another), which is a half
> arsed way to do what you are suggesting. He suggested this would be
> hairy because the existing users might not be safe to be called there.
> This was an attempt at a minimal change to fix that use.
> 
>> BTW, note that qemu-nbd (and qemu-io too) does call timers.
> 
> I'd thought I tested qemu-io. qemu-convert definitely does not.

qemu-io is the tool that is used by the unit tests.  Conversion is in
qemu-img.

Paolo

> Alex
> 
>> Paolo
>>
>>> Any qemu
>>> clock can be used, and times are specified in nanoseconds.
>>>
>>> Timed bottom halves can be used where timers cannot. For instance,
>>> in block drivers where there is no mainloop that calls timers
>>> (qemu-nbd, qemu-img), or where (per address@hidden) the
>>> aio code loops internally and thus timers never get called.
>>>
>>> Signed-off-by: Alex Bligh <address@hidden>
>>
>>
> 
> 
>