Re: [Qemu-devel] [PATCH v5 02/12] qcow2: Implement bdrv_make_empty()

[Max Reitz]

On 22.04.2014 16:23, Kevin Wolf wrote:
> Am 17.04.2014 um 23:59 hat Max Reitz geschrieben:
> > Implement bdrv_make_empty() by making all clusters in the image fall
> > through to the backing file (via the now modified discard).
> >
> > Signed-off-by: Max Reitz <address@hidden>
> > ---
> >   block/qcow2.c | 22 ++++++++++++++++++++++
> >   1 file changed, 22 insertions(+)
> >
> > diff --git a/block/qcow2.c b/block/qcow2.c
> > index 1e7b7d5..4d70665 100644
> > --- a/block/qcow2.c
> > +++ b/block/qcow2.c
> > @@ -2006,6 +2006,27 @@ fail:
> >       return ret;
> >   }
> >   
> > +static int qcow2_make_empty(BlockDriverState *bs)
> > +{
> > +     uint64_t start_sector;
> > +     int ret = 0;
> > +
> > +     /* The step taken here may not exceed INT_MAX when multiplied by
> > +      * BDRV_SECTOR_SIZE. 64k is arbitrary, but works well. */
> So you really mean something like this?
>
> int max_sectors_per_iteration = (INT_MAX / BDRV_SECTOR_SIZE);

I like 64k. ;-)

Yes, you're right, using an arbitrary value is probably not for the best.

> > +     for (start_sector = 0; start_sector < bs->total_sectors;
> > +          start_sector += 65536)
> > +     {
> > +         ret = qcow2_discard_clusters(bs, start_sector * BDRV_SECTOR_SIZE,
> > +                 MIN(65536, bs->total_sectors - start_sector),
> > +                 QCOW2_DISCARD_REQUEST, true);
> QCOW2_DISCARD_REQUEST is for requests that come from the guest. SNAPSHOT
> would be a nice fit if we reinterpreted it so that it doesn't only refer
> to internal snapshots but also to external ones. I would be okay with
> OTHER as well if you prefer.
>
> Perhaps a look at the defaults can help us: SNAPSHOT defaults to on,
> OTHER to off. I think it totally makes sense to physically discard
> clusters in qcow2_make_empty() in almost every case, so that might be a
> good reason to use SNAPSHOT here.

My problem with using SNAPSHOT would be that this function is that there 
is no indication of this function being meant for committing snapshots 
only; however, in practice it is obviously only used for that purpose, 
thus I can accept it.

But I do like reasoning with the defaults. ;-)

I'll go for SNAPSHOT then and include an explanatory comment.

Max

> > +         if (ret < 0) {
> > +             break;
> > +         }
> > +     }
> > +
> > +     return ret;
> > +}
> > +
> Kevin