[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 01/11] block: replace in_use with refcnt_soft
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [PATCH v2 01/11] block: replace in_use with refcnt_soft and refcnt_hard |
Date: |
Tue, 23 Jul 2013 15:34:38 +0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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()?
- [Qemu-devel] [PATCH v2 00/11] Point-in-time snapshot exporting over NBD, Fam Zheng, 2013/07/17
- [Qemu-devel] [PATCH v2 01/11] block: replace in_use with refcnt_soft and refcnt_hard, Fam Zheng, 2013/07/17
- Re: [Qemu-devel] [PATCH v2 01/11] block: replace in_use with refcnt_soft and refcnt_hard, Paolo Bonzini, 2013/07/17
- Re: [Qemu-devel] [PATCH v2 01/11] block: replace in_use with refcnt_soft and refcnt_hard, Stefan Hajnoczi, 2013/07/23
- Re: [Qemu-devel] [PATCH v2 01/11] block: replace in_use with refcnt_soft and refcnt_hard, Fam Zheng, 2013/07/23
- Re: [Qemu-devel] [PATCH v2 01/11] block: replace in_use with refcnt_soft and refcnt_hard,
Stefan Hajnoczi <=
- Re: [Qemu-devel] [PATCH v2 01/11] block: replace in_use with refcnt_soft and refcnt_hard, Fam Zheng, 2013/07/23
- Re: [Qemu-devel] [PATCH v2 01/11] block: replace in_use with refcnt_soft and refcnt_hard, Stefan Hajnoczi, 2013/07/24
- Re: [Qemu-devel] [PATCH v2 01/11] block: replace in_use with refcnt_soft and refcnt_hard, Fam Zheng, 2013/07/24
- Re: [Qemu-devel] [PATCH v2 01/11] block: replace in_use with refcnt_soft and refcnt_hard, Stefan Hajnoczi, 2013/07/25
[Qemu-devel] [PATCH v2 02/11] block: use refcnt for bs->backing_hd and bs->file, Fam Zheng, 2013/07/17
[Qemu-devel] [PATCH v2 03/11] block: use refcnt for drive_init/drive_uninit, Fam Zheng, 2013/07/17
[Qemu-devel] [PATCH v2 04/11] block: use refcnt for device attach/detach, Fam Zheng, 2013/07/17
[Qemu-devel] [PATCH v2 05/11] migration: omit drive ref as we have bdrv_ref now, Fam Zheng, 2013/07/17