Re: [Qemu-devel] [PATCH v6 12/13] block: Block "device IO" during bdrv_drain and bdrv_drain_all

[Max Reitz <mreitz@redhat.com>]

On 25.05.2015 04:48, Fam Zheng wrote:
> On Sat, 05/23 19:11, Max Reitz wrote:
>> On 21.05.2015 08:43, Fam Zheng wrote:
>>> We don't want new requests from guest, so block the operation around the
>>> nested poll.
>>>
>>> It also avoids looping forever when iothread is submitting a lot of requests.
>>>
>>> Signed-off-by: Fam Zheng <famz@redhat.com>
>>> ---
>>>   block/io.c | 22 ++++++++++++++++++++--
>>>   1 file changed, 20 insertions(+), 2 deletions(-)
>> Hm, I don't know about this. When I see someone calling
>> bdrv_drain()/bdrv_drain_all(), I'm expecting that every request has been
>> drained afterwards. This patch implies that this is not necessarily the
>> case, because apparently in some configurations the guest can still submit
>> I/O even while bdrv_drain() is running,
> In dataplane, aio_poll in bdrv_drain_all will poll the ioeventfd, which could
> call the handlers of virtio queues. That's how guest I/O sneaks in.
>
>
>> but this means that even after this
>> patch, the same can happen if I/O is submitted after bdrv_op_unblock() and
>> before anything the caller of bdrv_drain() wants to do while the BDS is
>> still drained. So this looks to me more like the caller must ensure that the
>> BDS won't receive new requests, and do so before bdrv_drain() is called.
> Yes, callers of bdrv_drain*() should use a blocker like you reasoned. Other
> patches in this series looked at qmp_transaction, but there are more, which may
> still be wrong until they're fixed.

Well, I can't say that I like fixes which are known (or at least 
suspected) to not be complete very much, but well... It's always better 
to fix something than nothing at all. The issue always is not forgetting 
about the potential problem.

Reviewed-by: Max Reitz <mreitz@redhat.com>

> This patch, however, fixes one of the potential issues of those callers:
>
>>> It also avoids looping forever when iothread is submitting a lot of
>>> requests.
> Fam