From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60240) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V1tkc-00008q-C3 for qemu-devel@nongnu.org; Wed, 24 Jul 2013 03:44:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V1tkZ-0003uU-AS for qemu-devel@nongnu.org; Wed, 24 Jul 2013 03:44:46 -0400 Received: from mx1.redhat.com ([209.132.183.28]:3114) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V1tkZ-0003uH-3Q for qemu-devel@nongnu.org; Wed, 24 Jul 2013 03:44:43 -0400 Date: Wed, 24 Jul 2013 15:44:38 +0800 From: Fam Zheng Message-ID: <20130724074438.GA26034@T430s.redhat.com> References: <1374054136-28741-1-git-send-email-famz@redhat.com> <1374054136-28741-2-git-send-email-famz@redhat.com> <20130723093600.GB20256@stefanha-thinkpad.redhat.com> <20130723103225.GA7183@T430s.redhat.com> <20130723133438.GA27684@stefanha-thinkpad.redhat.com> <20130724003953.GB7183@T430s.redhat.com> <20130724073504.GB31445@stefanha-thinkpad.muc.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20130724073504.GB31445@stefanha-thinkpad.muc.redhat.com> Subject: Re: [Qemu-devel] [PATCH v2 01/11] block: replace in_use with refcnt_soft and refcnt_hard Reply-To: famz@redhat.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: kwolf@redhat.com, hbrock@redhat.com, qemu-devel@nongnu.org, rjones@redhat.com, imain@redhat.com, Stefan Hajnoczi , pbonzini@redhat.com 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() > > > > > > > > ... > > > > bdrv_unref(bs) > > > > > > > > > > > > or with hard ref: > > > > bs = bdrv_new() > > > > > > > > > > > > bdrv_hard_ref(bs) > > > > ... > > > > bdrv_hard_unref(bs) > > > > > > > > bdrv_unref(bs) > > > > > > > > > > > > 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? -- Fam