On Sat, Sep 13, 2014 at 11:53:58PM +0800, Jun Li wrote: > On Fri, 09/05 16:33, Stefan Hajnoczi wrote: > > On Mon, Sep 01, 2014 at 06:52:48PM +0800, Jun Li wrote: > > > > How does this patch handle self-describing refcount blocks? I think > > they will keep the refcount block alive forever because your code will > > not decide to free them. > > > > Sorry, I have ignored self-describing refcount blocks. :)
For this...
> > This patch should also discard the refcount block if we decide to free
> > it (in the same way that we discard at cluster_offset).
> >
> > > diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> > > index 43665b8..63f36e6 100644
> > > --- a/block/qcow2-refcount.c
> > > +++ b/block/qcow2-refcount.c
> > > @@ -586,6 +586,37 @@ static int QEMU_WARN_UNUSED_RESULT
> > > update_refcount(BlockDriverState *bs,
> > > if (refcount == 0 && s->discard_passthrough[type]) {
> > > update_refcount_discard(bs, cluster_offset, s->cluster_size);
> > > }
> > > +
> > > + /* When refcount block is NULL, update refcount table */
> > > + if (block_index == 0) {
> >
> > What is the purpose of block_index == 0?
>
> Here is want to reduce the probability of running the following code. Only
> when block_index == 0, we will run the following code to free refcount block.
...and this reason, I consider this approach incomplete.
The approach is unreliable because a change to refcount update ordering
could change leak behavior.
Either free refcount blocks to avoid leaks in all cases, or don't
bother.
Stefan
pgpUQQZqHdyKP.pgp
Description: PGP signature
