Re: [Qemu-devel] [PATCH 1/7] block: Convert BlockDriverState.in_use to refcount

[Fam Zheng]

On Tue, 07/02 12:21, Paolo Bonzini wrote:
> Il 02/07/2013 07:59, Fam Zheng ha scritto:
> > Use numeric value to replace in_use flag in BDS, this will make
> > lifecycle management with ref count possible. This patch only replaces
> > existing uses of bdrv_set_in_use, so no logic change here.
> 
> This still does not entirely explain the rules for who sets in_use (or
> gets a reference) and who checks in_use.
> 
> Why should offline commit worry about the disk being shared, for
> example?  The reason "of course" is that a background job might modify
> bs->backing_hd while commit is running (or might expect bs->backing_hd
> to not change).  However, this is in no way related to a reference count.
> 
> So I think your series is doing two things right (setting the in_use
> flag for BDSes in the ->file and ->backing_hd chains; turning the in_use
> flag into a counter) and one wrong (tying bdrv_in_use checks to the
> lifecycle).  I wonder thus if we need two counters: the "in use" counter
> and the reference count for the lifecycle.

But the two are not orthognal, it's somehow like rlock and wlock.  What
I try is to simplify the (usual) logic that a BDS user calls
bdrv_get_ref() for lifecycle as well as bdrv_set_in_use() to secure the
state. With refcount as a sign for in_use, they just need inference if
its ref count is more than one: it's shared, don't modify it, and expect
it to change, like rwlock implies.

I think commit should worry about the disk being shared: We eventually
need the target of drive-backup to be exported through NBD, which means
it has a device id, and its backing_hd is also attached to guest device,
so we can't commit it. That said, I think a BDS being shared is an
evidence for not to make any change. Do we have any exception on this
rule?

For NBD there is close notifier, so I want to drop the refcount (PATCH
3/7).

> > -void bdrv_set_in_use(BlockDriverState *bs, int in_use)
> > +void bdrv_put_ref(BlockDriverState *bs)
> > +{
> > +    assert(bs->refcount > 0);
> > +    bs->refcount--;
> > +}
> > +
> > +void bdrv_get_ref(BlockDriverState *bs)
> >  {
> > -    assert(bs->in_use != in_use);
> > -    bs->in_use = in_use;
> > +    bs->refcount++;
> >  }
> 
> The convention that we use in QEMU is bdrv_ref/bdrv_unref.

I'm following drive_get_ref() here.

Thanks.

Fam