Re: [PATCH v4 4/4] python/aqmp: add socket bind step to legacy.py
On Thu, Feb 3, 2022 at 4:38 AM Daniel P. Berrangé wrote:
>
> On Wed, Feb 02, 2022 at 02:08:59PM -0500, John Snow wrote:
> > On Tue, Feb 1, 2022 at 2:46 PM Kevin Wolf wrote:
> > >
> > > Am 01.02.2022 um 19:32 hat John Snow geschrieben:
> > > > On Tue, Feb 1, 2022 at 8:21 AM Kevin Wolf wrote:
> > > > >
> > > > > Am 01.02.2022 um 05:11 hat John Snow geschrieben:
> > > > > > The synchronous QMP library would bind to the server address during
> > > > > > __init__(). The new library delays this to the accept() call,
> > > > > > because
> > > > > > binding occurs inside of the call to start_[unix_]server(), which
> > > > > > is an
> > > > > > async method -- so it cannot happen during __init__ anymore.
> > > > > >
> > > > > > Python 3.7+ adds the ability to create the server (and thus the
> > > > > > bind()
> > > > > > call) and begin the active listening in separate steps, but we don't
> > > > > > have that functionality in 3.6, our current minimum.
> > > > > >
> > > > > > Therefore ... Add a temporary workaround that allows the synchronous
> > > > > > version of the client to bind the socket in advance, guaranteeing
> > > > > > that
> > > > > > there will be a UNIX socket in the filesystem ready for the QEMU
> > > > > > client
> > > > > > to connect to without a race condition.
> > > > > >
> > > > > > (Yes, it's a bit ugly. Fixing it more nicely will have to wait
> > > > > > until our
> > > > > > minimum Python version is 3.7+.)
> > > I guess the relevant question in the context of this patch is whether
> > > sync.py will need a similar two-phase setup as legacy.py. Do you think
> > > you can do without it when you have to reintroduce this behaviour here
> > > to fix bugs?
> > >
> >
> > Hm, honestly, no. I think you'll still need the two-phase in the sync
> > version no matter what -- if you expect to be able to launch QMP and
> > QEMU from the same process, anyway. I suppose it's just a matter of
> > what you *call* it and what/where the arguments are.
> >
> > I could just call it bind(), and it does whatever it needs to (Either
> > creating a socket or, in 3.7+, instantiating more of the asyncio loop
> > machinery).
> > The signature for accept() would need to change to (perhaps)
> > optionally accepting no arguments; i.e. you can do either of the
> > following:
> >
> > (1) qmp.bind('/tmp/sock')
> > qmp.accept()
> >
> > (2) qmp.accept('/tmp/sock')
> >
> > The risk is that the interface is just a touch more complex, the docs
> > get a bit more cluttered, there's a slight loss of clarity, the
> > accept() method would need to check to make sure you didn't give it an
> > address argument if you've already given it one, etc. Necessary
> > trade-off for the flexibility, though.
>
> Doing a bind() inside an accept() call is really strange
> design IMHO.
>
Yeah, it was possibly a mistake to use the same names as the very
famous and widely known POSIX calls. I didn't intend for them to be
precisely 1:1.
What I intended was that each QMPClient() object could either receive
an incoming connection or dial out and make one. I happened to name
these accept() and connect(). Under the hood, we do "whatever we have
to" in order to make those methods work. This was the simplest
possible interface I could imagine; and it also mimics the (sync) qmp
code in python/qemu/qmp. That code doesn't have distinct
bind-listen-accept steps; it performs bind and listen immediately
during __init__, then socket.accept() is called during the accept()
method call.
However, In python asyncio, we're encouraged to use the
asyncio.start_server() and asyncio.start_unix_server() functions
instead. These are async calls, and they cannot occur during
__init__(), because __init__() isn't an async coroutine. This compels
us to move the bind step out of init and into an async method. In the
interest of keeping the new interface looking very similar to the old
one, I pushed the entirety of the bind-listen-accept sequence into the
async accept(...) method.
A little weird, but that's why it happened that way. (The concept of a
QMP client being the socket server is also a little weird and defies
the traditional expectation, too, IMO. Oh well, we're here now.)
For comparison, the sync interface:
qmp = QEMUMonitorProtocol('qemu.sock', server=True) # bind(), listen()
qmp.accept() # accept()
And the async interface:
qmp = QMPClient()
await qmp.accept('qemu.sock') # bind, listen, and accept
That said, I could probably make this interface a lot more traditional
for actual /servers/. I have patches to add a QMPServer() class, but
it adopted the "exactly one peer" limitation that QMPClient() has. If
you think there is some value in developing the idea further, I could
refactor the code to be a *lot* more traditional and have a listening
server that acts as a factory for generating connected client
instances. It just isn't something that I think I'll have merge ready
in less than a week.
> bind() is a one-time-only initialization operation
Re: [PULL 0/4] Python patches
On Thu, Feb 3, 2022 at 11:52 AM Peter Maydell wrote: > > On Thu, 3 Feb 2022 at 16:38, John Snow wrote: > > > On Thu, Feb 3, 2022, 11:20 AM Peter Maydell > > wrote: > >> Summary of Failures: > >> > >> 1/1 qemu:block / qemu-iotests qcow2 ERROR 243.14s exit status 1 I'm not too familiar with this new test runner, yet. (Is this error even anything to do with the python lib? I guess I can't rule it out...) I just got a clean run of 'make vm-build-netbsd', so I'm using that output as reference and making some guesses. If I search the output for 'qcow2', I see the following output (with possibly many lines between each hit): 1/1 qemu:block / qemu-iotests qcow2RUNNING >>> MALLOC_PERTURB_=205 PYTHON=/usr/pkg/bin/python3.7 /bin/sh >>> /home/qemu/qemu-test.lj6FNa/build/../src/tests/qemu-iotests/../check-block.sh >>> qcow2 ▶ 1/1 qcow2 001OK ▶ 1/1 qcow2 002OK ▶ 1/1 qcow2 004OK ... and so on and so forth ... ▶ 1/1 qcow2 299OK ▶ 1/1 qcow2 313SKIP ▶ 1/1 qcow2 nbd-qemu-allocationSKIP ▶ 1/1 qcow2 qsd-jobs OK 1/1 qemu:block / qemu-iotests qcow2OK 176.35s 74 subtests passed I tried modifying 040 to fail on purpose, and I see: ▶ 1/1 qcow2 039OK ▶ 1/1 qcow2 040FAIL ▶ 1/1 qcow2 041OK [...] ▶ 1/1 qcow2 nbd-qemu-allocationOK ▶ 1/1 qcow2 qsd-jobs OK 1/1 qemu:block / qemu-iotests qcow2ERROR 106.06s exit status 1 Summary of Failures: 1/1 qemu:block / qemu-iotests qcow2 ERROR 106.06s exit status 1 I don't think I see it on the output you mailed, but can you point out which test is failing, at least? Grepping for 'FAIL' should be helpful. --js
Re: [PATCH v2 0/3] hw/nvme: zoned random write area
On Jan 27 09:19, Klaus Jensen wrote:
> From: Klaus Jensen
>
> This series adds support for a zoned random write area as standardized
> in TP 4076 ("Zoned Random Write Area").
>
> v2:
> * fixed rsvd member in NvmeZoneSendCmd (Keith)
> * dropped patch 2 ("hw/nvme: add zone attribute get/set helpers")
> * amended patch 4 to open code the helpers removed from patch 2
>
> Klaus Jensen (3):
> hw/nvme: add struct for zone management send
> hw/nvme: add ozcs enum
> hw/nvme: add support for zoned random write area
>
> hw/nvme/ctrl.c | 181 +--
> hw/nvme/ns.c | 61 ++-
> hw/nvme/nvme.h | 10 +++
> hw/nvme/trace-events | 1 +
> include/block/nvme.h | 40 +-
> 5 files changed, 266 insertions(+), 27 deletions(-)
>
Applied to nvme-next.
signature.asc
Description: PGP signature
Re: [PULL 0/4] Python patches
On Thu, 3 Feb 2022 at 16:38, John Snow wrote: > On Thu, Feb 3, 2022, 11:20 AM Peter Maydell wrote: >> Summary of Failures: >> >> 1/1 qemu:block / qemu-iotests qcow2 ERROR 243.14s exit status 1 >> >> >> Ok: 0 >> Expected Fail: 0 >> Fail: 1 >> Unexpected Pass:0 >> Skipped:0 >> Timeout:0 >> >> Full log written to >> /home/qemu/qemu-test.yiYr4m/build/meson-logs/iotestslog.txt >> ▶ 147/704 /bdrv-drain/deletion/drain >>OK >> ▶ 178/704 /crypto/task/complete >>OK >> ▶ 178/704 /crypto/task/datafree >>OK >> [etc] > Any chance of seeing that meson-logs/iotestslog.txt file? Sorry, no. The VM runs, and it produces output to stdout, and then it goes away again. The test cases and test harnesses *must* output to standard output any information that might be useful for diagnosing problems. The same scenario applies for the gitlab CI jobs -- all we get is the job's output. -- PMM
Re: [PATCH 1/2] block: Lock AioContext for drain_end in blockdev-reopen
On 03.02.22 15:05, Kevin Wolf wrote: bdrv_subtree_drained_end() requires the caller to hold the AioContext lock for the drained node. Not doing this for nodes outside of the main AioContext leads to crashes when AIO_WAIT_WHILE() needs to wait and tries to temporarily release the lock. Fixes: 3908b7a8994fa5ef7a89aa58cd5a02fc58141592 Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2046659 Reported-by: Qing Wang Signed-off-by: Kevin Wolf --- blockdev.c | 11 ++- 1 file changed, 10 insertions(+), 1 deletion(-) Reviewed-by: Hanna Reitz
Re: [PATCH 2/2] iotests: Test blockdev-reopen with iothreads and throttling
On 03.02.22 15:05, Kevin Wolf wrote: The 'throttle' block driver implements .bdrv_co_drain_end, so blockdev-reopen will have to wait for it to complete in the polling loop at the end of qmp_blockdev_reopen(). This makes AIO_WAIT_WHILE() release the AioContext lock, which causes a crash if the lock hasn't correctly been taken. Signed-off-by: Kevin Wolf --- tests/qemu-iotests/245 | 36 +--- tests/qemu-iotests/245.out | 4 ++-- 2 files changed, 35 insertions(+), 5 deletions(-) Reviewed-by: Hanna Reitz
Re: [PULL 0/4] Python patches
On Thu, Feb 3, 2022, 11:20 AM Peter Maydell wrote: > On Thu, 3 Feb 2022 at 01:59, John Snow wrote: > > > > The following changes since commit > 47cc1a3655135b89fa75c2824fbddd29df874612: > > > > Merge remote-tracking branch 'remotes/kwolf-gitlab/tags/for-upstream' > into staging (2022-02-01 19:48:15 +) > > > > are available in the Git repository at: > > > > https://gitlab.com/jsnow/qemu.git tags/python-pull-request > > > > for you to fetch changes up to b0b662bb2b340d63529672b5bdae596a6243c4d0: > > > > python/aqmp: add socket bind step to legacy.py (2022-02-02 14:12:22 > -0500) > > > > > > Python patches > > > > Peter: I expect this to address the iotest 040,041 failures you observed > > on NetBSD. If it doesn't, let me know. > > I still see this one, which is different from the 040,041 stuff, > and where 'make check' is for some reason giving a lot less useful > detail. (This is a prexisting intermittent from before this patchset). > I'm assuming there's less detail because of the meson test-runner doing some io redirection into the logfile. [etc] > ▶ 175/704 /io/channel/pipe/sync >OK > ▶ 175/704 /io/channel/pipe/async >OK > 175/704 qemu:unit / test-io-channel-file >OK 0.11s 5 subtests passed > > 177/704 qemu:unit / test-io-channel-tls >RUNNING > >>> G_TEST_BUILDDIR=/home/qemu/qemu-test.yiYr4m/build/tests/unit > MALLOC_PERTURB_=5 G_TEST_SRCDIR=/home/qemu/qemu-test.yiYr4m/src/tests/unit > /home/ > qemu/qemu-test.yiYr4m/build/tests/unit/test-io-channel-tls --tap -k > ▶ 176/704 /io/channel/socket/ipv4-sync >OK > ▶ 176/704 /io/channel/socket/ipv4-async >OK > ▶ 176/704 /io/channel/socket/ipv4-fd >OK > ▶ 176/704 /io/channel/socket/ipv6-sync >OK > ▶ 176/704 /io/channel/socket/ipv6-async >OK > ▶ 176/704 /io/channel/socket/unix-sync >OK > ▶ 176/704 /io/channel/socket/unix-async >OK > ▶ 176/704 /io/channel/socket/unix-fd-pass >OK > ▶ 176/704 /io/channel/socket/unix-listen-cleanup >OK > 176/704 qemu:unit / test-io-channel-socket >OK 0.13s 9 subtests passed > > ▶ 1/1 qcow2 qsd-jobsOK > 1/1 qemu:block / qemu-iotests qcow2 ERROR 243.14s exit status 1 > > 178/704 qemu:unit / test-io-task >RUNNING > >>> G_TEST_BUILDDIR=/home/qemu/qemu-test.yiYr4m/build/tests/unit > MALLOC_PERTURB_=194 > G_TEST_SRCDIR=/home/qemu/qemu-test.yiYr4m/src/tests/unit /hom > e/qemu/qemu-test.yiYr4m/build/tests/unit/test-io-task --tap -k > ▶ 147/704 /bdrv-drain/blockjob/iothread/error/drain_subtree >OK > > Summary of Failures: > > 1/1 qemu:block / qemu-iotests qcow2 ERROR 243.14s exit status 1 > > > Ok: 0 > Expected Fail: 0 > Fail: 1 > Unexpected Pass:0 > Skipped:0 > Timeout:0 > > Full log written to > /home/qemu/qemu-test.yiYr4m/build/meson-logs/iotestslog.txt > ▶ 147/704 /bdrv-drain/deletion/drain >OK > ▶ 178/704 /crypto/task/complete >OK > ▶ 178/704 /crypto/task/datafree >OK > [etc] > > thanks > -- PMM > Any chance of seeing that meson-logs/iotestslog.txt file? >
[PATCH 7/7] iotests/281: Let NBD connection yield in iothread
Put an NBD block device into an I/O thread, and then read data from it,
hoping that the NBD connection will yield during that read. When it
does, the coroutine must be reentered in the block device's I/O thread,
which will only happen if the NBD block driver attaches the connection's
QIOChannel to the new AioContext. It did not do that after 4ddb5d2fde
("block/nbd: drop connection_co") and prior to "block/nbd: Move s->ioc
on AioContext change", which would cause an assertion failure.
To improve our chances of yielding, the NBD server is throttled to
reading 64 kB/s, and the NBD client reads 128 kB, so it should yield at
some point.
Signed-off-by: Hanna Reitz
---
tests/qemu-iotests/281 | 28 +---
tests/qemu-iotests/281.out | 4 ++--
2 files changed, 27 insertions(+), 5 deletions(-)
diff --git a/tests/qemu-iotests/281 b/tests/qemu-iotests/281
index 4fb3cd30dd..5e1339bd75 100755
--- a/tests/qemu-iotests/281
+++ b/tests/qemu-iotests/281
@@ -253,8 +253,9 @@ class TestYieldingAndTimers(iotests.QMPTestCase):
self.create_nbd_export()
# Simple VM with an NBD block device connected to the NBD export
-# provided by the QSD
+# provided by the QSD, and an (initially unused) iothread
self.vm = iotests.VM()
+self.vm.add_object('iothread,id=iothr')
self.vm.add_blockdev('nbd,node-name=nbd,server.type=unix,' +
f'server.path={self.sock},export=exp,' +
'reconnect-delay=1,open-timeout=1')
@@ -299,19 +300,40 @@ class TestYieldingAndTimers(iotests.QMPTestCase):
# thus not see the error, and so the test will pass.)
time.sleep(2)
+def test_yield_in_iothread(self):
+# Move the NBD node to the I/O thread; the NBD block driver should
+# attach the connection's QIOChannel to that thread's AioContext, too
+result = self.vm.qmp('x-blockdev-set-iothread',
+ node_name='nbd', iothread='iothr')
+self.assert_qmp(result, 'return', {})
+
+# Do some I/O that will be throttled by the QSD, so that the network
+# connection hopefully will yield here. When it is resumed, it must
+# then be resumed in the I/O thread's AioContext.
+result = self.vm.qmp('human-monitor-command',
+ command_line='qemu-io nbd "read 0 128K"')
+self.assert_qmp(result, 'return', '')
+
def create_nbd_export(self):
assert self.qsd is None
-# Simple NBD export of a null-co BDS
+# Export a throttled null-co BDS: Reads are throttled (max 64 kB/s),
+# writes are not.
self.qsd = QemuStorageDaemon(
+'--object',
+'throttle-group,id=thrgr,x-bps-read=65536,x-bps-read-max=65536',
+
'--blockdev',
'null-co,node-name=null,read-zeroes=true',
+'--blockdev',
+'throttle,node-name=thr,file=null,throttle-group=thrgr',
+
'--nbd-server',
f'addr.type=unix,addr.path={self.sock}',
'--export',
-'nbd,id=exp,node-name=null,name=exp,writable=true'
+'nbd,id=exp,node-name=thr,name=exp,writable=true'
)
def stop_nbd_export(self):
diff --git a/tests/qemu-iotests/281.out b/tests/qemu-iotests/281.out
index 914e3737bd..3f8a935a08 100644
--- a/tests/qemu-iotests/281.out
+++ b/tests/qemu-iotests/281.out
@@ -1,5 +1,5 @@
-.
+..
--
-Ran 5 tests
+Ran 6 tests
OK
--
2.34.1
[PATCH 6/7] block/nbd: Move s->ioc on AioContext change
s->ioc must always be attached to the NBD node's AioContext. If that
context changes, s->ioc must be attached to the new context.
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2033626
Signed-off-by: Hanna Reitz
---
block/nbd.c | 45 +
1 file changed, 45 insertions(+)
diff --git a/block/nbd.c b/block/nbd.c
index dc6c3f3bbc..5853d85d60 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -2055,6 +2055,42 @@ static void nbd_cancel_in_flight(BlockDriverState *bs)
nbd_co_establish_connection_cancel(s->conn);
}
+static void nbd_attach_aio_context(BlockDriverState *bs,
+ AioContext *new_context)
+{
+BDRVNBDState *s = bs->opaque;
+
+/* The open_timer is used only during nbd_open() */
+assert(!s->open_timer);
+
+/*
+ * The reconnect_delay_timer is scheduled in I/O paths when the
+ * connection is lost, to cancel the reconnection attempt after a
+ * given time. Once this attempt is done (successfully or not),
+ * nbd_reconnect_attempt() ensures the timer is deleted before the
+ * respective I/O request is resumed.
+ * Since the AioContext can only be changed when a node is drained,
+ * the reconnect_delay_timer cannot be active here.
+ */
+assert(!s->reconnect_delay_timer);
+
+if (s->ioc) {
+qio_channel_attach_aio_context(s->ioc, new_context);
+}
+}
+
+static void nbd_detach_aio_context(BlockDriverState *bs)
+{
+BDRVNBDState *s = bs->opaque;
+
+assert(!s->open_timer);
+assert(!s->reconnect_delay_timer);
+
+if (s->ioc) {
+qio_channel_detach_aio_context(s->ioc);
+}
+}
+
static BlockDriver bdrv_nbd = {
.format_name= "nbd",
.protocol_name = "nbd",
@@ -2078,6 +2114,9 @@ static BlockDriver bdrv_nbd = {
.bdrv_dirname = nbd_dirname,
.strong_runtime_opts= nbd_strong_runtime_opts,
.bdrv_cancel_in_flight = nbd_cancel_in_flight,
+
+.bdrv_attach_aio_context= nbd_attach_aio_context,
+.bdrv_detach_aio_context= nbd_detach_aio_context,
};
static BlockDriver bdrv_nbd_tcp = {
@@ -2103,6 +2142,9 @@ static BlockDriver bdrv_nbd_tcp = {
.bdrv_dirname = nbd_dirname,
.strong_runtime_opts= nbd_strong_runtime_opts,
.bdrv_cancel_in_flight = nbd_cancel_in_flight,
+
+.bdrv_attach_aio_context= nbd_attach_aio_context,
+.bdrv_detach_aio_context= nbd_detach_aio_context,
};
static BlockDriver bdrv_nbd_unix = {
@@ -2128,6 +2170,9 @@ static BlockDriver bdrv_nbd_unix = {
.bdrv_dirname = nbd_dirname,
.strong_runtime_opts= nbd_strong_runtime_opts,
.bdrv_cancel_in_flight = nbd_cancel_in_flight,
+
+.bdrv_attach_aio_context= nbd_attach_aio_context,
+.bdrv_detach_aio_context= nbd_detach_aio_context,
};
static void bdrv_nbd_init(void)
--
2.34.1
[PATCH 2/7] block/nbd: Delete open timer when done
We start the open timer to cancel the connection attempt after a while. Once nbd_do_establish_connection() has returned, the attempt is over, and we no longer need the timer. Delete it before returning from nbd_open(), so that it does not persist for longer. It has no use after nbd_open(), and just like the reconnect delay timer, it might well be dangerous if it were to fire afterwards. Signed-off-by: Hanna Reitz --- block/nbd.c | 8 1 file changed, 8 insertions(+) diff --git a/block/nbd.c b/block/nbd.c index 16cd7fef77..5ff8a57314 100644 --- a/block/nbd.c +++ b/block/nbd.c @@ -1885,11 +1885,19 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags, goto fail; } +/* + * The connect attempt is done, so we no longer need this timer. + * Delete it, because we do not want it to be around when this node + * is drained or closed. + */ +open_timer_del(s); + nbd_client_connection_enable_retry(s->conn); return 0; fail: +open_timer_del(s); nbd_clear_bdrvstate(bs); return ret; } -- 2.34.1
[PATCH 3/7] block/nbd: Assert there are no timers when closed
Our two timers must not remain armed beyond nbd_clear_bdrvstate(), or they will access freed data when they fire. This patch is separate from the patches that actually fix the issue (HEAD^^ and HEAD^) so that you can run the associated regression iotest (281) on a configuration that reproducibly exposes the bug. Signed-off-by: Hanna Reitz --- block/nbd.c | 4 1 file changed, 4 insertions(+) diff --git a/block/nbd.c b/block/nbd.c index 5ff8a57314..dc6c3f3bbc 100644 --- a/block/nbd.c +++ b/block/nbd.c @@ -110,6 +110,10 @@ static void nbd_clear_bdrvstate(BlockDriverState *bs) yank_unregister_instance(BLOCKDEV_YANK_INSTANCE(bs->node_name)); +/* Must not leave timers behind that would access freed data */ +assert(!s->reconnect_delay_timer); +assert(!s->open_timer); + object_unref(OBJECT(s->tlscreds)); qapi_free_SocketAddress(s->saddr); s->saddr = NULL; -- 2.34.1
[PATCH 5/7] iotests/281: Test lingering timers
Prior to "block/nbd: Delete reconnect delay timer when done" and
"block/nbd: Delete open timer when done", both of those timers would
remain scheduled even after successfully (re-)connecting to the server,
and they would not even be deleted when the BDS is deleted.
This test constructs exactly this situation:
(1) Configure an @open-timeout, so the open timer is armed, and
(2) Configure a @reconnect-delay and trigger a reconnect situation
(which succeeds immediately), so the reconnect delay timer is armed.
Then we immediately delete the BDS, and sleep for longer than the
@open-timeout and @reconnect-delay. Prior to said patches, this caused
one (or both) of the timer CBs to access already-freed data.
Accessing freed data may or may not crash, so this test can produce
false successes, but I do not know how to show the problem in a better
or more reliable way. If you run this test on "block/nbd: Assert there
are no timers when closed" and without the fix patches mentioned above,
you should reliably see an assertion failure.
(But all other tests that use the reconnect delay timer (264 and 277)
will fail in that configuration, too; as will nbd-reconnect-on-open,
which uses the open timer.)
Remove this test from the quick group because of the two second sleep
this patch introduces.
(I decided to put this test case into 281, because the main bug this
series addresses is in the interaction of the NBD block driver and I/O
threads, which is precisely the scope of 281. The test case for that
other bug will also be put into the test class added here.
Also, excuse the test class's name, I couldn't come up with anything
better. The "yield" part will make sense two patches from now.)
Signed-off-by: Hanna Reitz
---
tests/qemu-iotests/281 | 79 +-
tests/qemu-iotests/281.out | 4 +-
2 files changed, 79 insertions(+), 4 deletions(-)
diff --git a/tests/qemu-iotests/281 b/tests/qemu-iotests/281
index 318e333939..4fb3cd30dd 100755
--- a/tests/qemu-iotests/281
+++ b/tests/qemu-iotests/281
@@ -1,5 +1,5 @@
#!/usr/bin/env python3
-# group: rw quick
+# group: rw
#
# Test cases for blockdev + IOThread interactions
#
@@ -20,8 +20,9 @@
#
import os
+import time
import iotests
-from iotests import qemu_img
+from iotests import qemu_img, QemuStorageDaemon
image_len = 64 * 1024 * 1024
@@ -243,6 +244,80 @@ class TestBlockdevBackupAbort(iotests.QMPTestCase):
# Hangs on failure, we expect this error.
self.assert_qmp(result, 'error/class', 'GenericError')
+# Test for RHBZ#2033626
+class TestYieldingAndTimers(iotests.QMPTestCase):
+sock = os.path.join(iotests.sock_dir, 'nbd.sock')
+qsd = None
+
+def setUp(self):
+self.create_nbd_export()
+
+# Simple VM with an NBD block device connected to the NBD export
+# provided by the QSD
+self.vm = iotests.VM()
+self.vm.add_blockdev('nbd,node-name=nbd,server.type=unix,' +
+ f'server.path={self.sock},export=exp,' +
+ 'reconnect-delay=1,open-timeout=1')
+
+self.vm.launch()
+
+def tearDown(self):
+self.stop_nbd_export()
+self.vm.shutdown()
+
+def test_timers_with_blockdev_del(self):
+# The NBD BDS will have had an active open timer, because setUp() gave
+# a positive value for @open-timeout. It should be gone once the BDS
+# has been opened.
+# (But there used to be a bug where it remained active, which will
+# become important below.)
+
+# Stop and restart the NBD server, and do some I/O on the client to
+# trigger a reconnect and start the reconnect delay timer
+self.stop_nbd_export()
+self.create_nbd_export()
+
+result = self.vm.qmp('human-monitor-command',
+ command_line='qemu-io nbd "write 0 512"')
+self.assert_qmp(result, 'return', '')
+
+# Reconnect is done, so the reconnect delay timer should be gone.
+# (This is similar to how the open timer should be gone after open,
+# and similarly there used to be a bug where it was not gone.)
+
+# Delete the BDS to see whether both timers are gone. If they are not,
+# they will remain active, fire later, and then access freed data.
+# (Or, with "block/nbd: Assert there are no timers when closed"
+# applied, the assertions added in that patch will fail.)
+result = self.vm.qmp('blockdev-del', node_name='nbd')
+self.assert_qmp(result, 'return', {})
+
+# Give the timers some time to fire (both have a timeout of 1 s).
+# (Sleeping in an iotest may ring some alarm bells, but note that if
+# the timing is off here, the test will just always pass. If we kill
+# the VM too early, then we just kill the timers before they can fire,
+# thus not see the error, and so the test will pass.)
+time.sleep(2)
+
+
[PATCH 4/7] iotests.py: Add QemuStorageDaemon class
This is a rather simple class that allows creating a QSD instance
running in the background and stopping it when no longer needed.
The __del__ handler is a safety net for when something goes so wrong in
a test that e.g. the tearDown() method is not called (e.g. setUp()
launches the QSD, but then launching a VM fails). We do not want the
QSD to continue running after the test has failed, so __del__() will
take care to kill it.
Signed-off-by: Hanna Reitz
---
tests/qemu-iotests/iotests.py | 42 +++
1 file changed, 42 insertions(+)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 8cdb381f2a..c75e402b87 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -73,6 +73,8 @@
qemu_prog = os.environ.get('QEMU_PROG', 'qemu')
qemu_opts = os.environ.get('QEMU_OPTIONS', '').strip().split(' ')
+qsd_prog = os.environ.get('QSD_PROG', 'qemu-storage-daemon')
+
gdb_qemu_env = os.environ.get('GDB_OPTIONS')
qemu_gdb = []
if gdb_qemu_env:
@@ -345,6 +347,46 @@ def cmd(self, cmd):
return self._read_output()
+class QemuStorageDaemon:
+def __init__(self, *args: str, instance_id: Optional[str] = None):
+if not instance_id:
+instance_id = 'a'
+
+self.pidfile = os.path.join(test_dir, f'qsd-{instance_id}-pid')
+all_args = [qsd_prog] + list(args) + ['--pidfile', self.pidfile]
+
+# Cannot use with here, we want the subprocess to stay around
+# pylint: disable=consider-using-with
+self._p = subprocess.Popen(all_args)
+while not os.path.exists(self.pidfile):
+if self._p.poll() is not None:
+cmd = ' '.join(all_args)
+raise RuntimeError(
+'qemu-storage-daemon terminated with exit code ' +
+f'{self._p.returncode}: {cmd}')
+
+time.sleep(0.01)
+
+with open(self.pidfile, encoding='utf-8') as f:
+self._pid = int(f.read().strip())
+
+assert self._pid == self._p.pid
+
+def stop(self, kill_signal=15):
+self._p.send_signal(kill_signal)
+self._p.wait()
+self._p = None
+
+try:
+os.remove(self.pidfile)
+except OSError:
+pass
+
+def __del__(self):
+if self._p is not None:
+self.stop(kill_signal=9)
+
+
def qemu_nbd(*args):
'''Run qemu-nbd in daemon mode and return the parent's exit code'''
return subprocess.call(qemu_nbd_args + ['--fork'] + list(args))
--
2.34.1
[PATCH 1/7] block/nbd: Delete reconnect delay timer when done
We start the reconnect delay timer to cancel the reconnection attempt after a while. Once nbd_co_do_establish_connection() has returned, this attempt is over, and we no longer need the timer. Delete it before returning from nbd_reconnect_attempt(), so that it does not persist beyond the I/O request that was paused for reconnecting; we do not want it to fire in a drained section, because all sort of things can happen in such a section (e.g. the AioContext might be changed, and we do not want the timer to fire in the wrong context; or the BDS might even be deleted, and so the timer CB would access already-freed data). Signed-off-by: Hanna Reitz --- block/nbd.c | 7 +++ 1 file changed, 7 insertions(+) diff --git a/block/nbd.c b/block/nbd.c index 63dbfa807d..16cd7fef77 100644 --- a/block/nbd.c +++ b/block/nbd.c @@ -381,6 +381,13 @@ static coroutine_fn void nbd_reconnect_attempt(BDRVNBDState *s) } nbd_co_do_establish_connection(s->bs, NULL); + +/* + * The reconnect attempt is done (maybe successfully, maybe not), so + * we no longer need this timer. Delete it so it will not outlive + * this I/O request (so draining removes all timers). + */ +reconnect_delay_timer_del(s); } static coroutine_fn int nbd_receive_replies(BDRVNBDState *s, uint64_t handle) -- 2.34.1
[PATCH 0/7] block/nbd: Move s->ioc on AioContext change
Hi, I’ve sent an RFC for this before, which can be found here: https://lists.nongnu.org/archive/html/qemu-block/2022-01/msg00765.html ...and is basically patch 6 in this series. That was an RFC for two reasons: (1) I didn’t know what to do with the two timers that the NBD BDS has (the open timer and the reconnect delay timer), and (2) it didn’t include a regression test. This v1 addresses (2) in the obvious manner (by adding a test), and (1) like Vladimir has proposed, namely by asserting there are no timers on AioContext change, because there shouldn’t by any. The problem is that that assertion is wrong for both timers. As far as I can tell, both of them are created so they will cancel the respective (re-)connection attempt after a user-configurable interval. However, they are not deleted when that attempt succeeds (or otherwise returns before the interval). So if the attempt does succeed, both of them will persist for however long they are configured, and they are are never disarmed/deleted anywhere, not even when the BDS is freed. That’s a problem beyond “what do I do with them on AioContext change”, because it means if you delete the BDS when one of those timers is still active, the timer will still fire afterwards and access (and dereference!) freed data. The solution should be clear, though, because as Vladimir has said, they simply shouldn’t persist. So once the respective (re-)connection attempt returns, this series makes it so they are deleted (patches 1 and 2). Patch 3 adds an assertion that the timers are gone when the BDS is closed, so that we definitely won’t run into a situation where they access freed data. Hanna Reitz (7): block/nbd: Delete reconnect delay timer when done block/nbd: Delete open timer when done block/nbd: Assert there are no timers when closed iotests.py: Add QemuStorageDaemon class iotests/281: Test lingering timers block/nbd: Move s->ioc on AioContext change iotests/281: Let NBD connection yield in iothread block/nbd.c | 64 + tests/qemu-iotests/281| 101 +- tests/qemu-iotests/281.out| 4 +- tests/qemu-iotests/iotests.py | 42 ++ 4 files changed, 207 insertions(+), 4 deletions(-) -- 2.34.1
Re: [PULL 0/4] Python patches
On Thu, 3 Feb 2022 at 01:59, John Snow wrote: > > The following changes since commit 47cc1a3655135b89fa75c2824fbddd29df874612: > > Merge remote-tracking branch 'remotes/kwolf-gitlab/tags/for-upstream' into > staging (2022-02-01 19:48:15 +) > > are available in the Git repository at: > > https://gitlab.com/jsnow/qemu.git tags/python-pull-request > > for you to fetch changes up to b0b662bb2b340d63529672b5bdae596a6243c4d0: > > python/aqmp: add socket bind step to legacy.py (2022-02-02 14:12:22 -0500) > > > Python patches > > Peter: I expect this to address the iotest 040,041 failures you observed > on NetBSD. If it doesn't, let me know. I still see this one, which is different from the 040,041 stuff, and where 'make check' is for some reason giving a lot less useful detail. (This is a prexisting intermittent from before this patchset). [etc] ▶ 175/704 /io/channel/pipe/sync OK ▶ 175/704 /io/channel/pipe/async OK 175/704 qemu:unit / test-io-channel-file OK 0.11s 5 subtests passed 177/704 qemu:unit / test-io-channel-tls RUNNING >>> G_TEST_BUILDDIR=/home/qemu/qemu-test.yiYr4m/build/tests/unit >>> MALLOC_PERTURB_=5 G_TEST_SRCDIR=/home/qemu/qemu-test.yiYr4m/src/tests/unit >>> /home/ qemu/qemu-test.yiYr4m/build/tests/unit/test-io-channel-tls --tap -k ▶ 176/704 /io/channel/socket/ipv4-sync OK ▶ 176/704 /io/channel/socket/ipv4-async OK ▶ 176/704 /io/channel/socket/ipv4-fd OK ▶ 176/704 /io/channel/socket/ipv6-sync OK ▶ 176/704 /io/channel/socket/ipv6-async OK ▶ 176/704 /io/channel/socket/unix-sync OK ▶ 176/704 /io/channel/socket/unix-async OK ▶ 176/704 /io/channel/socket/unix-fd-pass OK ▶ 176/704 /io/channel/socket/unix-listen-cleanup OK 176/704 qemu:unit / test-io-channel-socket OK 0.13s 9 subtests passed ▶ 1/1 qcow2 qsd-jobsOK 1/1 qemu:block / qemu-iotests qcow2 ERROR 243.14s exit status 1 178/704 qemu:unit / test-io-task RUNNING >>> G_TEST_BUILDDIR=/home/qemu/qemu-test.yiYr4m/build/tests/unit >>> MALLOC_PERTURB_=194 >>> G_TEST_SRCDIR=/home/qemu/qemu-test.yiYr4m/src/tests/unit /hom e/qemu/qemu-test.yiYr4m/build/tests/unit/test-io-task --tap -k ▶ 147/704 /bdrv-drain/blockjob/iothread/error/drain_subtree OK Summary of Failures: 1/1 qemu:block / qemu-iotests qcow2 ERROR 243.14s exit status 1 Ok: 0 Expected Fail: 0 Fail: 1 Unexpected Pass:0 Skipped:0 Timeout:0 Full log written to /home/qemu/qemu-test.yiYr4m/build/meson-logs/iotestslog.txt ▶ 147/704 /bdrv-drain/deletion/drain OK ▶ 178/704 /crypto/task/complete OK ▶ 178/704 /crypto/task/datafree OK [etc] thanks -- PMM
Re: [PATCH 09/12] jobs: ensure sleep in job_sleep_ns is fully performed
On 26/01/2022 12:21, Stefan Hajnoczi wrote:
> On Tue, Jan 18, 2022 at 11:27:35AM -0500, Emanuele Giuseppe Esposito wrote:
>> If a drain happens while a job is sleeping, the timeout
>> gets cancelled and the job continues once the drain ends.
>> This is especially bad for the sleep performed in commit and stream
>> jobs, since that is dictated by ratelimit to maintain a certain speed.
>>
>> Basically the execution path is the followig:
>
> s/followig/following/
>
>> 1. job calls job_sleep_ns, and yield with a timer in @ns ns.
>> 2. meanwhile, a drain is executed, and
>>child_job_drained_{begin/end} could be executed as ->drained_begin()
>>and ->drained_end() callbacks.
>>Therefore child_job_drained_begin() enters the job, that continues
>>execution in job_sleep_ns() and calls job_pause_point_locked().
>> 3. job_pause_point_locked() detects that we are in the middle of a
>>drain, and firstly deletes any existing timer and then yields again,
>>waiting for ->drained_end().
>> 4. Once draining is finished, child_job_drained_end() runs and resumes
>>the job. At this point, the timer has been lost and we just resume
>>without checking if enough time has passed.
>>
>> This fix implies that from now onwards, job_sleep_ns will force the job
>> to sleep @ns, even if it is wake up (purposefully or not) in the middle
>> of the sleep. Therefore qemu-iotests test might run a little bit slower,
>> depending on the speed of the job. Setting a job speed to values like "1"
>> is not allowed anymore (unless you want to wait forever).
>>
>> Because of this fix, test_stream_parallel() in tests/qemu-iotests/030
>> takes too long, since speed of stream job is just 1024 and before
>> it was skipping all the wait thanks to the drains. Increase the
>> speed to 256 * 1024. Exactly the same happens for test 151.
>>
>> Instead we need to sleep less in test_cancel_ready() test-blockjob.c,
>> so that the job will be able to exit the sleep and transition to ready
>> before the main loop asserts.
>
> I remember seeing Hanna and Kevin use carefully rate-limited jobs in
> qemu-iotests. They might have thoughts on whether this patch is
> acceptable or not.
>
I think the speed was carefully set as "slow enough" just to give time
to the operation to happen while the job was running. Anyways, all tests
I run work as intended, I just increased their speed slightly.
Having speed=1 would make e job really really slow.
Emanuele
Re: [RFC] thread-pool: Add option to fix the pool size
On Thu, Feb 03, 2022 at 10:56:49AM +, Daniel P. Berrangé wrote: > On Thu, Feb 03, 2022 at 10:53:07AM +, Stefan Hajnoczi wrote: > > On Wed, Feb 02, 2022 at 06:52:34PM +0100, Nicolas Saenz Julienne wrote: > > > The thread pool regulates itself: when idle, it kills threads until > > > empty, when in demand, it creates new threads until full. This behaviour > > > doesn't play well with latency sensitive workloads where the price of > > > creating a new thread is too high. For example, when paired with qemu's > > > '-mlock', or using safety features like SafeStack, creating a new thread > > > has been measured take multiple milliseconds. > > > > > > In order to mitigate this let's introduce a new option to set a fixed > > > pool size. The threads will be created during the pool's initialization, > > > remain available during its lifetime regardless of demand, and destroyed > > > upon freeing it. A properly characterized workload will then be able to > > > configure the pool to avoid any latency spike. > > > > > > Signed-off-by: Nicolas Saenz Julienne > > > > > > --- > > > > > > The fix I propose here works for my specific use-case, but I'm pretty > > > sure it'll need to be a bit more versatile to accommodate other > > > use-cases. > > > > > > Some questions: > > > > > > - Is unanimously setting these parameters for any pool instance too > > > limiting? It'd make sense to move the options into the AioContext the > > > pool belongs to. IIUC, for the general block use-case, this would be > > > 'qemu_aio_context' as initialized in qemu_init_main_loop(). > > > > Yes, qemu_aio_context is the main loop's AioContext. It's used unless > > IOThreads are configured. > > > > It's nice to have global settings that affect all AioContexts, so I > > think this patch is fine for now. > > > > In the future IOThread-specific parameters could be added if individual > > IOThread AioContexts need tuning (similar to how poll-max-ns works > > today). > > > > > - Currently I'm setting two pool properties through a single qemu > > > option. The pool's size and dynamic behaviour, or lack thereof. I > > > think it'd be better to split them into separate options. I thought of > > > different ways of expressing this (min/max-size where static happens > > > when min-size=max-size, size and static/dynamic, etc..), but you might > > > have ideas on what could be useful to other use-cases. > > > > Yes, "min" and "max" is more flexible than fixed-size=n. fixed-size=n is > > equivalent to min=n,max=n. The current default policy is min=0,max=64. > > If you want more threads you could do min=0,max=128. If you want to > > reserve 1 thread all the time use min=1,max=64. > > > > I would go with min and max. > > This commit also exposes this as a new top level command line > argument. Given our aim to eliminate QemuOpts and use QAPI/QOM > properties for everything I think we need a different approach. > > I'm not sure which exisiting QAPI/QOM option it most appropriate > to graft these tunables onto ? -machine ? -accel ? Or is there > no good fit yet ? Yep, I didn't comment on this because I don't have a good suggestion. In terms of semantics I think we should have: 1. A global default value that all new AioContext take. The QEMU main loop's qemu_aio_context will use this and all IOThread AioContext will use it (unless they have been overridden). I would define it on --machine because that's the "global" object for a guest, but that's not very satisfying. 2. (Future patch) --object iothread,thread-pool-min=N,thread-pool-max=M just like poll-max-ns and friends. This allows the values to be set on a per-IOThread basis. Stefan signature.asc Description: PGP signature
[PATCH 0/2] block: Fix crash in blockdev-reopen with iothreads
Kevin Wolf (2): block: Lock AioContext for drain_end in blockdev-reopen iotests: Test blockdev-reopen with iothreads and throttling blockdev.c | 11 ++- tests/qemu-iotests/245 | 36 +--- tests/qemu-iotests/245.out | 4 ++-- 3 files changed, 45 insertions(+), 6 deletions(-) -- 2.31.1
[PATCH 1/2] block: Lock AioContext for drain_end in blockdev-reopen
bdrv_subtree_drained_end() requires the caller to hold the AioContext
lock for the drained node. Not doing this for nodes outside of the main
AioContext leads to crashes when AIO_WAIT_WHILE() needs to wait and
tries to temporarily release the lock.
Fixes: 3908b7a8994fa5ef7a89aa58cd5a02fc58141592
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2046659
Reported-by: Qing Wang
Signed-off-by: Kevin Wolf
---
blockdev.c | 11 ++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/blockdev.c b/blockdev.c
index 8197165bb5..42e098b458 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3530,6 +3530,7 @@ void qmp_blockdev_reopen(BlockdevOptionsList
*reopen_list, Error **errp)
{
BlockReopenQueue *queue = NULL;
GSList *drained = NULL;
+GSList *p;
/* Add each one of the BDS that we want to reopen to the queue */
for (; reopen_list != NULL; reopen_list = reopen_list->next) {
@@ -3579,7 +3580,15 @@ void qmp_blockdev_reopen(BlockdevOptionsList
*reopen_list, Error **errp)
fail:
bdrv_reopen_queue_free(queue);
-g_slist_free_full(drained, (GDestroyNotify) bdrv_subtree_drained_end);
+for (p = drained; p; p = p->next) {
+BlockDriverState *bs = p->data;
+AioContext *ctx = bdrv_get_aio_context(bs);
+
+aio_context_acquire(ctx);
+bdrv_subtree_drained_end(bs);
+aio_context_release(ctx);
+}
+g_slist_free(drained);
}
void qmp_blockdev_del(const char *node_name, Error **errp)
--
2.31.1
[PATCH 2/2] iotests: Test blockdev-reopen with iothreads and throttling
The 'throttle' block driver implements .bdrv_co_drain_end, so
blockdev-reopen will have to wait for it to complete in the polling
loop at the end of qmp_blockdev_reopen(). This makes AIO_WAIT_WHILE()
release the AioContext lock, which causes a crash if the lock hasn't
correctly been taken.
Signed-off-by: Kevin Wolf
---
tests/qemu-iotests/245 | 36 +---
tests/qemu-iotests/245.out | 4 ++--
2 files changed, 35 insertions(+), 5 deletions(-)
diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index 24ac43f70e..8cbed7821b 100755
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -1138,12 +1138,13 @@ class TestBlockdevReopen(iotests.QMPTestCase):
self.assertEqual(self.get_node('hd1'), None)
self.assert_qmp(self.get_node('hd2'), 'ro', True)
-def run_test_iothreads(self, iothread_a, iothread_b, errmsg = None):
-opts = hd_opts(0)
+def run_test_iothreads(self, iothread_a, iothread_b, errmsg = None,
+ opts_a = None, opts_b = None):
+opts = opts_a or hd_opts(0)
result = self.vm.qmp('blockdev-add', conv_keys = False, **opts)
self.assert_qmp(result, 'return', {})
-opts2 = hd_opts(2)
+opts2 = opts_b or hd_opts(2)
result = self.vm.qmp('blockdev-add', conv_keys = False, **opts2)
self.assert_qmp(result, 'return', {})
@@ -1194,6 +1195,35 @@ class TestBlockdevReopen(iotests.QMPTestCase):
def test_iothreads_switch_overlay(self):
self.run_test_iothreads('', 'iothread0')
+def test_iothreads_with_throttling(self):
+# Create a throttle-group object
+opts = { 'qom-type': 'throttle-group', 'id': 'group0',
+ 'limits': { 'iops-total': 1000 } }
+result = self.vm.qmp('object-add', conv_keys = False, **opts)
+self.assert_qmp(result, 'return', {})
+
+# Options with a throttle filter between format and protocol
+opts = [
+{
+'driver': iotests.imgfmt,
+'node-name': f'hd{idx}',
+'file' : {
+'node-name': f'hd{idx}-throttle',
+'driver': 'throttle',
+'throttle-group': 'group0',
+'file': {
+'driver': 'file',
+'node-name': f'hd{idx}-file',
+'filename': hd_path[idx],
+},
+},
+}
+for idx in (0, 2)
+]
+
+self.run_test_iothreads('iothread0', 'iothread0', None,
+opts[0], opts[1])
+
if __name__ == '__main__':
iotests.activate_logging()
iotests.main(supported_fmts=["qcow2"],
diff --git a/tests/qemu-iotests/245.out b/tests/qemu-iotests/245.out
index 4eced19294..a4e04a3266 100644
--- a/tests/qemu-iotests/245.out
+++ b/tests/qemu-iotests/245.out
@@ -17,8 +17,8 @@ read 1/1 bytes at offset 262152
read 1/1 bytes at offset 262160
1 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-...
+
--
-Ran 25 tests
+Ran 26 tests
OK
--
2.31.1
Re: [PATCH 01/12] introduce BDRV_POLL_WHILE_UNLOCKED
On 26/01/2022 11:49, Stefan Hajnoczi wrote:
> On Tue, Jan 18, 2022 at 11:27:27AM -0500, Emanuele Giuseppe Esposito wrote:
>> Same as BDRV_POLL_WHILE, but uses AIO_WAIT_WHILE_UNLOCKED.
>>
>> Signed-off-by: Emanuele Giuseppe Esposito
>> ---
>> include/block/block-global-state.h | 5 +
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/include/block/block-global-state.h
>> b/include/block/block-global-state.h
>> index 419fe8427f..7ad9496f56 100644
>> --- a/include/block/block-global-state.h
>> +++ b/include/block/block-global-state.h
>> @@ -158,6 +158,11 @@ void bdrv_drain_all(void);
>> AIO_WAIT_WHILE(bdrv_get_aio_context(bs_), \
>> cond); })
>>
>> +#define BDRV_POLL_WHILE_UNLOCKED(bs, cond) ({ \
>> +BlockDriverState *bs_ = (bs); \
>> +AIO_WAIT_WHILE_UNLOCKED(bdrv_get_aio_context(bs_), \
>> +cond); })
>
> No doc comments? When and why is this API useful? Are there any
> preconditions or assumptions (e.g. cond must be thread-safe)?
>
I am planning to add the following comment above the macro:
/*
* Unlocked counterpart of BDRV_POLL_WHILE. Uses AIO_WAIT_WHILE_UNLOCKED,
* so it does not release+acquire the AioContext lock if we are in the
* main loop, therefore the caller does not need to take it.
* This function avoids taking the AioContext lock unnecessarly, so use
* it only when sure that the lock is not taken already, otherwise it
* might cause deadlocks.
*
* @cond must be thread-safe.
*/
Emanuele
Re: [PATCH 10/12] block.c: add subtree_drains where needed
On 19/01/2022 10:47, Paolo Bonzini wrote: > > About this: > >> + * TODO: this is called by job_unref with lock held, because >> + * afterwards it calls bdrv_try_set_aio_context. >> + * Once all of this is fixed, take care of removing >> + * the aiocontext lock and make this function _unlocked. > > It may be clear to you, but it's quite cryptic: > I think you figured it by looking at the job patches, but: > - which lock is held by job_unref()? Also, would it make more sense to > talk about block_job_free() rather than job_unref()? I can't quite > follow where the AioContext lock is taken. AioContext lock. I think this is a change I made in the job patches, so comparing it with the master would make this piece harder to understand. In the job series, I reduce the granularity of the AioContext lock, ending up having it only around few callbacks of JobDriver, namely ->free(). This is why I talk about job_unref, because it calls ->free. The actual lock is taken in job_unref, but the caller (->free) is block_job_free. Yes it makes more sense mentioning block_job_free. > - what is "all of this", and what do you mean by "not safe yet"? Do > both refer to bdrv_try_set_aio_context() needing the AioContext lock? Yes > - what is "this function" (that should become _unlocked)? bdrv_subtree_drained_begin This is the new comment I intend to put: /* * TODO: this function is called by BlockJobDriver's ->free() * callback, block_job_free. * We unfortunately must still take the AioContext lock around * ->free() in job_unref, because of the bdrv_try_set_aio_context * call below that still releases/acquires it. * Once bdrv_try_set_aio_context does not require the AioContext lock, * take care of removing it around ->free() and replace * the following bdrv_subtree_drained_begin with its * _unlocked version. */ > > I think you could also split the patch in multiple parts for different > call chains. In particular bdrv_set_backing_hd can be merged with the > patch to bdrv_reopen_parse_file_or_backing, since both of them deal with > bdrv_set_file_or_backing_noperm. > Ok, I will try to do that. Emanuele
Re: [PATCH V2 for-6.2 0/2] fixes for bdrv_co_block_status
On Thu, Feb 03, 2022 at 12:42:30PM +0100, Peter Lieven wrote: Am 01.02.22 um 15:39 schrieb Kevin Wolf: Am 13.01.2022 um 15:44 hat Peter Lieven geschrieben: V1->V2: Patch 1: Treat a hole just like an unallocated area. [Ilya] Patch 2: Apply workaround only for pre-Quincy librbd versions and ensure default striping and non child images. [Ilya] Peter Lieven (2): block/rbd: fix handling of holes in .bdrv_co_block_status block/rbd: workaround for ceph issue #53784 Thanks, applied to the block branch. Kevin Hi Kevin, thanks for taking care of this. I was a few days out of office. @Stefano: it seems Kevin addresses your comments that should have gone into a V3. Yep :-) Thanks, Stefano
Re: [PATCH V2 for-6.2 0/2] fixes for bdrv_co_block_status
Am 01.02.22 um 15:39 schrieb Kevin Wolf: > Am 13.01.2022 um 15:44 hat Peter Lieven geschrieben: >> V1->V2: >> Patch 1: Treat a hole just like an unallocated area. [Ilya] >> Patch 2: Apply workaround only for pre-Quincy librbd versions and >> ensure default striping and non child images. [Ilya] >> >> Peter Lieven (2): >> block/rbd: fix handling of holes in .bdrv_co_block_status >> block/rbd: workaround for ceph issue #53784 > Thanks, applied to the block branch. > > Kevin > Hi Kevin, thanks for taking care of this. I was a few days out of office. @Stefano: it seems Kevin addresses your comments that should have gone into a V3. Best, Peter
Re: [PATCH 05/12] test-bdrv-drain.c: adapt test to the coming subtree drains
On 19/01/2022 10:18, Paolo Bonzini wrote: > On 1/18/22 17:27, Emanuele Giuseppe Esposito wrote: >> - First of all, inconsistency between block_job_create under >> aiocontext lock that internally calls blk_insert_bs and therefore >> bdrv_replace_child_noperm, and blk_insert_bs that is called two lines >> above in the same test without aiocontext. There seems to be no >> reason on why we need the lock there, so move the aiocontext lock further >> down. >> >> - test_detach_indirect: here it is simply a matter of wrong callbacks >> used. In the original test, there was only a subtree drain, so >> overriding .drained_begin was not a problem. Now that we want to have >> additional subtree drains, we risk to call the test callback >> to early, or multiple times. We do not want that, so override >> the callback only when we actually want to use it. > > The language here is a bit overcomplicated. Don't think that you're > writing Italian, instead use simple sentences. > > First, the test is inconsistent about taking the AioContext lock when > calling bdrv_replace_child_noperm. bdrv_replace_child_noperm is reached > in two places: from blk_insert_bs directly, and via block_job_create. > Only the second does it with the AioContext lock taken, and there seems > to be no reason why the lock is needed. Move aio_context_acquire > further down. [Any reason why you don't move it even further down, to > cover only job_start?] The lock is left just to cover block_job_add_bdrv, since it internally releases and then acquires the lock. Fixing that is a future TODO. job_start did not and does not need the AioContext lock :) > > Second, test_detach_indirect is only interested in observing the first > call to .drained_begin. In the original test, there was only a single > subtree drain; however, with additional drains introduced in > bdrv_replace_child_noperm, the test callback would be called too early > and/or multiple times. Override the callback only when we actually want > to use it, and put back the original after it's been invoked. > > This could also be split in two commits. > Will update the commit message, thank you! Emanuele
Re: [RFC] thread-pool: Add option to fix the pool size
On Thu, Feb 03, 2022 at 10:53:07AM +, Stefan Hajnoczi wrote: > On Wed, Feb 02, 2022 at 06:52:34PM +0100, Nicolas Saenz Julienne wrote: > > The thread pool regulates itself: when idle, it kills threads until > > empty, when in demand, it creates new threads until full. This behaviour > > doesn't play well with latency sensitive workloads where the price of > > creating a new thread is too high. For example, when paired with qemu's > > '-mlock', or using safety features like SafeStack, creating a new thread > > has been measured take multiple milliseconds. > > > > In order to mitigate this let's introduce a new option to set a fixed > > pool size. The threads will be created during the pool's initialization, > > remain available during its lifetime regardless of demand, and destroyed > > upon freeing it. A properly characterized workload will then be able to > > configure the pool to avoid any latency spike. > > > > Signed-off-by: Nicolas Saenz Julienne > > > > --- > > > > The fix I propose here works for my specific use-case, but I'm pretty > > sure it'll need to be a bit more versatile to accommodate other > > use-cases. > > > > Some questions: > > > > - Is unanimously setting these parameters for any pool instance too > > limiting? It'd make sense to move the options into the AioContext the > > pool belongs to. IIUC, for the general block use-case, this would be > > 'qemu_aio_context' as initialized in qemu_init_main_loop(). > > Yes, qemu_aio_context is the main loop's AioContext. It's used unless > IOThreads are configured. > > It's nice to have global settings that affect all AioContexts, so I > think this patch is fine for now. > > In the future IOThread-specific parameters could be added if individual > IOThread AioContexts need tuning (similar to how poll-max-ns works > today). > > > - Currently I'm setting two pool properties through a single qemu > > option. The pool's size and dynamic behaviour, or lack thereof. I > > think it'd be better to split them into separate options. I thought of > > different ways of expressing this (min/max-size where static happens > > when min-size=max-size, size and static/dynamic, etc..), but you might > > have ideas on what could be useful to other use-cases. > > Yes, "min" and "max" is more flexible than fixed-size=n. fixed-size=n is > equivalent to min=n,max=n. The current default policy is min=0,max=64. > If you want more threads you could do min=0,max=128. If you want to > reserve 1 thread all the time use min=1,max=64. > > I would go with min and max. This commit also exposes this as a new top level command line argument. Given our aim to eliminate QemuOpts and use QAPI/QOM properties for everything I think we need a different approach. I'm not sure which exisiting QAPI/QOM option it most appropriate to graft these tunables onto ? -machine ? -accel ? Or is there no good fit yet ? Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [RFC] thread-pool: Add option to fix the pool size
On Wed, Feb 02, 2022 at 06:52:34PM +0100, Nicolas Saenz Julienne wrote: > The thread pool regulates itself: when idle, it kills threads until > empty, when in demand, it creates new threads until full. This behaviour > doesn't play well with latency sensitive workloads where the price of > creating a new thread is too high. For example, when paired with qemu's > '-mlock', or using safety features like SafeStack, creating a new thread > has been measured take multiple milliseconds. > > In order to mitigate this let's introduce a new option to set a fixed > pool size. The threads will be created during the pool's initialization, > remain available during its lifetime regardless of demand, and destroyed > upon freeing it. A properly characterized workload will then be able to > configure the pool to avoid any latency spike. > > Signed-off-by: Nicolas Saenz Julienne > > --- > > The fix I propose here works for my specific use-case, but I'm pretty > sure it'll need to be a bit more versatile to accommodate other > use-cases. > > Some questions: > > - Is unanimously setting these parameters for any pool instance too > limiting? It'd make sense to move the options into the AioContext the > pool belongs to. IIUC, for the general block use-case, this would be > 'qemu_aio_context' as initialized in qemu_init_main_loop(). Yes, qemu_aio_context is the main loop's AioContext. It's used unless IOThreads are configured. It's nice to have global settings that affect all AioContexts, so I think this patch is fine for now. In the future IOThread-specific parameters could be added if individual IOThread AioContexts need tuning (similar to how poll-max-ns works today). > - Currently I'm setting two pool properties through a single qemu > option. The pool's size and dynamic behaviour, or lack thereof. I > think it'd be better to split them into separate options. I thought of > different ways of expressing this (min/max-size where static happens > when min-size=max-size, size and static/dynamic, etc..), but you might > have ideas on what could be useful to other use-cases. Yes, "min" and "max" is more flexible than fixed-size=n. fixed-size=n is equivalent to min=n,max=n. The current default policy is min=0,max=64. If you want more threads you could do min=0,max=128. If you want to reserve 1 thread all the time use min=1,max=64. I would go with min and max. > > Some background on my workload: I'm using IDE emulation, the guest is an > old RTOS that doesn't support virtio, using 'aio=native' isn't possible > either (unaligned IO accesses). I thought QEMU's block layer creates bounce buffers for unaligned accesses, handling both memory buffer alignment and LBA alignment necessary for aio=native,cache=none? signature.asc Description: PGP signature
Re: [PATCH 10/12] block.c: add subtree_drains where needed
On 02/02/2022 18:38, Paolo Bonzini wrote: > On 2/2/22 16:37, Emanuele Giuseppe Esposito wrote: >> So we have disk B with backing file C, and new disk A that wants to have >> backing file C. >> >> I think I understand what you mean, so in theory the operation would be >> - create new child >> - add child to A->children list >> - add child to C->parents list >> >> So in theory we need to >> * drain A (without subtree), because it can't happen that child nodes of >> A have in-flight requests that look at A status (children list), >> right? >> In other words, if A has another node X, can a request in X inspect >> A->children I am not sure if this can happen. It doesn't seem so, though. All functions inspecting ->parents are either GS or don't call recursive function that go down on children list again. >> * drain C, as parents can inspect C status (like B). Same assumption >> here, C->children[x]->bs cannot have requests inspecting C->parents >> list? What if C->children[x]->bs drains? we would have a child inspecting C->parents. > > In that case (i.e. if parents have to be drained, but children need not) > bdrv_drained_begin_unlocked would be enough, right? Should be, if that is the case. > > That would mean that ->children is I/O state but ->parents is global > state. I think it's quite a bit more complicated to analyze and to > understand. Not sure I follow this one, why is ->parents GS? it is also used by the drain API... Emanuele
Re: [PATCH v4 4/4] python/aqmp: add socket bind step to legacy.py
On Wed, Feb 02, 2022 at 02:08:59PM -0500, John Snow wrote:
> On Tue, Feb 1, 2022 at 2:46 PM Kevin Wolf wrote:
> >
> > Am 01.02.2022 um 19:32 hat John Snow geschrieben:
> > > On Tue, Feb 1, 2022 at 8:21 AM Kevin Wolf wrote:
> > > >
> > > > Am 01.02.2022 um 05:11 hat John Snow geschrieben:
> > > > > The synchronous QMP library would bind to the server address during
> > > > > __init__(). The new library delays this to the accept() call, because
> > > > > binding occurs inside of the call to start_[unix_]server(), which is
> > > > > an
> > > > > async method -- so it cannot happen during __init__ anymore.
> > > > >
> > > > > Python 3.7+ adds the ability to create the server (and thus the bind()
> > > > > call) and begin the active listening in separate steps, but we don't
> > > > > have that functionality in 3.6, our current minimum.
> > > > >
> > > > > Therefore ... Add a temporary workaround that allows the synchronous
> > > > > version of the client to bind the socket in advance, guaranteeing that
> > > > > there will be a UNIX socket in the filesystem ready for the QEMU
> > > > > client
> > > > > to connect to without a race condition.
> > > > >
> > > > > (Yes, it's a bit ugly. Fixing it more nicely will have to wait until
> > > > > our
> > > > > minimum Python version is 3.7+.)
> > I guess the relevant question in the context of this patch is whether
> > sync.py will need a similar two-phase setup as legacy.py. Do you think
> > you can do without it when you have to reintroduce this behaviour here
> > to fix bugs?
> >
>
> Hm, honestly, no. I think you'll still need the two-phase in the sync
> version no matter what -- if you expect to be able to launch QMP and
> QEMU from the same process, anyway. I suppose it's just a matter of
> what you *call* it and what/where the arguments are.
>
> I could just call it bind(), and it does whatever it needs to (Either
> creating a socket or, in 3.7+, instantiating more of the asyncio loop
> machinery).
> The signature for accept() would need to change to (perhaps)
> optionally accepting no arguments; i.e. you can do either of the
> following:
>
> (1) qmp.bind('/tmp/sock')
> qmp.accept()
>
> (2) qmp.accept('/tmp/sock')
>
> The risk is that the interface is just a touch more complex, the docs
> get a bit more cluttered, there's a slight loss of clarity, the
> accept() method would need to check to make sure you didn't give it an
> address argument if you've already given it one, etc. Necessary
> trade-off for the flexibility, though.
Doing a bind() inside an accept() call is really strange
design IMHO.
bind() is a one-time-only initialization operation for startup,
where as accept() is a runtime operation invoked repeatedly.
bind() is also an op that is reasonably likely to fail
due to something already holding the socket address, and thus
an error condition that you want to report to an application
during its early startup phase.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v4 4/4] python/aqmp: add socket bind step to legacy.py
Am 02.02.2022 um 20:08 hat John Snow geschrieben:
> > I guess the relevant question in the context of this patch is whether
> > sync.py will need a similar two-phase setup as legacy.py. Do you think
> > you can do without it when you have to reintroduce this behaviour here
> > to fix bugs?
> >
>
> Hm, honestly, no. I think you'll still need the two-phase in the sync
> version no matter what -- if you expect to be able to launch QMP and
> QEMU from the same process, anyway. I suppose it's just a matter of
> what you *call* it and what/where the arguments are.
>
> I could just call it bind(), and it does whatever it needs to (Either
> creating a socket or, in 3.7+, instantiating more of the asyncio loop
> machinery).
> The signature for accept() would need to change to (perhaps)
> optionally accepting no arguments; i.e. you can do either of the
> following:
>
> (1) qmp.bind('/tmp/sock')
> qmp.accept()
>
> (2) qmp.accept('/tmp/sock')
>
> The risk is that the interface is just a touch more complex, the docs
> get a bit more cluttered, there's a slight loss of clarity, the
> accept() method would need to check to make sure you didn't give it an
> address argument if you've already given it one, etc. Necessary
> trade-off for the flexibility, though.
Hm, how about copying the create_server() interface instead? So it
would become:
(1) qmp.create_server('/tmp/sock', start_serving=False)
qmp.start_serving()
(2) qmp.create_server('/tmp/sock')
Then you get the connection details only in a single place. (The names
should probably be changed because we're still a QMP client even though
we're technically the server on the socket level, but you get the idea.)
Kevin
Re: [PULL 18/20] block/nbd: drop connection_co
Am 02.02.22 um 15:21 schrieb Hanna Reitz:
> On 02.02.22 14:53, Eric Blake wrote:
>> On Wed, Feb 02, 2022 at 12:49:36PM +0100, Fabian Ebner wrote:
>>> Am 27.09.21 um 23:55 schrieb Eric Blake:
From: Vladimir Sementsov-Ogievskiy
OK, that's a big rewrite of the logic.
Pre-patch we have an always running coroutine - connection_co. It does
reply receiving and reconnecting. And it leads to a lot of difficult
and unobvious code around drained sections and context switch. We also
abuse bs->in_flight counter which is increased for connection_co and
temporary decreased in points where we want to allow drained section to
begin. One of these place is in another file: in nbd_read_eof() in
nbd/client.c.
We also cancel reconnect and requests waiting for reconnect on drained
begin which is not correct. And this patch fixes that.
Let's finally drop this always running coroutine and go another way:
do both reconnect and receiving in request coroutines.
>>> Hi,
>>>
>>> while updating our stack to 6.2, one of our live-migration tests stopped
>>> working (backtrace is below) and bisecting led me to this patch.
>>>
>>> The VM has a single qcow2 disk (converting to raw doesn't make a
>>> difference) and the issue only appears when using iothread (for both
>>> virtio-scsi-pci and virtio-block-pci).
>>>
>>> Reverting 1af7737871fb3b66036f5e520acb0a98fc2605f7 (which lives on top)
>>> and 4ddb5d2fde6f22b2cf65f314107e890a7ca14fcf (the commit corresponding
>>> to this patch) in v6.2.0 makes the migration work again.
>>>
>>> Backtrace:
>>>
>>> Thread 1 (Thread 0x7f9d93458fc0 (LWP 56711) "kvm"):
>>> #0 __GI_raise (sig=sig@entry=6) at
>>> ../sysdeps/unix/sysv/linux/raise.c:50
>>> #1 0x7f9d9d6bc537 in __GI_abort () at abort.c:79
>>> #2 0x7f9d9d6bc40f in __assert_fail_base (fmt=0x7f9d9d825128
>>> "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", assertion=0x5579153763f8
>>> "qemu_get_current_aio_context() == qemu_coroutine_get_aio_context(co)",
>>> file=0x5579153764f9 "../io/channel.c", line=483, function=>> out>) at assert.c:92
>> Given that this assertion is about which aio context is set, I wonder
>> if the conversation at
>> https://lists.gnu.org/archive/html/qemu-devel/2022-02/msg00096.html is
>> relevant; if so, Vladimir may already be working on the patch.
>
> It should be exactly that patch:
>
> https://lists.gnu.org/archive/html/qemu-devel/2022-01/msg06222.html
>
> (From the discussion it appears that for v1 I need to ensure the
> reconnection timer is deleted immediately once reconnecting succeeds,
> and then that should be good to move out of the RFC state.)
Thanks for the quick responses and happy to hear you're already working
on it! With the RFC, the issue is gone for me.
>
> Basically, I expect qemu to crash every time that you try to use an NBD
> block device in an I/O thread (unless you don’t do any I/O), for example
> this is the simplest reproducer I know of:
>
> $ qemu-nbd --fork -k /tmp/nbd.sock -f raw null-co://
>
> $ qemu-system-x86_64 \
> -object iothread,id=iothr0 \
> -device virtio-scsi,id=vscsi,iothread=iothr0 \
> -blockdev '{
> "driver": "nbd",
> "node-name": "nbd",
> "server": {
> "type": "unix",
> "path": "/tmp/nbd.sock"
> } }' \
> -device scsi-hd,bus=vscsi.0,drive=nbd
> qemu-system-x86_64: ../qemu-6.2.0/io/channel.c:483:
> qio_channel_restart_read: Assertion `qemu_get_current_aio_context() ==
> qemu_coroutine_get_aio_context(co)' failed.
> qemu-nbd: Disconnect client, due to: Unable to read from socket:
> Connection reset by peer
> [1] 108747 abort (core dumped) qemu-system-x86_64 -object
> iothread,id=iothr0 -device -blockdev -device
>
>
Interestingly, the reproducer didn't crash the very first time I tried
it. I did get the same error after ^C-ing though, and on subsequent
tries it mostly crashed immediately, but very occasionally it didn't.
