Re: [PATCH v3 19/25] backup: move to block-copy

[Max Reitz]

On 26.10.20 18:18, Vladimir Sementsov-Ogievskiy wrote:
> This brings async request handling and block-status driven chunk sizes
> to backup out of the box, which improves backup performance.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   block/backup.c | 187 ++++++++++++++++++++++++++++++++-----------------
>   1 file changed, 121 insertions(+), 66 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

Irrelevant notes below.

> diff --git a/block/backup.c b/block/backup.c
> index 449b763ce4..0d1cd64eab 100644
> --- a/block/backup.c
> +++ b/block/backup.c

(Just something I noted when looking for remaining instances of 
“ratelimit”: backup.c includes ratelimit.h, which I don’t think it needs 
to do any longer)

[...]

>   static int coroutine_fn backup_loop(BackupBlockJob *job)
>   {

[...]

> +        if (!block_copy_call_finished(s) &&
> +            job_is_cancelled(&job->common.job))
> +        {

Just for the sake of clarity: If !block_copy_call_finished(), then 
job_is_cancelled() must be true, right?

(I.e., could be an assertion instead of the second part of the 
condition.  I don’t mind how it is, but then again, it did made me ask.)

> +            /*
> +             * Note that we can't use job_yield() here, as it doesn't work for
> +             * cancelled job.
> +             */
> +            block_copy_call_cancel(s);
> +            job->wait = true;
> +            qemu_coroutine_yield();
> +            assert(block_copy_call_finished(s));
> +            ret = 0;
> +            goto out;
> +        }
> +
> +        if (job_is_cancelled(&job->common.job) ||
> +            block_copy_call_succeeded(s))
> +        {
> +            ret = 0;
> +            goto out;
> +        }
> +
> +        if (block_copy_call_cancelled(s)) {
> +            /*
> +             * Job is not cancelled but only block-copy call. This is possible
> +             * after job pause. Now the pause is finished, start new block-copy
> +             * iteration.
> +             */
> +            block_copy_call_free(s);
> +            continue;

If one were to avoid all continues, we could alternatively put the whole 
error handling into a block_copy_call_failed() condition, and have the 
block_copy_call_free() common for both the cancel and the fail case.

*shrug*

> +        }
> +
> +        /* The only remaining case is failed block-copy call. */
> +        assert(block_copy_call_failed(s));
> +
> +        ret = block_copy_call_status(s, &error_is_read);
> +        act = backup_error_action(job, error_is_read, -ret);
> +        switch (act) {
> +        case BLOCK_ERROR_ACTION_REPORT:
> +            goto out;
> +        case BLOCK_ERROR_ACTION_STOP:
> +            /*
> +             * Go to pause prior to starting new block-copy call on the next
> +             * iteration.
> +             */
> +            job_pause_point(&job->common.job);
> +            break;
> +        case BLOCK_ERROR_ACTION_IGNORE:
> +            /* Proceed to new block-copy call to retry. */
> +            break;
> +        default:
> +            abort();
> +        }
> +
> +        block_copy_call_free(s);
>       }
>   
> - out:
> -    bdrv_dirty_iter_free(bdbi);
> +out:
> +    block_copy_call_free(s);

Reads a bit weird after the block_copy_call_free(s) at the end of the 
while (true) loop, but is entirely correct, of course.  (And I can’t 
offer any better alternative.)

Max

> +    job->bg_bcs_call = NULL;
>       return ret;
>   }