Re: [PATCH] mirror: Avoid assertion failed in mirror_run

[Hanna Reitz <hreitz@redhat.com>]

[CC-ing qemu-block, Vladimir, Kevin, and John – when sending patches, 
please look into the MAINTAINERS file or use the 
scripts/get_maintainer.pl script to find out who to CC on them.  It’s 
very to overlook patches on qemu-devel :/]

On 07.12.21 11:56, Yi Wang wrote:
> From: Long YunJian <long.yunjian@zte.com.cn>
>
> when blockcommit from active leaf node, sometimes, we get assertion failed with
> "mirror_run: Assertion `QLIST_EMPTY(&bs->tracked_requests)' failed" messages.
> According to the core file, we find bs->tracked_requests has IO request,
> so assertion failed.
> (gdb) bt
> #0  0x00007f410df707cf in raise () from /lib64/libc.so.6
> #1  0x00007f410df5ac05 in abort () from /lib64/libc.so.6
> #2  0x00007f410df5aad9 in __assert_fail_base.cold.0 () from /lib64/libc.so.6
> #3  0x00007f410df68db6 in __assert_fail () from /lib64/libc.so.6
> #4  0x0000556915635371 in mirror_run (job=0x556916ff8600, errp=<optimized out>) at block/mirror.c:1092
> #5  0x00005569155e6c53 in job_co_entry (opaque=0x556916ff8600) at job.c:904
> #6  0x00005569156d9483 in coroutine_trampoline (i0=<optimized out>, i1=<optimized out>) at util/coroutine-ucontext.c:115
> (gdb) p s->mirror_top_bs->backing->bs->tracked_requests
> $12 = {lh_first = 0x7f3f07bfb8b0}
> (gdb) p s->mirror_top_bs->backing->bs->tracked_requests->lh_first
> $13 = (struct BdrvTrackedRequest *) 0x7f3f07bfb8b0
>
> Actually, before excuting assert(QLIST_EMPTY(&bs->tracked_requests)),
> it will excute mirror_flush(s). It may handle new I/O request and maybe
> pending I/O during this flush. Just likes in bdrv_close fuction,
> bdrv_drain(bs) followed by bdrv_flush(bs), we should add bdrv_drain fuction
> to handle pending I/O after mirror_flush.

Oh.  How is that happening, though?  I would have expected that flushing 
the target BB (and associated BDS) only flushes requests to the OS and 
lower layers, but the source node (which is `bs`) should (in the case of 
commit) always be above the target, so I wouldn’t have expected it to 
get any new requests due to this flush.

Do you have a reproducer for this?

> Signed-off-by: Long YunJian <long.yunjian@zte.com.cn>
> Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>
> ---
>   block/mirror.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/block/mirror.c b/block/mirror.c
> index efec2c7674..1eec356310 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -1079,6 +1079,8 @@ static int coroutine_fn mirror_run(Job *job, Error **errp)
>                   s->in_drain = false;
>                   continue;
>               }
> +            /* in case flush left pending I/O */
> +            bdrv_drain(bs);

I don’t think this works, because if we drain, we would also need to 
flush the target again.  Essentially I believe we’d basically need 
something like

do {
     bdrv_drained_begin(bs);
     mirror_flush(s);
     if (!QLIST_EMPTY(&bs->tracked_requests)) {
         bdrv_drained_end(bs);
     }
} while (!QLIST_EMPTY(&bs->tracked_requests));

(Which I know is really ugly)

Hanna

>   
>               /* The two disks are in sync.  Exit and report successful
>                * completion.