Re: [PATCH 1/5] migration: Fix possible deadloop of ram save process

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> When starting ram saving procedure (especially at the completion phase),
> always set last_seen_block to non-NULL to make sure we can always correctly
> detect the case where "we've migrated all the dirty pages".
> 
> Then we'll guarantee both last_seen_block and pss.block will be valid
> always before the loop starts.
> 
> See the comment in the code for some details.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>

Yeh I guess it can currently only happen during restart?

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  migration/ram.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index dc1de9ddbc..1d42414ecc 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -2546,14 +2546,22 @@ static int ram_find_and_save_block(RAMState *rs)
>          return pages;
>      }
>  
> +    /*
> +     * Always keep last_seen_block/last_page valid during this procedure,
> +     * because find_dirty_block() relies on these values (e.g., we compare
> +     * last_seen_block with pss.block to see whether we searched all the
> +     * ramblocks) to detect the completion of migration.  Having NULL value
> +     * of last_seen_block can conditionally cause below loop to run forever.
> +     */
> +    if (!rs->last_seen_block) {
> +        rs->last_seen_block = QLIST_FIRST_RCU(&ram_list.blocks);
> +        rs->last_page = 0;
> +    }
> +
>      pss.block = rs->last_seen_block;
>      pss.page = rs->last_page;
>      pss.complete_round = false;
>  
> -    if (!pss.block) {
> -        pss.block = QLIST_FIRST_RCU(&ram_list.blocks);
> -    }
> -
>      do {
>          again = true;
>          found = get_queued_page(rs, &pss);
> -- 
> 2.32.0
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK