From patchwork Wed Nov 7 13:10:00 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Daniel Henrique Barboza X-Patchwork-Id: 994253 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=nongnu.org (client-ip=2001:4830:134:3::11; helo=lists.gnu.org; envelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org; receiver=) Authentication-Results: ozlabs.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.b="Yj2PSzSO"; dkim-atps=neutral Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 42qmyZ1Yj3z9s55 for ; Thu, 8 Nov 2018 00:11:32 +1100 (AEDT) Received: from localhost ([::1]:48133 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gKNcH-0006v8-Nd for incoming@patchwork.ozlabs.org; Wed, 07 Nov 2018 08:11:29 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50460) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gKNbh-0006t3-7H for qemu-devel@nongnu.org; Wed, 07 Nov 2018 08:10:54 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1gKNbb-0002uu-9I for qemu-devel@nongnu.org; Wed, 07 Nov 2018 08:10:49 -0500 Received: from mail-qk1-x743.google.com ([2607:f8b0:4864:20::743]:40382) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1gKNba-0002Hr-TN for qemu-devel@nongnu.org; Wed, 07 Nov 2018 08:10:47 -0500 Received: by mail-qk1-x743.google.com with SMTP id y16so19417748qki.7 for ; Wed, 07 Nov 2018 05:10:29 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id:in-reply-to:references; bh=3URNUhrANwM9pXjlHDma6571zhyjIg+B7Iv4mkmm0nw=; b=Yj2PSzSOg+rnQpgpZNiJsu5SqMEF0wR6RDY1MVP7dlw27/0iPsHQH1yJUrQ+9+GUoS AfrsdH7Coz8wy+Ri6PSB+NDp/TXXMXcVtZhyTPjBCY9y0Mh0s1Wxw/6ZQ2xAPlDQT9l2 49Eko+icV91ajXhnUcit4K3T1nbsZgKyLRUSX2jw2AjEnx8ALXAGLZu/qKpNRAyKlGiZ ZRUkGnMH2aHMkZql4LVRiN3c8YYbToU6GHPchZ+VFGlY+zDNbP8tajnmh2Repfsmu2RE 3vi/QZecai9lH/YNSaORxEOCJM8XnUXIi+17wMGzqm9X+ZMzIphl8ggQYUu9CgJQQbVB G5MA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=3URNUhrANwM9pXjlHDma6571zhyjIg+B7Iv4mkmm0nw=; b=byi0+mnO+njn2wHXUjl4R2WR0J9Yat4nyeiMK0IoVIXIy1EMFFOY8eWTqbw4YdHgbG 8HBvev5ap4F1GSpKzPYhSRLCct6uG4Fju+Nm+0xPQbM9XajYbNeRaY2UHhdd5iAVdmT7 gcTBmqPZ5p31gxrkLP25osGmYoSCBd6HMi6Th55vfXCTz/KYxF0mpYEd2OBs9gj4Yxik 2/H6L8dHt/xz0v3pLdjubCPPX+/PmH5Efl07aW6bSv9M7w+xsiFNLIJnI0OOKYKxkuOD ps7orJWrFBsLEa3eeKLpuurDQNAmVni3IsO9JPeEJZbV31r8tM/uNWEfcbeQjY6Smq5R 9qlA== X-Gm-Message-State: AGRZ1gJQ0jrKfV8cWg8CiJXm1sjvRkXKFxvLEFs/Z3Y4NIgWHVy3xuZ6 H2Bn0MrLp8yv3iDZcEb88JVJK0lYfYg= X-Google-Smtp-Source: AJdET5eJJuM4omzbWlCyZQURLOpcGRu4hZ99SfSbblWGMwrm7dBisUwmkLrnLBVa38ULYjjrzOeFdA== X-Received: by 2002:a0c:d5ba:: with SMTP id g55mr139911qvi.37.1541596228851; Wed, 07 Nov 2018 05:10:28 -0800 (PST) Received: from localhost.localdomain ([2804:431:f700:ba45:6a73:9179:662c:d5bc]) by smtp.gmail.com with ESMTPSA id i35-v6sm317282qtb.16.2018.11.07.05.10.27 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Wed, 07 Nov 2018 05:10:28 -0800 (PST) From: Daniel Henrique Barboza To: qemu-devel@nongnu.org Date: Wed, 7 Nov 2018 11:10:00 -0200 Message-Id: <20181107131000.27744-4-danielhb413@gmail.com> X-Mailer: git-send-email 2.17.2 In-Reply-To: <20181107131000.27744-1-danielhb413@gmail.com> References: <20181107131000.27744-1-danielhb413@gmail.com> X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 2607:f8b0:4864:20::743 Subject: [Qemu-devel] [PATCH for-3.2 v3 3/3] qcow2-snapshot: remove redundant find_snapshot_by_id_and_name call X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: kwolf@redhat.com, armbru@redhat.com, Daniel Henrique Barboza , dgilbert@redhat.com, mreitz@redhat.com Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: "Qemu-devel" In qcow2_snapshot_create there is the following code block: /* Generate an ID */ find_new_snapshot_id(bs, sn_info->id_str, sizeof(sn_info->id_str)); /* Check that the ID is unique */ if (find_snapshot_by_id_and_name(bs, sn_info->id_str, NULL) >= 0) { return -EEXIST; } find_new_snapshot_id cycles through all snapshots, getting the id_str as an unsigned long int, calculating the max id_max value of all the existing id_strs and writing in the id_str pointer id_max + 1: for(i = 0; i < s->nb_snapshots; i++) { sn = s->snapshots + i; id = strtoul(sn->id_str, NULL, 10); if (id > id_max) id_max = id; } snprintf(id_str, id_str_size, "%lu", id_max + 1); Here, sn_info->id_str will have the unique value id_max + 1. Right after that, find_snapshot_by_id_and_name is called with id = sn_info->id_str and name = NULL. This will cause the function to execute the following: } else if (id) { for (i = 0; i < s->nb_snapshots; i++) { if (!strcmp(s->snapshots[i].id_str, id)) { return i; } } } In short, we're searching the existing snapshots to see if sn_info->id_str matches any existing id, right after we set in the previous line a sn_info->id_str value that is already unique. The first code block goes way back to commit 585f8587ad, a 2006 commit from Fabrice Bellard that simply says "new qcow2 disk image format". No more info is provided about this logic in any subsequent commits that moved this code block around. I can't say about the original design, but the current logic is redundant. bdrv_snapshot_create is called in aio_context lock, forbidding any concurrent call to accidentally create a new snapshot between the find_new_snapshot_id and find_snapshot_by_id_and_name calls. What we're ending up doing is to cycle through the snapshots two times for no viable reason. This patch eliminates the redundancy by removing the 'id is unique' check that calls find_snapshot_by_id_and_name. Signed-off-by: Daniel Henrique Barboza --- block/qcow2-snapshot.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c index bb6a5b7516..20e8472191 100644 --- a/block/qcow2-snapshot.c +++ b/block/qcow2-snapshot.c @@ -358,11 +358,6 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info) /* Generate an ID */ find_new_snapshot_id(bs, sn_info->id_str, sizeof(sn_info->id_str)); - /* Check that the ID is unique */ - if (find_snapshot_by_id_and_name(bs, sn_info->id_str, NULL) >= 0) { - return -EEXIST; - } - /* Populate sn with passed data */ sn->id_str = g_strdup(sn_info->id_str); sn->name = g_strdup(sn_info->name);