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

[Kevin Wolf]

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