From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37807) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V4T4G-00064L-OP for qemu-devel@nongnu.org; Wed, 31 Jul 2013 05:51:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V4T46-0003lm-9X for qemu-devel@nongnu.org; Wed, 31 Jul 2013 05:51:40 -0400 Received: from mx1.redhat.com ([209.132.183.28]:29902) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V4T45-0003lS-Vv for qemu-devel@nongnu.org; Wed, 31 Jul 2013 05:51:30 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r6V9pStT012524 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Wed, 31 Jul 2013 05:51:28 -0400 Date: Wed, 31 Jul 2013 17:51:17 +0800 From: Fam Zheng Message-ID: <20130731095117.GA1145@T430s.nay.redhat.com> References: <1375170777-31457-1-git-send-email-famz@redhat.com> <1375170777-31457-4-git-send-email-famz@redhat.com> <20130730145848.GE9018@stefanha-thinkpad.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20130730145848.GE9018@stefanha-thinkpad.redhat.com> Subject: Re: [Qemu-devel] [PATCH v2 3/7] block: implement reference count for BlockDriverState Reply-To: famz@redhat.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: kwolf@redhat.com, jcody@redhat.com, qemu-devel@nongnu.org On Tue, 07/30 16:58, Stefan Hajnoczi wrote: > On Tue, Jul 30, 2013 at 03:52:53PM +0800, Fam Zheng wrote: > > @@ -1518,6 +1519,9 @@ static void bdrv_move_feature_fields(BlockDriverState *bs_dest, > > /* dirty bitmap */ > > bs_dest->dirty_bitmap = bs_src->dirty_bitmap; > > > > + /* reference count */ > > + bs_dest->refcnt = bs_src->refcnt; > > + > > /* job */ > > bs_dest->in_use = bs_src->in_use; > > bs_dest->job = bs_src->job; > > Not sure this is correct, but then bdrv_swap() is hard to reason > about... :) > > Imagine an emulated storage controller holds a reference to the > BlockDriverState. When we create an external snapshot we'll > bdrv_swap(old_top, new_top). > > We must not move new_top's refcount into old_top since the old_top > object is still being referenced by the emulated storage controller. > When the emulated storage controller does bdrv_unref() we'll hit the > recount < 0 assertion and be accessing freed memory. > When we swap old_top and new_top, we want to swap all fields except for these, so we use bdrv_move_feature_fields() to move them back (bdrv_swap): tmp = *bs_new; *bs_new = *bs_old; *bs_old = tmp; /* there are some fields that should not be swapped, move them back */ bdrv_move_feature_fields(&tmp, bs_old); bdrv_move_feature_fields(bs_old, bs_new); bdrv_move_feature_fields(bs_new, &tmp); And I agree that refcnt is one of the fields that shouldn't be moved, so it's in bdrv_move_feature_fields(). So isn't above right? Without these lines, it *is* swapped. > > @@ -4392,6 +4396,24 @@ int64_t bdrv_get_dirty_count(BlockDriverState *bs) > > } > > } > > > > +/* Get a reference to bs */ > > +void bdrv_ref(BlockDriverState *bs) > > +{ > > + bs->refcnt++; > > +} > > + > > +/* Release a previously grabbed reference to bs. > > + * If after releasing, reference count is zero, the BlockDriverState is > > + * deleted. */ > > +void bdrv_unref(BlockDriverState *bs) > > +{ > > + assert(bs->refcnt > 0); > > + if (--bs->refcnt == 0) { > > + bdrv_close(bs); > > + bdrv_delete(bs); > > bdrv_delete() already calls bdrv_close() internally, no need to call > bdrv_close() here. OK! -- Fam