[Qemu-devel] [PATCH v3] block: Turn on "unmap" in active commit

[Qemu-devel] [PATCH v3] block: Turn on "unmap" in active commit

[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.

This also enables it for block-commit.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
v3: Change the right parameter.
v2: Add "unmap" to block-commit as well. [Kevin]
---
 block/mirror.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/mirror.c b/block/mirror.c
index f9d1fec..8847ec5 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1042,7 +1042,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, true, cb, opaque, &local_err,
                      &commit_active_job_driver, false, base, auto_complete);
     if (local_err) {
         error_propagate(errp, local_err);
-- 
2.7.4

Re: [Qemu-devel] [PATCH v3] block: Turn on "unmap" in active commit

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1644 bytes --]

On 09/27/2016 06:14 AM, Fam Zheng wrote:
> 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.
> 
> This also enables it for block-commit.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
> v3: Change the right parameter.

Doesn't affect this patch, but it may be worth using the 'boxed':true
notation in the .json file to make it more compact to pass job
information around via a struct rather than a large mess of parameters
where you are likely to get the wrong one.

> v2: Add "unmap" to block-commit as well. [Kevin]
> ---
>  block/mirror.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index f9d1fec..8847ec5 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -1042,7 +1042,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, true, cb, opaque, &local_err,
>                       &commit_active_job_driver, false, base, auto_complete);

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v3] block: Turn on "unmap" in active commit

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 892 bytes --]

On 09/27/2016 06:14 AM, Fam Zheng wrote:
> 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

s/commiting/committing/

> the original image.
> 
> This also enables it for block-commit.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
> v3: Change the right parameter.
> v2: Add "unmap" to block-commit as well. [Kevin]
> ---
>  block/mirror.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH v3] block: Turn on "unmap" in active commit

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1496 bytes --]

On Tue, Sep 27, 2016 at 07:14:52PM +0800, Fam Zheng wrote:
> 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.
> 
> This also enables it for block-commit.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
> v3: Change the right parameter.
> v2: Add "unmap" to block-commit as well. [Kevin]
> ---
>  block/mirror.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index f9d1fec..8847ec5 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -1042,7 +1042,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, true, cb, opaque, &local_err,
>                       &commit_active_job_driver, false, base, auto_complete);
>      if (local_err) {
>          error_propagate(errp, local_err);

Why is unmap an option at all?

What's wrong with using BDRV_REQ_MAY_UNMAP on all
blk_aio_pwrite_zeroes() calls?

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH v3] block: Turn on "unmap" in active commit

[Fam Zheng]

On Thu, 09/29 13:41, Stefan Hajnoczi wrote:
> On Tue, Sep 27, 2016 at 07:14:52PM +0800, Fam Zheng wrote:
> > 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.
> > 
> > This also enables it for block-commit.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> > v3: Change the right parameter.
> > v2: Add "unmap" to block-commit as well. [Kevin]
> > ---
> >  block/mirror.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/block/mirror.c b/block/mirror.c
> > index f9d1fec..8847ec5 100644
> > --- a/block/mirror.c
> > +++ b/block/mirror.c
> > @@ -1042,7 +1042,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, true, cb, opaque, &local_err,
> >                       &commit_active_job_driver, false, base, auto_complete);
> >      if (local_err) {
> >          error_propagate(errp, local_err);
> 
> Why is unmap an option at all?
> 
> What's wrong with using BDRV_REQ_MAY_UNMAP on all
> blk_aio_pwrite_zeroes() calls?

Because unmap is an QMP option of drive-backup. I think in the drive-mirror
context, it mitigates the limitation that we have no control over target's
BDRV_O_UNMAP (always inherited from source).

Fam

Re: [Qemu-devel] [Qemu-block] [PATCH v3] block: Turn on "unmap" in active commit

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2583 bytes --]

On Fri, Sep 30, 2016 at 10:12:05AM +0800, Fam Zheng wrote:
> On Thu, 09/29 13:41, Stefan Hajnoczi wrote:
> > On Tue, Sep 27, 2016 at 07:14:52PM +0800, Fam Zheng wrote:
> > > 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.
> > > 
> > > This also enables it for block-commit.
> > > 
> > > Signed-off-by: Fam Zheng <famz@redhat.com>
> > > ---
> > > v3: Change the right parameter.
> > > v2: Add "unmap" to block-commit as well. [Kevin]
> > > ---
> > >  block/mirror.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/block/mirror.c b/block/mirror.c
> > > index f9d1fec..8847ec5 100644
> > > --- a/block/mirror.c
> > > +++ b/block/mirror.c
> > > @@ -1042,7 +1042,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, true, cb, opaque, &local_err,
> > >                       &commit_active_job_driver, false, base, auto_complete);
> > >      if (local_err) {
> > >          error_propagate(errp, local_err);
> > 
> > Why is unmap an option at all?
> > 
> > What's wrong with using BDRV_REQ_MAY_UNMAP on all
> > blk_aio_pwrite_zeroes() calls?
> 
> Because unmap is an QMP option of drive-backup. I think in the drive-mirror
> context, it mitigates the limitation that we have no control over target's
> BDRV_O_UNMAP (always inherited from source).

That doesn't explain why unmap is an option.

Is it because the user might not want to unmap the target and would
rather writes zeroes?

For example in qcow2 they might want to leave the data clusters
allocated and simply mark their contents zeroed:

"Standard Cluster Descriptor:

    Bit       0:    If set to 1, the cluster reads as all zeros. The host
                    cluster offset can be used to describe a preallocation,
                    but it won't be used for reading data from this cluster,
                    nor is data read from the backing file if the cluster is
                    unallocated."

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH v3] block: Turn on "unmap" in active commit

[Kevin Wolf]

Am 30.09.2016 um 04:12 hat Fam Zheng geschrieben:
> On Thu, 09/29 13:41, Stefan Hajnoczi wrote:
> > On Tue, Sep 27, 2016 at 07:14:52PM +0800, Fam Zheng wrote:
> > > 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.
> > > 
> > > This also enables it for block-commit.
> > > 
> > > Signed-off-by: Fam Zheng <famz@redhat.com>
> > > ---
> > > v3: Change the right parameter.
> > > v2: Add "unmap" to block-commit as well. [Kevin]
> > > ---
> > >  block/mirror.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/block/mirror.c b/block/mirror.c
> > > index f9d1fec..8847ec5 100644
> > > --- a/block/mirror.c
> > > +++ b/block/mirror.c
> > > @@ -1042,7 +1042,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, true, cb, opaque, &local_err,
> > >                       &commit_active_job_driver, false, base, auto_complete);
> > >      if (local_err) {
> > >          error_propagate(errp, local_err);
> > 
> > Why is unmap an option at all?
> > 
> > What's wrong with using BDRV_REQ_MAY_UNMAP on all
> > blk_aio_pwrite_zeroes() calls?
> 
> Because unmap is an QMP option of drive-backup. I think in the drive-mirror
> context, it mitigates the limitation that we have no control over target's
> BDRV_O_UNMAP (always inherited from source).

Wouldn't the more straightforward implementation then be if
qmp_drive_mirror() set BDRV_O_UNMAP for the target depending on the flag
rather than passing the flag down to the mirror job?

Hm... And should BDRV_O_UNMAP really be a BlockBackend option rather
than a BDS one? We already enable it unconditionally on non-root nodes
and it seems to make sense to me to allow discard e.g. from a block job,
but not from the guest.

Kevin

Re: [Qemu-devel] [PATCH v3] block: Turn on "unmap" in active commit

[Jeff Cody]

On Tue, Sep 27, 2016 at 07:14:52PM +0800, Fam Zheng wrote:
> 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.
> 
> This also enables it for block-commit.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
> v3: Change the right parameter.
> v2: Add "unmap" to block-commit as well. [Kevin]
> ---
>  block/mirror.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index f9d1fec..8847ec5 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -1042,7 +1042,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, true, cb, opaque, &local_err,
>                       &commit_active_job_driver, false, base, auto_complete);
>      if (local_err) {
>          error_propagate(errp, local_err);
> -- 
> 2.7.4
> 

Thanks,

Applied to my block branch:

git://github.com/codyprime/qemu-kvm-jtc.git block

-Jeff