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

[Stefan Hajnoczi <stefanha@gmail.com>]

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