Re: [Qemu-devel] [PATCH v4 0/8] nvdimm: guarantee persistence of QEMU writes to persistent memory

[Stefan Hajnoczi <stefanha@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4632 bytes --]

On Wed, Feb 28, 2018 at 03:25:50PM +0800, Haozhong Zhang wrote:
> QEMU writes to vNVDIMM backends in the vNVDIMM label emulation and
> live migration. If the backend is on the persistent memory, QEMU needs
> to take proper operations to ensure its writes persistent on the
> persistent memory. Otherwise, a host power failure may result in the
> loss the guest data on the persistent memory.
> 
> This v3 patch series is based on Marcel's patch "mem: add share
> parameter to memory-backend-ram" [1] because of the changes in patch 1.
> 
> [1] https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg03858.html
> 
> Previous versions can be found at
> v3: https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg04365.html
> v2: https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg01579.html
> v1: https://lists.gnu.org/archive/html/qemu-devel/2017-12/msg05040.html
> 
> Changes in v4:
>  * (Patch 2) Fix compilation errors found by patchew.
> 
> Changes in v3:
>  * (Patch 5) Add a is_pmem flag to ram_handle_compressed() and handle
>    PMEM writes in it, so we don't need the _common function.
>  * (Patch 6) Expose qemu_get_buffer_common so we can remove the
>    unnecessary qemu_get_buffer_to_pmem wrapper.
>  * (Patch 8) Add a is_pmem flag to xbzrle_decode_buffer() and handle
>    PMEM writes in it, so we can remove the unnecessary
>    xbzrle_decode_buffer_{common, to_pmem}.
>  * Move libpmem stubs to stubs/pmem.c and fix the compilation failures
>    of test-{xbzrle,vmstate}.c.
> 
> Changes in v2:
>  * (Patch 1) Use a flags parameter in file ram allocation functions.
>  * (Patch 2) Add a new option 'pmem' to hostmem-file.
>  * (Patch 3) Use libpmem to operate on the persistent memory, rather
>    than re-implementing those operations in QEMU.
>  * (Patch 5-8) Consider the write persistence in the migration path.
> 
> Haozhong Zhang (8):
>   [1/8] memory, exec: switch file ram allocation functions to 'flags' parameters
>   [2/8] hostmem-file: add the 'pmem' option
>   [3/8] configure: add libpmem support
>   [4/8] mem/nvdimm: ensure write persistence to PMEM in label emulation
>   [5/8] migration/ram: ensure write persistence on loading zero pages to PMEM
>   [6/8] migration/ram: ensure write persistence on loading normal pages to PMEM
>   [7/8] migration/ram: ensure write persistence on loading compressed pages to PMEM
>   [8/8] migration/ram: ensure write persistence on loading xbzrle pages to PMEM
> 
>  backends/hostmem-file.c             | 27 +++++++++++++++++++-
>  configure                           | 35 ++++++++++++++++++++++++++
>  docs/nvdimm.txt                     | 14 +++++++++++
>  exec.c                              | 20 ++++++++++++---
>  hw/mem/nvdimm.c                     |  9 ++++++-
>  include/exec/memory.h               | 12 +++++++--
>  include/exec/ram_addr.h             | 28 +++++++++++++++++++--
>  include/migration/qemu-file-types.h |  2 ++
>  include/qemu/pmem.h                 | 27 ++++++++++++++++++++
>  memory.c                            |  8 +++---
>  migration/qemu-file.c               | 29 ++++++++++++++--------
>  migration/ram.c                     | 49 +++++++++++++++++++++++++++----------
>  migration/ram.h                     |  2 +-
>  migration/rdma.c                    |  2 +-
>  migration/xbzrle.c                  |  8 ++++--
>  migration/xbzrle.h                  |  3 ++-
>  numa.c                              |  2 +-
>  qemu-options.hx                     |  9 ++++++-
>  stubs/Makefile.objs                 |  1 +
>  stubs/pmem.c                        | 37 ++++++++++++++++++++++++++++
>  tests/Makefile.include              |  4 +--
>  tests/test-xbzrle.c                 |  4 +--
>  22 files changed, 285 insertions(+), 47 deletions(-)
>  create mode 100644 include/qemu/pmem.h
>  create mode 100644 stubs/pmem.c

A few thoughts:

1. Can you use pmem_is_pmem() to auto-detect the pmem=on|off value?

2. The migration/ram code is invasive.  Is it really necessary to
   persist data each time pages are loaded from a migration stream?  It
   seems simpler to migrate as normal and call pmem_persist() just once
   after RAM has been migrated but before the migration completes.

3. This is independent of this patch series and can be done later.
   NVDIMM seems incompatible with post-copy live migration.  It would be
   good to have a postcopy_add_blocker() API so that a nice error
   message is printed if post-copy live migration is attempted.

The code itself seems fine though:

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]