Re: [PATCH v3 1/6] block-jobs: flush target at the end of .run()

[Max Reitz]

On 05.03.21 18:35, Vladimir Sementsov-Ogievskiy wrote:
> We are going to implement compressed write cache to improve performance
> of compressed backup when target is opened in O_DIRECT mode. We
> definitely want to flush the cache at backup finish, and if flush fails
> it should be reported as block-job failure, not simply ignored in
> bdrv_close(). So, teach all block-jobs to flush their targets at the
> end.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   include/block/blockjob_int.h | 18 ++++++++++++++++++
>   block/backup.c               |  8 +++++---
>   block/commit.c               |  2 ++
>   block/mirror.c               |  2 ++
>   block/stream.c               |  2 ++
>   blockjob.c                   | 16 ++++++++++++++++
>   6 files changed, 45 insertions(+), 3 deletions(-)

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

Just a nit on the function’s description.

> diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
> index 6633d83da2..6ef3123120 100644
> --- a/include/block/blockjob_int.h
> +++ b/include/block/blockjob_int.h
> @@ -119,4 +119,22 @@ int64_t block_job_ratelimit_get_delay(BlockJob *job, 
> uint64_t n);
>   BlockErrorAction block_job_error_action(BlockJob *job, BlockdevOnError on_err,
>                                           int is_read, int error);
>   
> +/**
> + * block_job_final_target_flush:
> + * @job: The job to signal an error for if flush failed.
> + * @target_bs: The bs to flush.
> + * @ret: Will be updated (to return code of bdrv_flush()) only if it is zero
> + *       now. This is a bit unusual interface but all callers are comfortable
> + *       with it.
> + *
> + * The function is intended to be called at the end of .run() for any data
> + * copying job.
> + *
> + * There are may be some internal caches in format layers of target,
-are, *in the format layers of the target

> + * like compressed_cache in qcow2 format. So we should call flush to
> + * be sure that all data reached the destination protocol layer.
> + */
> +void block_job_final_target_flush(BlockJob *job, BlockDriverState *target_bs,
> +                                  int *ret);
> +
>   #endif