[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 2/2] mirror: Wait only for in-flight operations
From: |
Max Reitz |
Subject: |
Re: [PATCH v2 2/2] mirror: Wait only for in-flight operations |
Date: |
Thu, 26 Mar 2020 19:27:14 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.6.0 |
On 26.03.20 16:36, Kevin Wolf wrote:
> mirror_wait_for_free_in_flight_slot() just picks a random operation to
> wait for. However, a MirrorOp is already in s->ops_in_flight when
> mirror_co_read() waits for free slots, so if not enough slots are
> immediately available, an operation can end up waiting for itself, or
> two or more operations can wait for each other to complete, which
> results in a hang.
>
> Fix this by adding a flag to MirrorOp that tells us if the request is
> already in flight (and therefore occupies slots that it will later
> free), and picking only such operations for waiting.
>
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1794692
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
> block/mirror.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/block/mirror.c b/block/mirror.c
> index 393131b135..88414d1653 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
[...]
> @@ -1318,6 +1324,7 @@ static MirrorOp *coroutine_fn
> active_write_prepare(MirrorBlockJob *s,
> .offset = offset,
> .bytes = bytes,
> .is_active_write = true,
> + .is_in_flight = true,
> };
> qemu_co_queue_init(&op->waiting_requests);
> QTAILQ_INSERT_TAIL(&s->ops_in_flight, op, next);
>
There is a mirror_wait_on_conflicts() call after this. I was a bit
worried about dependencies there. But I don’t think there’s any
problem, because mirror_wait_for_any_operation() is only called by:
(1) mirror_wait_for_free_in_flight_slot(), which makes it look for
non-active operations only, and
(2) mirror_run(), which specifically waits for all active operations to
settle, so it makes sense to wait for all of them, even when they are
still doing their own dependency-waiting.
But still, I’m not sure whether this is conceptually the best thing to
do. I think what we actually want is for
mirror_wait_for_free_in_flight_slot() to only wait for in-flight
operations; but the call in mirror_run() that waits for active-mirror
operations wants to wait for all active-mirror operations, not just the
ones that are in flight.
So I think conceptually it would make more sense to set is_in_flight
only after mirror_wait_on_conflicts(), and ensure that the
mirror_wait_for_any_operation() call from mirror_run() ignores
is_in_flight. E.g. by having another parameter “bool in_flight” for
mirror_wait_for_any_operation() that chooses whether to check
is_in_flight or whether to ignore it.
In practice, @in_flight would always be the same as @active, but they
are different things. But that would mean we would always ignore
is_in_flight for active-mirror operations.
In practice, there’s no difference to what this patch does, i.e. to just
let active-mirror operations have is_in_flight to be always true and let
mirror_wait_for_any_operation() check is_in_flight unconditionally.
So I don’t know. Maybe this is a start:
Functionally-reviewed-by: Max Reitz <address@hidden>
Max
signature.asc
Description: OpenPGP digital signature