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

[Denis V. Lunev]

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

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

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?

Den



> > This means in particular that defaults for block accounting statistics
> > are changed and account_invalid/account_failed are actually initialized
> > as false instead of true originally.
> >
> > This commit changes things to match original world. It adds
> > account_invalid/account_failed properties to BlockConf and setups them
> > accordingly.
> >
> > Signed-off-by: Denis V. Lunev <den@openvz.org>
> > CC: Peter Krempa <pkrempa@redhat.com>
> > CC: Markus Armbruster <armbru@redhat.com>
> > CC: John Snow <jsnow@redhat.com>
> > CC: Kevin Wolf <kwolf@redhat.com>
> > CC: Hanna Reitz <hreitz@redhat.com>
> > ---
> >   hw/block/block.c           |  2 +
> >   include/hw/block/block.h   |  7 +++-
> >   tests/qemu-iotests/172.out | 76 ++++++++++++++++++++++++++++++++++++++
> >   3 files changed, 84 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/block/block.c b/hw/block/block.c
> > index 25f45df723..53b100cdc3 100644
> > --- a/hw/block/block.c
> > +++ b/hw/block/block.c
> > @@ -205,6 +205,8 @@ bool blkconf_apply_backend_options(BlockConf 
> > *conf, bool readonly,
>      BlockdevOnError rerror;
> >       blk_set_enable_write_cache(blk, wce);
> >       blk_set_on_error(blk, rerror, werror);
> >   +    block_acct_setup(blk_get_stats(blk), conf->account_invalid,
> > +                     conf->account_failed);
> >       return true;
> >   }
> >   diff --git a/include/hw/block/block.h b/include/hw/block/block.h
> > index 5902c0440a..ffd439fc83 100644
> > --- a/include/hw/block/block.h
> > +++ b/include/hw/block/block.h
> > @@ -31,6 +31,7 @@ typedef struct BlockConf {
> >       uint32_t lcyls, lheads, lsecs;
> >       OnOffAuto wce;
> >       bool share_rw;
> > +    bool account_invalid, account_failed;
> >       BlockdevOnError rerror;
> >       BlockdevOnError werror;
> >   } BlockConf;
> > @@ -61,7 +62,11 @@ static inline unsigned int 
> > get_physical_block_exp(BlockConf *conf)
> >                          _conf.discard_granularity, 
> > -1),                  \
> >       DEFINE_PROP_ON_OFF_AUTO("write-cache", _state, 
> > _conf.wce,           \
> > ON_OFF_AUTO_AUTO),                          \
> > -    DEFINE_PROP_BOOL("share-rw", _state, _conf.share_rw, false)
> > +    DEFINE_PROP_BOOL("share-rw", _state, _conf.share_rw, 
> > false),        \
> > +    DEFINE_PROP_BOOL("account-invalid", 
> > _state,                         \
> > +                     _conf.account_invalid, 
> > true),                      \
> > +    DEFINE_PROP_BOOL("account-failed", 
> > _state,                          \
> > +                     _conf.account_failed, true)
> >     #define DEFINE_BLOCK_PROPERTIES(_state, 
> > _conf)                          \
> >       DEFINE_PROP_DRIVE("drive", _state, 
> > _conf.blk),                      \
> > diff --git a/tests/qemu-iotests/172.out b/tests/qemu-iotests/172.out
> > index 9479b92185..a6c451e098 100644
> > --- a/tests/qemu-iotests/172.out
> > +++ b/tests/qemu-iotests/172.out
> > @@ -28,6 +28,8 @@ Formatting 'TEST_DIR/t.IMGFMT.3', fmt=IMGFMT 
> > size=737280
> >                   discard_granularity = 4294967295 (4 GiB)
> >                   write-cache = "auto"
> >                   share-rw = false
> > +                account-invalid = true
> > +                account-failed = true
> >                   drive-type = "288"
> >     @@ -55,6 +57,8 @@ Testing: -fda TEST_DIR/t.qcow2
> >                   discard_granularity = 4294967295 (4 GiB)
> >                   write-cache = "auto"
> >                   share-rw = false
> > +                account-invalid = true
> > +                account-failed = true
> >                   drive-type = "144"
> >   floppy0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2)
> >       Attached to:      /machine/unattached/device[N]
> > @@ -92,6 +96,8 @@ Testing: -fdb TEST_DIR/t.qcow2
> >                   discard_granularity = 4294967295 (4 GiB)
> >                   write-cache = "auto"
> >                   share-rw = false
> > +                account-invalid = true
> > +                account-failed = true
> >                   drive-type = "144"
> >                 dev: floppy, id ""
> >                   unit = 0 (0x0)
> > @@ -104,6 +110,8 @@ Testing: -fdb TEST_DIR/t.qcow2
> >                   discard_granularity = 4294967295 (4 GiB)
>
> [..]