On Wed, Jul 24, 2013 at 03:44:38PM +0800, Fam Zheng wrote:
> On Wed, 07/24 09:35, Stefan Hajnoczi wrote:
> > On Wed, Jul 24, 2013 at 08:39:53AM +0800, Fam Zheng wrote:
> > > On Tue, 07/23 15:34, Stefan Hajnoczi wrote:
> > > > On Tue, Jul 23, 2013 at 06:32:25PM +0800, Fam Zheng wrote:
> > > > > 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.
> > > >
> > > > Why is hard ref necessary when we already have
> > > > bdrv_in_use()/bdrv_set_in_use()?
> > >
> > > Keeping the names of bdrv_in_use() and bdrv_set_in_use() is noting
> > > wrong, if no more than one "hard ref" is enforced. However I think we
> > > should manage lifecycle with both bdrv_ref and bdrv_set_in_use, so name
> > > them both ref sounds consistent: make it clearer to caller both hard ref
> > > (in_use) and soft ref preserve the bs from being released.
> >
> > I actually find "hard"/"soft" ref naming confusing and prefer keeping
> > bdrv_in_use(). Refcount is for object lifetime whereas in_use is for
> > "busy" status.
> >
> OK, do you suggest keeping in_use as is and call bdrv_delete(bs) in
> bdrv_unref() regardless of bs->in_use?
Yes, because in_use is orthogonal to object lifetime.
Stefan
