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

[Denis V. Lunev]

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
> > > -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.
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.
https://lists.defectivebydesign.org/archive/html/qemu-devel/2020-07/msg03772.html

I will definitely have to come back to this later.

Den