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