| Message ID | 1374054136-28741-2-git-send-email-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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.
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.
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.
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()?
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.
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
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?
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
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;
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(-)