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
