Re: [Qemu-devel] [PATCH 00/10] RFC: Optimize nvdimm kind memory for snapshot.

[Stefan Hajnoczi <stefanha@gmail.com>]

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

On Tue, Mar 13, 2018 at 04:33:43PM +0800, junyan.he@intel.com wrote:
> From: Junyan He <junyan.he@intel.com>
> 
> The nvdimm size is huge, sometimes it is more than 256G or even more.
> This is a huge burden for snapshot saving. One snapshot point with
> nvdimm may occupy more than 50G disk space even with compression
> enabled.
> We need to introduce dependent snapshot manner to solve this problem.

What is the status of this patch series?

Kevin and Juan: Review is needed from the qcow2 and migration maintainers.

This patch series adds a function to clone qcow2 snapshots (sharing
clusters via the refcount) and then uses it to snapshot NVDIMM contents
incrementally during savevm.  This way only dirty NVDIMM clusters need
to be written into the qcow2 file, saving a lot of space and time.  The
drawback is that dirty memory logging needs to be enabled while the
guest is running.

The approach makes sense to me and could be used for other migration
state beyond NVDIMM in the future.  I think it's worth iterating this
patch series and merging it.

It was difficult for me to understand some of the English.  Do you have
a colleague who can review the English and suggest how to rephrase the
text?

I didn't found the "snapshot dependency" name unclear.  I suggest
calling the new API .bdrv_snapshot_clone().  This makes it clear that a
new snapshot is created based on an existing one.

New APIs are missing doc comments.  Please document the functions,
arguments, and the return values - especially for extern functions
(those declared in header files).

> The first snapshot point should always be saved completely, and enable
> dirty log trace after saving for nvdimm memory region. The later snapshot
> point should add the reference to previous snapshot's nvdimm data and
> just saving dirty pages. This can save a lot of disk and time if the
> snapshot operations are triggered frequently.
> We add save_snapshot_dependency functions to QCOW2 file system firstly, the
> later snapshot will add reference to previous dependent snapshot's data
> cluster. There is an alignment problem here, the dependent data should
> always be cluster aligned. We need to add some padding data when saving
> the snapshot to make it always cluster aligned.
> The logic between nvdimm and ram for snapshot saving is a little confused
> now, we need to exclude nvdimm kind memory region from ram list and the
> dirty log tracing setting is also not very clear. Maybe we can separate the
> snapshot saving from the migration logic later to make code clean.
> In theory, this kind of manner can apply to any kind of memory. But because
> it need to turn dirty log trace on, the performance may decline. So we just
> enable it for nvdimm kind memory firstly.
> 
> Signed-off-by: Junyan He <junyan.he@intel.com>
> ---
> Makefile.target              |    1 +
> block/qcow2-snapshot.c       |  154 ++++++++++++++++++++++
> block/qcow2.c                |    2 +
> block/qcow2.h                |    7 +
> block/snapshot.c             |   45 +++++++
> exec.c                       |    7 +
> hw/ppc/spapr.c               |    2 +-
> hw/s390x/s390-stattrib.c     |    2 +-
> include/block/block_int.h    |    9 ++
> include/block/snapshot.h     |    7 +
> include/exec/memory.h        |    9 ++
> include/exec/ram_addr.h      |    2 +
> include/migration/misc.h     |    4 +
> include/migration/register.h |    2 +-
> include/migration/snapshot.h |    3 +
> memory.c                     |   18 ++-
> migration/block.c            |    2 +-
> migration/nvdimm.c           | 1033 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> migration/qemu-file.c        |   61 +++++++++
> migration/qemu-file.h        |   14 ++
> migration/ram.c              |   19 ++-
> migration/savevm.c           |   62 ++++++++-
> vl.c                         |    1 +
> 23 files changed, 1452 insertions(+), 14 deletions(-)
> 

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