[PATCH 0/5] migration: Bug fixes (prepare for preempt-full)

[PATCH 0/5] migration: Bug fixes (prepare for preempt-full)

[Peter Xu]

This patchset does bug fixes that I found when testing preempt-full.

Patch 1 should fix a possible deadloop when I hit when testing the
preempt-full code.  I didn't verify it because it's so hard to trigger, but
the logic should be explained in the patch.

Patch 2 fixes a race condition I can easily trigger with the latest
preempt-full code when running with recovery+tls test.  The bug hides quite
deep and took time to debug.  Fundamentally it's about qemufile API, I hope
someday we can have something better than that but still so far there's no
strong enough reason to rework the whole thing.

Patch 3-4 are two patches to disable either postcopy or preempt mode only
for xbzrle/compression.

Patch 5 is something nice to have to optimize the bitmap ops.

The last two patches are actually part of my preempt-full RFC series.

I picked these patches out explicitly from preempt-full series, because at
least patches 1-4 fix real bugs in current code base, so they should get
more focus.

Thanks,

Peter Xu (5):
  migration: Fix possible deadloop of ram save process
  migration: Fix race on qemu_file_shutdown()
  migration: Disallow xbzrle with postcopy
  migration: Disallow postcopy preempt to be used with compress
  migration: Use non-atomic ops for clear log bitmap

 include/exec/ram_addr.h | 11 +++++-----
 include/exec/ramblock.h |  3 +++
 include/qemu/bitmap.h   |  1 +
 migration/migration.c   | 16 +++++++++++++++
 migration/qemu-file.c   | 27 ++++++++++++++++++++++---
 migration/ram.c         | 16 +++++++++++----
 util/bitmap.c           | 45 +++++++++++++++++++++++++++++++++++++++++
 7 files changed, 107 insertions(+), 12 deletions(-)

-- 
2.32.0

[PATCH 1/5] migration: Fix possible deadloop of ram save process

[Peter Xu]

When starting ram saving procedure (especially at the completion phase),
always set last_seen_block to non-NULL to make sure we can always correctly
detect the case where "we've migrated all the dirty pages".

Then we'll guarantee both last_seen_block and pss.block will be valid
always before the loop starts.

See the comment in the code for some details.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/ram.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index dc1de9ddbc..1d42414ecc 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2546,14 +2546,22 @@ static int ram_find_and_save_block(RAMState *rs)
         return pages;
     }
 
+    /*
+     * Always keep last_seen_block/last_page valid during this procedure,
+     * because find_dirty_block() relies on these values (e.g., we compare
+     * last_seen_block with pss.block to see whether we searched all the
+     * ramblocks) to detect the completion of migration.  Having NULL value
+     * of last_seen_block can conditionally cause below loop to run forever.
+     */
+    if (!rs->last_seen_block) {
+        rs->last_seen_block = QLIST_FIRST_RCU(&ram_list.blocks);
+        rs->last_page = 0;
+    }
+
     pss.block = rs->last_seen_block;
     pss.page = rs->last_page;
     pss.complete_round = false;
 
-    if (!pss.block) {
-        pss.block = QLIST_FIRST_RCU(&ram_list.blocks);
-    }
-
     do {
         again = true;
         found = get_queued_page(rs, &pss);
-- 
2.32.0

Re: [PATCH 1/5] migration: Fix possible deadloop of ram save process

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> When starting ram saving procedure (especially at the completion phase),
> always set last_seen_block to non-NULL to make sure we can always correctly
> detect the case where "we've migrated all the dirty pages".
> 
> Then we'll guarantee both last_seen_block and pss.block will be valid
> always before the loop starts.
> 
> See the comment in the code for some details.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>

Yeh I guess it can currently only happen during restart?

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  migration/ram.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index dc1de9ddbc..1d42414ecc 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -2546,14 +2546,22 @@ static int ram_find_and_save_block(RAMState *rs)
>          return pages;
>      }
>  
> +    /*
> +     * Always keep last_seen_block/last_page valid during this procedure,
> +     * because find_dirty_block() relies on these values (e.g., we compare
> +     * last_seen_block with pss.block to see whether we searched all the
> +     * ramblocks) to detect the completion of migration.  Having NULL value
> +     * of last_seen_block can conditionally cause below loop to run forever.
> +     */
> +    if (!rs->last_seen_block) {
> +        rs->last_seen_block = QLIST_FIRST_RCU(&ram_list.blocks);
> +        rs->last_page = 0;
> +    }
> +
>      pss.block = rs->last_seen_block;
>      pss.page = rs->last_page;
>      pss.complete_round = false;
>  
> -    if (!pss.block) {
> -        pss.block = QLIST_FIRST_RCU(&ram_list.blocks);
> -    }
> -
>      do {
>          again = true;
>          found = get_queued_page(rs, &pss);
> -- 
> 2.32.0
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 1/5] migration: Fix possible deadloop of ram save process

[Peter Xu]

On Thu, Sep 22, 2022 at 03:49:38PM +0100, Dr. David Alan Gilbert wrote:
> * Peter Xu (peterx@redhat.com) wrote:
> > When starting ram saving procedure (especially at the completion phase),
> > always set last_seen_block to non-NULL to make sure we can always correctly
> > detect the case where "we've migrated all the dirty pages".
> > 
> > Then we'll guarantee both last_seen_block and pss.block will be valid
> > always before the loop starts.
> > 
> > See the comment in the code for some details.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> 
> Yeh I guess it can currently only happen during restart?

There're only two places to clear last_seen_block:

ram_state_reset[2683]          rs->last_seen_block = NULL;
ram_postcopy_send_discard_bitmap[2876] rs->last_seen_block = NULL;

Where for the reset case:

ram_state_init[2994]           ram_state_reset(*rsp);
ram_state_resume_prepare[3110] ram_state_reset(rs);
ram_save_iterate[3271]         ram_state_reset(rs);

So I think it can at least happen in two places, either (1) postcopy just
started (assume when postcopy starts accidentally when all dirty pages were
migrated?), or (2) postcopy recover from failure.

In my case I triggered this deadloop when I was debugging the other bug
fixed by the next patch where it was postcopy recovery (on tls), but only
once..  So currently I'm still not 100% sure whether this is the same
problem, but logically it could trigger.

I also remember I used to hit very rare deadloops before too, maybe they're
the same thing because I did test recovery a lot.

> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Thanks!

-- 
Peter Xu

Re: [PATCH 1/5] migration: Fix possible deadloop of ram save process

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> On Thu, Sep 22, 2022 at 03:49:38PM +0100, Dr. David Alan Gilbert wrote:
> > * Peter Xu (peterx@redhat.com) wrote:
> > > When starting ram saving procedure (especially at the completion phase),
> > > always set last_seen_block to non-NULL to make sure we can always correctly
> > > detect the case where "we've migrated all the dirty pages".
> > > 
> > > Then we'll guarantee both last_seen_block and pss.block will be valid
> > > always before the loop starts.
> > > 
> > > See the comment in the code for some details.
> > > 
> > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > 
> > Yeh I guess it can currently only happen during restart?
> 
> There're only two places to clear last_seen_block:
> 
> ram_state_reset[2683]          rs->last_seen_block = NULL;
> ram_postcopy_send_discard_bitmap[2876] rs->last_seen_block = NULL;
> 
> Where for the reset case:
> 
> ram_state_init[2994]           ram_state_reset(*rsp);
> ram_state_resume_prepare[3110] ram_state_reset(rs);
> ram_save_iterate[3271]         ram_state_reset(rs);
> 
> So I think it can at least happen in two places, either (1) postcopy just
> started (assume when postcopy starts accidentally when all dirty pages were
> migrated?), or (2) postcopy recover from failure.

Oh, (1) is a more general problem then; yeh.

> In my case I triggered this deadloop when I was debugging the other bug
> fixed by the next patch where it was postcopy recovery (on tls), but only
> once..  So currently I'm still not 100% sure whether this is the same
> problem, but logically it could trigger.
> 
> I also remember I used to hit very rare deadloops before too, maybe they're
> the same thing because I did test recovery a lot.

Note; 'deadlock' not 'deadloop'.

Dave

> > 
> > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> Thanks!
> 
> -- 
> Peter Xu
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 1/5] migration: Fix possible deadloop of ram save process

[Peter Xu]

On Thu, Sep 22, 2022 at 05:41:30PM +0100, Dr. David Alan Gilbert wrote:
> * Peter Xu (peterx@redhat.com) wrote:
> > On Thu, Sep 22, 2022 at 03:49:38PM +0100, Dr. David Alan Gilbert wrote:
> > > * Peter Xu (peterx@redhat.com) wrote:
> > > > When starting ram saving procedure (especially at the completion phase),
> > > > always set last_seen_block to non-NULL to make sure we can always correctly
> > > > detect the case where "we've migrated all the dirty pages".
> > > > 
> > > > Then we'll guarantee both last_seen_block and pss.block will be valid
> > > > always before the loop starts.
> > > > 
> > > > See the comment in the code for some details.
> > > > 
> > > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > 
> > > Yeh I guess it can currently only happen during restart?
> > 
> > There're only two places to clear last_seen_block:
> > 
> > ram_state_reset[2683]          rs->last_seen_block = NULL;
> > ram_postcopy_send_discard_bitmap[2876] rs->last_seen_block = NULL;
> > 
> > Where for the reset case:
> > 
> > ram_state_init[2994]           ram_state_reset(*rsp);
> > ram_state_resume_prepare[3110] ram_state_reset(rs);
> > ram_save_iterate[3271]         ram_state_reset(rs);
> > 
> > So I think it can at least happen in two places, either (1) postcopy just
> > started (assume when postcopy starts accidentally when all dirty pages were
> > migrated?), or (2) postcopy recover from failure.
> 
> Oh, (1) is a more general problem then; yeh.
> 
> > In my case I triggered this deadloop when I was debugging the other bug
> > fixed by the next patch where it was postcopy recovery (on tls), but only
> > once..  So currently I'm still not 100% sure whether this is the same
> > problem, but logically it could trigger.
> > 
> > I also remember I used to hit very rare deadloops before too, maybe they're
> > the same thing because I did test recovery a lot.
> 
> Note; 'deadlock' not 'deadloop'.

(Oops I somehow forgot there's still this series pending..)

Here it's not about a lock, or maybe I should add a space ("dead loop")?

-- 
Peter Xu

Re: [PATCH 1/5] migration: Fix possible deadloop of ram save process

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> On Thu, Sep 22, 2022 at 05:41:30PM +0100, Dr. David Alan Gilbert wrote:
> > * Peter Xu (peterx@redhat.com) wrote:
> > > On Thu, Sep 22, 2022 at 03:49:38PM +0100, Dr. David Alan Gilbert wrote:
> > > > * Peter Xu (peterx@redhat.com) wrote:
> > > > > When starting ram saving procedure (especially at the completion phase),
> > > > > always set last_seen_block to non-NULL to make sure we can always correctly
> > > > > detect the case where "we've migrated all the dirty pages".
> > > > > 
> > > > > Then we'll guarantee both last_seen_block and pss.block will be valid
> > > > > always before the loop starts.
> > > > > 
> > > > > See the comment in the code for some details.
> > > > > 
> > > > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > > 
> > > > Yeh I guess it can currently only happen during restart?
> > > 
> > > There're only two places to clear last_seen_block:
> > > 
> > > ram_state_reset[2683]          rs->last_seen_block = NULL;
> > > ram_postcopy_send_discard_bitmap[2876] rs->last_seen_block = NULL;
> > > 
> > > Where for the reset case:
> > > 
> > > ram_state_init[2994]           ram_state_reset(*rsp);
> > > ram_state_resume_prepare[3110] ram_state_reset(rs);
> > > ram_save_iterate[3271]         ram_state_reset(rs);
> > > 
> > > So I think it can at least happen in two places, either (1) postcopy just
> > > started (assume when postcopy starts accidentally when all dirty pages were
> > > migrated?), or (2) postcopy recover from failure.
> > 
> > Oh, (1) is a more general problem then; yeh.
> > 
> > > In my case I triggered this deadloop when I was debugging the other bug
> > > fixed by the next patch where it was postcopy recovery (on tls), but only
> > > once..  So currently I'm still not 100% sure whether this is the same
> > > problem, but logically it could trigger.
> > > 
> > > I also remember I used to hit very rare deadloops before too, maybe they're
> > > the same thing because I did test recovery a lot.
> > 
> > Note; 'deadlock' not 'deadloop'.
> 
> (Oops I somehow forgot there's still this series pending..)
> 
> Here it's not about a lock, or maybe I should add a space ("dead loop")?

So the normal phrases I'm used to are:
  'deadlock' - two threads waiting for each other
  'livelock' - two threads spinning for each other

Dave

> -- 
> Peter Xu
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[PATCH 2/5] migration: Fix race on qemu_file_shutdown()

[Peter Xu]

In qemu_file_shutdown(), there's a possible race if with current order of
operation.  There're two major things to do:

  (1) Do real shutdown() (e.g. shutdown() syscall on socket)
  (2) Update qemufile's last_error

We must do (2) before (1) otherwise there can be a race condition like:

      page receiver                     other thread
      -------------                     ------------
      qemu_get_buffer()
                                        do shutdown()
        returns 0 (buffer all zero)
        (meanwhile we didn't check this retcode)
      try to detect IO error
        last_error==NULL, IO okay
      install ALL-ZERO page
                                        set last_error
      --> guest crash!

To fix this, we can also check retval of qemu_get_buffer(), but not all
APIs can be properly checked and ultimately we still need to go back to
qemu_file_get_error().  E.g. qemu_get_byte() doesn't return error.

Maybe some day a rework of qemufile API is really needed, but for now keep
using qemu_file_get_error() and fix it by not allowing that race condition
to happen.  Here shutdown() is indeed special because the last_error was
emulated.  For real -EIO errors it'll always be set when e.g. sendmsg()
error triggers so we won't miss those ones, only shutdown() is a bit tricky
here.

Cc: Daniel P. Berrange <berrange@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/qemu-file.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index 4f400c2e52..2d5f74ffc2 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -79,6 +79,30 @@ int qemu_file_shutdown(QEMUFile *f)
     int ret = 0;
 
     f->shutdown = true;
+
+    /*
+     * We must set qemufile error before the real shutdown(), otherwise
+     * there can be a race window where we thought IO all went though
+     * (because last_error==NULL) but actually IO has already stopped.
+     *
+     * If without correct ordering, the race can happen like this:
+     *
+     *      page receiver                     other thread
+     *      -------------                     ------------
+     *      qemu_get_buffer()
+     *                                        do shutdown()
+     *        returns 0 (buffer all zero)
+     *        (we didn't check this retcode)
+     *      try to detect IO error
+     *        last_error==NULL, IO okay
+     *      install ALL-ZERO page
+     *                                        set last_error
+     *      --> guest crash!
+     */
+    if (!f->last_error) {
+        qemu_file_set_error(f, -EIO);
+    }
+
     if (!qio_channel_has_feature(f->ioc,
                                  QIO_CHANNEL_FEATURE_SHUTDOWN)) {
         return -ENOSYS;
@@ -88,9 +112,6 @@ int qemu_file_shutdown(QEMUFile *f)
         ret = -EIO;
     }
 
-    if (!f->last_error) {
-        qemu_file_set_error(f, -EIO);
-    }
     return ret;
 }
 
-- 
2.32.0

Re: [PATCH 2/5] migration: Fix race on qemu_file_shutdown()

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> In qemu_file_shutdown(), there's a possible race if with current order of
> operation.  There're two major things to do:
> 
>   (1) Do real shutdown() (e.g. shutdown() syscall on socket)
>   (2) Update qemufile's last_error
> 
> We must do (2) before (1) otherwise there can be a race condition like:
> 
>       page receiver                     other thread
>       -------------                     ------------
>       qemu_get_buffer()
>                                         do shutdown()
>         returns 0 (buffer all zero)
>         (meanwhile we didn't check this retcode)
>       try to detect IO error
>         last_error==NULL, IO okay
>       install ALL-ZERO page
>                                         set last_error
>       --> guest crash!
> 
> To fix this, we can also check retval of qemu_get_buffer(), but not all
> APIs can be properly checked and ultimately we still need to go back to
> qemu_file_get_error().  E.g. qemu_get_byte() doesn't return error.
> 
> Maybe some day a rework of qemufile API is really needed, but for now keep
> using qemu_file_get_error() and fix it by not allowing that race condition
> to happen.  Here shutdown() is indeed special because the last_error was
> emulated.  For real -EIO errors it'll always be set when e.g. sendmsg()
> error triggers so we won't miss those ones, only shutdown() is a bit tricky
> here.
> 
> Cc: Daniel P. Berrange <berrange@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Oh that's kind of fun,


Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  migration/qemu-file.c | 27 ++++++++++++++++++++++++---
>  1 file changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> index 4f400c2e52..2d5f74ffc2 100644
> --- a/migration/qemu-file.c
> +++ b/migration/qemu-file.c
> @@ -79,6 +79,30 @@ int qemu_file_shutdown(QEMUFile *f)
>      int ret = 0;
>  
>      f->shutdown = true;
> +
> +    /*
> +     * We must set qemufile error before the real shutdown(), otherwise
> +     * there can be a race window where we thought IO all went though
> +     * (because last_error==NULL) but actually IO has already stopped.
> +     *
> +     * If without correct ordering, the race can happen like this:
> +     *
> +     *      page receiver                     other thread
> +     *      -------------                     ------------
> +     *      qemu_get_buffer()
> +     *                                        do shutdown()
> +     *        returns 0 (buffer all zero)
> +     *        (we didn't check this retcode)
> +     *      try to detect IO error
> +     *        last_error==NULL, IO okay
> +     *      install ALL-ZERO page
> +     *                                        set last_error
> +     *      --> guest crash!
> +     */
> +    if (!f->last_error) {
> +        qemu_file_set_error(f, -EIO);
> +    }
> +
>      if (!qio_channel_has_feature(f->ioc,
>                                   QIO_CHANNEL_FEATURE_SHUTDOWN)) {
>          return -ENOSYS;
> @@ -88,9 +112,6 @@ int qemu_file_shutdown(QEMUFile *f)
>          ret = -EIO;
>      }
>  
> -    if (!f->last_error) {
> -        qemu_file_set_error(f, -EIO);
> -    }
>      return ret;
>  }
>  
> -- 
> 2.32.0
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 2/5] migration: Fix race on qemu_file_shutdown()

[Daniel P. Berrangé]

On Tue, Sep 20, 2022 at 06:37:57PM -0400, Peter Xu wrote:
> In qemu_file_shutdown(), there's a possible race if with current order of
> operation.  There're two major things to do:
> 
>   (1) Do real shutdown() (e.g. shutdown() syscall on socket)
>   (2) Update qemufile's last_error
> 
> We must do (2) before (1) otherwise there can be a race condition like:
> 
>       page receiver                     other thread
>       -------------                     ------------
>       qemu_get_buffer()
>                                         do shutdown()
>         returns 0 (buffer all zero)
>         (meanwhile we didn't check this retcode)
>       try to detect IO error
>         last_error==NULL, IO okay
>       install ALL-ZERO page
>                                         set last_error
>       --> guest crash!
> 
> To fix this, we can also check retval of qemu_get_buffer(), but not all
> APIs can be properly checked and ultimately we still need to go back to
> qemu_file_get_error().  E.g. qemu_get_byte() doesn't return error.
> 
> Maybe some day a rework of qemufile API is really needed, but for now keep
> using qemu_file_get_error() and fix it by not allowing that race condition
> to happen.  Here shutdown() is indeed special because the last_error was
> emulated.  For real -EIO errors it'll always be set when e.g. sendmsg()
> error triggers so we won't miss those ones, only shutdown() is a bit tricky
> here.

The ultimate answer really is to stop using QEMUFile entirely and just
do migration with the QIOChannel objects directly. The work I did in the
last cycle to remove the QEMUFileOps callbacks was another piece of the
puzzle in moving in that direction, by ensuring that every QEMUFile is
now backed by a QIOChannel. All QEMUFile is doing now is adding the I/O
caching layer and some convenience APIs for I/O operations.

So the next step would be to add a  QIOChannelCached class that can wrap
over another QIOChannel, to add the I/O buffering functionality that
currently exists in QEMUFile. Once that's done, it'd be at the stage
where we could look at how to use QIOChannel APIs for VMstate. It would
likely involve wiring up an Error **errp  parameter into a great many
places so we get synchronous error propagation instead of out-of-band
error checking, so a phased transition would need to be figured out.

With 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 2/5] migration: Fix race on qemu_file_shutdown()

[Peter Xu]

On Thu, Sep 22, 2022 at 05:58:25PM +0100, Daniel P. Berrangé wrote:
> On Tue, Sep 20, 2022 at 06:37:57PM -0400, Peter Xu wrote:
> > In qemu_file_shutdown(), there's a possible race if with current order of
> > operation.  There're two major things to do:
> > 
> >   (1) Do real shutdown() (e.g. shutdown() syscall on socket)
> >   (2) Update qemufile's last_error
> > 
> > We must do (2) before (1) otherwise there can be a race condition like:
> > 
> >       page receiver                     other thread
> >       -------------                     ------------
> >       qemu_get_buffer()
> >                                         do shutdown()
> >         returns 0 (buffer all zero)
> >         (meanwhile we didn't check this retcode)
> >       try to detect IO error
> >         last_error==NULL, IO okay
> >       install ALL-ZERO page
> >                                         set last_error
> >       --> guest crash!
> > 
> > To fix this, we can also check retval of qemu_get_buffer(), but not all
> > APIs can be properly checked and ultimately we still need to go back to
> > qemu_file_get_error().  E.g. qemu_get_byte() doesn't return error.
> > 
> > Maybe some day a rework of qemufile API is really needed, but for now keep
> > using qemu_file_get_error() and fix it by not allowing that race condition
> > to happen.  Here shutdown() is indeed special because the last_error was
> > emulated.  For real -EIO errors it'll always be set when e.g. sendmsg()
> > error triggers so we won't miss those ones, only shutdown() is a bit tricky
> > here.
> 
> The ultimate answer really is to stop using QEMUFile entirely and just
> do migration with the QIOChannel objects directly. The work I did in the
> last cycle to remove the QEMUFileOps callbacks was another piece of the
> puzzle in moving in that direction, by ensuring that every QEMUFile is
> now backed by a QIOChannel. All QEMUFile is doing now is adding the I/O
> caching layer and some convenience APIs for I/O operations.
> 
> So the next step would be to add a  QIOChannelCached class that can wrap
> over another QIOChannel, to add the I/O buffering functionality that
> currently exists in QEMUFile. Once that's done, it'd be at the stage
> where we could look at how to use QIOChannel APIs for VMstate. It would
> likely involve wiring up an Error **errp  parameter into a great many
> places so we get synchronous error propagation instead of out-of-band
> error checking, so a phased transition would need to be figured out.

Yes, Error** sounds very good to have.

So far I'm not satisfied with qemufile api majorly because of that error
handling, especially on *get() interfaces.

Besides that, do you have anything else in mind that would make
QIOChannelCached better than qemufile (e.g. on how we do caching)?

Thanks,

-- 
Peter Xu

Re: [PATCH 2/5] migration: Fix race on qemu_file_shutdown()

[Daniel P. Berrangé]

On Thu, Sep 22, 2022 at 03:37:11PM -0400, Peter Xu wrote:
> On Thu, Sep 22, 2022 at 05:58:25PM +0100, Daniel P. Berrangé wrote:
> > On Tue, Sep 20, 2022 at 06:37:57PM -0400, Peter Xu wrote:
> > > In qemu_file_shutdown(), there's a possible race if with current order of
> > > operation.  There're two major things to do:
> > > 
> > >   (1) Do real shutdown() (e.g. shutdown() syscall on socket)
> > >   (2) Update qemufile's last_error
> > > 
> > > We must do (2) before (1) otherwise there can be a race condition like:
> > > 
> > >       page receiver                     other thread
> > >       -------------                     ------------
> > >       qemu_get_buffer()
> > >                                         do shutdown()
> > >         returns 0 (buffer all zero)
> > >         (meanwhile we didn't check this retcode)
> > >       try to detect IO error
> > >         last_error==NULL, IO okay
> > >       install ALL-ZERO page
> > >                                         set last_error
> > >       --> guest crash!
> > > 
> > > To fix this, we can also check retval of qemu_get_buffer(), but not all
> > > APIs can be properly checked and ultimately we still need to go back to
> > > qemu_file_get_error().  E.g. qemu_get_byte() doesn't return error.
> > > 
> > > Maybe some day a rework of qemufile API is really needed, but for now keep
> > > using qemu_file_get_error() and fix it by not allowing that race condition
> > > to happen.  Here shutdown() is indeed special because the last_error was
> > > emulated.  For real -EIO errors it'll always be set when e.g. sendmsg()
> > > error triggers so we won't miss those ones, only shutdown() is a bit tricky
> > > here.
> > 
> > The ultimate answer really is to stop using QEMUFile entirely and just
> > do migration with the QIOChannel objects directly. The work I did in the
> > last cycle to remove the QEMUFileOps callbacks was another piece of the
> > puzzle in moving in that direction, by ensuring that every QEMUFile is
> > now backed by a QIOChannel. All QEMUFile is doing now is adding the I/O
> > caching layer and some convenience APIs for I/O operations.
> > 
> > So the next step would be to add a  QIOChannelCached class that can wrap
> > over another QIOChannel, to add the I/O buffering functionality that
> > currently exists in QEMUFile. Once that's done, it'd be at the stage
> > where we could look at how to use QIOChannel APIs for VMstate. It would
> > likely involve wiring up an Error **errp  parameter into a great many
> > places so we get synchronous error propagation instead of out-of-band
> > error checking, so a phased transition would need to be figured out.
> 
> Yes, Error** sounds very good to have.
> 
> So far I'm not satisfied with qemufile api majorly because of that error
> handling, especially on *get() interfaces.
> 
> Besides that, do you have anything else in mind that would make
> QIOChannelCached better than qemufile (e.g. on how we do caching)?

Depends what you mean by better ? I think the caching code would be
a bit easier to understand, because QEMUFile gets a bit confusing
about which logic is used for read side and which is used for the
write side.

With 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 2/5] migration: Fix race on qemu_file_shutdown()

[Peter Xu]

On Fri, Sep 23, 2022 at 08:14:25AM +0100, Daniel P. Berrangé wrote:
> > Besides that, do you have anything else in mind that would make
> > QIOChannelCached better than qemufile (e.g. on how we do caching)?
> 
> Depends what you mean by better ? I think the caching code would be
> a bit easier to understand, because QEMUFile gets a bit confusing
> about which logic is used for read side and which is used for the
> write side.

I see, looking forward to it.  Thanks,

-- 
Peter Xu

[PATCH 3/5] migration: Disallow xbzrle with postcopy

[Peter Xu]

It's not supported since the 1st day, as ram_load_postcopy does not handle
RAM_SAVE_FLAG_XBZRLE.  Mark it disabled explicitly.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/migration/migration.c b/migration/migration.c
index bb8bbddfe4..fb4066dfb4 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1268,6 +1268,11 @@ static bool migrate_caps_check(bool *cap_list,
             error_setg(errp, "Postcopy is not compatible with ignore-shared");
             return false;
         }
+
+        if (cap_list[MIGRATION_CAPABILITY_XBZRLE]) {
+            error_setg(errp, "Postcopy is not compatible with xbzrle");
+            return false;
+        }
     }
 
     if (cap_list[MIGRATION_CAPABILITY_BACKGROUND_SNAPSHOT]) {
-- 
2.32.0

Re: [PATCH 3/5] migration: Disallow xbzrle with postcopy

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> It's not supported since the 1st day, as ram_load_postcopy does not handle
> RAM_SAVE_FLAG_XBZRLE.  Mark it disabled explicitly.

We've already got a check in ram_save_page:

    if (rs->xbzrle_enabled && !migration_in_postcopy()) {
        pages = save_xbzrle_page(rs, &p, current_addr, block,
                                 offset);

so that's supposed to allow you to enable xbzrle with postcopy and take
advantage of xbzrle during the precopy phase.

Dave

> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/migration.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index bb8bbddfe4..fb4066dfb4 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1268,6 +1268,11 @@ static bool migrate_caps_check(bool *cap_list,
>              error_setg(errp, "Postcopy is not compatible with ignore-shared");
>              return false;
>          }
> +
> +        if (cap_list[MIGRATION_CAPABILITY_XBZRLE]) {
> +            error_setg(errp, "Postcopy is not compatible with xbzrle");
> +            return false;
> +        }
>      }
>  
>      if (cap_list[MIGRATION_CAPABILITY_BACKGROUND_SNAPSHOT]) {
> -- 
> 2.32.0
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 3/5] migration: Disallow xbzrle with postcopy

[Peter Xu]

On Thu, Sep 22, 2022 at 04:56:21PM +0100, Dr. David Alan Gilbert wrote:
> * Peter Xu (peterx@redhat.com) wrote:
> > It's not supported since the 1st day, as ram_load_postcopy does not handle
> > RAM_SAVE_FLAG_XBZRLE.  Mark it disabled explicitly.
> 
> We've already got a check in ram_save_page:
> 
>     if (rs->xbzrle_enabled && !migration_in_postcopy()) {
>         pages = save_xbzrle_page(rs, &p, current_addr, block,
>                                  offset);
> 
> so that's supposed to allow you to enable xbzrle with postcopy and take
> advantage of xbzrle during the precopy phase.

Ah!  Makes sense.  I'll drop this one.

-- 
Peter Xu

[PATCH 4/5] migration: Disallow postcopy preempt to be used with compress

[Peter Xu]

The preempt mode requires the capability to assign channel for each of the
page, while the compression logic will currently assign pages to different
compress thread/local-channel so potentially they're incompatible.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/migration/migration.c b/migration/migration.c
index fb4066dfb4..07c74a79a2 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1341,6 +1341,17 @@ static bool migrate_caps_check(bool *cap_list,
             error_setg(errp, "Postcopy preempt requires postcopy-ram");
             return false;
         }
+
+        /*
+         * Preempt mode requires urgent pages to be sent in separate
+         * channel, OTOH compression logic will disorder all pages into
+         * different compression channels, which is not compatible with the
+         * preempt assumptions on channel assignments.
+         */
+        if (cap_list[MIGRATION_CAPABILITY_COMPRESS]) {
+            error_setg(errp, "Postcopy preempt not compatible with compress");
+            return false;
+        }
     }
 
     return true;
-- 
2.32.0

Re: [PATCH 4/5] migration: Disallow postcopy preempt to be used with compress

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> The preempt mode requires the capability to assign channel for each of the
> page, while the compression logic will currently assign pages to different
> compress thread/local-channel so potentially they're incompatible.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  migration/migration.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index fb4066dfb4..07c74a79a2 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1341,6 +1341,17 @@ static bool migrate_caps_check(bool *cap_list,
>              error_setg(errp, "Postcopy preempt requires postcopy-ram");
>              return false;
>          }
> +
> +        /*
> +         * Preempt mode requires urgent pages to be sent in separate
> +         * channel, OTOH compression logic will disorder all pages into
> +         * different compression channels, which is not compatible with the
> +         * preempt assumptions on channel assignments.
> +         */
> +        if (cap_list[MIGRATION_CAPABILITY_COMPRESS]) {
> +            error_setg(errp, "Postcopy preempt not compatible with compress");
> +            return false;
> +        }
>      }
>  
>      return true;
> -- 
> 2.32.0
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[PATCH 5/5] migration: Use non-atomic ops for clear log bitmap

[Peter Xu]

Since we already have bitmap_mutex to protect either the dirty bitmap or
the clear log bitmap, we don't need atomic operations to set/clear/test on
the clear log bitmap.  Switching all ops from atomic to non-atomic
versions, meanwhile touch up the comments to show which lock is in charge.

Introduced non-atomic version of bitmap_test_and_clear_atomic(), mostly the
same as the atomic version but simplified a few places, e.g. dropped the
"old_bits" variable, and also the explicit memory barriers.

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/exec/ram_addr.h | 11 +++++-----
 include/exec/ramblock.h |  3 +++
 include/qemu/bitmap.h   |  1 +
 util/bitmap.c           | 45 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 55 insertions(+), 5 deletions(-)

diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index f3e0c78161..5092a2e0ff 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -42,7 +42,8 @@ static inline long clear_bmap_size(uint64_t pages, uint8_t shift)
 }
 
 /**
- * clear_bmap_set: set clear bitmap for the page range
+ * clear_bmap_set: set clear bitmap for the page range.  Must be with
+ * bitmap_mutex held.
  *
  * @rb: the ramblock to operate on
  * @start: the start page number
@@ -55,12 +56,12 @@ static inline void clear_bmap_set(RAMBlock *rb, uint64_t start,
 {
     uint8_t shift = rb->clear_bmap_shift;
 
-    bitmap_set_atomic(rb->clear_bmap, start >> shift,
-                      clear_bmap_size(npages, shift));
+    bitmap_set(rb->clear_bmap, start >> shift, clear_bmap_size(npages, shift));
 }
 
 /**
- * clear_bmap_test_and_clear: test clear bitmap for the page, clear if set
+ * clear_bmap_test_and_clear: test clear bitmap for the page, clear if set.
+ * Must be with bitmap_mutex held.
  *
  * @rb: the ramblock to operate on
  * @page: the page number to check
@@ -71,7 +72,7 @@ static inline bool clear_bmap_test_and_clear(RAMBlock *rb, uint64_t page)
 {
     uint8_t shift = rb->clear_bmap_shift;
 
-    return bitmap_test_and_clear_atomic(rb->clear_bmap, page >> shift, 1);
+    return bitmap_test_and_clear(rb->clear_bmap, page >> shift, 1);
 }
 
 static inline bool offset_in_ramblock(RAMBlock *b, ram_addr_t offset)
diff --git a/include/exec/ramblock.h b/include/exec/ramblock.h
index 6cbedf9e0c..adc03df59c 100644
--- a/include/exec/ramblock.h
+++ b/include/exec/ramblock.h
@@ -53,6 +53,9 @@ struct RAMBlock {
      * and split clearing of dirty bitmap on the remote node (e.g.,
      * KVM).  The bitmap will be set only when doing global sync.
      *
+     * It is only used during src side of ram migration, and it is
+     * protected by the global ram_state.bitmap_mutex.
+     *
      * NOTE: this bitmap is different comparing to the other bitmaps
      * in that one bit can represent multiple guest pages (which is
      * decided by the `clear_bmap_shift' variable below).  On
diff --git a/include/qemu/bitmap.h b/include/qemu/bitmap.h
index 82a1d2f41f..3ccb00865f 100644
--- a/include/qemu/bitmap.h
+++ b/include/qemu/bitmap.h
@@ -253,6 +253,7 @@ void bitmap_set(unsigned long *map, long i, long len);
 void bitmap_set_atomic(unsigned long *map, long i, long len);
 void bitmap_clear(unsigned long *map, long start, long nr);
 bool bitmap_test_and_clear_atomic(unsigned long *map, long start, long nr);
+bool bitmap_test_and_clear(unsigned long *map, long start, long nr);
 void bitmap_copy_and_clear_atomic(unsigned long *dst, unsigned long *src,
                                   long nr);
 unsigned long bitmap_find_next_zero_area(unsigned long *map,
diff --git a/util/bitmap.c b/util/bitmap.c
index f81d8057a7..8d12e90a5a 100644
--- a/util/bitmap.c
+++ b/util/bitmap.c
@@ -240,6 +240,51 @@ void bitmap_clear(unsigned long *map, long start, long nr)
     }
 }
 
+bool bitmap_test_and_clear(unsigned long *map, long start, long nr)
+{
+    unsigned long *p = map + BIT_WORD(start);
+    const long size = start + nr;
+    int bits_to_clear = BITS_PER_LONG - (start % BITS_PER_LONG);
+    unsigned long mask_to_clear = BITMAP_FIRST_WORD_MASK(start);
+    bool dirty = false;
+
+    assert(start >= 0 && nr >= 0);
+
+    /* First word */
+    if (nr - bits_to_clear > 0) {
+        if ((*p) & mask_to_clear) {
+            dirty = true;
+        }
+        *p &= ~mask_to_clear;
+        nr -= bits_to_clear;
+        bits_to_clear = BITS_PER_LONG;
+        p++;
+    }
+
+    /* Full words */
+    if (bits_to_clear == BITS_PER_LONG) {
+        while (nr >= BITS_PER_LONG) {
+            if (*p) {
+                dirty = true;
+                *p = 0;
+            }
+            nr -= BITS_PER_LONG;
+            p++;
+        }
+    }
+
+    /* Last word */
+    if (nr) {
+        mask_to_clear &= BITMAP_LAST_WORD_MASK(size);
+        if ((*p) & mask_to_clear) {
+            dirty = true;
+        }
+        *p &= ~mask_to_clear;
+    }
+
+    return dirty;
+}
+
 bool bitmap_test_and_clear_atomic(unsigned long *map, long start, long nr)
 {
     unsigned long *p = map + BIT_WORD(start);
-- 
2.32.0