Re: [Qemu-devel] [PATCH 2/3] block: change drain to look only at one child at a time

[Kevin Wolf]

Am 07.10.2016 um 18:19 hat Paolo Bonzini geschrieben:
> bdrv_requests_pending is checking children to also wait until internal
> requests (such as metadata writes) have completed.  However, checking
> children is in general overkill.  Children requests can be of two kinds:
> 
> - requests caused by an operation on bs, e.g. a bdrv_aio_write to bs
> causing a write to bs->file->bs.  In this case, the parent's in_flight
> count will always be incremented by at least one for every request in
> the child.

What about node with multiple parents? The in_flight count will be
incremented for one of the parents, but is this good enough if we want
to drain the other one?

Actually, I don't think we ever bothered to consciously define what a
drain operation does, i.e. if only the requests on a single node are
drained or all nodes below it, too. Currently it is implemented
recursively and this is what the bdrv_drain() comment says, too:

 * Wait for pending requests to complete on a single BlockDriverState subtree,
 * and suspend block driver's internal I/O until next request arrives.

If we want to keep the operation defined for the whole subtree, your
assumption made here is wrong. If we want to change it, we need to audit
all callers and check if they need to recurse themselves now.

> - asynchronous metadata writes or flushes.  Such writes can be started
> even if bs's in_flight count is zero, but not after the .bdrv_drain
> callback has been invoked.

I don't think it actually makes a difference for the code, but this
could be data as well (imagine a writeback cache implemented in qemu as
I proposed to do in order to mitigate COW perfomance hits in qcow2).

> This patch therefore changes bdrv_drain to finish I/O in the parent
> (after which the parent's in_flight will be locked to zero), call
> bdrv_drain (after which the parent will not generate I/O on the child
> anymore), and then wait for internal I/O in the children to complete.
> 
> Signed-off-by: Paolo Bonzini <address@hidden>

However, it seems the only way in which you rely on the wrong assumption
is where the old code assumed the same. We should be calling the
BdrvChild .drained_begin/end callbacks for the children, with or without
this patch, so this is a preexisting bug.

Kevin