From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46499) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bWlDg-0000GV-Pt for qemu-devel@nongnu.org; Mon, 08 Aug 2016 10:07:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bWlDe-0003os-JC for qemu-devel@nongnu.org; Mon, 08 Aug 2016 10:07:55 -0400 Date: Mon, 8 Aug 2016 16:07:43 +0200 From: Kevin Wolf Message-ID: <20160808140743.GD25992@noname.str.redhat.com> References: <1469175475-15420-1-git-send-email-den@openvz.org> <1469175475-15420-14-git-send-email-den@openvz.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1469175475-15420-14-git-send-email-den@openvz.org> Subject: Re: [Qemu-devel] [PATCH v7 13/16] drive-backup: added support for data compression List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Denis V. Lunev" Cc: qemu-block@nongnu.org, qemu-devel@nongnu.org, Pavel Butsykin , Jeff Cody , Markus Armbruster , Eric Blake , John Snow , Stefan Hajnoczi Am 22.07.2016 um 10:17 hat Denis V. Lunev geschrieben: > From: Pavel Butsykin > > 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 > Reviewed-by: Stefan Hajnoczi > Signed-off-by: Denis V. Lunev > CC: Jeff Cody > CC: Markus Armbruster > CC: Eric Blake > CC: John Snow > CC: Stefan Hajnoczi > CC: Kevin Wolf > @@ -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