[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 1/1] block: add missed block_acct_setup with new block device

From: Denis V. Lunev
Subject: Re: [PATCH 1/1] block: add missed block_acct_setup with new block device init procedure
Date: Fri, 29 Jul 2022 14:36:23 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0

On 29.07.2022 11:13, Kevin Wolf wrote:
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
instead block nodes are created by -blockdev / qmp: blockdev-add, and
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

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.

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.
I have said about very small patch like the following

iris ~/src/qemu $ git diff
diff --git a/block/accounting.c b/block/accounting.c
index 2030851d79..c20d6ba9a0 100644
--- a/block/accounting.c
+++ b/block/accounting.c
@@ -38,6 +38,8 @@ void block_acct_init(BlockAcctStats *stats)
     if (qtest_enabled()) {
         clock_type = QEMU_CLOCK_VIRTUAL;
+    stats->account_invalid = true;
+    stats->account_failed = true;

 void block_acct_setup(BlockAcctStats *stats, bool account_invalid,
iris ~/src/qemu $

but your proposal with ON_OFF_AUTO will work for me too.

The real question - do we really need to publish this option
for the external to configure it?

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.

I have to say sorry again. I have found this place once I have
reverted to my very old series discussed here + some late
additions on top of it done by Vladimir.

I will definitely have to come back to this later.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]