Re: [PATCH v3 03/17] migration: Block migration comment or code is wrong

[Avihai Horon]

On 08/11/2022 20:36, Vladimir Sementsov-Ogievskiy wrote:
> External email: Use caution opening links or attachments
>
>
> On 11/3/22 19:16, Avihai Horon wrote:
> > From: Juan Quintela <quintela@redhat.com>
> >
> > And it appears that what is wrong is the code. During bulk stage we
> > need to make sure that some block is dirty, but no games with
> > max_size at all.
>
> :) That made me interested in, why we need this one block, so I 
> decided to search through the history.
>
> And what I see? Haha, that was my commit 04636dc410b163c 
> "migration/block: fix pending() return value" [1], which you actually 
> revert with this patch.
>
> So, at least we should note, that it's a revert of [1].
>
> Still that this will reintroduce the bug fixed by [1].
>
> As I understand the problem is (was) that in block_save_complete() we 
> finalize only dirty blocks, but don't finalize the bulk phase if it's 
> not finalized yet. So, we can fix block_save_complete() to finalize 
> the bulk phase, instead of hacking with pending in [1].
>
> Interesting, why we need this one block, described in the comment you 
> refer to? Was it an incomplete workaround for the same problem, 
> described in [1]? If so, we can fix block_save_complete() and remove 
> this if() together with the comment from block_save_pending().
I am not familiar with block migration.
I can drop this patch in next version.

Juan/Stefan, could you help here?

> > Signed-off-by: Juan Quintela <quintela@redhat.com>
> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >   migration/block.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/migration/block.c b/migration/block.c
> > index b3d680af75..39ce4003c6 100644
> > --- a/migration/block.c
> > +++ b/migration/block.c
> > @@ -879,8 +879,8 @@ static void block_save_pending(void *opaque, 
> > uint64_t max_size,
> >       blk_mig_unlock();
> >
> >       /* Report at least one block pending during bulk phase */
> > -    if (pending <= max_size && !block_mig_state.bulk_completed) {
> > -        pending = max_size + BLK_MIG_BLOCK_SIZE;
> > +    if (!pending && !block_mig_state.bulk_completed) {
> > +        pending = BLK_MIG_BLOCK_SIZE;
> >       }
> >
> >       trace_migration_block_save_pending(pending);
>
> --
> Best regards,
> Vladimir