qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [PATCH v2 00/11] aio: Introduce handler type to fix nes


From: Kevin Wolf
Subject: Re: [Qemu-block] [PATCH v2 00/11] aio: Introduce handler type to fix nested aio_poll for dataplane
Date: Fri, 11 Sep 2015 12:39:26 +0200
User-agent: Mutt/1.5.21 (2010-09-15)

Am 29.07.2015 um 06:42 hat Fam Zheng geschrieben:
> v2: Switch to disable/enable model. [Paolo]
> 
> Most existing nested aio_poll()'s in block layer are inconsiderate of
> dispatching potential new r/w requests from ioeventfds and nbd exports, which
> might result in responsiveness issues (e.g. bdrv_drain_all will not return 
> when
> new requests keep coming), or even wrong semantics (e.g. qmp_transaction 
> cannot
> enforce atomicity due to aio_poll in bdrv_drain_all).
> 
> Previous attampts to address this issue include new op blocker[1], 
> bdrv_lock[2]
> and nested AioContext (patches not posted to qemu-devel).
> 
> This approach is based on the idea proposed by Paolo Bonzini. The original 
> idea
> is introducing "aio_context_disable_client / aio_context_enable_client to
> filter AioContext handlers according to the "client", e.g.
> AIO_CLIENT_DATAPLANE (ioeventfd), AIO_CLIENT_PROTOCOL, AIO_CLIENT_NBD_SERVER,
> AIO_CLIENT_CONTEXT, ... Extend aio_set_{event_notifier,fd}_handler to pass a
> client (type)." 
> 
> After this series, block layer aio_poll() will only process those "protocol"
> fds that are used in block I/O, plus the ctx->notifier for aio_notify();  
> other
> aio_poll()'s keep unchanged.
> 
> The biggest advantage over approaches [1] and [2] is, no change is needed in
> virtio-{blk,scsi}-dataplane code, also this doesn't depend on converting QMP 
> to
> coroutines.

It seems that I haven't replied on the mailing list yet, even though I
think I already said this in person at KVM Forum: This series fixes only
a special case of the real problem, which is that bdrv_drain/all at a
single point doesn't make a lot of sense, but needs to be replaced by a
whole section with exclusive access, like a bdrv_drained_begin/end pair.

To be clear: Anything that works with types of users instead of
individual users is bound to fall short of being a complete solution. I
don't prefer partial solutions when we know there is a bigger problem.

This series addresses your immediate need of protecting against new data
plane requests, which it arguably achieves. The second case I always
have in mind is Berto's case where he has multiple streaming block jobs
in the same backing file chain [1].

This involves a bdrv_reopen() of the target BDS to make it writable, and
bdrv_reopen() uses bdrv_drain_all() so drivers don't have to cope with
running requests while reopening themselves. It can however involve
nested event loops for synchronous operations like bdrv_flush(), and if
those can process completions of block jobs, which can respond by doing
anything to the respective node, things can go wrong.

You don't solve this by adding client types (then problematic request
would be PROTOCOL in your proposal and you can never exclude that), but
you really need to have bdrv_drained_being/end pairs, where only
requests issued in between are processed and everything else waits.

Kevin

[1] https://lists.gnu.org/archive/html/qemu-devel/2015-06/msg06273.html



reply via email to

[Prev in Thread] Current Thread [Next in Thread]