Re: [Qemu-devel] [PATCH 3/3] block/backup: refactor write_flags

[John Snow]

On 7/30/19 12:32 PM, Vladimir Sementsov-Ogievskiy wrote:
> write flags are constant, let's store it in BackupBlockJob instead of
> recalculating. It also makes two boolean fields to be unused, so,
> drop them.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>  block/backup.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/block/backup.c b/block/backup.c
> index c5f941101a..4651649e9d 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -47,7 +47,6 @@ typedef struct BackupBlockJob {
>      uint64_t len;
>      uint64_t bytes_read;
>      int64_t cluster_size;
> -    bool compress;
>      NotifierWithReturn before_write;
>      QLIST_HEAD(, CowRequest) inflight_reqs;
>  
> @@ -55,7 +54,7 @@ typedef struct BackupBlockJob {
>      bool use_copy_range;
>      int64_t copy_range_size;
>  
> -    bool serialize_target_writes;
> +    BdrvRequestFlags write_flags;
>  } BackupBlockJob;
>  
>  static const BlockJobDriver backup_job_driver;
> @@ -110,10 +109,6 @@ static int coroutine_fn 
> backup_cow_with_bounce_buffer(BackupBlockJob *job,
>      BlockBackend *blk = job->common.blk;
>      int nbytes;
>      int read_flags = is_write_notifier ? BDRV_REQ_NO_SERIALISING : 0;
> -    int write_flags =
> -            (job->serialize_target_writes ? BDRV_REQ_SERIALISING : 0) |
> -            (job->compress ? BDRV_REQ_WRITE_COMPRESSED : 0);
> -
>  
>      assert(QEMU_IS_ALIGNED(start, job->cluster_size));
>      hbitmap_reset(job->copy_bitmap, start, job->cluster_size);
> @@ -132,7 +127,7 @@ static int coroutine_fn 
> backup_cow_with_bounce_buffer(BackupBlockJob *job,
>      }
>  
>      ret = blk_co_pwrite(job->target, start, nbytes, *bounce_buffer,
> -                        write_flags);
> +                        job->write_flags);
>      if (ret < 0) {
>          trace_backup_do_cow_write_fail(job, start, ret);
>          if (error_is_read) {
> @@ -160,7 +155,6 @@ static int coroutine_fn 
> backup_cow_with_offload(BackupBlockJob *job,
>      BlockBackend *blk = job->common.blk;
>      int nbytes;
>      int read_flags = is_write_notifier ? BDRV_REQ_NO_SERIALISING : 0;
> -    int write_flags = job->serialize_target_writes ? BDRV_REQ_SERIALISING : 
> 0;
>  
>      assert(QEMU_IS_ALIGNED(job->copy_range_size, job->cluster_size));
>      assert(QEMU_IS_ALIGNED(start, job->cluster_size));
> @@ -168,7 +162,7 @@ static int coroutine_fn 
> backup_cow_with_offload(BackupBlockJob *job,
>      nr_clusters = DIV_ROUND_UP(nbytes, job->cluster_size);
>      hbitmap_reset(job->copy_bitmap, start, job->cluster_size * nr_clusters);
>      ret = blk_co_copy_range(blk, start, job->target, start, nbytes,
> -                            read_flags, write_flags);
> +                            read_flags, job->write_flags);
>      if (ret < 0) {
>          trace_backup_do_cow_copy_range_fail(job, start, ret);
>          hbitmap_set(job->copy_bitmap, start, job->cluster_size * 
> nr_clusters);
> @@ -638,10 +632,16 @@ BlockJob *backup_job_create(const char *job_id, 
> BlockDriverState *bs,
>      job->sync_mode = sync_mode;
>      job->sync_bitmap = sync_mode == MIRROR_SYNC_MODE_INCREMENTAL ?
>                         sync_bitmap : NULL;
> -    job->compress = compress;
>  
> -    /* Detect image-fleecing (and similar) schemes */
> -    job->serialize_target_writes = bdrv_chain_contains(target, bs);
> +    /*
> +     * Set write flags:
> +     *  1. Detect image-fleecing (and similar) schemes
> +     *  2. Handle compression
> +     */
> +    job->write_flags =
> +            (bdrv_chain_contains(target, bs) ? BDRV_REQ_SERIALISING : 0) |
> +            (compress ? BDRV_REQ_WRITE_COMPRESSED : 0);
> +
>      job->cluster_size = cluster_size;
>      job->copy_bitmap = copy_bitmap;
>      copy_bitmap = NULL;
> 

What happens if you did pass BDRV_REQ_WRITE_COMPRESSED to
blk_co_copy_range? Is that rejected somewhere in the stack?

I had just assumed it wouldn't work quite right because of the nature of
copy offloading, but I don't actually know what it does do.

This seems like a pretty minor cleanup, but why not. I like dropping
extra fields when I can:

Reviewed-by: John Snow <address@hidden>