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

[Qemu-devel] [PATCH v2] 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>
---
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..8f6f506 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1043,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,
-                     &commit_active_job_driver, false, base, auto_complete);
+                     &commit_active_job_driver, true, base, auto_complete);
     if (local_err) {
         error_propagate(errp, local_err);
         goto error_restore_flags;
-- 
2.7.4

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

[Fam Zheng]

On Tue, 09/27 17:33, 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>

Sent too soon. I see a number of iotests failures (020 040 097 129),
still investigating.

Fam

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

[Fam Zheng]

On Tue, 09/27 17:33, 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>
> ---
> 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..8f6f506 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -1043,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,
> -                     &commit_active_job_driver, false, base, auto_complete);
> +                     &commit_active_job_driver, true, base, auto_complete);

I'm an idiot! I changed the wrong parameter (is_none_mode).

Will send v3.

Fam

>      if (local_err) {
>          error_propagate(errp, local_err);
>          goto error_restore_flags;
> -- 
> 2.7.4
> 
> 

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

[Kevin Wolf]

Am 27.09.2016 um 13:04 hat Fam Zheng geschrieben:
> On Tue, 09/27 17:33, 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>
> > ---
> > 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..8f6f506 100644
> > --- a/block/mirror.c
> > +++ b/block/mirror.c
> > @@ -1043,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,
> > -                     &commit_active_job_driver, false, base, auto_complete);
> > +                     &commit_active_job_driver, true, base, auto_complete);
> 
> I'm an idiot! I changed the wrong parameter (is_none_mode).

Actually, the idiotic part is having a function with eighteen
parameters. Not too surprising that someone confuses them sooner or
later...

Perhaps we could enable QAPI boxing for blockdev-mirror and then just
pass down a struct. Then we could have designated initialisers here in
commit_active_start() and it would be obvious if the wrong one is
changed.

Not something to do in this series, of course, just a general idea for
improvement.

Kevin

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

[Fam Zheng]

On Tue, 09/27 13:15, Kevin Wolf wrote:
> Am 27.09.2016 um 13:04 hat Fam Zheng geschrieben:
> > On Tue, 09/27 17:33, 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>
> > > ---
> > > 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..8f6f506 100644
> > > --- a/block/mirror.c
> > > +++ b/block/mirror.c
> > > @@ -1043,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,
> > > -                     &commit_active_job_driver, false, base, auto_complete);
> > > +                     &commit_active_job_driver, true, base, auto_complete);
> > 
> > I'm an idiot! I changed the wrong parameter (is_none_mode).
> 
> Actually, the idiotic part is having a function with eighteen
> parameters. Not too surprising that someone confuses them sooner or
> later...
> 
> Perhaps we could enable QAPI boxing for blockdev-mirror and then just
> pass down a struct. Then we could have designated initialisers here in
> commit_active_start() and it would be obvious if the wrong one is
> changed.

Good, one more thing in my TODO list. Thanks.

Fam

> 
> Not something to do in this series, of course, just a general idea for
> improvement.
> 
> Kevin