Re: [Qemu-devel] [PATCH v2 01/11] block: replace in_use with refcnt_soft and refcnt_hard
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()
> > > > >
> > > > > ...
> > > > > bdrv_unref(bs)
> > > > >
> > > > >
> > > > > or with hard ref:
> > > > > bs = bdrv_new()
> > > > >
> > > > >
> > > > > bdrv_hard_ref(bs)
> > > > > ...
> > > > > bdrv_hard_unref(bs)
> > > > >
> > > > > bdrv_unref(bs)
> > > > >
> > > > >
> > > > > 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
Re: [Qemu-devel] [PATCH v2 01/11] block: replace in_use with refcnt_soft and refcnt_hard
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()
> > > >
> > > > ...
> > > > bdrv_unref(bs)
> > > >
> > > >
> > > > or with hard ref:
> > > > bs = bdrv_new()
> > > >
> > > >
> > > > bdrv_hard_ref(bs)
> > > > ...
> > > > bdrv_hard_unref(bs)
> > > >
> > > > bdrv_unref(bs)
> > > >
> > > >
> > > > 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?
--
Fam
Re: [Qemu-devel] [PATCH v2 01/11] block: replace in_use with refcnt_soft and refcnt_hard
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()
> > >
> > > ...
> > > bdrv_unref(bs)
> > >
> > >
> > > or with hard ref:
> > > bs = bdrv_new()
> > >
> > >
> > > bdrv_hard_ref(bs)
> > > ...
> > > bdrv_hard_unref(bs)
> > >
> > > bdrv_unref(bs)
> > >
> > >
> > > 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.
Stefan
Re: [Qemu-devel] [PATCH v2 01/11] block: replace in_use with refcnt_soft and refcnt_hard
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()
> >
> > ...
> > bdrv_unref(bs)
> >
> >
> > or with hard ref:
> > bs = bdrv_new()
> >
> >
> > bdrv_hard_ref(bs)
> > ...
> > bdrv_hard_unref(bs)
> >
> > bdrv_unref(bs)
> >
> >
> > 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.
--
Fam
Re: [Qemu-devel] [PATCH v2 01/11] block: replace in_use with refcnt_soft and refcnt_hard
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()
>
> ...
> bdrv_unref(bs)
>
>
> or with hard ref:
> bs = bdrv_new()
>
>
> bdrv_hard_ref(bs)
> ...
> bdrv_hard_unref(bs)
>
> bdrv_unref(bs)
>
>
> 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()?
Re: [Qemu-devel] [PATCH v2 01/11] block: replace in_use with refcnt_soft and refcnt_hard
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()
...
bdrv_unref(bs)
or with hard ref:
bs = bdrv_new()
bdrv_hard_ref(bs)
...
bdrv_hard_unref(bs)
bdrv_unref(bs)
The second bdrv_hard_ref call to a bs returns false, caller check the
return value.
--
Fam
Re: [Qemu-devel] [PATCH v2 01/11] block: replace in_use with refcnt_soft and refcnt_hard
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.
Re: [Qemu-devel] [PATCH v2 01/11] block: replace in_use with refcnt_soft and refcnt_hard
On Wed, 07/17 14:26, Paolo Bonzini wrote: > Il 17/07/2013 11:42, Fam Zheng ha scritto: > > 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(). > > Pardon the stupid question: why do we need a "soft" reference? Added in other patch of the series: drive_add if=none,file=foo.qcow2,backing=bar ^^^ This is the reason, with it we can add arbitrary devices referencing bar as backing_hd, which we can't backtrack from "bar" when user runs drive_del, so we can't simply delete. Either we close all children (and descendants), or we keep it until no one references it. In this context, it's solved with reference count. Why not hard? Becasue hard ref blocks block job. -- Fam
Re: [Qemu-devel] [PATCH v2 01/11] block: replace in_use with refcnt_soft and refcnt_hard
Il 17/07/2013 11:42, Fam Zheng ha scritto: > 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(). Pardon the stupid question: why do we need a "soft" reference? We have these behaviors: - a sync:'none' backup job doesn't stop until cancelled - cancelling or completing the job closes the target device, which in turn stops the NBD server and removes the need to access the source device via backing_hd - ejecting the source device cancels the job, which in turn also removes the need to access the source device via backing_hd blockdev-backup can sipmly add a reference to the DriveInfo of the target that it receives. backup_start has to choose between using drive_put_ref and bdrv_delete on the target, and can do so using drive_get_by_blockdev. backup_start can also mark the target in use, so that drive_del is prevented while backup_start is running. After the target device is closed (and not in_use anymore), all you need to do is invoke drive_del to delete the BDS. I don't dislike the series, but I wonder if all this machinery is actually needed in 1.6. Paolo
[Qemu-devel] [PATCH v2 01/11] block: replace in_use with refcnt_soft and refcnt_hard
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().
Signed-off-by: Fam Zheng
---
block-migration.c | 4 ++--
block.c | 40 +++-
blockjob.c | 6 +++---
hw/block/dataplane/virtio-blk.c | 4 ++--
include/block/block.h | 5 +++--
include/block/block_int.h | 3 ++-
6 files changed, 43 insertions(+), 19 deletions(-)
diff --git a/block-migration.c b/block-migration.c
index 2fd7699..d558410 100644
--- a/block-migration.c
+++ b/block-migration.c
@@ -321,7 +321,7 @@ static void init_blk_migration_it(void *opaque,
BlockDriverState *bs)
bmds->shared_base = block_mig_state.shared_base;
alloc_aio_bitmap(bmds);
drive_get_ref(drive_get_by_blockdev(bs));
-bdrv_set_in_use(bs, 1);
+bdrv_ref(bs, true);
block_mig_state.total_sector_sum += sectors;
@@ -557,7 +557,7 @@ static void blk_mig_cleanup(void)
blk_mig_lock();
while ((bmds = QSIMPLEQ_FIRST(&block_mig_state.bmds_list)) != NULL) {
QSIMPLEQ_REMOVE_HEAD(&block_mig_state.bmds_list, entry);
-bdrv_set_in_use(bmds->bs, 0);
+bdrv_unref(bmds->bs, true);
drive_put_ref(drive_get_by_blockdev(bmds->bs));
g_free(bmds->aio_bitmap);
g_free(bmds);
diff --git a/block.c b/block.c
index b560241..4170ff6 100644
--- a/block.c
+++ b/block.c
@@ -1511,8 +1511,11 @@ static void bdrv_move_feature_fields(BlockDriverState
*bs_dest,
/* dirty bitmap */
bs_dest->dirty_bitmap = bs_src->dirty_bitmap;
+/* reference count */
+bs_dest->refcnt_soft= bs_src->refcnt_soft;
+bs_dest->refcnt_hard= bs_src->refcnt_hard;
+
/* job */
-bs_dest->in_use = bs_src->in_use;
bs_dest->job= bs_src->job;
/* keep the same entry in bdrv_states */
@@ -1542,7 +1545,6 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState
*bs_old)
assert(bs_new->dirty_bitmap == NULL);
assert(bs_new->job == NULL);
assert(bs_new->dev == NULL);
-assert(bs_new->in_use == 0);
assert(bs_new->io_limits_enabled == false);
assert(bs_new->block_timer == NULL);
@@ -1561,7 +1563,6 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState
*bs_old)
/* Check a few fields that should remain attached to the device */
assert(bs_new->dev == NULL);
assert(bs_new->job == NULL);
-assert(bs_new->in_use == 0);
assert(bs_new->io_limits_enabled == false);
assert(bs_new->block_timer == NULL);
@@ -1598,7 +1599,6 @@ void bdrv_delete(BlockDriverState *bs)
{
assert(!bs->dev);
assert(!bs->job);
-assert(!bs->in_use);
/* remove from list, if necessary */
bdrv_make_anon(bs);
@@ -4374,15 +4374,37 @@ int64_t bdrv_get_dirty_count(BlockDriverState *bs)
}
}
-void bdrv_set_in_use(BlockDriverState *bs, int in_use)
+/* Get a soft or hard reference to bs */
+void bdrv_ref(BlockDriverState *bs, bool is_hard)
+{
+if (is_hard) {
+bs->refcnt_hard++;
+} else {
+bs->refcnt_soft++;
+}
+}
+
+/* Release a previously grabbed reference to bs, need to specify if it is hard
+ * or soft. If after this releasing, both soft and hard reference counts are
+ * zero, the BlockDriverState is deleted. */
+void bdrv_unref(BlockDriverState *bs, bool is_hard)
{
-assert(bs->in_use != in_use);
-bs->in_use = in_use;
+if (is_hard) {
+assert(bs->refcnt_hard > 0);
+bs->refcnt_hard--;
+} else {
+assert(bs->refcnt_soft > 0);
+bs->refcnt_soft--;
+}
+if (bs->refcnt_hard == 0 && bs->refcnt_soft == 0) {
+bdrv_close(bs);
+bdrv_delete(bs);
+}
}
-int bdrv_in_use(BlockDriverState *bs)
+bool bdrv_in_use(BlockDriverState *bs)
{
-return bs->in_use;
+return bs->refcnt_hard > 0;
}
void bdrv_iostatus_enable(BlockDriverState *bs)
diff --git a/blockjob.c b/blockjob.c
index ca80df1..24f07f9 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -45,7 +45,7 @
