From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37064) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Uw6xN-0003bm-8V for qemu-devel@nongnu.org; Mon, 08 Jul 2013 04:38:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Uw6xK-0002Sm-Je for qemu-devel@nongnu.org; Mon, 08 Jul 2013 04:38:01 -0400 Received: from mx1.redhat.com ([209.132.183.28]:54967) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Uw6xK-0002Sd-Bl for qemu-devel@nongnu.org; Mon, 08 Jul 2013 04:37:58 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r688buFY009301 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 8 Jul 2013 04:37:57 -0400 Date: Mon, 8 Jul 2013 16:37:55 +0800 From: Fam Zheng Message-ID: <20130708083755.GA24551@T430s.redhat.com> References: <1372744789-997-1-git-send-email-famz@redhat.com> <1372744789-997-2-git-send-email-famz@redhat.com> <51D2A9B8.90508@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <51D2A9B8.90508@redhat.com> Subject: Re: [Qemu-devel] [PATCH 1/7] block: Convert BlockDriverState.in_use to refcount Reply-To: famz@redhat.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: kwolf@redhat.com, rjones@redhat.com, obarenbo@redhat.com, roliveri@redhat.com, hbrock@redhat.com, armbru@redhat.com, qemu-devel@nongnu.org, pmyers@redhat.com, imain@redhat.com, stefanha@redhat.com 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