[Qemu-devel] [PATCH] qemu-img: Allow unmap backing image for zeroed clusters

[Qemu-devel] [PATCH] qemu-img: Allow unmap backing image for zeroed clusters

[Fam Zheng]

We already specified BDRV_O_UNMAP when opening images in 'qemu-img
commit', but didn't turn on the "unmap" in the active commit job. This
patch fixes that so that zeroed clusters in top image can be discarded
which is desired in the virt-sparsify use case, where a temporary
overlay is created and fstrim'ed before commiting back, to free space in
the original image.

The block-commit keeps the existing behavior.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/mirror.c            | 5 +++--
 block/replication.c       | 2 +-
 blockdev.c                | 3 ++-
 include/block/block_int.h | 4 +++-
 qemu-img.c                | 2 +-
 5 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index f9d1fec..5892cf6 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1002,7 +1002,8 @@ void commit_active_start(const char *job_id, BlockDriverState *bs,
                          BlockdevOnError on_error,
                          BlockCompletionFunc *cb,
                          void *opaque, Error **errp,
-                         bool auto_complete)
+                         bool auto_complete,
+                         bool unmap)
 {
     int64_t length, base_length;
     int orig_base_flags;
@@ -1042,7 +1043,7 @@ void commit_active_start(const char *job_id, BlockDriverState *bs,
 
     mirror_start_job(job_id, bs, base, NULL, speed, 0, 0,
                      MIRROR_LEAVE_BACKING_CHAIN,
-                     on_error, on_error, false, cb, opaque, &local_err,
+                     on_error, on_error, unmap, cb, opaque, &local_err,
                      &commit_active_job_driver, false, base, auto_complete);
     if (local_err) {
         error_propagate(errp, local_err);
diff --git a/block/replication.c b/block/replication.c
index 3bd1cf1..b8921a6 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -624,7 +624,7 @@ static void replication_stop(ReplicationState *rs, bool failover, Error **errp)
         commit_active_start("replication-commit", s->active_disk->bs,
                             s->secondary_disk->bs, 0, BLOCKDEV_ON_ERROR_REPORT,
                             replication_done,
-                            bs, errp, true);
+                            bs, errp, true, false);
         break;
     default:
         aio_context_release(aio_context);
diff --git a/blockdev.c b/blockdev.c
index 29c6561..8f9125a 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3136,7 +3136,8 @@ void qmp_block_commit(bool has_job_id, const char *job_id, const char *device,
             goto out;
         }
         commit_active_start(has_job_id ? job_id : NULL, bs, base_bs, speed,
-                            on_error, block_job_cb, bs, &local_err, false);
+                            on_error, block_job_cb, bs, &local_err, false,
+                            false);
     } else {
         commit_start(has_job_id ? job_id : NULL, bs, base_bs, top_bs, speed,
                      on_error, block_job_cb, bs,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index ef3c047..9b1e5d8 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -694,13 +694,15 @@ void commit_start(const char *job_id, BlockDriverState *bs,
  * @opaque: Opaque pointer value passed to @cb.
  * @errp: Error object.
  * @auto_complete: Auto complete the job.
+ * @unmap: Unmap zero clusters on base.
  *
  */
 void commit_active_start(const char *job_id, BlockDriverState *bs,
                          BlockDriverState *base, int64_t speed,
                          BlockdevOnError on_error,
                          BlockCompletionFunc *cb,
-                         void *opaque, Error **errp, bool auto_complete);
+                         void *opaque, Error **errp, bool auto_complete,
+                         bool unmap);
 /*
  * mirror_start:
  * @job_id: The id of the newly-created job, or %NULL to use the
diff --git a/qemu-img.c b/qemu-img.c
index ceffefe..f2397e6 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -929,7 +929,7 @@ static int img_commit(int argc, char **argv)
     };
 
     commit_active_start("commit", bs, base_bs, 0, BLOCKDEV_ON_ERROR_REPORT,
-                        common_block_job_cb, &cbi, &local_err, false);
+                        common_block_job_cb, &cbi, &local_err, false, true);
     if (local_err) {
         goto done;
     }
-- 
2.7.4

Re: [Qemu-devel] [PATCH] qemu-img: Allow unmap backing image for zeroed clusters

[Kevin Wolf]

Am 27.09.2016 um 04:20 hat Fam Zheng geschrieben:
> We already specified BDRV_O_UNMAP when opening images in 'qemu-img
> commit', but didn't turn on the "unmap" in the active commit job. This
> patch fixes that so that zeroed clusters in top image can be discarded
> which is desired in the virt-sparsify use case, where a temporary
> overlay is created and fstrim'ed before commiting back, to free space in
> the original image.
> 
> The block-commit keeps the existing behavior.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>

Is there a good reason for not using discard in an active commit if the
image was opened with BDRV_O_UNMAP? That is, wouldn't it make sense to
just set the unmap option for the mirror unconditionally and also for
block-commit?

If there is a reason, we should probably add an option to block-commit,
but I still feels that enabling it should be the default then.

Kevin

Re: [Qemu-devel] [PATCH] qemu-img: Allow unmap backing image for zeroed clusters

[Fam Zheng]

On Tue, 09/27 10:41, Kevin Wolf wrote:
> Am 27.09.2016 um 04:20 hat Fam Zheng geschrieben:
> > We already specified BDRV_O_UNMAP when opening images in 'qemu-img
> > commit', but didn't turn on the "unmap" in the active commit job. This
> > patch fixes that so that zeroed clusters in top image can be discarded
> > which is desired in the virt-sparsify use case, where a temporary
> > overlay is created and fstrim'ed before commiting back, to free space in
> > the original image.
> > 
> > The block-commit keeps the existing behavior.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> 
> Is there a good reason for not using discard in an active commit if the
> image was opened with BDRV_O_UNMAP? That is, wouldn't it make sense to
> just set the unmap option for the mirror unconditionally and also for
> block-commit?
> 
> If there is a reason, we should probably add an option to block-commit,
> but I still feels that enabling it should be the default then.

I didn't touch block-commit because I wasn't sure, but you made a good point,
we can turn this on there too.

Fam