Re: [PATCH v4 00/12] Improve reliability of VM tests
On 12/07/2022 20.34, John Snow wrote: On Fri, Jul 8, 2022 at 11:35 AM John Snow wrote: Note: patches 10-12 are included for testing simplicity, they shouldn't be merged. They will be included in a forthcoming block PR. Patches 1-9 are fully reviewed. Whose tree should this go in? I can take them - unless Alex beats me with his next testing pull request ;-) Queued to my testing-next now: https://gitlab.com/thuth/qemu/-/commits/testing-next Thomas
Re: [PATCH v4 00/12] Improve reliability of VM tests
On 08/07/2022 17.34, John Snow wrote: Note: patches 10-12 are included for testing simplicity, they shouldn't be merged. They will be included in a forthcoming block PR. V4: - Addressed concern by Marc-Andre in patch 01. - Squashed Ubuntu patches (rth) This patch series attempts to improve the reliability of several of the VM test targets. In particular, both CentOS 8 tests are non-functional because CentOS 8 was EOL at the beginning of this calendar year, with repositories and mirrors going offline. I also remove the ubuntu.i386 test because we no longer support Ubuntu 18.04 nor do we have explicit need of an i386 build test. After this series, I am able to successfully run every VM target on an x86_64 host, except: - ubuntu.aarch64: Hangs often during testing, see below. - centos.aarch64: Hangs often during testing, see below. - haiku.x86_64: Build failures not addressed by this series, see https://lists.gnu.org/archive/html/qemu-devel/2022-06/msg02103.html The unit tests that I see fail most often under aarch64 are: - virtio-net-failover: Seems to like to hang on openbsd - migration-test: Tends to hang under aarch64 tcg Future work (next version? next series?); - Try to get centos.aarch64 working reliably under TCG - Upgrade ubuntu.aarch64 to 20.04 after fixing centos.aarch64 - Fix the Haiku build test, if possible. - Ensure I can reliably run and pass "make vm-build-all". (Remove VMs from this recipe if necessary.) Not sure whether it's related to your patches, but when testing these I just got an error while running 'vm-build-openbsd' : VM-BUILD openbsd fatal: not a valid object name: failed to archive qemu Failed to prepare guest environment Traceback (most recent call last): File "/home/thuth/devel/qemu/tests/vm/basevm.py", line 641, in main vm.add_source_dir(args.build_qemu) File "/home/thuth/devel/qemu/tests/vm/basevm.py", line 277, in add_source_dir stdout=self._stdout, stderr=self._stderr) File "/usr/lib64/python3.6/subprocess.py", line 311, in check_call raise CalledProcessError(retcode, cmd) subprocess.CalledProcessError: Command '['./scripts/archive-source.sh', '/home/thuth/tmp/qemu-build/vm-test-rm_z92hq.tmp/data-44e42.tar']' returned non-zero exit status 1. The error did not occur again when running the command again, though. Thomas
Re: [RFC PATCH 8/8] block: remove all unused ->can_set_aio_ctx and ->set_aio_ctx callbacks
Am 15/07/2022 um 16:34 schrieb Hanna Reitz: > On 12.07.22 23:19, Emanuele Giuseppe Esposito wrote: >> Together with all _can_set_ and _set_ APIs, as they are not needed >> anymore. >> >> Signed-off-by: Emanuele Giuseppe Esposito >> --- >> block.c | 196 - >> block/block-backend.c | 33 - >> blockjob.c | 35 -- >> include/block/block-global-state.h | 9 -- >> include/block/block_int-common.h | 4 - >> 5 files changed, 277 deletions(-) > > Looks good! I’d just like a follow-up commit that also drops > bdrv_try_set_aio_context(), so it’s all gone (I think that’s the final > remnant?). > It's the same for me, I thought renaming bdrv_try_set_aio_context was a little bit unnecessary. You want to rename it to something else, or directly call bdrv_child_try_change_aio_context? I agree with the rest of comments in this series :) Emanuele
Re: [RFC PATCH 3/8] RFC: block: use transactions as a replacement of ->{can_}set_aio_context()
Am 14/07/2022 um 18:45 schrieb Hanna Reitz: > On 12.07.22 23:19, Emanuele Giuseppe Esposito wrote: >> - >> RFC because I am not sure about the AioContext locks. >> - Do we need to take the new AioContext lock? what does it protect? >> - Taking the old AioContext lock is required now, because of >> bdrv_drained_begin calling AIO_WAIT_WHILE that unlocks the >> aiocontext. If we replace it with AIO_WAIT_WHILE_UNLOCKED, >> could we get rid of taking every time the old AioContext? >> drain would be enough to protect the graph modification. > > It’s been a while, but as far as I remember (which may be wrong), the > reason for how the locks are supposed to be taken was mostly that we > need some defined state so that we know how to invoke > bdrv_drained_begin() and bdrv_drained_end() (i.e. call the first one > as-is, and switch the locks around for the latter one). Right, so bdrv_drained_begin must be always under old aiocontext lock, bdrv_drained_end always under the new one. If so, then I think I can make the whole thing even cleaner: 1. bdrv_drained_begin is taken in bdrv_change_aio_context, and not in a transaction. This was the initial idea, but somehow it didn't work, something was failing. However, I must have changed something now because all tests are passing :) Then old aiocontext lock is assumed to be taken by the caller. 2. old aiocontext lock is released 3. set_aio_context (actual detach + attach operation) are kept in a commit transaciton, as it is now. They don't need any aiocontext lock, as far as I understand. bdrv_drained_end is implemented in a .clean transaction 4. new aiocontext lock is taken 5. either tran_abort or tran_commit, depending on the result of the recursion. Disadvantage: the unlock/lock mess is still there, but at least we know now why we need that. Advantage: nothing should break (because it is similar as it was before), no double lock held, and I can always add an additional patch where I use _UNLOCKED functions and see that happens. In addition, no tran_add_tail needed. > > The idea of using _UNLOCKED sounds interesting, almost too obvious. I > can’t see why that wouldn’t work, actually. > >> -- >> >> Simplify the way the aiocontext can be changed in a BDS graph. >> There are currently two problems in bdrv_try_set_aio_context: >> - There is a confusion of AioContext locks taken and released, because >> we assume that old aiocontext is always taken and new one is >> taken inside. > > Yep, and that assumption is just broken in some cases, which is the main > pain point I’m feeling with it right now. > > For example, look at bdrv_attach_child_common(): Here, we attach a child > to a parent, so we need to get them into a common AioContext. So first > we try to put the child into the parent’s context, and if that fails, > we’ll try the other way, putting the parent into the child’s context. > > The problem is clear: The bdrv_try_set_aio_context() call requires us to > hold the child’s current context but not the parent’s, and the > child_class->set_aio_ctx() call requires the exact opposite. But we > never switch the context we have acquired, so this can’t both work. > Better yet, nowhere is it defined what context a caller to > bdrv_attach_child_common() will hold. > > In practice, what happens here most of the time is that something will > be moved from the main context to some other context, and since we’re in > the main context already, that’ll just work. But you can construct > cases where something is attempted to be moved from an I/O thread into a > different thread and then you’ll get a crash. > > I’d be happy if we could do away with the requirement of having to hold > any lock for changing a node’s AioContext. > >> - It doesn't look very safe to call bdrv_drained_begin while some >> nodes have already switched to the new aiocontext and others haven't. >> This could be especially dangerous because bdrv_drained_begin >> polls, so >> something else could be executed while graph is in an inconsistent >> state. >> >> Additional minor nitpick: can_set and set_ callbacks both traverse the >> graph, both using the ignored list of visited nodes in a different way. >> >> Therefore, get rid of all of this and introduce a new callback, >> change_aio_context, that uses transactions to efficiently, cleanly >> and most importantly safely change the aiocontext of a graph. >> >> This new callback is a "merge" of the two previous ones: >> - Just like can_set_aio_context, recursively traverses the graph. >> Marks all nodes that are visited using a GList, and checks if >> they *could* change the aio_context. >> - For each node that passes the above check, add a new transaction >> that implements a callback that effectively changes the aiocontext. >> - If a node is a BDS, add two transactions: one taking care of draining >> the node at the beginning of the list (so that will be executed first) >> and one at t
Re: [RFC PATCH 3/8] RFC: block: use transactions as a replacement of ->{can_}set_aio_context()
Am 14/07/2022 um 18:45 schrieb Hanna Reitz: >> + * First, go recursively through all nodes in the graph, and see if they >> + * can change their AioContext. >> + * If so, add for each node a new transaction with a callback to >> change the >> + * AioContext with the new one. >> + * Once recursion is completed, if all nodes are allowed to change their >> + * AioContext then commit the transaction, running all callbacks in >> order. >> + * Otherwise don't do anything. > > Nice explanation, but I’d start with something more simple to describe > the external interface, like “Change bs's and recursively all of its > parents' and children's AioContext to the given new context, returning > an error if that isn't possible. If ignore_child is not NULL, that > child (and its subgraph) will not be touched.” You want to have your suggestion as a replacement or addition to mine? > >> + * >> + * This function still requires the caller to take the bs current >> + * AioContext lock, otherwise draining could fail since AIO_WAIT_WHILE > > Well, let’s just say “will” instead of “could”. Callers don’t need to > know they can get lucky when they ignore the rules. > >> + * assumes the lock is always held if bs is in another AioContext. >> + */ >> +int bdrv_child_try_change_aio_context(BlockDriverState *bs, >> AioContext *ctx, >> + BdrvChild *ignore_child, Error >> **errp) > > Do the other functions need to be public? Outside of this file, this > one seems sufficient to me, but I may well be overlooking something. > > Also, why is this function called bdrv_child_*, and not just > bdrv_try_change_aio_context()? (Or maybe have a > bdrv_try_change_aio_context() variant without the ignore_child > parameter, just like bdrv_try_set_aio_context()?) > > Actually, wouldn’t just bdrv_change_aio_context() be sufficient? I > mean, it isn’t called bdrv_try_co_pwritev(). Most functions try to do > something and return errors if they can’t. Not sure if we need to make > that clear in the name. > > (I may be wrong, but I suspect bdrv_try_set_aio_context() is only named > such that the compiler will check that the bdrv_set_aio_context() > interface is completely unused; > 42a65f02f9b380bd8074882d5844d4ea033389cc’s commit message does seem to > confirm that.) Ok, I will change the name in bdrv_change_aio_context. And I now understand your suggestion on the last patch to remove bdrv_try_set_aio_context. Emanuele
Re: [PATCH 1/2] block/parallels: Fix buffer-based write call
On 14.07.2022 15:28, Hanna Reitz wrote:
Commit a4072543ccdddbd241d5962d9237b8b41fd006bf has changed the I/O here
from working on a local one-element I/O vector to just using the buffer
directly (using the bdrv_co_pread()/bdrv_co_pwrite() helper functions
introduced shortly before).
However, it only changed the bdrv_co_preadv() call to bdrv_co_pread() -
the subsequent bdrv_co_pwritev() call stayed this way, and so still
expects a QEMUIOVector pointer instead of a plain buffer. We must
change that to be a bdrv_co_pwrite() call.
Fixes: a4072543ccdddbd241d5962d ("block/parallels: use buffer-based io")
Signed-off-by: Hanna Reitz
---
block/parallels.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 8b235b9505..a229c06f25 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -241,8 +241,8 @@ static int64_t allocate_clusters(BlockDriverState *bs,
int64_t sector_num,
return ret;
}
-ret = bdrv_co_pwritev(bs->file, s->data_end * BDRV_SECTOR_SIZE,
- nb_cow_bytes, buf, 0);
+ret = bdrv_co_pwrite(bs->file, s->data_end * BDRV_SECTOR_SIZE,
+ nb_cow_bytes, buf, 0);
qemu_vfree(buf);
if (ret < 0) {
return ret;
Reviewed-by: Denis V. Lunev
Re: [PATCH 2/2] iotests/131: Add parallels regression test
On 14.07.2022 15:28, Hanna Reitz wrote:
Test an allocating write to a parallels image that has a backing node.
Before HEAD^, doing so used to give me a failed assertion (when the
backing node contains only `42` bytes; the results varies with the value
chosen, for `0` bytes, for example, all I get is EIO).
Signed-off-by: Hanna Reitz
---
tests/qemu-iotests/131 | 35 ++-
tests/qemu-iotests/131.out | 13 +
2 files changed, 47 insertions(+), 1 deletion(-)
diff --git a/tests/qemu-iotests/131 b/tests/qemu-iotests/131
index d7456cae5b..a847692b4c 100755
--- a/tests/qemu-iotests/131
+++ b/tests/qemu-iotests/131
@@ -43,7 +43,7 @@ _supported_os Linux
inuse_offset=$((0x2c))
-size=64M
+size=$((64 * 1024 * 1024))
CLUSTER_SIZE=64k
IMGFMT=parallels
_make_test_img $size
@@ -70,6 +70,39 @@ _check_test_img
_check_test_img -r all
{ $QEMU_IO -c "read -P 0x11 64k 64k" "$TEST_IMG"; } 2>&1 | _filter_qemu_io |
_filter_testdir
+echo "== allocate with backing =="
+# Verify that allocating clusters works fine even when there is a backing
image.
+# Regression test for a bug where we would pass a buffer read from the backing
+# node as a QEMUIOVector object, which could cause anything from I/O errors
over
+# assertion failures to invalid reads from memory.
+
+# Clear image
+_make_test_img $size
+# Create base image
+TEST_IMG="$TEST_IMG.base" _make_test_img $size
+
+# Write some data to the base image (which would trigger an assertion failure
if
+# interpreted as a QEMUIOVector)
+$QEMU_IO -c 'write -P 42 0 64k' "$TEST_IMG.base" | _filter_qemu_io
+
+# Parallels does not seem to support storing a backing filename in the image
+# itself, so we need to build our backing chain on the command line
+imgopts="driver=$IMGFMT,file.driver=$IMGPROTO,file.filename=$TEST_IMG"
+imgopts+=",backing.driver=$IMGFMT"
+imgopts+=",backing.file.driver=$IMGPROTO,backing.file.filename=$TEST_IMG.base"
+
+# Cause allocation in the top image
+QEMU_IO_OPTIONS=$QEMU_IO_OPTIONS_NO_FMT \
+$QEMU_IO --image-opts "$imgopts" -c 'write -P 1 0 64' | _filter_qemu_io
+
+# Verify
+QEMU_IO_OPTIONS=$QEMU_IO_OPTIONS_NO_FMT \
+$QEMU_IO --image-opts "$imgopts" \
+-c 'read -P 1 0 64' \
+-c "read -P 42 64 $((64 * 1024 - 64))" \
+-c "read -P 0 64k $((size - 64 * 1024))" \
+| _filter_qemu_io
+
# success, all done
echo "*** done"
rm -f $seq.full
diff --git a/tests/qemu-iotests/131.out b/tests/qemu-iotests/131.out
index 70da03dee5..de5ef7a8f5 100644
--- a/tests/qemu-iotests/131.out
+++ b/tests/qemu-iotests/131.out
@@ -37,4 +37,17 @@ Double checking the fixed image now...
No errors were found on the image.
read 65536/65536 bytes at offset 65536
64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== allocate with backing ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864
+wrote 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 64/64 bytes at offset 0
+64 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 64/64 bytes at offset 0
+64 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 65472/65472 bytes at offset 64
+63.938 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 67043328/67043328 bytes at offset 65536
+63.938 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
*** done
Reviewed-by: Denis V. Lunev
Re: [PATCH v5 5/8] blockdev: Add a new IF type IF_OTHER
Hao Wu writes:
> This type is used to represent block devs that are not suitable to
> be represented by other existing types.
>
> A sample use is to represent an at24c eeprom device defined in
> hw/nvram/eeprom_at24c.c. The block device can be used to contain the
> content of the said eeprom device.
>
> Signed-off-by: Hao Wu
Let me add a bit more history in the hope of helping other reviewers.
v3 of this series[1] used IF_NONE for the EEPROM device.
Peter Maydell questioned that[2], and we concluded it's wrong. I wrote
[A] board can use any traditional interface type. If none of them
matches, and common decency prevents use of a non-matching one,
invent a new one. We could do IF_OTHER.
Thomas Huth cleaned up the existing abuse of IF_NONE to use IF_PFLASH
instead, in commit 6b717a8d44:
hw/misc/sifive_u_otp: Use IF_PFLASH for the OTP device instead of IF_NONE
Configuring a drive with "if=none" is meant for creation of a backend
only, it should not get automatically assigned to a device frontend.
Use "if=pflash" for the One-Time-Programmable device instead (like
it is e.g. also done for the efuse device in hw/arm/xlnx-zcu102.c).
Since the old way of configuring the device has already been published
with the previous QEMU versions, we cannot remove this immediately, but
have to deprecate it and support it for at least two more releases.
Signed-off-by: Thomas Huth
Acked-by: Philippe Mathieu-Daudé
Reviewed-by: Markus Armbruster
Reviewed-by: Alistair Francis
Message-id: 2029102549.217755-1-th...@redhat.com
Signed-off-by: Alistair Francis
An OTP device isn't really a parallel flash, and neither are eFuses.
More fast-and-lose use of IF_PFLASH may exist in the tree, and maybe of
other interface types, too.
This patch introduces IF_OTHER. The patch after next uses it for an
EEPROM device.
Do we want IF_OTHER?
If no, I guess we get to abuse IF_PFLASH some more.
If yes, I guess we should use IF_PFLASH only for actual parallel flash
memory going forward. Cleaning up existing abuse of IF_PFLASH may not
be worth the trouble, though.
Thoughts?
> ---
> blockdev.c| 4 +++-
> include/sysemu/blockdev.h | 1 +
> 2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 9230888e34..befd69ac5f 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -82,6 +82,7 @@ static const char *const if_name[IF_COUNT] = {
> [IF_MTD] = "mtd",
> [IF_SD] = "sd",
> [IF_VIRTIO] = "virtio",
> +[IF_OTHER] = "other",
> [IF_XEN] = "xen",
> };
>
> @@ -726,7 +727,8 @@ QemuOptsList qemu_legacy_drive_opts = {
> },{
> .name = "if",
> .type = QEMU_OPT_STRING,
> -.help = "interface (ide, scsi, sd, mtd, floppy, pflash, virtio)",
> +.help = "interface (ide, scsi, sd, mtd, floppy, pflash, virtio,"
> +" other)",
> },{
> .name = "file",
> .type = QEMU_OPT_STRING,
> diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
> index 3211b16513..d9dd5af291 100644
> --- a/include/sysemu/blockdev.h
> +++ b/include/sysemu/blockdev.h
> @@ -21,6 +21,7 @@ typedef enum {
> */
> IF_NONE = 0,
> IF_IDE, IF_SCSI, IF_FLOPPY, IF_PFLASH, IF_MTD, IF_SD, IF_VIRTIO, IF_XEN,
> +IF_OTHER,
> IF_COUNT
> } BlockInterfaceType;
[1] https://lists.gnu.org/archive/html/qemu-devel/2021-11/msg00372.html
[2] Subject: does drive_get_next(IF_NONE) make sense?
Message-ID:
https://lists.gnu.org/archive/html/qemu-devel/2021-11/msg00710.html
Re: [PATCH v5 7/8] hw/arm: Set drive property for at24c eeprom
Hao Wu writes: > This patch allows the user to attach an external drive as a property > for an onboard at24c eeprom device. What's the contents of the EEPROM before the patch? I guess the patch lets users replace that contents. Why would a user want to do that? > It uses an unit number to > distinguish different devices. > > Signed-off-by: Hao Wu
Re: [PATCH v5 5/8] blockdev: Add a new IF type IF_OTHER
On 18/07/2022 11.49, Markus Armbruster wrote: [...] An OTP device isn't really a parallel flash, and neither are eFuses. More fast-and-lose use of IF_PFLASH may exist in the tree, and maybe of other interface types, too. This patch introduces IF_OTHER. The patch after next uses it for an EEPROM device. Do we want IF_OTHER? If no, I guess we get to abuse IF_PFLASH some more. If yes, I guess we should use IF_PFLASH only for actual parallel flash memory going forward. Cleaning up existing abuse of IF_PFLASH may not be worth the trouble, though. Thoughts? Maybe we should simply rename IF_PFLASH to IF_EPROM or IF_FLASH to make it clear that it is not only about parallel flashes anymore? Just my 0.02 €. Thomas
Re: [RFC v4 0/9] Add support for zoned device
Sam Li writes: > Markus Armbruster 于2022年7月12日周二 13:47写道： >> >> Sam Li writes: >> >> > This patch series adds support for zoned device to virtio-blk emulation. >> > Zoned >> > Storage can support sequential writes, which reduces write amplification >> > in SSD, >> > leading to higher write throughput and increased capacity. >> >> Forgive me if this has already been discussed, or is explained deeper in >> the patch series... >> >> The commit message sounds like you're extending virtio-blk to optionally >> emulate zoned storage. Correct? > > Yes! The main purpose is to emulate zoned storage only for the zoned > device files. Right now, QEMU sees those as regular block devices. > >> PATCH 1 adds a new block block device driver 'zoned_host_device', and >> PATCH 9 exposes it in QAPI. This is for passing through a zoned host >> device, correct? > > Yes! It allows the guest os see zoned host device. It is still in > development. Maybe the implementations will change later. Your cover letter only mentions the virtio-blk part, not the pass-through part. Please correct that if you need to respin.
Re: [PATCH 1/2] block/parallels: Fix buffer-based write call
On 7/14/22 16:28, Hanna Reitz wrote:
Commit a4072543ccdddbd241d5962d9237b8b41fd006bf has changed the I/O here
from working on a local one-element I/O vector to just using the buffer
directly (using the bdrv_co_pread()/bdrv_co_pwrite() helper functions
introduced shortly before).
However, it only changed the bdrv_co_preadv() call to bdrv_co_pread() -
the subsequent bdrv_co_pwritev() call stayed this way, and so still
expects a QEMUIOVector pointer instead of a plain buffer. We must
change that to be a bdrv_co_pwrite() call.
Fixes: a4072543ccdddbd241d5962d ("block/parallels: use buffer-based io")
Signed-off-by: Hanna Reitz
Reviewed-by: Vladimir Sementsov-Ogievskiy
--
Best regards,
Vladimir
Re: [PATCH] block/rbd: support driver-specific reopen
On 7/1/22 11:41, Hanna Reitz wrote:
On 13.04.22 14:26, Raphael Pour wrote:
>> }
- return ret;
+ /*
+ * Remove all keys from the generic layer which
+ * can't be converted by rbd
+ * >
Does any other driver do this? Shouldn’t we leave them there so that
the generic layer can verify that they aren’t changed?
+ qdict_del(state->options, "driver");
+ qdict_del(state->options, "node-name");
+ qdict_del(state->options, "auto-read-only");
+ qdict_del(state->options, "discard");
+ qdict_del(state->options, "cache");
Because AFAIU this would mean that users could attempt to change e.g.
the @cache option, and wouldn’t receive an error back, even though there
is no support for changing it.
+
+ /*
+ * To maintain the compatibility prior the rbd-reopen,
+ * where the generic layer can be altered without any
+ * rbd argument given,
What does “the generic layer can be altered” mean? As far as I
understand, it was only possible to change between read/write and
read-only access.
+
+ keypairs = g_strdup(qdict_get_try_str(state->options,
"=keyvalue-pairs"));
+ if (keypairs) {
+ qdict_del(state->options, "=keyvalue-pairs");
+ }
+
+ secretid = g_strdup(qdict_get_try_str(state->options,
"password-secret"));
+ if (secretid) {
+ qdict_del(state->options, "password-secret");
+ }
+
+ r = qemu_rbd_convert_options(state->options, &opts, &local_err);
+ if (local_err) {
+ /*
+ * If keypairs are present, that means some options are
present in
+ * the modern option format. Don't attempt to parse legacy
option
+ * formats, as we won't support mixed usage.
+ */
+ if (keypairs) {
+ error_propagate(errp, local_err);
+ goto out;
+ }
+
+ /*
+ * If the initial attempt to convert and process the options
failed,
+ * we may be attempting to open an image file that has the
rbd options
+ * specified in the older format consisting of all key/value
pairs
+ * encoded in the filename. Go ahead and attempt to parse the
+ * filename, and see if we can pull out the required options.
+ */
+ r = qemu_rbd_attempt_legacy_options(state->options, &opts,
&keypairs);
+ if (r < 0) {
+ /*
+ * Propagate the original error, not the legacy parsing
fallback
+ * error, as the latter was just a best-effort attempt.
+ */
+ error_propagate(errp, local_err);
+ goto out;
+ }
+ /*
+ * Take care whenever deciding to actually deprecate; once
this ability
+ * is removed, we will not be able to open any images with
legacy-styled
+ * backing image strings.
+ */
+ warn_report("RBD options encoded in the filename as keyvalue
pairs "
+ "is deprecated");
+ }
+
+ /*
+ * Remove the processed options from the QDict (the visitor
processes
+ * _all_ options in the QDict)
+ */
+ while ((e = qdict_first(state->options))) {
+ qdict_del(state->options, e->key);
+ }
>
> OK, I see why you’d then want to remove all non-RBD options before this
> point. Other block drivers seem to solve this by having an
> X_runtime_opts QemuOptsList, which contains all driver-handled options,
> so they can then use qemu_opts_absorb_qdict() to extract the options
> they can handle. (Compare e.g. qcow2_update_options_prepare().)
>
Looking through all the drivers, rbd seems to be the only one not
absorbing its options via runtime_opts but instead using
qemu_rbd_convert_options. The converter visits all the options from
BlockdevOptionsRbd defined in block-core.json. If there is an unknown
option the conversion fails with EINVAL.
The runtime_opts in contrast to the already defined json schema with the
visitor-code-generation seem to be redundant. Do you have some insights
why and when this redundancy is reasonable?
I came up with several alternatives:
1. add own runtime_opts:
- pro: "the qemu way", it behaves like most of the drivers
- con: redundancy to qemu_rbd_convert_options which does the same
except skipping the generic block layer options and adds +1k lines
- con: I couldn't figure out how to add non-primitive options to the
runtime_opts like encrypt or server
2. tell visitor to ignore unknown keys (?)
3. parse rbd options manually (opposite of deleting the generic block keys)
I agree deleting the generic block opts isn't optimal.
What do you think?
Your remaining points are also reasonable and I'll submit their fix
along with the solution to the issue above.
OpenPGP_0xCDB1EBB785C5EB7E.asc
Description: OpenPGP public key
OpenPGP_signature
Description: OpenPGP digital signature
Re: [PATCH 2/2] iotests/131: Add parallels regression test
On 7/14/22 16:28, Hanna Reitz wrote: Test an allocating write to a parallels image that has a backing node. Before HEAD^, doing so used to give me a failed assertion (when the backing node contains only `42` bytes; the results varies with the value chosen, for `0` bytes, for example, all I get is EIO). Signed-off-by: Hanna Reitz Reviewed-by: Vladimir Sementsov-Ogievskiy -- Best regards, Vladimir
Re: [PATCH 1/1] block: add missed block_acct_setup with new block device init procedure
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
CC: Peter Krempa
CC: Markus Armbruster
CC: John Snow
CC: Kevin Wolf
CC: Hanna Reitz
---
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)
Re: [RFC PATCH 8/8] block: remove all unused ->can_set_aio_ctx and ->set_aio_ctx callbacks
Am 18/07/2022 um 10:45 schrieb Emanuele Giuseppe Esposito: > > > Am 15/07/2022 um 16:34 schrieb Hanna Reitz: >> On 12.07.22 23:19, Emanuele Giuseppe Esposito wrote: >>> Together with all _can_set_ and _set_ APIs, as they are not needed >>> anymore. >>> >>> Signed-off-by: Emanuele Giuseppe Esposito >>> --- >>> block.c | 196 - >>> block/block-backend.c | 33 - >>> blockjob.c | 35 -- >>> include/block/block-global-state.h | 9 -- >>> include/block/block_int-common.h | 4 - >>> 5 files changed, 277 deletions(-) >> >> Looks good! I’d just like a follow-up commit that also drops >> bdrv_try_set_aio_context(), so it’s all gone (I think that’s the final >> remnant?). >> > > It's the same for me, I thought renaming bdrv_try_set_aio_context was a > little bit unnecessary. You want to rename it to something else, or > directly call bdrv_child_try_change_aio_context? Wait we have 2 functions that need to be renamed: - bdrv_child_try_change_aio_context, called only once in block-backend because we need the ignore_child parameter. - bdrv_try_set_aio_context, public, called everywhere, wraps bdrv_child_try_change_aio_context setting ignore_child=NULL. Name suggestions? > > I agree with the rest of comments in this series :) > > Emanuele >
Re: [RFC PATCH 2/8] transactions: add tran_add_back
On 7/14/22 17:13, Hanna Reitz wrote: that we want to run before the others but still only when invoking finalize/commit/abort. I don’t understand this yet (but perhaps it’ll become clearer with the following patches); doesn’t the new function do the opposite? I.e., basically add some clean-up that’s only used after everything else? I agree about the commit message being incorrect, but is there any reason why transactions work LIFO by default? Is there anything that requires that behavior? Paolo
Re: [PATCH 03/12] qga: Replace '--blacklist' command line option by '--denylist'
On 03/02/2021 17.02, Michael Roth wrote: On Wed, Feb 03, 2021 at 04:47:08PM +0100, Kevin Wolf wrote: Am 03.02.2021 um 13:45 hat BALATON Zoltan geschrieben: On Wed, 3 Feb 2021, Daniel P. Berrangé wrote: On Tue, Feb 02, 2021 at 09:58:15PM +0100, Philippe Mathieu-Daudé wrote: Follow the inclusive terminology from the "Conscious Language in your Open Source Projects" guidelines [*] and replace the word "blacklist" appropriately. Keep the --blacklist available for backward compatibility. [*] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fconscious-lang%2Fconscious-lang-docs%2Fblob%2Fmain%2Ffaq.md&data=04%7C01%7Cmichael.roth%40amd.com%7Cd17bb9d899914df4e04108d8c85b068f%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637479640585250068%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=HM%2Fg%2B79VIjp%2BR9bIVBDPkYHHbFa9C3sGMvhomxhJdgE%3D&reserved=0 Signed-off-by: Philippe Mathieu-Daudé --- docs/interop/qemu-ga.rst | 2 +- qga/main.c | 6 -- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/docs/interop/qemu-ga.rst b/docs/interop/qemu-ga.rst index 9a590bf95cb..89596e646de 100644 --- a/docs/interop/qemu-ga.rst +++ b/docs/interop/qemu-ga.rst @@ -79,7 +79,7 @@ Options Daemonize after startup (detach from terminal). -.. option:: -b, --blacklist=LIST +.. option:: -b, --denylist=LIST Comma-separated list of RPCs to disable (no spaces, ``?`` to list available RPCs). diff --git a/qga/main.c b/qga/main.c index 249fe06e8e5..66177b9e93d 100644 --- a/qga/main.c +++ b/qga/main.c @@ -257,7 +257,8 @@ QEMU_COPYRIGHT "\n" #ifdef _WIN32 " -s, --service service commands: install, uninstall, vss-install, vss-uninstall\n" #endif -" -b, --blacklist comma-separated list of RPCs to disable (no spaces, \"?\"\n" +" --blacklist backward compatible alias for --denylist (deprecated)\n" +" -b, --denylistcomma-separated list of RPCs to disable (no spaces, \"?\"\n" "-b" is a bit odd as a short name now, but i guess that's not the end of the world. Maybe -b, --block or --block-rpc? Not the best but at least preserves consistency with the short option. I was thinking about something like --blocked-rpcs, too, so +1 from me for your latter option. If we're touching these names, let's try to actually make them good, not just different. Neither --blacklist nor --denylist actually describe well what the option does. +1 on --blocked-rpcs Looks like there was never a follow up on this patch? Philippe, could you maybe respin with the suggested changes? Thomas
Re: [RFC PATCH 7/8] block: use the new _change_ API instead of _can_set_ and _set_
On 7/12/22 23:19, Emanuele Giuseppe Esposito wrote: +/* No need to ignore `child`, because it has been detached already */ ignore = NULL; -child->klass->set_aio_ctx(child, s->old_parent_ctx, &ignore); +ret = child->klass->change_aio_ctx(child, s->old_parent_ctx, &ignore, + tran, &error_abort); g_slist_free(ignore); +tran_finalize(tran, ret ? ret : -1); Should this instead assert that ret is true, and call tran_commit() directly? Paolo
Re: [RFC PATCH 7/8] block: use the new _change_ API instead of _can_set_ and _set_
On 7/12/22 23:19, Emanuele Giuseppe Esposito wrote:
diff --git a/block/block-backend.c b/block/block-backend.c
index 674eaaa2bf..6e90ac3a6a 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -2184,8 +2184,12 @@ static int blk_do_set_aio_context(BlockBackend *blk,
AioContext *new_context,
bdrv_ref(bs);
if (update_root_node) {
-ret = bdrv_child_try_set_aio_context(bs, new_context, blk->root,
- errp);
+/*
+ * update_root_node MUST be false for
blk_root_set_aio_ctx_commit(),
+ * as we are already in the commit function of a transaction.
+ */
+ret = bdrv_child_try_change_aio_context(bs, new_context, blk->root,
+errp);
if (ret < 0) {
bdrv_unref(bs);
return ret;
Looking further at blk_do_set_aio_context:
if (tgm->throttle_state) {
bdrv_drained_begin(bs);
throttle_group_detach_aio_context(tgm);
throttle_group_attach_aio_context(tgm, new_context);
bdrv_drained_end(bs);
}
Perhaps the drained_begin/drained_end pair can be moved to
blk_set_aio_context? It shouldn't be needed from the change_aio_ctx
callback, because bs is already drained. If so, blk_do_set_aio_context
would become just:
if (tgm->throttle_state) {
throttle_group_detach_aio_context(tgm);
throttle_group_attach_aio_context(tgm, new_context);
}
blk->ctx = new_context;
and blk_set_aio_context would be something like:
if (bs) {
bdrv_ref(bs);
ret = bdrv_child_try_set_aio_context(bs, new_context, blk->root,
errp);
if (ret < 0) {
goto out_no_drain;
}
bdrv_drained_begin(bs);
}
ret = blk_do_set_aio_context(blk, new_context, errp);
if (bs) {
bdrv_drained_end(bs);
out_no_drain;
bdrv_unref(bs);
}
return ret;
Paolo
Re: [PATCH v4 00/12] Improve reliability of VM tests
On Mon, Jul 18, 2022 at 4:17 AM Thomas Huth wrote: > > On 08/07/2022 17.34, John Snow wrote: > > Note: patches 10-12 are included for testing simplicity, they shouldn't > > be merged. They will be included in a forthcoming block PR. > > > > V4: > > > > - Addressed concern by Marc-Andre in patch 01. > > - Squashed Ubuntu patches (rth) > > > > This patch series attempts to improve the reliability of several of the > > VM test targets. In particular, both CentOS 8 tests are non-functional > > because CentOS 8 was EOL at the beginning of this calendar year, with > > repositories and mirrors going offline. > > > > I also remove the ubuntu.i386 test because we no longer support Ubuntu > > 18.04 nor do we have explicit need of an i386 build test. > > > > After this series, I am able to successfully run every VM target on an > > x86_64 host, except: > > > > - ubuntu.aarch64: Hangs often during testing, see below. > > - centos.aarch64: Hangs often during testing, see below. > > - haiku.x86_64: Build failures not addressed by this series, see > >https://lists.gnu.org/archive/html/qemu-devel/2022-06/msg02103.html > > > > The unit tests that I see fail most often under aarch64 are: > > > > - virtio-net-failover: Seems to like to hang on openbsd > > - migration-test: Tends to hang under aarch64 tcg > > > > Future work (next version? next series?); > > > > - Try to get centos.aarch64 working reliably under TCG > > - Upgrade ubuntu.aarch64 to 20.04 after fixing centos.aarch64 > > - Fix the Haiku build test, if possible. > > - Ensure I can reliably run and pass "make vm-build-all". > >(Remove VMs from this recipe if necessary.) > > Not sure whether it's related to your patches, but when testing these I just > got an error while running 'vm-build-openbsd' : > > VM-BUILD openbsd > fatal: not a valid object name: > failed to archive qemu > Failed to prepare guest environment > Traceback (most recent call last): >File "/home/thuth/devel/qemu/tests/vm/basevm.py", line 641, in main > vm.add_source_dir(args.build_qemu) >File "/home/thuth/devel/qemu/tests/vm/basevm.py", line 277, in > add_source_dir > stdout=self._stdout, stderr=self._stderr) >File "/usr/lib64/python3.6/subprocess.py", line 311, in check_call > raise CalledProcessError(retcode, cmd) > subprocess.CalledProcessError: Command '['./scripts/archive-source.sh', > '/home/thuth/tmp/qemu-build/vm-test-rm_z92hq.tmp/data-44e42.tar']' returned > non-zero exit status 1. > > The error did not occur again when running the command again, though. > > Thomas > I haven't seen this one before, admittedly. Looks like scripts/archive-source.sh L52 choked? git archive --format tar "$(tree_ish)" > "$tar_file" I'm not sure what "fatal: not a valid object name:" might be referring to. Maybe tree_ish picked up something that tasted bad? I really don't know.
