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

[Kevin Wolf]

Am 29.07.2022 um 14:36 hat Denis V. Lunev geschrieben:
> 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;
>  }

Yes, this looks good to me and we'll need it either way, even if we add
the ON_OFF_AUTO property to devices (because we need to set the right
default for 'auto').

>  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?

As I said above, I don't know if anyone uses the option.

It would be needed for full feature parity of -blockdev with -drive,
but if the option isn't used by anyone, maybe full feature parity isn't
something we even want.

> > > 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

Oh, we never merged this upstream it seems?

> I will definitely have to come back to this later.
> 
> Den

Kevin