Re: [Qemu-block] [PATCH v7 13/16] drive-backup: added support for data compression

[Kevin Wolf]

Am 22.07.2016 um 10:17 hat Denis V. Lunev geschrieben:
> From: Pavel Butsykin <address@hidden>
> 
> The idea is simple - backup is "written-once" data. It is written block
> by block and it is large enough. It would be nice to save storage
> space and compress it.
> 
> The patch adds a flag to the qmp/hmp drive-backup command which enables
> block compression. Compression should be implemented in the format driver
> to enable this feature.
> 
> There are some limitations of the format driver to allow compressed writes.
> We can write data only once. Though for backup this is perfectly fine.
> These limitations are maintained by the driver and the error will be
> reported if we are doing something wrong.
> 
> Signed-off-by: Pavel Butsykin <address@hidden>
> Reviewed-by: Stefan Hajnoczi <address@hidden>
> Signed-off-by: Denis V. Lunev <address@hidden>
> CC: Jeff Cody <address@hidden>
> CC: Markus Armbruster <address@hidden>
> CC: Eric Blake <address@hidden>
> CC: John Snow <address@hidden>
> CC: Stefan Hajnoczi <address@hidden>
> CC: Kevin Wolf <address@hidden>

> @@ -154,7 +155,8 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job,
>                                         bounce_qiov.size, BDRV_REQ_MAY_UNMAP);
>          } else {
>              ret = blk_co_pwritev(job->target, start * job->cluster_size,
> -                                 bounce_qiov.size, &bounce_qiov, 0);
> +                                 bounce_qiov.size, &bounce_qiov,
> +                                 job->compress ? BDRV_REQ_WRITE_COMPRESSED : 
> 0);

I love this hunk. Looks so simple. :-)

> @@ -3242,8 +3245,8 @@ static void do_drive_backup(DriveBackup *backup, 
> BlockJobTxn *txn, Error **errp)
>      }
>  
>      backup_start(backup->job_id, bs, target_bs, backup->speed, backup->sync,
> -                 bmap, backup->on_source_error, backup->on_target_error,
> -                 block_job_cb, bs, txn, &local_err);
> +                 bmap, backup->compress, backup->on_source_error,
> +                 backup->on_target_error, block_job_cb, bs, txn, &local_err);
>      bdrv_unref(target_bs);
>      if (local_err != NULL) {
>          error_propagate(errp, local_err);
> @@ -3317,7 +3320,7 @@ void do_blockdev_backup(BlockdevBackup *backup, 
> BlockJobTxn *txn, Error **errp)
>          }
>      }
>      backup_start(backup->job_id, bs, target_bs, backup->speed, backup->sync,
> -                 NULL, backup->on_source_error, backup->on_target_error,
> +                 NULL, false, backup->on_source_error, 
> backup->on_target_error,
>                   block_job_cb, bs, txn, &local_err);
>      if (local_err != NULL) {
>          error_propagate(errp, local_err);

blockdev-backup is supposed to be the primary interface in the future,
so I would like to avoid introducing a feature gap where drive-backup is
more powerful. Can you please post a small diff to squash in so that
this patch adds the option to both commands? Wiring this up should be
trivial.

Kevin