Il 17/09/2013 18:50, Jan Kiszka ha scritto:
> On 2013-09-17 18:38, Alex Bligh wrote:
>>
>> On 17 Sep 2013, at 17:19, Paolo Bonzini wrote:
>>
>>> Il 17/09/2013 18:09, Jan Kiszka ha scritto:
>>>> On 2013-08-13 16:22, Stefan Hajnoczi wrote:
>>>>> On Tue, Aug 13, 2013 at 03:45:44PM +0200, Jan Kiszka wrote:
>>>>>> Yeah:
>>>>>>
>>>>>> - /* No AIO operations? Get us out of here */
>>>>>> - if (!busy) {
>>>>>> + /* early return if we only have the aio_notify() fd */
>>>>>> + if (ctx->pollfds->len == 1) {
>>>>>> return progress;
>>>>>> }
>>>>>>
>>>>>> So this is even worse for my use case.
>>>>>
>>>>> We can change the semantics of aio_poll() so long as we don't break
>>>>> existing callers and tests. It would make sense to do that after
>>>>> merging the io_flush and AioContext timers series.
>>>>
>>>> Need to pick up this topic again because above change is now mainline
>>>> and breaks aio_poll-based timer threads:
>>>>
>>>> How can we make progress with overcoming that check, at least for the
>>>> timer thread use case? Additional argument "truly_block" for aio_poll?
>>>
>>> I wonder if we still need that "if" at all. Guys, do you remember what
>>> it is good for? O:-)
>>
>> I can't remember whether the code above was in my patch or Stefan's
>> subsequent ones, but I had suggested
>> if (blocking && (ctx->pollfds->len <= 1))
>>
>> On reflection, I don't think the 'if' line should be there at all.
>>
>> I think the argument was that it was meant to be for stupidity
>> protection, i.e. where someone calls with blocking set to 1 and no
>> fds, it would block indefinitely (if there were no timers)
>> as it would select with no timeout and no fds.
>>
>> Personally I think if you call things this way you deserve all you
>> get. I'm not sure attempting to rescue the user by returning
>> immediately is a great plan. If there are truly no fds at all
>> (not even the notify fd) and an infinite timeout perhaps we should
>> set the timeout to a second and log an error?
>>
>> I presume the reason it's breaking aio_poll based timer threads is
>> because there is only one fd (the aio_notify_fd), there are
>> no other fd's, but there is a timeout (from the timers)?
>
> Right.
>
>> If
>> that's true, I think we /shouldn't/ return. Equally if there
>> are no timers but something is genuinely attempting to wait
>> on an aio_notify, I don't think we should return.
>>
>
> In any case, test-aio seems to stress that if clause. If we remove it,
> the test case hangs infinitely. But I'm more worried about understanding
> if there are actual users depending on the current behavior
> (bdrv_drain_all?).
Yes, bdrv_drain_all comes to mind. But now we should be able to fix
this old remark:
/* FIXME: We do not have timer support here, so this is effectively
* a busy wait.
*/
QTAILQ_FOREACH(bs, &bdrv_states, list) {
if (bdrv_start_throttled_reqs(bs)) {
busy = true;
}
}
busy = bdrv_requests_pending_all();
busy |= aio_poll(qemu_get_aio_context(), busy);
Alex, what's missing before block.c and QED can use aio_timer_new on
the main AioContext, instead of timer_new?
Paolo
