Recent change pmem related breaks Xen migration

Recent change pmem related breaks Xen migration

[Anthony PERARD]

Hi,

Commit bd108a44bc29 ("migration: ram: Switch to ram block writeback")
breaks migration on Xen. We have:
  ramblock_ptr: Assertion `offset_in_ramblock(block, offset)' failed.

I've track it down to qemu_ram_writeback() calling ramblock_ptr()
unconditionally, even when the result will not be used.

Maybe we could call ramblock_ptr() twice in that function? I've prepared
a patch.


FYI, full-ish trace on restore of a xen guest:
#3  0x00007f82d0848526 in __assert_fail () from /usr/lib/libc.so.6
#4  0x0000562dc4578122 in ramblock_ptr (block=0x562dc5ebe2a0, offset=0) at /root/build/qemu/include/exec/ram_addr.h:120
#5  0x0000562dc457d1b7 in qemu_ram_writeback (block=0x562dc5ebe2a0, start=0, length=515899392) at /root/build/qemu/exec.c:2169
#6  0x0000562dc45e8941 in qemu_ram_block_writeback (block=0x562dc5ebe2a0) at /root/build/qemu/include/exec/ram_addr.h:182
#7  0x0000562dc45f0b56 in ram_load_cleanup (opaque=0x562dc510fe00 <ram_state>) at /root/build/qemu/migration/ram.c:3983
#8  0x0000562dc49970b6 in qemu_loadvm_state_cleanup () at migration/savevm.c:2415
#9  0x0000562dc4997548 in qemu_loadvm_state (f=0x562dc6a1c600) at migration/savevm.c:2597
#10 0x0000562dc4987be7 in process_incoming_migration_co (opaque=0x0) at migration/migration.c:454
#11 0x0000562dc4b907e5 in coroutine_trampoline (i0=-962514432, i1=22061) at util/coroutine-ucontext.c:115

And *block in ramblock_ptr():
(gdb) p *block
$2 = {
  rcu = {
    next = 0x0, 
    func = 0x0
  }, 
  mr = 0x562dc512e140 <ram_memory>, 
  host = 0x0, 
  colo_cache = 0x0, 
  offset = 0, 
  used_length = 515899392, 
  max_length = 515899392, 
  resized = 0x0, 
  flags = 16, 
  idstr = "xen.ram", '\000' <repeats 248 times>, 
  next = {
    le_next = 0x562dc67bf7e0, 
    le_prev = 0x562dc510f1a0 <ram_list+64>
  }, 
  ramblock_notifiers = {
    lh_first = 0x0
  }, 
  fd = -1, 
  page_size = 4096, 
  bmap = 0x0, 
  receivedmap = 0x562dc6a24a60, 
  clear_bmap = 0x0, 
  clear_bmap_shift = 0 '\000'
}

Cheers,

-- 
Anthony PERARD

[PATCH] Memory: Only call ramblock_ptr when needed in qemu_ram_writeback

[Anthony PERARD]

It is possible that a ramblock doesn't have memory that QEMU can
access, this is the case with the Xen hypervisor.

In order to avoid to trigger an assert, only call ramblock_ptr() when
needed in qemu_ram_writeback(). This should fix migration of Xen
guests that was broken with bd108a44bc29 ("migration: ram: Switch to
ram block writeback").

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 exec.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/exec.c b/exec.c
index a34c34818404..b11010e0cb4c 100644
--- a/exec.c
+++ b/exec.c
@@ -2166,14 +2166,13 @@ int qemu_ram_resize(RAMBlock *block, ram_addr_t newsize, Error **errp)
  */
 void qemu_ram_writeback(RAMBlock *block, ram_addr_t start, ram_addr_t length)
 {
-    void *addr = ramblock_ptr(block, start);
-
     /* The requested range should fit in within the block range */
     g_assert((start + length) <= block->used_length);
 
 #ifdef CONFIG_LIBPMEM
     /* The lack of support for pmem should not block the sync */
     if (ramblock_is_pmem(block)) {
+        void *addr = ramblock_ptr(block, start);
         pmem_persist(addr, length);
         return;
     }
@@ -2184,6 +2183,7 @@ void qemu_ram_writeback(RAMBlock *block, ram_addr_t start, ram_addr_t length)
          * specified as persistent (or is not one) - use the msync.
          * Less optimal but still achieves the same goal
          */
+        void *addr = ramblock_ptr(block, start);
         if (qemu_msync(addr, length, block->fd)) {
             warn_report("%s: failed to sync memory range: start: "
                     RAM_ADDR_FMT " length: " RAM_ADDR_FMT,
-- 
Anthony PERARD

Re: [PATCH] Memory: Only call ramblock_ptr when needed in qemu_ram_writeback

[Beata Michalska]

Hi Anthony,

On Thu, 19 Dec 2019 at 15:43, Anthony PERARD <anthony.perard@citrix.com> wrote:
>
> It is possible that a ramblock doesn't have memory that QEMU can
> access, this is the case with the Xen hypervisor.
>
> In order to avoid to trigger an assert, only call ramblock_ptr() when
> needed in qemu_ram_writeback(). This should fix migration of Xen
> guests that was broken with bd108a44bc29 ("migration: ram: Switch to
> ram block writeback").
>
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  exec.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index a34c34818404..b11010e0cb4c 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2166,14 +2166,13 @@ int qemu_ram_resize(RAMBlock *block, ram_addr_t newsize, Error **errp)
>   */
>  void qemu_ram_writeback(RAMBlock *block, ram_addr_t start, ram_addr_t length)
>  {
> -    void *addr = ramblock_ptr(block, start);
> -
>      /* The requested range should fit in within the block range */
>      g_assert((start + length) <= block->used_length);
>
>  #ifdef CONFIG_LIBPMEM
>      /* The lack of support for pmem should not block the sync */
>      if (ramblock_is_pmem(block)) {
> +        void *addr = ramblock_ptr(block, start);
>          pmem_persist(addr, length);
>          return;
>      }
> @@ -2184,6 +2183,7 @@ void qemu_ram_writeback(RAMBlock *block, ram_addr_t start, ram_addr_t length)
>           * specified as persistent (or is not one) - use the msync.
>           * Less optimal but still achieves the same goal
>           */
> +        void *addr = ramblock_ptr(block, start);
>          if (qemu_msync(addr, length, block->fd)) {
>              warn_report("%s: failed to sync memory range: start: "
>                      RAM_ADDR_FMT " length: " RAM_ADDR_FMT,

We could also do :
void *addr = block->host ? ramblock_ptr : NULL

Looks good to me thought.
Thanks for fixing.

BR

Beata
> --
> Anthony PERARD
>

Re: [PATCH] Memory: Only call ramblock_ptr when needed in qemu_ram_writeback

[Juan Quintela]

Anthony PERARD <anthony.perard@citrix.com> wrote:
> It is possible that a ramblock doesn't have memory that QEMU can
> access, this is the case with the Xen hypervisor.
>
> In order to avoid to trigger an assert, only call ramblock_ptr() when
> needed in qemu_ram_writeback(). This should fix migration of Xen
> guests that was broken with bd108a44bc29 ("migration: ram: Switch to
> ram block writeback").
>
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

This is exec.c, nothing related to migration.

Paolo, are you taking this one?
It could even go through the trivial one.

Thanks, Juan.

Re: [PATCH] Memory: Only call ramblock_ptr when needed in qemu_ram_writeback

[Anthony PERARD]

On Thu, Dec 19, 2019 at 07:10:24PM +0100, Juan Quintela wrote:
> Anthony PERARD <anthony.perard@citrix.com> wrote:
> > It is possible that a ramblock doesn't have memory that QEMU can
> > access, this is the case with the Xen hypervisor.
> >
> > In order to avoid to trigger an assert, only call ramblock_ptr() when
> > needed in qemu_ram_writeback(). This should fix migration of Xen
> > guests that was broken with bd108a44bc29 ("migration: ram: Switch to
> > ram block writeback").
> >
> > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> 
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> 
> This is exec.c, nothing related to migration.
> 
> Paolo, are you taking this one?
> It could even go through the trivial one.

Hi,

I'm going to send a pull request for the xen queue with this patch.
Unless that's an issue?

Cheers,

-- 
Anthony PERARD

Re: [PATCH] Memory: Only call ramblock_ptr when needed in qemu_ram_writeback

[Paolo Bonzini]

On 25/02/20 17:02, Anthony PERARD wrote:
> On Thu, Dec 19, 2019 at 07:10:24PM +0100, Juan Quintela wrote:
>> Anthony PERARD <anthony.perard@citrix.com> wrote:
>>> It is possible that a ramblock doesn't have memory that QEMU can
>>> access, this is the case with the Xen hypervisor.
>>>
>>> In order to avoid to trigger an assert, only call ramblock_ptr() when
>>> needed in qemu_ram_writeback(). This should fix migration of Xen
>>> guests that was broken with bd108a44bc29 ("migration: ram: Switch to
>>> ram block writeback").
>>>
>>> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
>>
>> Reviewed-by: Juan Quintela <quintela@redhat.com>
>>
>> This is exec.c, nothing related to migration.
>>
>> Paolo, are you taking this one?
>> It could even go through the trivial one.
> 
> Hi,
> 
> I'm going to send a pull request for the xen queue with this patch.
> Unless that's an issue?

No, absolutely.

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo

Re: Recent change pmem related breaks Xen migration

[Beata Michalska]

Hi Anthony,

On Thu, 19 Dec 2019 at 15:42, Anthony PERARD <anthony.perard@citrix.com> wrote:
>
> Hi,
>
> Commit bd108a44bc29 ("migration: ram: Switch to ram block writeback")
> breaks migration on Xen. We have:
>   ramblock_ptr: Assertion `offset_in_ramblock(block, offset)' failed.
>
> I've track it down to qemu_ram_writeback() calling ramblock_ptr()
> unconditionally, even when the result will not be used.
>
> Maybe we could call ramblock_ptr() twice in that function? I've prepared
> a patch.
>
>
> FYI, full-ish trace on restore of a xen guest:
> #3  0x00007f82d0848526 in __assert_fail () from /usr/lib/libc.so.6
> #4  0x0000562dc4578122 in ramblock_ptr (block=0x562dc5ebe2a0, offset=0) at /root/build/qemu/include/exec/ram_addr.h:120
> #5  0x0000562dc457d1b7 in qemu_ram_writeback (block=0x562dc5ebe2a0, start=0, length=515899392) at /root/build/qemu/exec.c:2169
> #6  0x0000562dc45e8941 in qemu_ram_block_writeback (block=0x562dc5ebe2a0) at /root/build/qemu/include/exec/ram_addr.h:182
> #7  0x0000562dc45f0b56 in ram_load_cleanup (opaque=0x562dc510fe00 <ram_state>) at /root/build/qemu/migration/ram.c:3983
> #8  0x0000562dc49970b6 in qemu_loadvm_state_cleanup () at migration/savevm.c:2415
> #9  0x0000562dc4997548 in qemu_loadvm_state (f=0x562dc6a1c600) at migration/savevm.c:2597
> #10 0x0000562dc4987be7 in process_incoming_migration_co (opaque=0x0) at migration/migration.c:454
> #11 0x0000562dc4b907e5 in coroutine_trampoline (i0=-962514432, i1=22061) at util/coroutine-ucontext.c:115
>
> And *block in ramblock_ptr():
> (gdb) p *block
> $2 = {
>   rcu = {
>     next = 0x0,
>     func = 0x0
>   },
>   mr = 0x562dc512e140 <ram_memory>,
>   host = 0x0,
>   colo_cache = 0x0,
>   offset = 0,
>   used_length = 515899392,
>   max_length = 515899392,
>   resized = 0x0,
>   flags = 16,
>   idstr = "xen.ram", '\000' <repeats 248 times>,
>   next = {
>     le_next = 0x562dc67bf7e0,
>     le_prev = 0x562dc510f1a0 <ram_list+64>
>   },
>   ramblock_notifiers = {
>     lh_first = 0x0
>   },
>   fd = -1,
>   page_size = 4096,
>   bmap = 0x0,
>   receivedmap = 0x562dc6a24a60,
>   clear_bmap = 0x0,
>   clear_bmap_shift = 0 '\000'
> }
>
> Cheers,
>
> --
> Anthony PERARD

I have already replied to your patch submission.
Looks good and thanks for fixing .

BR
Beata