| Message ID | 1474968813-14181-1-git-send-email-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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 > >
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
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
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;
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(-)