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