| Message ID | 20220711110725.425261-1-den@openvz.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] block: add missed block_acct_setup with new block device init procedure | expand |
On 11.07.2022 13: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. > > 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, > 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) > write-cache = "auto" > share-rw = false > + account-invalid = true > + account-failed = true > drive-type = "288" > floppy1 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) > Attached to: /machine/unattached/device[N] > @@ -145,6 +153,8 @@ Testing: -fda TEST_DIR/t.qcow2 -fdb TEST_DIR/t.qcow2.2 > 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) > @@ -157,6 +167,8 @@ Testing: -fda TEST_DIR/t.qcow2 -fdb TEST_DIR/t.qcow2.2 > 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] > @@ -199,6 +211,8 @@ Testing: -fdb > discard_granularity = 4294967295 (4 GiB) > write-cache = "auto" > share-rw = false > + account-invalid = true > + account-failed = true > drive-type = "288" > dev: floppy, id "" > unit = 0 (0x0) > @@ -211,6 +225,8 @@ Testing: -fdb > discard_granularity = 4294967295 (4 GiB) > write-cache = "auto" > share-rw = false > + account-invalid = true > + account-failed = true > drive-type = "288" > > > @@ -238,6 +254,8 @@ Testing: -drive if=floppy,file=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] > @@ -275,6 +293,8 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2,index=1 > 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) > @@ -287,6 +307,8 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2,index=1 > discard_granularity = 4294967295 (4 GiB) > write-cache = "auto" > share-rw = false > + account-invalid = true > + account-failed = true > drive-type = "288" > floppy1 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) > Attached to: /machine/unattached/device[N] > @@ -328,6 +350,8 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=floppy,file=TEST_DIR/t > 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) > @@ -340,6 +364,8 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=floppy,file=TEST_DIR/t > 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] > @@ -385,6 +411,8 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0 > discard_granularity = 4294967295 (4 GiB) > write-cache = "auto" > share-rw = false > + account-invalid = true > + account-failed = true > drive-type = "144" > none0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) > Attached to: /machine/peripheral-anon/device[N] > @@ -422,6 +450,8 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0,unit=1 > discard_granularity = 4294967295 (4 GiB) > write-cache = "auto" > share-rw = false > + account-invalid = true > + account-failed = true > drive-type = "144" > none0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) > Attached to: /machine/peripheral-anon/device[N] > @@ -459,6 +489,8 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qco > 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) > @@ -471,6 +503,8 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qco > discard_granularity = 4294967295 (4 GiB) > write-cache = "auto" > share-rw = false > + account-invalid = true > + account-failed = true > drive-type = "144" > none0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) > Attached to: /machine/peripheral-anon/device[N] > @@ -522,6 +556,8 @@ Testing: -fda TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl > 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) > @@ -534,6 +570,8 @@ Testing: -fda TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl > 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] > @@ -576,6 +614,8 @@ Testing: -fda TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl > 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) > @@ -588,6 +628,8 @@ Testing: -fda TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl > 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] > @@ -630,6 +672,8 @@ Testing: -fdb TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl > discard_granularity = 4294967295 (4 GiB) > write-cache = "auto" > share-rw = false > + account-invalid = true > + account-failed = true > drive-type = "144" > dev: floppy, id "" > unit = 1 (0x1) > @@ -642,6 +686,8 @@ Testing: -fdb TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl > discard_granularity = 4294967295 (4 GiB) > write-cache = "auto" > share-rw = false > + account-invalid = true > + account-failed = true > drive-type = "144" > floppy1 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) > Attached to: /machine/unattached/device[N] > @@ -684,6 +730,8 @@ Testing: -fdb TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl > discard_granularity = 4294967295 (4 GiB) > write-cache = "auto" > share-rw = false > + account-invalid = true > + account-failed = true > drive-type = "144" > dev: floppy, id "" > unit = 1 (0x1) > @@ -696,6 +744,8 @@ Testing: -fdb TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl > discard_granularity = 4294967295 (4 GiB) > write-cache = "auto" > share-rw = false > + account-invalid = true > + account-failed = true > drive-type = "144" > floppy1 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) > Attached to: /machine/unattached/device[N] > @@ -747,6 +797,8 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.q > 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) > @@ -759,6 +811,8 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.q > 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] > @@ -801,6 +855,8 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.q > 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) > @@ -813,6 +869,8 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.q > 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] > @@ -861,6 +919,8 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -global floppy.drive=none0 -device > discard_granularity = 4294967295 (4 GiB) > write-cache = "auto" > share-rw = false > + account-invalid = true > + account-failed = true > drive-type = "144" > none0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) > Attached to: /machine/peripheral-anon/device[N] > @@ -928,6 +988,8 @@ Testing: -device floppy > discard_granularity = 4294967295 (4 GiB) > write-cache = "auto" > share-rw = false > + account-invalid = true > + account-failed = true > drive-type = "288" > > Testing: -device floppy,drive-type=120 > @@ -952,6 +1014,8 @@ Testing: -device floppy,drive-type=120 > discard_granularity = 4294967295 (4 GiB) > write-cache = "auto" > share-rw = false > + account-invalid = true > + account-failed = true > drive-type = "120" > > Testing: -device floppy,drive-type=144 > @@ -976,6 +1040,8 @@ Testing: -device floppy,drive-type=144 > discard_granularity = 4294967295 (4 GiB) > write-cache = "auto" > share-rw = false > + account-invalid = true > + account-failed = true > drive-type = "144" > > Testing: -device floppy,drive-type=288 > @@ -1000,6 +1066,8 @@ Testing: -device floppy,drive-type=288 > discard_granularity = 4294967295 (4 GiB) > write-cache = "auto" > share-rw = false > + account-invalid = true > + account-failed = true > drive-type = "288" > > > @@ -1027,6 +1095,8 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0,drive-t > discard_granularity = 4294967295 (4 GiB) > write-cache = "auto" > share-rw = false > + account-invalid = true > + account-failed = true > drive-type = "120" > none0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) > Attached to: /machine/peripheral-anon/device[N] > @@ -1064,6 +1134,8 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0,drive-t > discard_granularity = 4294967295 (4 GiB) > write-cache = "auto" > share-rw = false > + account-invalid = true > + account-failed = true > drive-type = "288" > none0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) > Attached to: /machine/peripheral-anon/device[N] > @@ -1104,6 +1176,8 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0,logical > discard_granularity = 4294967295 (4 GiB) > write-cache = "auto" > share-rw = false > + account-invalid = true > + account-failed = true > drive-type = "144" > none0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) > Attached to: /machine/peripheral-anon/device[N] > @@ -1141,6 +1215,8 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0,physica > discard_granularity = 4294967295 (4 GiB) > write-cache = "auto" > share-rw = false > + account-invalid = true > + account-failed = true > drive-type = "144" > none0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) > Attached to: /machine/peripheral-anon/device[N] ping
On 11.07.2022 13: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. > > 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, > 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) > write-cache = "auto" > share-rw = false > + account-invalid = true > + account-failed = true > drive-type = "288" > floppy1 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) > Attached to: /machine/unattached/device[N] > @@ -145,6 +153,8 @@ Testing: -fda TEST_DIR/t.qcow2 -fdb TEST_DIR/t.qcow2.2 > 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) > @@ -157,6 +167,8 @@ Testing: -fda TEST_DIR/t.qcow2 -fdb TEST_DIR/t.qcow2.2 > 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] > @@ -199,6 +211,8 @@ Testing: -fdb > discard_granularity = 4294967295 (4 GiB) > write-cache = "auto" > share-rw = false > + account-invalid = true > + account-failed = true > drive-type = "288" > dev: floppy, id "" > unit = 0 (0x0) > @@ -211,6 +225,8 @@ Testing: -fdb > discard_granularity = 4294967295 (4 GiB) > write-cache = "auto" > share-rw = false > + account-invalid = true > + account-failed = true > drive-type = "288" > > > @@ -238,6 +254,8 @@ Testing: -drive if=floppy,file=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] > @@ -275,6 +293,8 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2,index=1 > 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) > @@ -287,6 +307,8 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2,index=1 > discard_granularity = 4294967295 (4 GiB) > write-cache = "auto" > share-rw = false > + account-invalid = true > + account-failed = true > drive-type = "288" > floppy1 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) > Attached to: /machine/unattached/device[N] > @@ -328,6 +350,8 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=floppy,file=TEST_DIR/t > 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) > @@ -340,6 +364,8 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=floppy,file=TEST_DIR/t > 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] > @@ -385,6 +411,8 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0 > discard_granularity = 4294967295 (4 GiB) > write-cache = "auto" > share-rw = false > + account-invalid = true > + account-failed = true > drive-type = "144" > none0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) > Attached to: /machine/peripheral-anon/device[N] > @@ -422,6 +450,8 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0,unit=1 > discard_granularity = 4294967295 (4 GiB) > write-cache = "auto" > share-rw = false > + account-invalid = true > + account-failed = true > drive-type = "144" > none0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) > Attached to: /machine/peripheral-anon/device[N] > @@ -459,6 +489,8 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qco > 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) > @@ -471,6 +503,8 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qco > discard_granularity = 4294967295 (4 GiB) > write-cache = "auto" > share-rw = false > + account-invalid = true > + account-failed = true > drive-type = "144" > none0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) > Attached to: /machine/peripheral-anon/device[N] > @@ -522,6 +556,8 @@ Testing: -fda TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl > 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) > @@ -534,6 +570,8 @@ Testing: -fda TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl > 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] > @@ -576,6 +614,8 @@ Testing: -fda TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl > 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) > @@ -588,6 +628,8 @@ Testing: -fda TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl > 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] > @@ -630,6 +672,8 @@ Testing: -fdb TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl > discard_granularity = 4294967295 (4 GiB) > write-cache = "auto" > share-rw = false > + account-invalid = true > + account-failed = true > drive-type = "144" > dev: floppy, id "" > unit = 1 (0x1) > @@ -642,6 +686,8 @@ Testing: -fdb TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl > discard_granularity = 4294967295 (4 GiB) > write-cache = "auto" > share-rw = false > + account-invalid = true > + account-failed = true > drive-type = "144" > floppy1 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) > Attached to: /machine/unattached/device[N] > @@ -684,6 +730,8 @@ Testing: -fdb TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl > discard_granularity = 4294967295 (4 GiB) > write-cache = "auto" > share-rw = false > + account-invalid = true > + account-failed = true > drive-type = "144" > dev: floppy, id "" > unit = 1 (0x1) > @@ -696,6 +744,8 @@ Testing: -fdb TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl > discard_granularity = 4294967295 (4 GiB) > write-cache = "auto" > share-rw = false > + account-invalid = true > + account-failed = true > drive-type = "144" > floppy1 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) > Attached to: /machine/unattached/device[N] > @@ -747,6 +797,8 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.q > 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) > @@ -759,6 +811,8 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.q > 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] > @@ -801,6 +855,8 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.q > 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) > @@ -813,6 +869,8 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.q > 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] > @@ -861,6 +919,8 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -global floppy.drive=none0 -device > discard_granularity = 4294967295 (4 GiB) > write-cache = "auto" > share-rw = false > + account-invalid = true > + account-failed = true > drive-type = "144" > none0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) > Attached to: /machine/peripheral-anon/device[N] > @@ -928,6 +988,8 @@ Testing: -device floppy > discard_granularity = 4294967295 (4 GiB) > write-cache = "auto" > share-rw = false > + account-invalid = true > + account-failed = true > drive-type = "288" > > Testing: -device floppy,drive-type=120 > @@ -952,6 +1014,8 @@ Testing: -device floppy,drive-type=120 > discard_granularity = 4294967295 (4 GiB) > write-cache = "auto" > share-rw = false > + account-invalid = true > + account-failed = true > drive-type = "120" > > Testing: -device floppy,drive-type=144 > @@ -976,6 +1040,8 @@ Testing: -device floppy,drive-type=144 > discard_granularity = 4294967295 (4 GiB) > write-cache = "auto" > share-rw = false > + account-invalid = true > + account-failed = true > drive-type = "144" > > Testing: -device floppy,drive-type=288 > @@ -1000,6 +1066,8 @@ Testing: -device floppy,drive-type=288 > discard_granularity = 4294967295 (4 GiB) > write-cache = "auto" > share-rw = false > + account-invalid = true > + account-failed = true > drive-type = "288" > > > @@ -1027,6 +1095,8 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0,drive-t > discard_granularity = 4294967295 (4 GiB) > write-cache = "auto" > share-rw = false > + account-invalid = true > + account-failed = true > drive-type = "120" > none0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) > Attached to: /machine/peripheral-anon/device[N] > @@ -1064,6 +1134,8 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0,drive-t > discard_granularity = 4294967295 (4 GiB) > write-cache = "auto" > share-rw = false > + account-invalid = true > + account-failed = true > drive-type = "288" > none0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) > Attached to: /machine/peripheral-anon/device[N] > @@ -1104,6 +1176,8 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0,logical > discard_granularity = 4294967295 (4 GiB) > write-cache = "auto" > share-rw = false > + account-invalid = true > + account-failed = true > drive-type = "144" > none0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) > Attached to: /machine/peripheral-anon/device[N] > @@ -1141,6 +1215,8 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0,physica > discard_granularity = 4294967295 (4 GiB) > write-cache = "auto" > share-rw = false > + account-invalid = true > + account-failed = true > drive-type = "144" > none0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) > Attached to: /machine/peripheral-anon/device[N] ping v2
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) [..]
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) > > [..] > >
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
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
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
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, 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) write-cache = "auto" share-rw = false + account-invalid = true + account-failed = true drive-type = "288" floppy1 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) Attached to: /machine/unattached/device[N] @@ -145,6 +153,8 @@ Testing: -fda TEST_DIR/t.qcow2 -fdb TEST_DIR/t.qcow2.2 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) @@ -157,6 +167,8 @@ Testing: -fda TEST_DIR/t.qcow2 -fdb TEST_DIR/t.qcow2.2 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] @@ -199,6 +211,8 @@ Testing: -fdb discard_granularity = 4294967295 (4 GiB) write-cache = "auto" share-rw = false + account-invalid = true + account-failed = true drive-type = "288" dev: floppy, id "" unit = 0 (0x0) @@ -211,6 +225,8 @@ Testing: -fdb discard_granularity = 4294967295 (4 GiB) write-cache = "auto" share-rw = false + account-invalid = true + account-failed = true drive-type = "288" @@ -238,6 +254,8 @@ Testing: -drive if=floppy,file=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] @@ -275,6 +293,8 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2,index=1 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) @@ -287,6 +307,8 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2,index=1 discard_granularity = 4294967295 (4 GiB) write-cache = "auto" share-rw = false + account-invalid = true + account-failed = true drive-type = "288" floppy1 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) Attached to: /machine/unattached/device[N] @@ -328,6 +350,8 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=floppy,file=TEST_DIR/t 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) @@ -340,6 +364,8 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=floppy,file=TEST_DIR/t 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] @@ -385,6 +411,8 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0 discard_granularity = 4294967295 (4 GiB) write-cache = "auto" share-rw = false + account-invalid = true + account-failed = true drive-type = "144" none0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) Attached to: /machine/peripheral-anon/device[N] @@ -422,6 +450,8 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0,unit=1 discard_granularity = 4294967295 (4 GiB) write-cache = "auto" share-rw = false + account-invalid = true + account-failed = true drive-type = "144" none0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) Attached to: /machine/peripheral-anon/device[N] @@ -459,6 +489,8 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qco 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) @@ -471,6 +503,8 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qco discard_granularity = 4294967295 (4 GiB) write-cache = "auto" share-rw = false + account-invalid = true + account-failed = true drive-type = "144" none0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) Attached to: /machine/peripheral-anon/device[N] @@ -522,6 +556,8 @@ Testing: -fda TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl 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) @@ -534,6 +570,8 @@ Testing: -fda TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl 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] @@ -576,6 +614,8 @@ Testing: -fda TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl 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) @@ -588,6 +628,8 @@ Testing: -fda TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl 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] @@ -630,6 +672,8 @@ Testing: -fdb TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl discard_granularity = 4294967295 (4 GiB) write-cache = "auto" share-rw = false + account-invalid = true + account-failed = true drive-type = "144" dev: floppy, id "" unit = 1 (0x1) @@ -642,6 +686,8 @@ Testing: -fdb TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl discard_granularity = 4294967295 (4 GiB) write-cache = "auto" share-rw = false + account-invalid = true + account-failed = true drive-type = "144" floppy1 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) Attached to: /machine/unattached/device[N] @@ -684,6 +730,8 @@ Testing: -fdb TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl discard_granularity = 4294967295 (4 GiB) write-cache = "auto" share-rw = false + account-invalid = true + account-failed = true drive-type = "144" dev: floppy, id "" unit = 1 (0x1) @@ -696,6 +744,8 @@ Testing: -fdb TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl discard_granularity = 4294967295 (4 GiB) write-cache = "auto" share-rw = false + account-invalid = true + account-failed = true drive-type = "144" floppy1 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) Attached to: /machine/unattached/device[N] @@ -747,6 +797,8 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.q 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) @@ -759,6 +811,8 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.q 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] @@ -801,6 +855,8 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.q 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) @@ -813,6 +869,8 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.q 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] @@ -861,6 +919,8 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -global floppy.drive=none0 -device discard_granularity = 4294967295 (4 GiB) write-cache = "auto" share-rw = false + account-invalid = true + account-failed = true drive-type = "144" none0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) Attached to: /machine/peripheral-anon/device[N] @@ -928,6 +988,8 @@ Testing: -device floppy discard_granularity = 4294967295 (4 GiB) write-cache = "auto" share-rw = false + account-invalid = true + account-failed = true drive-type = "288" Testing: -device floppy,drive-type=120 @@ -952,6 +1014,8 @@ Testing: -device floppy,drive-type=120 discard_granularity = 4294967295 (4 GiB) write-cache = "auto" share-rw = false + account-invalid = true + account-failed = true drive-type = "120" Testing: -device floppy,drive-type=144 @@ -976,6 +1040,8 @@ Testing: -device floppy,drive-type=144 discard_granularity = 4294967295 (4 GiB) write-cache = "auto" share-rw = false + account-invalid = true + account-failed = true drive-type = "144" Testing: -device floppy,drive-type=288 @@ -1000,6 +1066,8 @@ Testing: -device floppy,drive-type=288 discard_granularity = 4294967295 (4 GiB) write-cache = "auto" share-rw = false + account-invalid = true + account-failed = true drive-type = "288" @@ -1027,6 +1095,8 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0,drive-t discard_granularity = 4294967295 (4 GiB) write-cache = "auto" share-rw = false + account-invalid = true + account-failed = true drive-type = "120" none0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) Attached to: /machine/peripheral-anon/device[N] @@ -1064,6 +1134,8 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0,drive-t discard_granularity = 4294967295 (4 GiB) write-cache = "auto" share-rw = false + account-invalid = true + account-failed = true drive-type = "288" none0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) Attached to: /machine/peripheral-anon/device[N] @@ -1104,6 +1176,8 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0,logical discard_granularity = 4294967295 (4 GiB) write-cache = "auto" share-rw = false + account-invalid = true + account-failed = true drive-type = "144" none0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) Attached to: /machine/peripheral-anon/device[N] @@ -1141,6 +1215,8 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0,physica discard_granularity = 4294967295 (4 GiB) write-cache = "auto" share-rw = false + account-invalid = true + account-failed = true drive-type = "144" none0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2) Attached to: /machine/peripheral-anon/device[N]
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. 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(-)