[v2,01/11] block: replace in_use with refcnt_soft and refcnt_hard
diff mbox

Message ID 1374054136-28741-2-git-send-email-famz@redhat.com
State New
Headers show

Commit Message

Fam Zheng July 17, 2013, 9:42 a.m. UTC
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 <famz@redhat.com>
---
 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(-)

Comments

Paolo Bonzini July 17, 2013, 12:26 p.m. UTC | #1
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
Fam Zheng July 18, 2013, 4:53 a.m. UTC | #2
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.
Stefan Hajnoczi July 23, 2013, 9:36 a.m. UTC | #3
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.
Fam Zheng July 23, 2013, 10:32 a.m. UTC | #4
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.
Stefan Hajnoczi July 23, 2013, 1:34 p.m. UTC | #5
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()?
Fam Zheng July 24, 2013, 12:39 a.m. UTC | #6
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.
Stefan Hajnoczi July 24, 2013, 7:35 a.m. UTC | #7
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.

Stefan
Fam Zheng July 24, 2013, 7:44 a.m. UTC | #8
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?
Stefan Hajnoczi July 25, 2013, 7:52 a.m. UTC | #9
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

Patch
diff mbox

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 @@  void *block_job_create(const BlockJobType *job_type, BlockDriverState *bs,
         error_set(errp, QERR_DEVICE_IN_USE, bdrv_get_device_name(bs));
         return NULL;
     }
-    bdrv_set_in_use(bs, 1);
+    bdrv_ref(bs, true);
 
     job = g_malloc0(job_type->instance_size);
     job->job_type      = job_type;
@@ -63,7 +63,7 @@  void *block_job_create(const BlockJobType *job_type, BlockDriverState *bs,
         if (error_is_set(&local_err)) {
             bs->job = NULL;
             g_free(job);
-            bdrv_set_in_use(bs, 0);
+            bdrv_unref(bs, true);
             error_propagate(errp, local_err);
             return NULL;
         }
@@ -79,7 +79,7 @@  void block_job_completed(BlockJob *job, int ret)
     job->cb(job->opaque, ret);
     bs->job = NULL;
     g_free(job);
-    bdrv_set_in_use(bs, 0);
+    bdrv_unref(bs, true);
 }
 
 void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 0356665..3075e84 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -431,7 +431,7 @@  bool virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk,
     s->blk = blk;
 
     /* Prevent block operations that conflict with data plane thread */
-    bdrv_set_in_use(blk->conf.bs, 1);
+    bdrv_ref(blk->conf.bs, true);
 
     error_setg(&s->migration_blocker,
             "x-data-plane does not support migration");
@@ -450,7 +450,7 @@  void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane *s)
     virtio_blk_data_plane_stop(s);
     migrate_del_blocker(s->migration_blocker);
     error_free(s->migration_blocker);
-    bdrv_set_in_use(s->blk->conf.bs, 0);
+    bdrv_unref(s->blk->conf.bs, true);
     g_free(s);
 }
 
diff --git a/include/block/block.h b/include/block/block.h
index b6b9014..8df8794 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -354,8 +354,9 @@  int64_t bdrv_get_dirty_count(BlockDriverState *bs);
 void bdrv_enable_copy_on_read(BlockDriverState *bs);
 void bdrv_disable_copy_on_read(BlockDriverState *bs);
 
-void bdrv_set_in_use(BlockDriverState *bs, int in_use);
-int bdrv_in_use(BlockDriverState *bs);
+void bdrv_ref(BlockDriverState *bs, bool is_hard);
+void bdrv_unref(BlockDriverState *bs, bool is_hard);
+bool bdrv_in_use(BlockDriverState *bs);
 
 #ifdef CONFIG_LINUX_AIO
 int raw_get_aio_fd(BlockDriverState *bs);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index c6ac871..9de5a8f 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -294,7 +294,8 @@  struct BlockDriverState {
     BlockDeviceIoStatus iostatus;
     char device_name[32];
     HBitmap *dirty_bitmap;
-    int in_use; /* users other than guest access, eg. block migration */
+    int refcnt_soft;
+    int refcnt_hard;
     QTAILQ_ENTRY(BlockDriverState) list;
 
     QLIST_HEAD(, BdrvTrackedRequest) tracked_requests;