* [PATCH 0/3] quorum: Require WRITE perm with rewrite-corrupted @ 2020-11-13 21:17 Max Reitz 2020-11-13 21:17 ` [PATCH 1/3] " Max Reitz ` (3 more replies) 0 siblings, 4 replies; 6+ messages in thread From: Max Reitz @ 2020-11-13 21:17 UTC (permalink / raw) To: qemu-block; +Cc: Kevin Wolf, Alberto Garcia, qemu-devel, Max Reitz Hi, While reviewing Berto’s block-status/write-zeroes series for quorum, I wondered how quorum’s permission code handles rewrite-corrupted. It turns out it doesn’t, and so qemu with a read-only rewrite-corrupted quorum node simply crashes once there is a mismatch that leads to a rewrite. It looks to me like this bug has existed for quite some time, so I don’t think this series must go into 5.2. OTOH, it’s a simple bug fix, so I suppose it might as well. Max Reitz (3): quorum: Require WRITE perm with rewrite-corrupted iotests/081: Filter image format after testdir iotests/081: Test rewrite-corrupted without WRITE block/quorum.c | 5 ++++ tests/qemu-iotests/081 | 61 ++++++++++++++++++++++++++++++++++++-- tests/qemu-iotests/081.out | 27 +++++++++++++++++ 3 files changed, 90 insertions(+), 3 deletions(-) -- 2.28.0 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/3] quorum: Require WRITE perm with rewrite-corrupted 2020-11-13 21:17 [PATCH 0/3] quorum: Require WRITE perm with rewrite-corrupted Max Reitz @ 2020-11-13 21:17 ` Max Reitz 2020-11-16 12:22 ` Alberto Garcia 2020-11-13 21:17 ` [PATCH 2/3] iotests/081: Filter image format after testdir Max Reitz ` (2 subsequent siblings) 3 siblings, 1 reply; 6+ messages in thread From: Max Reitz @ 2020-11-13 21:17 UTC (permalink / raw) To: qemu-block; +Cc: Kevin Wolf, Alberto Garcia, qemu-devel, Max Reitz Using rewrite-corrupted means quorum may issue writes to its children just from receiving read requests from its parents. Thus, it must take the WRITE permission when rewrite-corrupted is used. Signed-off-by: Max Reitz <mreitz@redhat.com> --- block/quorum.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/block/quorum.c b/block/quorum.c index b70d365ba9..01e49e94a3 100644 --- a/block/quorum.c +++ b/block/quorum.c @@ -1195,7 +1195,12 @@ static void quorum_child_perm(BlockDriverState *bs, BdrvChild *c, uint64_t perm, uint64_t shared, uint64_t *nperm, uint64_t *nshared) { + BDRVQuorumState *s = bs->opaque; + *nperm = perm & DEFAULT_PERM_PASSTHROUGH; + if (s->rewrite_corrupted) { + *nperm |= BLK_PERM_WRITE; + } /* * We cannot share RESIZE or WRITE, as this would make the -- 2.28.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/3] quorum: Require WRITE perm with rewrite-corrupted 2020-11-13 21:17 ` [PATCH 1/3] " Max Reitz @ 2020-11-16 12:22 ` Alberto Garcia 0 siblings, 0 replies; 6+ messages in thread From: Alberto Garcia @ 2020-11-16 12:22 UTC (permalink / raw) To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel, Max Reitz On Fri 13 Nov 2020 10:17:16 PM CET, Max Reitz wrote: > Using rewrite-corrupted means quorum may issue writes to its children > just from receiving read requests from its parents. Thus, it must take > the WRITE permission when rewrite-corrupted is used. > > Signed-off-by: Max Reitz <mreitz@redhat.com> Reviewed-by: Alberto Garcia <berto@igalia.com> Berto ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/3] iotests/081: Filter image format after testdir 2020-11-13 21:17 [PATCH 0/3] quorum: Require WRITE perm with rewrite-corrupted Max Reitz 2020-11-13 21:17 ` [PATCH 1/3] " Max Reitz @ 2020-11-13 21:17 ` Max Reitz 2020-11-13 21:17 ` [PATCH 3/3] iotests/081: Test rewrite-corrupted without WRITE Max Reitz 2020-11-17 11:38 ` [PATCH 0/3] quorum: Require WRITE perm with rewrite-corrupted Kevin Wolf 3 siblings, 0 replies; 6+ messages in thread From: Max Reitz @ 2020-11-13 21:17 UTC (permalink / raw) To: qemu-block; +Cc: Kevin Wolf, Alberto Garcia, qemu-devel, Max Reitz Otherwise, this breaks whenever the test directory contains the image format (e.g. "/tmp/test-raw-file" is filtered to "/tmp/test-IMGFMT-file" instead of "TEST_DIR"). Signed-off-by: Max Reitz <mreitz@redhat.com> --- tests/qemu-iotests/081 | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tests/qemu-iotests/081 b/tests/qemu-iotests/081 index 537d40dfd5..253b7576be 100755 --- a/tests/qemu-iotests/081 +++ b/tests/qemu-iotests/081 @@ -45,15 +45,16 @@ _require_drivers quorum do_run_qemu() { - echo Testing: "$@" | _filter_imgfmt + echo Testing: "$@" $QEMU -nographic -qmp stdio -serial none "$@" echo } run_qemu() { - do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qemu | _filter_qmp\ - | _filter_qemu_io | _filter_generated_node_ids + do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_imgfmt | _filter_qemu \ + | _filter_qmp | _filter_qemu_io \ + | _filter_generated_node_ids } quorum="driver=raw,file.driver=quorum,file.vote-threshold=2" -- 2.28.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 3/3] iotests/081: Test rewrite-corrupted without WRITE 2020-11-13 21:17 [PATCH 0/3] quorum: Require WRITE perm with rewrite-corrupted Max Reitz 2020-11-13 21:17 ` [PATCH 1/3] " Max Reitz 2020-11-13 21:17 ` [PATCH 2/3] iotests/081: Filter image format after testdir Max Reitz @ 2020-11-13 21:17 ` Max Reitz 2020-11-17 11:38 ` [PATCH 0/3] quorum: Require WRITE perm with rewrite-corrupted Kevin Wolf 3 siblings, 0 replies; 6+ messages in thread From: Max Reitz @ 2020-11-13 21:17 UTC (permalink / raw) To: qemu-block; +Cc: Kevin Wolf, Alberto Garcia, qemu-devel, Max Reitz Test what happens when a rewrite-corrupted quorum node performs such a rewrite, while there is no parent that has taken the WRITE permission. Signed-off-by: Max Reitz <mreitz@redhat.com> --- tests/qemu-iotests/081 | 54 ++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/081.out | 27 +++++++++++++++++++ 2 files changed, 81 insertions(+) diff --git a/tests/qemu-iotests/081 b/tests/qemu-iotests/081 index 253b7576be..4e19972931 100755 --- a/tests/qemu-iotests/081 +++ b/tests/qemu-iotests/081 @@ -42,6 +42,7 @@ _supported_fmt raw _supported_proto file _supported_os Linux _require_drivers quorum +_require_devices virtio-scsi do_run_qemu() { @@ -155,6 +156,59 @@ echo "== checking that quorum has corrected the corrupted file ==" $QEMU_IO -c "read -P 0x32 0 $size" "$TEST_DIR/2.raw" | _filter_qemu_io +echo +echo "== using quorum rewrite corrupted mode without WRITE permission ==" + +# The same as above, but this time, do it on a quorum node whose only +# parent will not take the WRITE permission + +echo '-- corrupting --' +# Only corrupt a portion: The guest device (scsi-hd on virtio-scsi) +# will read some data (looking for a partition table to guess the +# disk's geometry), which would trigger a quorum mismatch if the +# beginning of the image was corrupted. The subsequent +# QUORUM_REPORT_BAD event would be suppressed (because at that point, +# there cannot have been a qmp_capabilities on the monitor). Because +# that event is rate-limited, the next QUORUM_REPORT_BAD that happens +# thanks to our qemu-io read (which should trigger a mismatch) would +# then be delayed past the VM quit and not appear in the output. +# So we keep the first 1M intact to see a QUORUM_REPORT_BAD resulting +# from the qemu-io invocation. +$QEMU_IO -c "write -P 0x42 1M 1M" "$TEST_DIR/2.raw" | _filter_qemu_io + +# Fix the corruption (on a read-only quorum node, i.e. without taking +# the WRITE permission on it -- its child nodes need to be R/W OTOH, +# so that rewrite-corrupted works) +echo +echo '-- running quorum --' +run_qemu \ + -blockdev file,node-name=file1,filename="$TEST_DIR/1.raw" \ + -blockdev file,node-name=file2,filename="$TEST_DIR/2.raw" \ + -blockdev file,node-name=file3,filename="$TEST_DIR/3.raw" \ + -blockdev '{ + "driver": "quorum", + "node-name": "quorum", + "read-only": true, + "vote-threshold": 2, + "rewrite-corrupted": true, + "children": [ "file1", "file2", "file3" ] + }' \ + -device virtio-scsi,id=scsi \ + -device scsi-hd,id=quorum-drive,bus=scsi.0,drive=quorum \ + <<EOF +{ "execute": "qmp_capabilities" } +{ + "execute": "human-monitor-command", + "arguments": { + "command-line": 'qemu-io -d quorum-drive "read -P 0x32 0 $size"' + } +} +{ "execute": "quit" } +EOF + +echo '-- checking that the image has been corrected --' +$QEMU_IO -c "read -P 0x32 0 $size" "$TEST_DIR/2.raw" | _filter_qemu_io + echo echo "== breaking quorum ==" diff --git a/tests/qemu-iotests/081.out b/tests/qemu-iotests/081.out index 04091b64e5..1974262fac 100644 --- a/tests/qemu-iotests/081.out +++ b/tests/qemu-iotests/081.out @@ -47,6 +47,33 @@ read 10485760/10485760 bytes at offset 0 read 10485760/10485760 bytes at offset 0 10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +== using quorum rewrite corrupted mode without WRITE permission == +-- corrupting -- +wrote 1048576/1048576 bytes at offset 1048576 +1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) + +-- running quorum -- +Testing: -blockdev file,node-name=file1,filename=TEST_DIR/1.IMGFMT -blockdev file,node-name=file2,filename=TEST_DIR/2.IMGFMT -blockdev file,node-name=file3,filename=TEST_DIR/3.IMGFMT -blockdev { + "driver": "quorum", + "node-name": "quorum", + "read-only": true, + "vote-threshold": 2, + "rewrite-corrupted": true, + "children": [ "file1", "file2", "file3" ] + } -device virtio-scsi,id=scsi -device scsi-hd,id=quorum-drive,bus=scsi.0,drive=quorum +QMP_VERSION +{"return": {}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "QUORUM_REPORT_BAD", "data": {"node-name": "file2", "sectors-count": 20480, "sector-num": 0, "type": "read"}} +read 10485760/10485760 bytes at offset 0 +10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +{"return": ""} +{"return": {}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}} + +-- checking that the image has been corrected -- +read 10485760/10485760 bytes at offset 0 +10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) + == breaking quorum == wrote 10485760/10485760 bytes at offset 0 10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) -- 2.28.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 0/3] quorum: Require WRITE perm with rewrite-corrupted 2020-11-13 21:17 [PATCH 0/3] quorum: Require WRITE perm with rewrite-corrupted Max Reitz ` (2 preceding siblings ...) 2020-11-13 21:17 ` [PATCH 3/3] iotests/081: Test rewrite-corrupted without WRITE Max Reitz @ 2020-11-17 11:38 ` Kevin Wolf 3 siblings, 0 replies; 6+ messages in thread From: Kevin Wolf @ 2020-11-17 11:38 UTC (permalink / raw) To: Max Reitz; +Cc: Alberto Garcia, qemu-devel, qemu-block Am 13.11.2020 um 22:17 hat Max Reitz geschrieben: > Hi, > > While reviewing Berto’s block-status/write-zeroes series for quorum, I > wondered how quorum’s permission code handles rewrite-corrupted. It > turns out it doesn’t, and so qemu with a read-only rewrite-corrupted > quorum node simply crashes once there is a mismatch that leads to a > rewrite. > > It looks to me like this bug has existed for quite some time, so I don’t > think this series must go into 5.2. OTOH, it’s a simple bug fix, so I > suppose it might as well. Thanks, applied to the block branch. Kevin ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-11-17 11:41 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-11-13 21:17 [PATCH 0/3] quorum: Require WRITE perm with rewrite-corrupted Max Reitz 2020-11-13 21:17 ` [PATCH 1/3] " Max Reitz 2020-11-16 12:22 ` Alberto Garcia 2020-11-13 21:17 ` [PATCH 2/3] iotests/081: Filter image format after testdir Max Reitz 2020-11-13 21:17 ` [PATCH 3/3] iotests/081: Test rewrite-corrupted without WRITE Max Reitz 2020-11-17 11:38 ` [PATCH 0/3] quorum: Require WRITE perm with rewrite-corrupted Kevin Wolf
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.