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

[Vladimir Sementsov-Ogievskiy]

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.

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.

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.


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

[..]


--
Best regards,
Vladimir