From patchwork Tue Mar 29 15:08:03 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kevin Wolf X-Patchwork-Id: 8688001 Return-Path: X-Original-To: patchwork-qemu-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 27B289F36E for ; Tue, 29 Mar 2016 15:09:44 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 45097201C0 for ; Tue, 29 Mar 2016 15:09:43 +0000 (UTC) Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 3331A20165 for ; Tue, 29 Mar 2016 15:09:42 +0000 (UTC) Received: from localhost ([::1]:47674 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1akvH3-0002Z1-GV for patchwork-qemu-devel@patchwork.kernel.org; Tue, 29 Mar 2016 11:09:41 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39603) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1akvGj-0002Rm-Ln for qemu-devel@nongnu.org; Tue, 29 Mar 2016 11:09:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1akvGi-00062c-9z for qemu-devel@nongnu.org; Tue, 29 Mar 2016 11:09:21 -0400 Received: from mx1.redhat.com ([209.132.183.28]:51466) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1akvGY-0005ze-3E; Tue, 29 Mar 2016 11:09:10 -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 (Postfix) with ESMTPS id CC85078228; Tue, 29 Mar 2016 15:09:09 +0000 (UTC) Received: from noname.redhat.com (ovpn-116-120.ams2.redhat.com [10.36.116.120]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u2TF912e011786; Tue, 29 Mar 2016 11:09:08 -0400 From: Kevin Wolf To: qemu-block@nongnu.org Date: Tue, 29 Mar 2016 17:08:03 +0200 Message-Id: <1459264128-12761-4-git-send-email-kwolf@redhat.com> In-Reply-To: <1459264128-12761-1-git-send-email-kwolf@redhat.com> References: <1459264128-12761-1-git-send-email-kwolf@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.24 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 209.132.183.28 Cc: kwolf@redhat.com, qemu-devel@nongnu.org Subject: [Qemu-devel] [PULL 03/48] block: Remove dirty bitmaps from bdrv_move_feature_fields() X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP This patch changes dirty bitmaps from following a BlockBackend in graph changes to sticking with the node they were created at. For the full discussion, read the following mailing list thread: [Qemu-block] block: Dirty bitmaps and COR in bdrv_move_feature_fields() https://lists.nongnu.org/archive/html/qemu-block/2016-02/msg00745.html In summary, the justification for this change is: * When moving the dirty bitmap to the top of the tree was introduced in bdrv_append() in commit a9fc4408, it didn't actually have any effect because there could never be a bitmap in use when bdrv_append() was called (op blockers would prevent this). This is still true today for all internal uses of dirty bitmaps. * Support for user-defined dirty bitmaps was introduced in 2.4, but we discouraged users from using it because we didn't consider it ready yet. Moreover, in 2.5, the bdrv_swap() removal introduced a bug that left dangling pointers if a dirty bitmap was present (the anchors of the dirty bitmap were swapped, but the back link in the first element wasn't updated), so it didn't even work correctly. * block-dirty-bitmap-add takes an arbitrary node name, even if no BlockBackend is attached. This suggests that it is a node level operation and not a BlockBackend one. Consequently, there is no reason for dirty bitmaps to stay with a BlockBackend that was attached to the node they were created for. * It was suggested that block-dirty-bitmap-add could track the node if a node name was specified, and track the BlockBackend if the device name was specified. This would however be inconsistent with other QMP commands. Commands that accept both device and node names currently interpret the device name just as an alias for the current root node of that BlockBackend. * Dirty bitmaps have a name that is only unique amongst the bitmaps in a specific node. Moving bitmaps could lead to name clashes. Automatic renaming would involve too much magic. * Persistent bitmaps are stored in a specific node. Moving them around automatically might be at least surprising, but it would probably also become a real problem because that would have to happen atomically without the management tool knowing of the operation. At the end of the day it seems to be very clear that it was a mistake to include dirty bitmaps in bdrv_move_feature_fields(). The functionality of moving bitmaps and/or attaching them to a BlockBackend instead will probably be needed, but it should be done with a new explicit QMP command or option. Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake --- block.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/block.c b/block.c index ee34c8c..6592b35 100644 --- a/block.c +++ b/block.c @@ -2249,9 +2249,6 @@ static void bdrv_move_feature_fields(BlockDriverState *bs_dest, /* dev info */ bs_dest->enable_write_cache = bs_src->enable_write_cache; - - /* dirty bitmap */ - bs_dest->dirty_bitmaps = bs_src->dirty_bitmaps; } static void change_parent_backing_link(BlockDriverState *from,