* [PATCH 1/5] migration: Fix missing join() of rp_thread
2021-07-21 1:21 [PATCH 0/5] migrations: Fix potential rare race of migration-test after yank Peter Xu
@ 2021-07-21 1:21 ` Peter Xu
2021-07-21 9:49 ` Dr. David Alan Gilbert
2021-07-21 1:21 ` [PATCH 2/5] migration: Shutdown src in await_return_path_close_on_source() Peter Xu
` (3 subsequent siblings)
4 siblings, 1 reply; 17+ messages in thread
From: Peter Xu @ 2021-07-21 1:21 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Lukas Straub, Juan Quintela,
Dr . David Alan Gilbert, peterx, Leonardo Bras Soares Passos
It's possible that the migration thread skip the join() of the rp_thread in
below race and crash on src right at finishing migration:
migration_thread rp_thread
---------------- ---------
migration_completion()
(before rp_thread quits)
from_dst_file=NULL
[thread got scheduled out]
s->rp_state.from_dst_file==NULL
(skip join() of rp_thread)
migrate_fd_cleanup()
qemu_fclose(s->to_dst_file)
yank_unregister_instance()
assert(yank_find_entry()) <------- crash
It could mostly happen with postcopy, but that shouldn't be required, e.g., I
think it could also trigger with MIGRATION_CAPABILITY_RETURN_PATH set.
It's suspected that above race could be the root cause of a recent (but rare)
migration-test break reported by either Dave or PMM:
https://lore.kernel.org/qemu-devel/YPamXAHwan%2FPPXLf@work-vm/
The issue is: from_dst_file is reset in the rp_thread, so if the thread reset
it to NULL fast enough then the migration thread will assume there's no
rp_thread at all.
This could potentially cause more severe issue (e.g. crash) after the yank code.
Fix it by using a boolean to keep "whether we've created rp_thread".
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
migration/migration.c | 4 +++-
migration/migration.h | 7 +++++++
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/migration/migration.c b/migration/migration.c
index 2d306582eb..21b94f75a3 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2867,6 +2867,7 @@ static int open_return_path_on_source(MigrationState *ms,
qemu_thread_create(&ms->rp_state.rp_thread, "return path",
source_return_path_thread, ms, QEMU_THREAD_JOINABLE);
+ ms->rp_state.rp_thread_created = true;
trace_open_return_path_on_source_continue();
@@ -2891,6 +2892,7 @@ static int await_return_path_close_on_source(MigrationState *ms)
}
trace_await_return_path_close_on_source_joining();
qemu_thread_join(&ms->rp_state.rp_thread);
+ ms->rp_state.rp_thread_created = false;
trace_await_return_path_close_on_source_close();
return ms->rp_state.error;
}
@@ -3170,7 +3172,7 @@ static void migration_completion(MigrationState *s)
* it will wait for the destination to send it's status in
* a SHUT command).
*/
- if (s->rp_state.from_dst_file) {
+ if (s->rp_state.rp_thread_created) {
int rp_error;
trace_migration_return_path_end_before();
rp_error = await_return_path_close_on_source(s);
diff --git a/migration/migration.h b/migration/migration.h
index 2ebb740dfa..c302879fad 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -195,6 +195,13 @@ struct MigrationState {
QEMUFile *from_dst_file;
QemuThread rp_thread;
bool error;
+ /*
+ * We can also check non-zero of rp_thread, but there's no "official"
+ * way to do this, so this bool makes it slightly more elegant.
+ * Checking from_dst_file for this is racy because from_dst_file will
+ * be cleared in the rp_thread!
+ */
+ bool rp_thread_created;
QemuSemaphore rp_sem;
} rp_state;
--
2.31.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 1/5] migration: Fix missing join() of rp_thread
2021-07-21 1:21 ` [PATCH 1/5] migration: Fix missing join() of rp_thread Peter Xu
@ 2021-07-21 9:49 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 17+ messages in thread
From: Dr. David Alan Gilbert @ 2021-07-21 9:49 UTC (permalink / raw)
To: Peter Xu
Cc: Peter Maydell, Lukas Straub, Leonardo Bras Soares Passos,
qemu-devel, Juan Quintela
* Peter Xu (peterx@redhat.com) wrote:
> It's possible that the migration thread skip the join() of the rp_thread in
> below race and crash on src right at finishing migration:
>
> migration_thread rp_thread
> ---------------- ---------
> migration_completion()
> (before rp_thread quits)
> from_dst_file=NULL
> [thread got scheduled out]
> s->rp_state.from_dst_file==NULL
> (skip join() of rp_thread)
> migrate_fd_cleanup()
> qemu_fclose(s->to_dst_file)
> yank_unregister_instance()
> assert(yank_find_entry()) <------- crash
>
> It could mostly happen with postcopy, but that shouldn't be required, e.g., I
> think it could also trigger with MIGRATION_CAPABILITY_RETURN_PATH set.
>
> It's suspected that above race could be the root cause of a recent (but rare)
> migration-test break reported by either Dave or PMM:
>
> https://lore.kernel.org/qemu-devel/YPamXAHwan%2FPPXLf@work-vm/
>
> The issue is: from_dst_file is reset in the rp_thread, so if the thread reset
> it to NULL fast enough then the migration thread will assume there's no
> rp_thread at all.
>
> This could potentially cause more severe issue (e.g. crash) after the yank code.
>
> Fix it by using a boolean to keep "whether we've created rp_thread".
>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
> migration/migration.c | 4 +++-
> migration/migration.h | 7 +++++++
> 2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 2d306582eb..21b94f75a3 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2867,6 +2867,7 @@ static int open_return_path_on_source(MigrationState *ms,
>
> qemu_thread_create(&ms->rp_state.rp_thread, "return path",
> source_return_path_thread, ms, QEMU_THREAD_JOINABLE);
> + ms->rp_state.rp_thread_created = true;
>
> trace_open_return_path_on_source_continue();
>
> @@ -2891,6 +2892,7 @@ static int await_return_path_close_on_source(MigrationState *ms)
> }
> trace_await_return_path_close_on_source_joining();
> qemu_thread_join(&ms->rp_state.rp_thread);
> + ms->rp_state.rp_thread_created = false;
> trace_await_return_path_close_on_source_close();
> return ms->rp_state.error;
> }
> @@ -3170,7 +3172,7 @@ static void migration_completion(MigrationState *s)
> * it will wait for the destination to send it's status in
> * a SHUT command).
> */
> - if (s->rp_state.from_dst_file) {
> + if (s->rp_state.rp_thread_created) {
> int rp_error;
> trace_migration_return_path_end_before();
> rp_error = await_return_path_close_on_source(s);
> diff --git a/migration/migration.h b/migration/migration.h
> index 2ebb740dfa..c302879fad 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -195,6 +195,13 @@ struct MigrationState {
> QEMUFile *from_dst_file;
> QemuThread rp_thread;
> bool error;
> + /*
> + * We can also check non-zero of rp_thread, but there's no "official"
> + * way to do this, so this bool makes it slightly more elegant.
> + * Checking from_dst_file for this is racy because from_dst_file will
> + * be cleared in the rp_thread!
> + */
> + bool rp_thread_created;
> QemuSemaphore rp_sem;
> } rp_state;
>
> --
> 2.31.1
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 2/5] migration: Shutdown src in await_return_path_close_on_source()
2021-07-21 1:21 [PATCH 0/5] migrations: Fix potential rare race of migration-test after yank Peter Xu
2021-07-21 1:21 ` [PATCH 1/5] migration: Fix missing join() of rp_thread Peter Xu
@ 2021-07-21 1:21 ` Peter Xu
2021-07-21 9:55 ` Dr. David Alan Gilbert
2021-07-21 1:21 ` [PATCH 3/5] migration: Introduce migration_ioc_[un]register_yank() Peter Xu
` (2 subsequent siblings)
4 siblings, 1 reply; 17+ messages in thread
From: Peter Xu @ 2021-07-21 1:21 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Lukas Straub, Juan Quintela,
Dr . David Alan Gilbert, peterx, Leonardo Bras Soares Passos
We have a logic in await_return_path_close_on_source() that we will explicitly
shutdown the socket when migration encounters errors. However it could be racy
because from_dst_file could have been reset right after checking it but before
passing it to qemu_file_shutdown() by the rp_thread.
Fix it by shutdown() on the src file instead. Since they must be a pair of
qemu files, shutdown on either of them will work the same.
Since at it, drop the check for from_dst_file directly, which makes the
behavior even more predictable.
Reported-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
migration/migration.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index 21b94f75a3..4f48cde796 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2882,12 +2882,15 @@ static int await_return_path_close_on_source(MigrationState *ms)
* rp_thread will exit, however if there's an error we need to cause
* it to exit.
*/
- if (qemu_file_get_error(ms->to_dst_file) && ms->rp_state.from_dst_file) {
+ if (qemu_file_get_error(ms->to_dst_file)) {
/*
* shutdown(2), if we have it, will cause it to unblock if it's stuck
- * waiting for the destination.
+ * waiting for the destination. We do shutdown on to_dst_file should
+ * also shutdown the from_dst_file as they're in a pair. We explicilty
+ * don't operate on from_dst_file because it's potentially racy
+ * (rp_thread could have reset it in parallel).
*/
- qemu_file_shutdown(ms->rp_state.from_dst_file);
+ qemu_file_shutdown(ms->to_dst_file);
mark_source_rp_bad(ms);
}
trace_await_return_path_close_on_source_joining();
--
2.31.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 2/5] migration: Shutdown src in await_return_path_close_on_source()
2021-07-21 1:21 ` [PATCH 2/5] migration: Shutdown src in await_return_path_close_on_source() Peter Xu
@ 2021-07-21 9:55 ` Dr. David Alan Gilbert
2021-07-21 15:40 ` Peter Xu
2021-07-21 15:57 ` Daniel P. Berrangé
0 siblings, 2 replies; 17+ messages in thread
From: Dr. David Alan Gilbert @ 2021-07-21 9:55 UTC (permalink / raw)
To: Peter Xu
Cc: Peter Maydell, Lukas Straub, Leonardo Bras Soares Passos,
qemu-devel, Juan Quintela
* Peter Xu (peterx@redhat.com) wrote:
> We have a logic in await_return_path_close_on_source() that we will explicitly
> shutdown the socket when migration encounters errors. However it could be racy
> because from_dst_file could have been reset right after checking it but before
> passing it to qemu_file_shutdown() by the rp_thread.
>
> Fix it by shutdown() on the src file instead. Since they must be a pair of
> qemu files, shutdown on either of them will work the same.
>
> Since at it, drop the check for from_dst_file directly, which makes the
> behavior even more predictable.
So while the existing code maybe racy, I'm not sure that this change
keeps the semantics; the channel may well have dup()'d the fd's for the
two directions, and I'm not convinced that a shutdown() on one will
necessarily impact the other; and if the shutdown doesn't happen the
rp_thread might not exit, and we might block on the koin.
Why don't we solve this a different way - how about we move the:
ms->rp_state.from_dst_file = NULL;
qemu_fclose(rp);
out of the source_return_path_thread and put it in
await_return_path_close_on_source, immediately after the join?
Then we *know* that the the rp thread isn't messing with it.
Dave
> Reported-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> migration/migration.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 21b94f75a3..4f48cde796 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2882,12 +2882,15 @@ static int await_return_path_close_on_source(MigrationState *ms)
> * rp_thread will exit, however if there's an error we need to cause
> * it to exit.
> */
> - if (qemu_file_get_error(ms->to_dst_file) && ms->rp_state.from_dst_file) {
> + if (qemu_file_get_error(ms->to_dst_file)) {
> /*
> * shutdown(2), if we have it, will cause it to unblock if it's stuck
> - * waiting for the destination.
> + * waiting for the destination. We do shutdown on to_dst_file should
> + * also shutdown the from_dst_file as they're in a pair. We explicilty
> + * don't operate on from_dst_file because it's potentially racy
> + * (rp_thread could have reset it in parallel).
> */
> - qemu_file_shutdown(ms->rp_state.from_dst_file);
> + qemu_file_shutdown(ms->to_dst_file);
> mark_source_rp_bad(ms);
> }
> trace_await_return_path_close_on_source_joining();
> --
> 2.31.1
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 2/5] migration: Shutdown src in await_return_path_close_on_source()
2021-07-21 9:55 ` Dr. David Alan Gilbert
@ 2021-07-21 15:40 ` Peter Xu
2021-07-21 18:00 ` Dr. David Alan Gilbert
2021-07-21 15:57 ` Daniel P. Berrangé
1 sibling, 1 reply; 17+ messages in thread
From: Peter Xu @ 2021-07-21 15:40 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: Peter Maydell, Lukas Straub, Leonardo Bras Soares Passos,
qemu-devel, Juan Quintela
On Wed, Jul 21, 2021 at 10:55:00AM +0100, Dr. David Alan Gilbert wrote:
> * Peter Xu (peterx@redhat.com) wrote:
> > We have a logic in await_return_path_close_on_source() that we will explicitly
> > shutdown the socket when migration encounters errors. However it could be racy
> > because from_dst_file could have been reset right after checking it but before
> > passing it to qemu_file_shutdown() by the rp_thread.
> >
> > Fix it by shutdown() on the src file instead. Since they must be a pair of
> > qemu files, shutdown on either of them will work the same.
> >
> > Since at it, drop the check for from_dst_file directly, which makes the
> > behavior even more predictable.
>
> So while the existing code maybe racy, I'm not sure that this change
> keeps the semantics; the channel may well have dup()'d the fd's for the
> two directions, and I'm not convinced that a shutdown() on one will
> necessarily impact the other; and if the shutdown doesn't happen the
> rp_thread might not exit, and we might block on the koin.
dup() seems fine as long as the backend file/socket is the same; but I get the
point here, e.g., potentially from/to channels can indeed be different ones.
>
> Why don't we solve this a different way - how about we move the:
> ms->rp_state.from_dst_file = NULL;
> qemu_fclose(rp);
>
> out of the source_return_path_thread and put it in
> await_return_path_close_on_source, immediately after the join?
> Then we *know* that the the rp thread isn't messing with it.
Yes that looks working for this special case of when rp_thread quits.
It's just that it'll make things a bit more complicated, previously
from_dst_file is only reset in rp_thread (for either paused or completed
migration), now we handle "migration completes" a bit different.
This also reminded me that maybe we can also use the qemu_file_lock mutex as we
use to try protect access to to_dst_file, because I think from_dst_file is
potentially racy in the same way. Even if we moved the reset to migration
thread, we still have e.g. migrate_fd_cancel() calling:
if (s->rp_state.from_dst_file) {
qemu_file_shutdown(s->rp_state.from_dst_file);
}
I think that is also racy too when running in the main thread, as either the
migration thread or rp_thread could have reset it right after the check.
So.. maybe I start to use the qemu_file_lock to cover from_dst_file cases too?
Then I'll cover at leasd both migrate_fd_cancel() and this case.
Thanks,
--
Peter Xu
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 2/5] migration: Shutdown src in await_return_path_close_on_source()
2021-07-21 15:40 ` Peter Xu
@ 2021-07-21 18:00 ` Dr. David Alan Gilbert
2021-07-21 18:12 ` Peter Xu
0 siblings, 1 reply; 17+ messages in thread
From: Dr. David Alan Gilbert @ 2021-07-21 18:00 UTC (permalink / raw)
To: Peter Xu
Cc: Peter Maydell, Lukas Straub, Leonardo Bras Soares Passos,
qemu-devel, Juan Quintela
* Peter Xu (peterx@redhat.com) wrote:
> On Wed, Jul 21, 2021 at 10:55:00AM +0100, Dr. David Alan Gilbert wrote:
> > * Peter Xu (peterx@redhat.com) wrote:
> > > We have a logic in await_return_path_close_on_source() that we will explicitly
> > > shutdown the socket when migration encounters errors. However it could be racy
> > > because from_dst_file could have been reset right after checking it but before
> > > passing it to qemu_file_shutdown() by the rp_thread.
> > >
> > > Fix it by shutdown() on the src file instead. Since they must be a pair of
> > > qemu files, shutdown on either of them will work the same.
> > >
> > > Since at it, drop the check for from_dst_file directly, which makes the
> > > behavior even more predictable.
> >
> > So while the existing code maybe racy, I'm not sure that this change
> > keeps the semantics; the channel may well have dup()'d the fd's for the
> > two directions, and I'm not convinced that a shutdown() on one will
> > necessarily impact the other; and if the shutdown doesn't happen the
> > rp_thread might not exit, and we might block on the koin.
>
> dup() seems fine as long as the backend file/socket is the same; but I get the
> point here, e.g., potentially from/to channels can indeed be different ones.
>
> >
> > Why don't we solve this a different way - how about we move the:
> > ms->rp_state.from_dst_file = NULL;
> > qemu_fclose(rp);
> >
> > out of the source_return_path_thread and put it in
> > await_return_path_close_on_source, immediately after the join?
> > Then we *know* that the the rp thread isn't messing with it.
>
> Yes that looks working for this special case of when rp_thread quits.
>
> It's just that it'll make things a bit more complicated, previously
> from_dst_file is only reset in rp_thread (for either paused or completed
> migration), now we handle "migration completes" a bit different.
>
> This also reminded me that maybe we can also use the qemu_file_lock mutex as we
> use to try protect access to to_dst_file, because I think from_dst_file is
> potentially racy in the same way. Even if we moved the reset to migration
> thread, we still have e.g. migrate_fd_cancel() calling:
>
> if (s->rp_state.from_dst_file) {
> qemu_file_shutdown(s->rp_state.from_dst_file);
> }
>
> I think that is also racy too when running in the main thread, as either the
> migration thread or rp_thread could have reset it right after the check.
>
> So.. maybe I start to use the qemu_file_lock to cover from_dst_file cases too?
> Then I'll cover at leasd both migrate_fd_cancel() and this case.
Yes, I think that's best; but try to do as little as possible with the
lock held.
Dave
> Thanks,
>
> --
> Peter Xu
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/5] migration: Shutdown src in await_return_path_close_on_source()
2021-07-21 9:55 ` Dr. David Alan Gilbert
2021-07-21 15:40 ` Peter Xu
@ 2021-07-21 15:57 ` Daniel P. Berrangé
1 sibling, 0 replies; 17+ messages in thread
From: Daniel P. Berrangé @ 2021-07-21 15:57 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: Peter Maydell, Lukas Straub, Juan Quintela, qemu-devel, Peter Xu,
Leonardo Bras Soares Passos
On Wed, Jul 21, 2021 at 10:55:00AM +0100, Dr. David Alan Gilbert wrote:
> * Peter Xu (peterx@redhat.com) wrote:
> > We have a logic in await_return_path_close_on_source() that we will explicitly
> > shutdown the socket when migration encounters errors. However it could be racy
> > because from_dst_file could have been reset right after checking it but before
> > passing it to qemu_file_shutdown() by the rp_thread.
> >
> > Fix it by shutdown() on the src file instead. Since they must be a pair of
> > qemu files, shutdown on either of them will work the same.
> >
> > Since at it, drop the check for from_dst_file directly, which makes the
> > behavior even more predictable.
>
> So while the existing code maybe racy, I'm not sure that this change
> keeps the semantics; the channel may well have dup()'d the fd's for the
> two directions, and I'm not convinced that a shutdown() on one will
> necessarily impact the other; and if the shutdown doesn't happen the
> rp_thread might not exit, and we might block on the koin.
My understanding is that 'shutdown' operation affects the state of
the socket connection. An FD is merely the way a socket is exposed
to userspace. Thus if you have multiple FDs all pointing to the same
underlying socket (thanks to dup()), then I expect that the effects
of 'shutdown' will apply equally to all of the FD copies.
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 :|
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 3/5] migration: Introduce migration_ioc_[un]register_yank()
2021-07-21 1:21 [PATCH 0/5] migrations: Fix potential rare race of migration-test after yank Peter Xu
2021-07-21 1:21 ` [PATCH 1/5] migration: Fix missing join() of rp_thread Peter Xu
2021-07-21 1:21 ` [PATCH 2/5] migration: Shutdown src in await_return_path_close_on_source() Peter Xu
@ 2021-07-21 1:21 ` Peter Xu
2021-07-21 9:58 ` Dr. David Alan Gilbert
2021-07-21 1:21 ` [PATCH 4/5] migration: Teach QEMUFile to be QIOChannel-aware Peter Xu
2021-07-21 1:21 ` [PATCH 5/5] migration: Move the yank unregister of channel_close out Peter Xu
4 siblings, 1 reply; 17+ messages in thread
From: Peter Xu @ 2021-07-21 1:21 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Lukas Straub, Juan Quintela,
Dr . David Alan Gilbert, peterx, Leonardo Bras Soares Passos
There're plenty of places in migration/* that checks against either socket or
tls typed ioc for yank operations. Provide two helpers to hide all these
information.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
migration/channel.c | 15 ++-------------
migration/multifd.c | 8 ++------
migration/qemu-file-channel.c | 8 ++------
migration/yank_functions.c | 28 ++++++++++++++++++++++++++++
migration/yank_functions.h | 2 ++
5 files changed, 36 insertions(+), 25 deletions(-)
diff --git a/migration/channel.c b/migration/channel.c
index 01275a9162..c4fc000a1a 100644
--- a/migration/channel.c
+++ b/migration/channel.c
@@ -44,13 +44,7 @@ void migration_channel_process_incoming(QIOChannel *ioc)
TYPE_QIO_CHANNEL_TLS)) {
migration_tls_channel_process_incoming(s, ioc, &local_err);
} else {
- if (object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET) ||
- object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_TLS)) {
- yank_register_function(MIGRATION_YANK_INSTANCE,
- migration_yank_iochannel,
- QIO_CHANNEL(ioc));
- }
-
+ migration_ioc_register_yank(ioc);
migration_ioc_process_incoming(ioc, &local_err);
}
@@ -94,12 +88,7 @@ void migration_channel_connect(MigrationState *s,
} else {
QEMUFile *f = qemu_fopen_channel_output(ioc);
- if (object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET) ||
- object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_TLS)) {
- yank_register_function(MIGRATION_YANK_INSTANCE,
- migration_yank_iochannel,
- QIO_CHANNEL(ioc));
- }
+ migration_ioc_register_yank(ioc);
qemu_mutex_lock(&s->qemu_file_lock);
s->to_dst_file = f;
diff --git a/migration/multifd.c b/migration/multifd.c
index ab41590e71..377da78f5b 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -987,12 +987,8 @@ int multifd_load_cleanup(Error **errp)
for (i = 0; i < migrate_multifd_channels(); i++) {
MultiFDRecvParams *p = &multifd_recv_state->params[i];
- if ((object_dynamic_cast(OBJECT(p->c), TYPE_QIO_CHANNEL_SOCKET) ||
- object_dynamic_cast(OBJECT(p->c), TYPE_QIO_CHANNEL_TLS))
- && OBJECT(p->c)->ref == 1) {
- yank_unregister_function(MIGRATION_YANK_INSTANCE,
- migration_yank_iochannel,
- QIO_CHANNEL(p->c));
+ if (OBJECT(p->c)->ref == 1) {
+ migration_ioc_unregister_yank(p->c);
}
object_unref(OBJECT(p->c));
diff --git a/migration/qemu-file-channel.c b/migration/qemu-file-channel.c
index fad340ea7a..867a5ed0c3 100644
--- a/migration/qemu-file-channel.c
+++ b/migration/qemu-file-channel.c
@@ -107,12 +107,8 @@ static int channel_close(void *opaque, Error **errp)
int ret;
QIOChannel *ioc = QIO_CHANNEL(opaque);
ret = qio_channel_close(ioc, errp);
- if ((object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET) ||
- object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_TLS))
- && OBJECT(ioc)->ref == 1) {
- yank_unregister_function(MIGRATION_YANK_INSTANCE,
- migration_yank_iochannel,
- QIO_CHANNEL(ioc));
+ if (OBJECT(ioc)->ref == 1) {
+ migration_ioc_unregister_yank(ioc);
}
object_unref(OBJECT(ioc));
return ret;
diff --git a/migration/yank_functions.c b/migration/yank_functions.c
index 96c90e17dc..23697173ae 100644
--- a/migration/yank_functions.c
+++ b/migration/yank_functions.c
@@ -11,6 +11,9 @@
#include "qapi/error.h"
#include "io/channel.h"
#include "yank_functions.h"
+#include "qemu/yank.h"
+#include "io/channel-socket.h"
+#include "io/channel-tls.h"
void migration_yank_iochannel(void *opaque)
{
@@ -18,3 +21,28 @@ void migration_yank_iochannel(void *opaque)
qio_channel_shutdown(ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
}
+
+/* Return whether yank is supported on this ioc */
+static bool migration_ioc_yank_supported(QIOChannel *ioc)
+{
+ return object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET) ||
+ object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_TLS);
+}
+
+void migration_ioc_register_yank(QIOChannel *ioc)
+{
+ if (migration_ioc_yank_supported(ioc)) {
+ yank_register_function(MIGRATION_YANK_INSTANCE,
+ migration_yank_iochannel,
+ QIO_CHANNEL(ioc));
+ }
+}
+
+void migration_ioc_unregister_yank(QIOChannel *ioc)
+{
+ if (migration_ioc_yank_supported(ioc)) {
+ yank_unregister_function(MIGRATION_YANK_INSTANCE,
+ migration_yank_iochannel,
+ QIO_CHANNEL(ioc));
+ }
+}
diff --git a/migration/yank_functions.h b/migration/yank_functions.h
index 055ea22523..74c7f18c91 100644
--- a/migration/yank_functions.h
+++ b/migration/yank_functions.h
@@ -15,3 +15,5 @@
* @opaque: QIOChannel to shutdown
*/
void migration_yank_iochannel(void *opaque);
+void migration_ioc_register_yank(QIOChannel *ioc);
+void migration_ioc_unregister_yank(QIOChannel *ioc);
--
2.31.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 3/5] migration: Introduce migration_ioc_[un]register_yank()
2021-07-21 1:21 ` [PATCH 3/5] migration: Introduce migration_ioc_[un]register_yank() Peter Xu
@ 2021-07-21 9:58 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 17+ messages in thread
From: Dr. David Alan Gilbert @ 2021-07-21 9:58 UTC (permalink / raw)
To: Peter Xu
Cc: Peter Maydell, Lukas Straub, Leonardo Bras Soares Passos,
qemu-devel, Juan Quintela
* Peter Xu (peterx@redhat.com) wrote:
> There're plenty of places in migration/* that checks against either socket or
> tls typed ioc for yank operations. Provide two helpers to hide all these
> information.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
> migration/channel.c | 15 ++-------------
> migration/multifd.c | 8 ++------
> migration/qemu-file-channel.c | 8 ++------
> migration/yank_functions.c | 28 ++++++++++++++++++++++++++++
> migration/yank_functions.h | 2 ++
> 5 files changed, 36 insertions(+), 25 deletions(-)
>
> diff --git a/migration/channel.c b/migration/channel.c
> index 01275a9162..c4fc000a1a 100644
> --- a/migration/channel.c
> +++ b/migration/channel.c
> @@ -44,13 +44,7 @@ void migration_channel_process_incoming(QIOChannel *ioc)
> TYPE_QIO_CHANNEL_TLS)) {
> migration_tls_channel_process_incoming(s, ioc, &local_err);
> } else {
> - if (object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET) ||
> - object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_TLS)) {
> - yank_register_function(MIGRATION_YANK_INSTANCE,
> - migration_yank_iochannel,
> - QIO_CHANNEL(ioc));
> - }
> -
> + migration_ioc_register_yank(ioc);
> migration_ioc_process_incoming(ioc, &local_err);
> }
>
> @@ -94,12 +88,7 @@ void migration_channel_connect(MigrationState *s,
> } else {
> QEMUFile *f = qemu_fopen_channel_output(ioc);
>
> - if (object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET) ||
> - object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_TLS)) {
> - yank_register_function(MIGRATION_YANK_INSTANCE,
> - migration_yank_iochannel,
> - QIO_CHANNEL(ioc));
> - }
> + migration_ioc_register_yank(ioc);
>
> qemu_mutex_lock(&s->qemu_file_lock);
> s->to_dst_file = f;
> diff --git a/migration/multifd.c b/migration/multifd.c
> index ab41590e71..377da78f5b 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -987,12 +987,8 @@ int multifd_load_cleanup(Error **errp)
> for (i = 0; i < migrate_multifd_channels(); i++) {
> MultiFDRecvParams *p = &multifd_recv_state->params[i];
>
> - if ((object_dynamic_cast(OBJECT(p->c), TYPE_QIO_CHANNEL_SOCKET) ||
> - object_dynamic_cast(OBJECT(p->c), TYPE_QIO_CHANNEL_TLS))
> - && OBJECT(p->c)->ref == 1) {
> - yank_unregister_function(MIGRATION_YANK_INSTANCE,
> - migration_yank_iochannel,
> - QIO_CHANNEL(p->c));
> + if (OBJECT(p->c)->ref == 1) {
> + migration_ioc_unregister_yank(p->c);
> }
>
> object_unref(OBJECT(p->c));
> diff --git a/migration/qemu-file-channel.c b/migration/qemu-file-channel.c
> index fad340ea7a..867a5ed0c3 100644
> --- a/migration/qemu-file-channel.c
> +++ b/migration/qemu-file-channel.c
> @@ -107,12 +107,8 @@ static int channel_close(void *opaque, Error **errp)
> int ret;
> QIOChannel *ioc = QIO_CHANNEL(opaque);
> ret = qio_channel_close(ioc, errp);
> - if ((object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET) ||
> - object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_TLS))
> - && OBJECT(ioc)->ref == 1) {
> - yank_unregister_function(MIGRATION_YANK_INSTANCE,
> - migration_yank_iochannel,
> - QIO_CHANNEL(ioc));
> + if (OBJECT(ioc)->ref == 1) {
> + migration_ioc_unregister_yank(ioc);
> }
> object_unref(OBJECT(ioc));
> return ret;
> diff --git a/migration/yank_functions.c b/migration/yank_functions.c
> index 96c90e17dc..23697173ae 100644
> --- a/migration/yank_functions.c
> +++ b/migration/yank_functions.c
> @@ -11,6 +11,9 @@
> #include "qapi/error.h"
> #include "io/channel.h"
> #include "yank_functions.h"
> +#include "qemu/yank.h"
> +#include "io/channel-socket.h"
> +#include "io/channel-tls.h"
>
> void migration_yank_iochannel(void *opaque)
> {
> @@ -18,3 +21,28 @@ void migration_yank_iochannel(void *opaque)
>
> qio_channel_shutdown(ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
> }
> +
> +/* Return whether yank is supported on this ioc */
> +static bool migration_ioc_yank_supported(QIOChannel *ioc)
> +{
> + return object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET) ||
> + object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_TLS);
> +}
> +
> +void migration_ioc_register_yank(QIOChannel *ioc)
> +{
> + if (migration_ioc_yank_supported(ioc)) {
> + yank_register_function(MIGRATION_YANK_INSTANCE,
> + migration_yank_iochannel,
> + QIO_CHANNEL(ioc));
> + }
> +}
> +
> +void migration_ioc_unregister_yank(QIOChannel *ioc)
> +{
> + if (migration_ioc_yank_supported(ioc)) {
> + yank_unregister_function(MIGRATION_YANK_INSTANCE,
> + migration_yank_iochannel,
> + QIO_CHANNEL(ioc));
> + }
> +}
> diff --git a/migration/yank_functions.h b/migration/yank_functions.h
> index 055ea22523..74c7f18c91 100644
> --- a/migration/yank_functions.h
> +++ b/migration/yank_functions.h
> @@ -15,3 +15,5 @@
> * @opaque: QIOChannel to shutdown
> */
> void migration_yank_iochannel(void *opaque);
> +void migration_ioc_register_yank(QIOChannel *ioc);
> +void migration_ioc_unregister_yank(QIOChannel *ioc);
> --
> 2.31.1
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 4/5] migration: Teach QEMUFile to be QIOChannel-aware
2021-07-21 1:21 [PATCH 0/5] migrations: Fix potential rare race of migration-test after yank Peter Xu
` (2 preceding siblings ...)
2021-07-21 1:21 ` [PATCH 3/5] migration: Introduce migration_ioc_[un]register_yank() Peter Xu
@ 2021-07-21 1:21 ` Peter Xu
2021-07-21 10:27 ` Dr. David Alan Gilbert
2021-07-21 1:21 ` [PATCH 5/5] migration: Move the yank unregister of channel_close out Peter Xu
4 siblings, 1 reply; 17+ messages in thread
From: Peter Xu @ 2021-07-21 1:21 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Lukas Straub, Juan Quintela,
Dr . David Alan Gilbert, peterx, Leonardo Bras Soares Passos
migration uses QIOChannel typed qemufiles. In follow up patches, we'll need
the capability to identify this fact, so that we can get the backing QIOChannel
from a QEMUFile.
We can also define types for QEMUFile but so far since we only need to be able
to identify QIOChannel, introduce a boolean which is simpler.
No functional change.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
migration/qemu-file-channel.c | 4 ++--
migration/qemu-file.c | 5 ++++-
migration/qemu-file.h | 2 +-
migration/ram.c | 2 +-
migration/savevm.c | 4 ++--
5 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/migration/qemu-file-channel.c b/migration/qemu-file-channel.c
index 867a5ed0c3..2f8b1fcd46 100644
--- a/migration/qemu-file-channel.c
+++ b/migration/qemu-file-channel.c
@@ -187,11 +187,11 @@ static const QEMUFileOps channel_output_ops = {
QEMUFile *qemu_fopen_channel_input(QIOChannel *ioc)
{
object_ref(OBJECT(ioc));
- return qemu_fopen_ops(ioc, &channel_input_ops);
+ return qemu_fopen_ops(ioc, &channel_input_ops, true);
}
QEMUFile *qemu_fopen_channel_output(QIOChannel *ioc)
{
object_ref(OBJECT(ioc));
- return qemu_fopen_ops(ioc, &channel_output_ops);
+ return qemu_fopen_ops(ioc, &channel_output_ops, true);
}
diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index 1eacf9e831..ada58c94dd 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -55,6 +55,8 @@ struct QEMUFile {
Error *last_error_obj;
/* has the file has been shutdown */
bool shutdown;
+ /* Whether opaque points to a QIOChannel */
+ bool has_ioc;
};
/*
@@ -101,7 +103,7 @@ bool qemu_file_mode_is_not_valid(const char *mode)
return false;
}
-QEMUFile *qemu_fopen_ops(void *opaque, const QEMUFileOps *ops)
+QEMUFile *qemu_fopen_ops(void *opaque, const QEMUFileOps *ops, bool has_ioc)
{
QEMUFile *f;
@@ -109,6 +111,7 @@ QEMUFile *qemu_fopen_ops(void *opaque, const QEMUFileOps *ops)
f->opaque = opaque;
f->ops = ops;
+ f->has_ioc = has_ioc;
return f;
}
diff --git a/migration/qemu-file.h b/migration/qemu-file.h
index a9b6d6ccb7..80d0e79fd1 100644
--- a/migration/qemu-file.h
+++ b/migration/qemu-file.h
@@ -119,7 +119,7 @@ typedef struct QEMUFileHooks {
QEMURamSaveFunc *save_page;
} QEMUFileHooks;
-QEMUFile *qemu_fopen_ops(void *opaque, const QEMUFileOps *ops);
+QEMUFile *qemu_fopen_ops(void *opaque, const QEMUFileOps *ops, bool has_ioc);
void qemu_file_set_hooks(QEMUFile *f, const QEMUFileHooks *hooks);
int qemu_get_fd(QEMUFile *f);
int qemu_fclose(QEMUFile *f);
diff --git a/migration/ram.c b/migration/ram.c
index b5fc454b2f..f2a86f9971 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -550,7 +550,7 @@ static int compress_threads_save_setup(void)
/* comp_param[i].file is just used as a dummy buffer to save data,
* set its ops to empty.
*/
- comp_param[i].file = qemu_fopen_ops(NULL, &empty_ops);
+ comp_param[i].file = qemu_fopen_ops(NULL, &empty_ops, false);
comp_param[i].done = true;
comp_param[i].quit = false;
qemu_mutex_init(&comp_param[i].mutex);
diff --git a/migration/savevm.c b/migration/savevm.c
index 72848b946c..96b5e5d639 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -168,9 +168,9 @@ static const QEMUFileOps bdrv_write_ops = {
static QEMUFile *qemu_fopen_bdrv(BlockDriverState *bs, int is_writable)
{
if (is_writable) {
- return qemu_fopen_ops(bs, &bdrv_write_ops);
+ return qemu_fopen_ops(bs, &bdrv_write_ops, false);
}
- return qemu_fopen_ops(bs, &bdrv_read_ops);
+ return qemu_fopen_ops(bs, &bdrv_read_ops, false);
}
--
2.31.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 4/5] migration: Teach QEMUFile to be QIOChannel-aware
2021-07-21 1:21 ` [PATCH 4/5] migration: Teach QEMUFile to be QIOChannel-aware Peter Xu
@ 2021-07-21 10:27 ` Dr. David Alan Gilbert
2021-07-21 10:57 ` Daniel P. Berrangé
0 siblings, 1 reply; 17+ messages in thread
From: Dr. David Alan Gilbert @ 2021-07-21 10:27 UTC (permalink / raw)
To: Peter Xu
Cc: Peter Maydell, Lukas Straub, Leonardo Bras Soares Passos,
qemu-devel, Juan Quintela
* Peter Xu (peterx@redhat.com) wrote:
> migration uses QIOChannel typed qemufiles. In follow up patches, we'll need
> the capability to identify this fact, so that we can get the backing QIOChannel
> from a QEMUFile.
>
> We can also define types for QEMUFile but so far since we only need to be able
> to identify QIOChannel, introduce a boolean which is simpler.
>
> No functional change.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
This is messy but I can't see another quick way; the better way would be
to add an OBJECT or QIOCHannel wrapper for BlockDriverState.
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
> migration/qemu-file-channel.c | 4 ++--
> migration/qemu-file.c | 5 ++++-
> migration/qemu-file.h | 2 +-
> migration/ram.c | 2 +-
> migration/savevm.c | 4 ++--
> 5 files changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/migration/qemu-file-channel.c b/migration/qemu-file-channel.c
> index 867a5ed0c3..2f8b1fcd46 100644
> --- a/migration/qemu-file-channel.c
> +++ b/migration/qemu-file-channel.c
> @@ -187,11 +187,11 @@ static const QEMUFileOps channel_output_ops = {
> QEMUFile *qemu_fopen_channel_input(QIOChannel *ioc)
> {
> object_ref(OBJECT(ioc));
> - return qemu_fopen_ops(ioc, &channel_input_ops);
> + return qemu_fopen_ops(ioc, &channel_input_ops, true);
> }
>
> QEMUFile *qemu_fopen_channel_output(QIOChannel *ioc)
> {
> object_ref(OBJECT(ioc));
> - return qemu_fopen_ops(ioc, &channel_output_ops);
> + return qemu_fopen_ops(ioc, &channel_output_ops, true);
> }
> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> index 1eacf9e831..ada58c94dd 100644
> --- a/migration/qemu-file.c
> +++ b/migration/qemu-file.c
> @@ -55,6 +55,8 @@ struct QEMUFile {
> Error *last_error_obj;
> /* has the file has been shutdown */
> bool shutdown;
> + /* Whether opaque points to a QIOChannel */
> + bool has_ioc;
> };
>
> /*
> @@ -101,7 +103,7 @@ bool qemu_file_mode_is_not_valid(const char *mode)
> return false;
> }
>
> -QEMUFile *qemu_fopen_ops(void *opaque, const QEMUFileOps *ops)
> +QEMUFile *qemu_fopen_ops(void *opaque, const QEMUFileOps *ops, bool has_ioc)
> {
> QEMUFile *f;
>
> @@ -109,6 +111,7 @@ QEMUFile *qemu_fopen_ops(void *opaque, const QEMUFileOps *ops)
>
> f->opaque = opaque;
> f->ops = ops;
> + f->has_ioc = has_ioc;
> return f;
> }
>
> diff --git a/migration/qemu-file.h b/migration/qemu-file.h
> index a9b6d6ccb7..80d0e79fd1 100644
> --- a/migration/qemu-file.h
> +++ b/migration/qemu-file.h
> @@ -119,7 +119,7 @@ typedef struct QEMUFileHooks {
> QEMURamSaveFunc *save_page;
> } QEMUFileHooks;
>
> -QEMUFile *qemu_fopen_ops(void *opaque, const QEMUFileOps *ops);
> +QEMUFile *qemu_fopen_ops(void *opaque, const QEMUFileOps *ops, bool has_ioc);
> void qemu_file_set_hooks(QEMUFile *f, const QEMUFileHooks *hooks);
> int qemu_get_fd(QEMUFile *f);
> int qemu_fclose(QEMUFile *f);
> diff --git a/migration/ram.c b/migration/ram.c
> index b5fc454b2f..f2a86f9971 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -550,7 +550,7 @@ static int compress_threads_save_setup(void)
> /* comp_param[i].file is just used as a dummy buffer to save data,
> * set its ops to empty.
> */
> - comp_param[i].file = qemu_fopen_ops(NULL, &empty_ops);
> + comp_param[i].file = qemu_fopen_ops(NULL, &empty_ops, false);
> comp_param[i].done = true;
> comp_param[i].quit = false;
> qemu_mutex_init(&comp_param[i].mutex);
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 72848b946c..96b5e5d639 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -168,9 +168,9 @@ static const QEMUFileOps bdrv_write_ops = {
> static QEMUFile *qemu_fopen_bdrv(BlockDriverState *bs, int is_writable)
> {
> if (is_writable) {
> - return qemu_fopen_ops(bs, &bdrv_write_ops);
> + return qemu_fopen_ops(bs, &bdrv_write_ops, false);
> }
> - return qemu_fopen_ops(bs, &bdrv_read_ops);
> + return qemu_fopen_ops(bs, &bdrv_read_ops, false);
> }
>
>
> --
> 2.31.1
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 4/5] migration: Teach QEMUFile to be QIOChannel-aware
2021-07-21 10:27 ` Dr. David Alan Gilbert
@ 2021-07-21 10:57 ` Daniel P. Berrangé
0 siblings, 0 replies; 17+ messages in thread
From: Daniel P. Berrangé @ 2021-07-21 10:57 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: Peter Maydell, Lukas Straub, Juan Quintela, qemu-devel, Peter Xu,
Leonardo Bras Soares Passos
On Wed, Jul 21, 2021 at 11:27:44AM +0100, Dr. David Alan Gilbert wrote:
> * Peter Xu (peterx@redhat.com) wrote:
> > migration uses QIOChannel typed qemufiles. In follow up patches, we'll need
> > the capability to identify this fact, so that we can get the backing QIOChannel
> > from a QEMUFile.
> >
> > We can also define types for QEMUFile but so far since we only need to be able
> > to identify QIOChannel, introduce a boolean which is simpler.
> >
> > No functional change.
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
>
> This is messy but I can't see another quick way; the better way would be
> to add an OBJECT or QIOCHannel wrapper for BlockDriverState.
I looked at making a QIOChannel for BlockDriverState but it was not
as easy as it might seem. The problem is that the QEMUFile
get_buffer / write_buffer methods take a offset at which the
I/O operation is required to be applied.
For the existing QIOChannel impl for migration, we simply ignore
the 'pos' argument entirely, since it is irrelevant for the main
migration channel doing streaming.
For a BlockDriverState based impl though I think we need to
honour "pos" in some manner.
I think it ought to be possible to rewrite the savevm code
so that it uses 'seek' in the few places it needs to, and
then we can drop "pos" from get_buffer/write_buffer, but
that requires careful consideration.
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 :|
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 5/5] migration: Move the yank unregister of channel_close out
2021-07-21 1:21 [PATCH 0/5] migrations: Fix potential rare race of migration-test after yank Peter Xu
` (3 preceding siblings ...)
2021-07-21 1:21 ` [PATCH 4/5] migration: Teach QEMUFile to be QIOChannel-aware Peter Xu
@ 2021-07-21 1:21 ` Peter Xu
2021-07-21 10:39 ` Dr. David Alan Gilbert
4 siblings, 1 reply; 17+ messages in thread
From: Peter Xu @ 2021-07-21 1:21 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Lukas Straub, Juan Quintela,
Dr . David Alan Gilbert, peterx, Leonardo Bras Soares Passos
It's efficient, but hackish to call yank unregister calls in channel_close(),
especially it'll be hard to debug when qemu crashed with some yank function
leaked.
Remove that hack, but instead explicitly unregister yank functions at the
places where needed, they are:
(on src)
- migrate_fd_cleanup
- postcopy_pause
(on dst)
- migration_incoming_state_destroy
- postcopy_pause_incoming
Some small helpers are introduced to achieve this task. One of them is called
migration_file_get_ioc(), which tries to fetch the ioc out of the qemu file.
It's a bit tricky because qemufile is also used for savevm/loadvm. We need to
check for NULL to bypass those. Please see comment above that helper for more
information.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
migration/migration.c | 5 +++++
migration/qemu-file-channel.c | 3 ---
migration/qemu-file.c | 12 ++++++++++++
migration/qemu-file.h | 2 ++
migration/savevm.c | 7 +++++++
migration/yank_functions.c | 14 ++++++++++++++
migration/yank_functions.h | 1 +
7 files changed, 41 insertions(+), 3 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index 4f48cde796..65b8c2eb52 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -59,6 +59,7 @@
#include "multifd.h"
#include "qemu/yank.h"
#include "sysemu/cpus.h"
+#include "yank_functions.h"
#define MAX_THROTTLE (128 << 20) /* Migration transfer speed throttling */
@@ -273,6 +274,7 @@ void migration_incoming_state_destroy(void)
}
if (mis->from_src_file) {
+ migration_ioc_unregister_yank_from_file(mis->from_src_file);
qemu_fclose(mis->from_src_file);
mis->from_src_file = NULL;
}
@@ -1811,6 +1813,7 @@ static void migrate_fd_cleanup(MigrationState *s)
* Close the file handle without the lock to make sure the
* critical section won't block for long.
*/
+ migration_ioc_unregister_yank_from_file(tmp);
qemu_fclose(tmp);
}
@@ -3337,6 +3340,8 @@ static MigThrError postcopy_pause(MigrationState *s)
/* Current channel is possibly broken. Release it. */
assert(s->to_dst_file);
+ /* Unregister yank for current channel */
+ migration_ioc_unregister_yank_from_file(s->to_dst_file);
qemu_mutex_lock(&s->qemu_file_lock);
file = s->to_dst_file;
s->to_dst_file = NULL;
diff --git a/migration/qemu-file-channel.c b/migration/qemu-file-channel.c
index 2f8b1fcd46..bb5a5752df 100644
--- a/migration/qemu-file-channel.c
+++ b/migration/qemu-file-channel.c
@@ -107,9 +107,6 @@ static int channel_close(void *opaque, Error **errp)
int ret;
QIOChannel *ioc = QIO_CHANNEL(opaque);
ret = qio_channel_close(ioc, errp);
- if (OBJECT(ioc)->ref == 1) {
- migration_ioc_unregister_yank(ioc);
- }
object_unref(OBJECT(ioc));
return ret;
}
diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index ada58c94dd..b32ff35e73 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -854,3 +854,15 @@ void qemu_file_set_blocking(QEMUFile *f, bool block)
f->ops->set_blocking(f->opaque, block, NULL);
}
}
+
+/*
+ * Return the ioc object if it's a migration channel. Note: it can return NULL
+ * for callers passing in a non-migration qemufile. E.g. see qemu_fopen_bdrv()
+ * and its usage in e.g. load_snapshot(). So we need to check against NULL
+ * before using it. If without the check, migration_incoming_state_destroy()
+ * could fail for load_snapshot().
+ */
+QIOChannel *migration_file_get_ioc(QEMUFile *file)
+{
+ return file->has_ioc ? QIO_CHANNEL(file->opaque) : NULL;
+}
diff --git a/migration/qemu-file.h b/migration/qemu-file.h
index 80d0e79fd1..59f3f78e8b 100644
--- a/migration/qemu-file.h
+++ b/migration/qemu-file.h
@@ -27,6 +27,7 @@
#include <zlib.h>
#include "exec/cpu-common.h"
+#include "io/channel.h"
/* Read a chunk of data from a file at the given position. The pos argument
* can be ignored if the file is only be used for streaming. The number of
@@ -179,5 +180,6 @@ void ram_control_load_hook(QEMUFile *f, uint64_t flags, void *data);
size_t ram_control_save_page(QEMUFile *f, ram_addr_t block_offset,
ram_addr_t offset, size_t size,
uint64_t *bytes_sent);
+QIOChannel *migration_file_get_ioc(QEMUFile *file);
#endif
diff --git a/migration/savevm.c b/migration/savevm.c
index 96b5e5d639..7b7b64bd13 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -65,6 +65,7 @@
#include "qemu/bitmap.h"
#include "net/announce.h"
#include "qemu/yank.h"
+#include "yank_functions.h"
const unsigned int postcopy_ram_discard_version;
@@ -2568,6 +2569,12 @@ static bool postcopy_pause_incoming(MigrationIncomingState *mis)
/* Clear the triggered bit to allow one recovery */
mis->postcopy_recover_triggered = false;
+ /*
+ * Unregister yank with either from/to src would work, since ioc behind it
+ * is the same
+ */
+ migration_ioc_unregister_yank_from_file(mis->from_src_file);
+
assert(mis->from_src_file);
qemu_file_shutdown(mis->from_src_file);
qemu_fclose(mis->from_src_file);
diff --git a/migration/yank_functions.c b/migration/yank_functions.c
index 23697173ae..1f35ba3512 100644
--- a/migration/yank_functions.c
+++ b/migration/yank_functions.c
@@ -14,6 +14,7 @@
#include "qemu/yank.h"
#include "io/channel-socket.h"
#include "io/channel-tls.h"
+#include "qemu-file.h"
void migration_yank_iochannel(void *opaque)
{
@@ -46,3 +47,16 @@ void migration_ioc_unregister_yank(QIOChannel *ioc)
QIO_CHANNEL(ioc));
}
}
+
+void migration_ioc_unregister_yank_from_file(QEMUFile *file)
+{
+ QIOChannel *ioc = migration_file_get_ioc(file);
+
+ if (ioc) {
+ /*
+ * For migration qemufiles, we'll always reach here. Though we'll skip
+ * calls from e.g. savevm/loadvm as they don't use yank.
+ */
+ migration_ioc_unregister_yank(ioc);
+ }
+}
diff --git a/migration/yank_functions.h b/migration/yank_functions.h
index 74c7f18c91..a7577955ed 100644
--- a/migration/yank_functions.h
+++ b/migration/yank_functions.h
@@ -17,3 +17,4 @@
void migration_yank_iochannel(void *opaque);
void migration_ioc_register_yank(QIOChannel *ioc);
void migration_ioc_unregister_yank(QIOChannel *ioc);
+void migration_ioc_unregister_yank_from_file(QEMUFile *file);
--
2.31.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 5/5] migration: Move the yank unregister of channel_close out
2021-07-21 1:21 ` [PATCH 5/5] migration: Move the yank unregister of channel_close out Peter Xu
@ 2021-07-21 10:39 ` Dr. David Alan Gilbert
2021-07-21 15:45 ` Peter Xu
0 siblings, 1 reply; 17+ messages in thread
From: Dr. David Alan Gilbert @ 2021-07-21 10:39 UTC (permalink / raw)
To: Peter Xu
Cc: Peter Maydell, Lukas Straub, Leonardo Bras Soares Passos,
qemu-devel, Juan Quintela
* Peter Xu (peterx@redhat.com) wrote:
> It's efficient, but hackish to call yank unregister calls in channel_close(),
> especially it'll be hard to debug when qemu crashed with some yank function
> leaked.
>
> Remove that hack, but instead explicitly unregister yank functions at the
> places where needed, they are:
>
> (on src)
> - migrate_fd_cleanup
> - postcopy_pause
>
> (on dst)
> - migration_incoming_state_destroy
> - postcopy_pause_incoming
>
> Some small helpers are introduced to achieve this task. One of them is called
> migration_file_get_ioc(), which tries to fetch the ioc out of the qemu file.
> It's a bit tricky because qemufile is also used for savevm/loadvm. We need to
> check for NULL to bypass those. Please see comment above that helper for more
> information.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
Yes
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
although....
> ---
> migration/migration.c | 5 +++++
> migration/qemu-file-channel.c | 3 ---
> migration/qemu-file.c | 12 ++++++++++++
> migration/qemu-file.h | 2 ++
> migration/savevm.c | 7 +++++++
> migration/yank_functions.c | 14 ++++++++++++++
> migration/yank_functions.h | 1 +
> 7 files changed, 41 insertions(+), 3 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 4f48cde796..65b8c2eb52 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -59,6 +59,7 @@
> #include "multifd.h"
> #include "qemu/yank.h"
> #include "sysemu/cpus.h"
> +#include "yank_functions.h"
>
> #define MAX_THROTTLE (128 << 20) /* Migration transfer speed throttling */
>
> @@ -273,6 +274,7 @@ void migration_incoming_state_destroy(void)
> }
>
> if (mis->from_src_file) {
> + migration_ioc_unregister_yank_from_file(mis->from_src_file);
> qemu_fclose(mis->from_src_file);
> mis->from_src_file = NULL;
> }
> @@ -1811,6 +1813,7 @@ static void migrate_fd_cleanup(MigrationState *s)
> * Close the file handle without the lock to make sure the
> * critical section won't block for long.
> */
> + migration_ioc_unregister_yank_from_file(tmp);
> qemu_fclose(tmp);
> }
>
> @@ -3337,6 +3340,8 @@ static MigThrError postcopy_pause(MigrationState *s)
>
> /* Current channel is possibly broken. Release it. */
> assert(s->to_dst_file);
> + /* Unregister yank for current channel */
> + migration_ioc_unregister_yank_from_file(s->to_dst_file);
> qemu_mutex_lock(&s->qemu_file_lock);
> file = s->to_dst_file;
> s->to_dst_file = NULL;
> diff --git a/migration/qemu-file-channel.c b/migration/qemu-file-channel.c
> index 2f8b1fcd46..bb5a5752df 100644
> --- a/migration/qemu-file-channel.c
> +++ b/migration/qemu-file-channel.c
> @@ -107,9 +107,6 @@ static int channel_close(void *opaque, Error **errp)
> int ret;
> QIOChannel *ioc = QIO_CHANNEL(opaque);
> ret = qio_channel_close(ioc, errp);
> - if (OBJECT(ioc)->ref == 1) {
> - migration_ioc_unregister_yank(ioc);
> - }
> object_unref(OBJECT(ioc));
> return ret;
> }
> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> index ada58c94dd..b32ff35e73 100644
> --- a/migration/qemu-file.c
> +++ b/migration/qemu-file.c
> @@ -854,3 +854,15 @@ void qemu_file_set_blocking(QEMUFile *f, bool block)
> f->ops->set_blocking(f->opaque, block, NULL);
> }
> }
> +
> +/*
> + * Return the ioc object if it's a migration channel. Note: it can return NULL
> + * for callers passing in a non-migration qemufile. E.g. see qemu_fopen_bdrv()
> + * and its usage in e.g. load_snapshot(). So we need to check against NULL
> + * before using it. If without the check, migration_incoming_state_destroy()
> + * could fail for load_snapshot().
> + */
> +QIOChannel *migration_file_get_ioc(QEMUFile *file)
> +{
> + return file->has_ioc ? QIO_CHANNEL(file->opaque) : NULL;
> +}
Do you think this should go in the previous patch where you created
has_ioc? Also the name is weird, qemu_file is probably right for
everything in here.
Dave
> diff --git a/migration/qemu-file.h b/migration/qemu-file.h
> index 80d0e79fd1..59f3f78e8b 100644
> --- a/migration/qemu-file.h
> +++ b/migration/qemu-file.h
> @@ -27,6 +27,7 @@
>
> #include <zlib.h>
> #include "exec/cpu-common.h"
> +#include "io/channel.h"
>
> /* Read a chunk of data from a file at the given position. The pos argument
> * can be ignored if the file is only be used for streaming. The number of
> @@ -179,5 +180,6 @@ void ram_control_load_hook(QEMUFile *f, uint64_t flags, void *data);
> size_t ram_control_save_page(QEMUFile *f, ram_addr_t block_offset,
> ram_addr_t offset, size_t size,
> uint64_t *bytes_sent);
> +QIOChannel *migration_file_get_ioc(QEMUFile *file);
>
> #endif
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 96b5e5d639..7b7b64bd13 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -65,6 +65,7 @@
> #include "qemu/bitmap.h"
> #include "net/announce.h"
> #include "qemu/yank.h"
> +#include "yank_functions.h"
>
> const unsigned int postcopy_ram_discard_version;
>
> @@ -2568,6 +2569,12 @@ static bool postcopy_pause_incoming(MigrationIncomingState *mis)
> /* Clear the triggered bit to allow one recovery */
> mis->postcopy_recover_triggered = false;
>
> + /*
> + * Unregister yank with either from/to src would work, since ioc behind it
> + * is the same
> + */
> + migration_ioc_unregister_yank_from_file(mis->from_src_file);
> +
> assert(mis->from_src_file);
> qemu_file_shutdown(mis->from_src_file);
> qemu_fclose(mis->from_src_file);
> diff --git a/migration/yank_functions.c b/migration/yank_functions.c
> index 23697173ae..1f35ba3512 100644
> --- a/migration/yank_functions.c
> +++ b/migration/yank_functions.c
> @@ -14,6 +14,7 @@
> #include "qemu/yank.h"
> #include "io/channel-socket.h"
> #include "io/channel-tls.h"
> +#include "qemu-file.h"
>
> void migration_yank_iochannel(void *opaque)
> {
> @@ -46,3 +47,16 @@ void migration_ioc_unregister_yank(QIOChannel *ioc)
> QIO_CHANNEL(ioc));
> }
> }
> +
> +void migration_ioc_unregister_yank_from_file(QEMUFile *file)
> +{
> + QIOChannel *ioc = migration_file_get_ioc(file);
> +
> + if (ioc) {
> + /*
> + * For migration qemufiles, we'll always reach here. Though we'll skip
> + * calls from e.g. savevm/loadvm as they don't use yank.
> + */
> + migration_ioc_unregister_yank(ioc);
> + }
> +}
> diff --git a/migration/yank_functions.h b/migration/yank_functions.h
> index 74c7f18c91..a7577955ed 100644
> --- a/migration/yank_functions.h
> +++ b/migration/yank_functions.h
> @@ -17,3 +17,4 @@
> void migration_yank_iochannel(void *opaque);
> void migration_ioc_register_yank(QIOChannel *ioc);
> void migration_ioc_unregister_yank(QIOChannel *ioc);
> +void migration_ioc_unregister_yank_from_file(QEMUFile *file);
> --
> 2.31.1
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 5/5] migration: Move the yank unregister of channel_close out
2021-07-21 10:39 ` Dr. David Alan Gilbert
@ 2021-07-21 15:45 ` Peter Xu
0 siblings, 0 replies; 17+ messages in thread
From: Peter Xu @ 2021-07-21 15:45 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: Peter Maydell, Lukas Straub, Leonardo Bras Soares Passos,
qemu-devel, Juan Quintela
On Wed, Jul 21, 2021 at 11:39:37AM +0100, Dr. David Alan Gilbert wrote:
> > +/*
> > + * Return the ioc object if it's a migration channel. Note: it can return NULL
> > + * for callers passing in a non-migration qemufile. E.g. see qemu_fopen_bdrv()
> > + * and its usage in e.g. load_snapshot(). So we need to check against NULL
> > + * before using it. If without the check, migration_incoming_state_destroy()
> > + * could fail for load_snapshot().
> > + */
> > +QIOChannel *migration_file_get_ioc(QEMUFile *file)
> > +{
> > + return file->has_ioc ? QIO_CHANNEL(file->opaque) : NULL;
> > +}
>
> Do you think this should go in the previous patch where you created
> has_ioc? Also the name is weird, qemu_file is probably right for
> everything in here.
Sure; let me just move it over with it renamed. Thanks,
--
Peter Xu
^ permalink raw reply [flat|nested] 17+ messages in thread