Re: [Qemu-devel] [Qemu-block] [PATCH v4] migration/block: move bdrv_is_allocated() into bb's AioContext

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2441 bytes --]

On Tue, May 09, 2017 at 08:59:30PM +0800, 858585 jemmy wrote:
> On Tue, May 9, 2017 at 4:54 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> > On Fri, May 05, 2017 at 04:03:49PM +0800, jemmy858585@gmail.com wrote:
> >> From: Lidong Chen <lidongchen@tencent.com>
> >>
> >> when block migration with high-speed, mig_save_device_bulk hold the
> >> BQL and invoke bdrv_is_allocated frequently. This patch moves
> >> bdrv_is_allocated() into bb's AioContext. It will execute without
> >> blocking other I/O activity.
> >>
> >> Signed-off-by: Lidong Chen <lidongchen@tencent.com>
> >> ---
> >>      v4 changelog:
> >>           Use the prototype code written by Stefan and fix some bug.
> >>           moves bdrv_is_allocated() into bb's AioContext.
> >> ---
> >>  migration/block.c | 48 +++++++++++++++++++++++++++++++++++++++---------
> >>  1 file changed, 39 insertions(+), 9 deletions(-)
> >
> > Added Paolo because he's been reworking AioContext and locking.
> >
> > The goal of this patch is to avoid waiting for bdrv_is_allocated() to
> > complete while holding locks.  Do bdrv_is_allocated() in the AioContext
> > so event processing continues after yield.
> 
> Hi Paolo:
>     Some information about the problem.

Lidong, please see my comment below:

> >> @@ -274,17 +298,23 @@ static int mig_save_device_bulk(QEMUFile *f, BlkMigDevState *bmds)
> >>      int nr_sectors;
> >>
> >>      if (bmds->shared_base) {
> >> +        AioContext *bb_ctx;
> >> +        Coroutine *co;
> >> +        MigNextAllocatedClusterData data = {
> >> +            .cur_sector = &cur_sector,
> >> +            .total_sectors = &total_sectors,
> >> +            .bb = bb,
> >> +        };
> >> +        qemu_event_init(&data.event, false);
> >> +
> >>          qemu_mutex_lock_iothread();
> >> -        aio_context_acquire(blk_get_aio_context(bb));
> >> -        /* Skip unallocated sectors; intentionally treats failure as
> >> -         * an allocated sector */
> >> -        while (cur_sector < total_sectors &&
> >> -               !bdrv_is_allocated(blk_bs(bb), cur_sector,
> >> -                                  MAX_IS_ALLOCATED_SEARCH, &nr_sectors)) {
> >> -            cur_sector += nr_sectors;
> >> -        }
> >> -        aio_context_release(blk_get_aio_context(bb));
> >> +        bdrv_inc_in_flight(blk_bs(bb));
> >
> > Please add a comment explaining why bdrv_inc_in_flight() is invoked.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]