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