Re: [Qemu-devel] [PATCH v2 01/11] block: replace in_use with refcnt_soft and refcnt_hard

[Fam Zheng]

On Tue, 07/23 11:36, Stefan Hajnoczi wrote:
> On Wed, Jul 17, 2013 at 05:42:06PM +0800, Fam Zheng wrote:
> > Introduce refcnt_soft (soft reference) and refcnt_hard (hard reference)
> > to BlockDriverState, since in_use mechanism cannot provide proper
> > management of lifecycle when a BDS is referenced in multiple places
> > (e.g. pointed to by another bs's backing_hd while also used as a block
> > job device, in the use case of image fleecing).
> > 
> > The original in_use case is considered a "hard reference" in this patch,
> > where the bs is busy and should not be used in other tasks that require
> > a hard reference. (However the interface doesn't force this, caller
> > still need to call bdrv_in_use() to check by itself.).
> > 
> > A soft reference is implemented but not used yet. It will be used in
> > following patches to manage the lifecycle together with hard reference.
> > 
> > If bdrv_ref() is called on a BDS, it must be released by exactly the
> > same numbers of bdrv_unref() with the same "soft/hard" type, and never
> > call bdrv_delete() directly. If the BDS is only used locally (unnamed),
> > bdrv_ref/bdrv_unref can be skipped and just use bdrv_delete().
> 
> It is risky to keep bdrv_delete() public.  I suggest replacing
> bdrv_delete() callers with bdrv_unref() and then making bdrv_delete()
> static in block.c.
> 
> This way it is impossible to make the mistake of calling bdrv_delete()
> on a BDS which has refcnt > 1.
> 
> I don't really understand this patch.  There are now two separate
> refcounts.  They must both reach 0 for deletion to occur.  I think
> you plan to treat the "hard" refcount like the in_use counter (there
> should only be 0 or 1 refs) but you don't enforce it.  It seems cleaner
> to keep in_use separate: let in_use callers take a refcount and also set
> in_use.

OK, I like your ideas, make bdrv_delete private is much cleaner. Will
fix in next revision.

I plan to make it like this:

    /* soft ref */
    void bdrv_{ref,unref}(bs)

    /* hard ref */
    bool bdrv_hard_{ref,unref}(bs)

usage:
    bs = bdrv_new()
    <implicit bdrv_ref(bs) called>
    ...
    bdrv_unref(bs)
    <automatically deleted here>

or with hard ref:
    bs = bdrv_new()
    <implicit bdrv_ref() called>

    bdrv_hard_ref(bs)
    ...
    bdrv_hard_unref(bs)

    bdrv_unref(bs)
    <automatically deleted here>

The second bdrv_hard_ref call to a bs returns false, caller check the
return value.

-- 
Fam