From patchwork Tue Jun 19 01:43:10 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Michael Roth X-Patchwork-Id: 931239 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=none (p=none dis=none) header.from=linux.vnet.ibm.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="gU0+oCef"; dkim-atps=neutral Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 418rPp0zz4z9s2t for ; Tue, 19 Jun 2018 11:45:18 +1000 (AEST) Received: from localhost ([::1]:38361 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fV5hr-0000dK-ML for incoming@patchwork.ozlabs.org; Mon, 18 Jun 2018 21:45:15 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44506) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fV5gf-00007K-W2 for qemu-devel@nongnu.org; Mon, 18 Jun 2018 21:44:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fV5ge-0007f3-Oy for qemu-devel@nongnu.org; Mon, 18 Jun 2018 21:44:02 -0400 Received: from mail-ot0-x235.google.com ([2607:f8b0:4003:c0f::235]:33924) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1fV5ge-0007eu-JD; Mon, 18 Jun 2018 21:44:00 -0400 Received: by mail-ot0-x235.google.com with SMTP id r18-v6so20779327otk.1; Mon, 18 Jun 2018 18:44:00 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=sender:from:to:cc:subject:date:message-id:in-reply-to:references; bh=cS6u/NIYryXPvoUb+Zo+tFYzGvs/Tsxq+3j3TvEUaXA=; b=gU0+oCef4/hrqA/xHKakFMAoO8cT/BSuhDD74Oj5/k7PfV7skelSuhuUpf6vir7Rw5 uikBxiJ74sU2SFr3riO0QBJx9G6WEY0lqAieAxo2bht+7+aLD0kcNMBeRjq34MuxsmNt JVjWSzPY/3UszvvZ6AAnkwa8d29ir5ncaWeFtLxfV0cxcr4eapITAfZwHHm3pyGfgoti gZBxMlRyriO3EjeroFLsQ3HFRkJFPm+IFsjutgjYXurOckKYvKQyr5A+gXcnk63H/k9s r1T3OXIKfz43dYW7CPVKQKgURonkLDRKVdz/hclmSJNyqW/eS5TfkqRRuoWU9+mN8UZ8 sPMQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:sender:from:to:cc:subject:date:message-id :in-reply-to:references; bh=cS6u/NIYryXPvoUb+Zo+tFYzGvs/Tsxq+3j3TvEUaXA=; b=hsQw1vpa/VXh+GD4qPKMVFouSg8dg0Tw8A5QA0vj9d+Rp+rc5nG1brp/cRJqA/JkD5 jtvfIlfbKuZb5b16Y+kjJ/ptsHVXGwcAGaVrUxFbl1WmJmDgFBGy0p4jnt4iBc0ZQHST J5MIbZ/suAs9C1EKkP85OzO4gHJKFqCyBwiQpPKZo9bT7P97c3exuJGi6JqQ9q17EAAc epHjmp5rGP21U516meO9hEIQFMVm6lyEWIuiXjWDzmxVIGwKNBFH4oT/btwnqg6fHHB6 HNeK5VUUJdHOJ77gKNo/2I3giZ/q6tBPTfydi/rDmLFoh91J5LXx0QtXgvPVCHgxLXP3 CrXA== X-Gm-Message-State: APt69E3LxUIt95YEzYgYlWpX8ixDUAYGclrY85vppgIEfJzgkp0QF+RX Aj2L9czKtZKw2FurVrQ3nBXeZM0y X-Google-Smtp-Source: ADUXVKJ7cr7ImvIIU6mP2tmEkqhk/0HL0ETs7gx5YkG4ttf1irt5wuUBu834mGlL4thTw0jKUCc2lA== X-Received: by 2002:a9d:5190:: with SMTP id y16-v6mr9695618otg.252.1529372639452; Mon, 18 Jun 2018 18:43:59 -0700 (PDT) Received: from localhost ([2600:1700:70:e488:b0ee:9bda:ee6f:91be]) by smtp.gmail.com with ESMTPSA id u13-v6sm6636783oiv.18.2018.06.18.18.43.58 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 18 Jun 2018 18:43:58 -0700 (PDT) From: Michael Roth To: qemu-devel@nongnu.org Date: Mon, 18 Jun 2018 20:43:10 -0500 Message-Id: <20180619014319.28272-105-mdroth@linux.vnet.ibm.com> X-Mailer: git-send-email 2.11.0 In-Reply-To: <20180619014319.28272-1-mdroth@linux.vnet.ibm.com> References: <20180619014319.28272-1-mdroth@linux.vnet.ibm.com> X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 2607:f8b0:4003:c0f::235 Subject: [Qemu-devel] [PATCH 104/113] throttle: Fix crash on reopen 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: Alberto Garcia , qemu-stable@nongnu.org, Max Reitz Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: "Qemu-devel" From: Alberto Garcia The throttle block filter can be reopened, and with this it is possible to change the throttle group that the filter belongs to. The way the code does that is the following: - On throttle_reopen_prepare(): create a new ThrottleGroupMember and attach it to the new throttle group. - On throttle_reopen_commit(): detach the old ThrottleGroupMember, delete it and replace it with the new one. The problem with this is that by replacing the ThrottleGroupMember the previous value of io_limits_disabled is lost, causing an assertion failure in throttle_co_drain_end(). This problem can be reproduced by reopening a throttle node: $QEMU -monitor stdio -object throttle-group,id=tg0,x-iops-total=1000 \ -blockdev node-name=hd0,driver=qcow2,file.driver=file,file.filename=hd.qcow2 \ -blockdev node-name=root,driver=throttle,throttle-group=tg0,file=hd0,read-only=on (qemu) block_stream root block/throttle.c:214: throttle_co_drain_end: Assertion `tgm->io_limits_disabled' failed. Since we only want to change the throttle group on reopen there's no need to create a ThrottleGroupMember and discard the old one. It's easier if we simply detach it from its current group and attach it to the new one. Signed-off-by: Alberto Garcia Message-id: 20180608151536.7378-1-berto@igalia.com Signed-off-by: Max Reitz (cherry picked from commit bc33c047d1ec0b35c9cd8be62bcefae2da28654f) Signed-off-by: Michael Roth --- block/throttle.c | 54 +++++++++++++++++++++++++++++++++--------------------- 1 file changed, 33 insertions(+), 21 deletions(-) diff --git a/block/throttle.c b/block/throttle.c index 833175ac77..d5903784c0 100644 --- a/block/throttle.c +++ b/block/throttle.c @@ -35,9 +35,12 @@ static QemuOptsList throttle_opts = { }, }; -static int throttle_configure_tgm(BlockDriverState *bs, - ThrottleGroupMember *tgm, - QDict *options, Error **errp) +/* + * If this function succeeds then the throttle group name is stored in + * @group and must be freed by the caller. + * If there's an error then @group remains unmodified. + */ +static int throttle_parse_options(QDict *options, char **group, Error **errp) { int ret; const char *group_name; @@ -62,8 +65,7 @@ static int throttle_configure_tgm(BlockDriverState *bs, goto fin; } - /* Register membership to group with name group_name */ - throttle_group_register_tgm(tgm, group_name, bdrv_get_aio_context(bs)); + *group = g_strdup(group_name); ret = 0; fin: qemu_opts_del(opts); @@ -74,6 +76,8 @@ static int throttle_open(BlockDriverState *bs, QDict *options, int flags, Error **errp) { ThrottleGroupMember *tgm = bs->opaque; + char *group; + int ret; bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file, false, errp); @@ -83,7 +87,14 @@ static int throttle_open(BlockDriverState *bs, QDict *options, bs->supported_write_flags = bs->file->bs->supported_write_flags; bs->supported_zero_flags = bs->file->bs->supported_zero_flags; - return throttle_configure_tgm(bs, tgm, options, errp); + ret = throttle_parse_options(options, &group, errp); + if (ret == 0) { + /* Register membership to group with name group_name */ + throttle_group_register_tgm(tgm, group, bdrv_get_aio_context(bs)); + g_free(group); + } + + return ret; } static void throttle_close(BlockDriverState *bs) @@ -159,35 +170,36 @@ static void throttle_attach_aio_context(BlockDriverState *bs, static int throttle_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue, Error **errp) { - ThrottleGroupMember *tgm; + int ret; + char *group = NULL; assert(reopen_state != NULL); assert(reopen_state->bs != NULL); - reopen_state->opaque = g_new0(ThrottleGroupMember, 1); - tgm = reopen_state->opaque; - - return throttle_configure_tgm(reopen_state->bs, tgm, reopen_state->options, - errp); + ret = throttle_parse_options(reopen_state->options, &group, errp); + reopen_state->opaque = group; + return ret; } static void throttle_reopen_commit(BDRVReopenState *reopen_state) { - ThrottleGroupMember *old_tgm = reopen_state->bs->opaque; - ThrottleGroupMember *new_tgm = reopen_state->opaque; + BlockDriverState *bs = reopen_state->bs; + ThrottleGroupMember *tgm = bs->opaque; + char *group = reopen_state->opaque; + + assert(group); - throttle_group_unregister_tgm(old_tgm); - g_free(old_tgm); - reopen_state->bs->opaque = new_tgm; + if (strcmp(group, throttle_group_get_name(tgm))) { + throttle_group_unregister_tgm(tgm); + throttle_group_register_tgm(tgm, group, bdrv_get_aio_context(bs)); + } + g_free(reopen_state->opaque); reopen_state->opaque = NULL; } static void throttle_reopen_abort(BDRVReopenState *reopen_state) { - ThrottleGroupMember *tgm = reopen_state->opaque; - - throttle_group_unregister_tgm(tgm); - g_free(tgm); + g_free(reopen_state->opaque); reopen_state->opaque = NULL; }