From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 896C9C00144 for ; Fri, 29 Jul 2022 09:18:33 +0000 (UTC) Received: from localhost ([::1]:57088 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1oHM8e-000182-3S for qemu-devel@archiver.kernel.org; Fri, 29 Jul 2022 05:18:32 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:43252) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1oHM40-0007fH-L0 for qemu-devel@nongnu.org; Fri, 29 Jul 2022 05:13:46 -0400 Received: from us-smtp-delivery-124.mimecast.com ([170.10.133.124]:43699) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1oHM3x-00050y-KS for qemu-devel@nongnu.org; Fri, 29 Jul 2022 05:13:43 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1659086020; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=GtbZod99ECF6vnOpdd33wDdVI09FDTfxI8iMqcPBQfE=; b=hnFbSmXsWrT8L821Bhk3L5qsR8+kuT44qeh7fyTGbuVZO8p+y4tJzroXO+YLBNQlhaUxif g8C4bLrxwZ0UXK8Cnc24SoZcbKIaLfCjeJIyGy+vGNiuvhr4fDTBjek+qO9W6WbKbgx1l9 ybhj0AKTM1ndo4fi7CIOSagRMDCvr6w= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-78-LzrRyygDMOqhKm-B4wrFWg-1; Fri, 29 Jul 2022 05:13:38 -0400 X-MC-Unique: LzrRyygDMOqhKm-B4wrFWg-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.rdu2.redhat.com [10.11.54.8]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 25427101A54E; Fri, 29 Jul 2022 09:13:38 +0000 (UTC) Received: from redhat.com (unknown [10.39.194.195]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 0EEDAC27D95; Fri, 29 Jul 2022 09:13:35 +0000 (UTC) Date: Fri, 29 Jul 2022 11:13:34 +0200 From: Kevin Wolf To: "Denis V. Lunev" Cc: Vladimir Sementsov-Ogievskiy , qemu-block@nongnu.org, qemu-devel@nongnu.org, qemu-stable@nongnu.org, Peter Krempa , Markus Armbruster , John Snow , Hanna Reitz Subject: Re: [PATCH 1/1] block: add missed block_acct_setup with new block device init procedure Message-ID: References: <20220711110725.425261-1-den@openvz.org> <1aa3921a-0e67-d580-9bf2-c098d242e380@yandex-team.ru> <66373021-7dad-953b-b244-75a4756a0b33@virtuozzo.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <66373021-7dad-953b-b244-75a4756a0b33@virtuozzo.com> X-Scanned-By: MIMEDefang 2.85 on 10.11.54.8 Received-SPF: pass client-ip=170.10.133.124; envelope-from=kwolf@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -21 X-Spam_score: -2.2 X-Spam_bar: -- X-Spam_report: (-2.2 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.082, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" Am 28.07.2022 um 21:27 hat Denis V. Lunev geschrieben: > On 28.07.2022 16:42, Vladimir Sementsov-Ogievskiy wrote: > > On 7/11/22 14:07, Denis V. Lunev wrote: > > > Commit 5f76a7aac156ca75680dad5df4a385fd0b58f6b1 is looking harmless from > > > the first glance, but it has changed things a lot. 'libvirt' uses it to > > > detect that it should follow new initialization way and this changes > > > things considerably. With this procedure followed, blockdev_init() is > > > not called anymore and thus block_acct_setup() helper is not called. > > > > I'm not sure that 5f76a7aac156ca is really the corner stone.. But yes, > > libvirt moved to "blockdev era", which means that we don't use old > > -drive, > > instead block nodes are created by -blockdev / qmp: blockdev-add, and > > attached > > to block devices by node-name. > > > git bisected, thus I am sure here > > > > And if I understand correctly blockdev_init() is called only on -drive > > path. > > > > I have some questions: > > > > 1. After this patch, don't we call block_acct_setup() twice on old path > > with -drive? That seems safe as block_acct_setup just assign fields of > > BlockAcctStats.. But that's doesn't look good. > > > hmmm I don't think it's actually correct because then a value that was explicitly set with -drive will by overridden by the default provided by the device. A possible solution would be to switch the defaults in the BlockBackend initialisation back to true, and then have a ON_OFF_AUTO property in the devices to allow overriding the default from -drive. With -blockdev, the BlockBackend default will be hard coded to true and the options of the devices will be the only way to change it. > > 2. Do we really need these options? Could we instead just enable > > accounting invalid and failed ops unconditionally? I doubt that someone > > will learn that these new options appeared and will use them to disable > > the failed/invalid accounting again. > > > I can move assignment of these fields to true int > block_acct_init() and forget about "configurable" > items in new path. I do not think that somebody > ever has these options set. Well, whether anyone uses the option is a different question. I don't know. But it has existed for many years. > The real question in this patch is that this initialization > was a precondition for old good "long IO" report > configuration, which should be "enableable". > > But  we could move this option to "tracked request" > layer only and this will solve my puzzle. So, I'll move > "long IO report" to tracked request level only and will > create an option for it on bdrv_ level and will avoid > it on blk_ accounting. > > What do you think? I'm not sure what you mean by "long IO report". Don't these switches just change which kind of operations are counted into statistics rather than changing the structure of the report? Conceptually, I would like accounting on the block node level, but it's not what we have been doing, so it would be a big change. Kevin