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

[Igor Mammedov]

On Tue, 12 Jun 2018 15:43:58 +0000
Junyan He <address@hidden> wrote:

> static void file_memory_backend_set_pmem(Object *o, bool value, Error **errp)
> {
>     HostMemoryBackend *backend = MEMORY_BACKEND(o);
>     HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(o);
> 
>     if (host_memory_backend_mr_inited(backend)) {
>         error_setg(errp, "cannot change property 'pmem' of %s '%s'",
>                    object_get_typename(o),
>                    object_get_canonical_path_component(o));
>         return;
>     }
> #ifndef CONFIG_LIBPMEM
>     if (value) {
>         warn_report("Lack of libpmem support while setting the 'pmem' of"
>                     " %s '%s'. We can not ensure the persistence of it"
>                     " without libpmem support.", object_get_typename(o),
>                     object_get_canonical_path_component(o));

maybe:
"compiled without libpmem support. pmem is enabled in test mode and data 
persistence is not guarantied which could lead to data loss"

>     }
> #endif
>     fb->is_pmem = value;
> }
> 
> 
> 
> Is this kind of hint or warning acceptable?
warning might tell typical user that he/she was using pmem incorrectly
but user could see it to late in logs after damage is done.

So nicer way would be if test mode enable separately and pmem=on would work
only if we can guaranty that it would actually work regardless of what is used
(cloud be libpmem if file is on pmem device or fsync if using regular backing 
storage)


> 
> ________________________________
> From: Qemu-devel <address@hidden> on behalf of Junyan He <address@hidden>
> Sent: Tuesday, June 12, 2018 3:27:38 PM
> To: Igor Mammedov
> Cc: Haozhong Zhang; address@hidden; address@hidden; address@hidden; 
> address@hidden; address@hidden; address@hidden; Junyan He; address@hidden; 
> address@hidden; address@hidden; address@hidden
> Subject: Re: [Qemu-devel] [PATCH V6 0/7] nvdimm: guarantee persistence of 
> QEMU writes to persistent memory
> 
> According to my understand, the file node on real persistent memory do not 
> need to be pmem=on
> 
> pmem=on is a feature for file backend class. For example, if we do not have 
> enough hard disk space
> 
> and we have enough pmem, we can use file on pmem the same as normal 
> file-backend on hard disk.
> 
> That is the file-backend on pmem does not necessary to a nvdimm backend, it 
> can be normal file mapping as well.
> 
> so
> 
>  > detect that backing file is located on pmem storage  
> 
> may not  be a good manner.
> 
> 
> > (1) Maybe we should error out if pmem=on but compiled without libpmem
> > and add 3rd state pmem=force for testing purposes.  
> 
> I think we can print a warning message to give user a hint, it can really 
> help user
> 
> when they mis-configure.
> 
> 
> ________________________________
> From: Qemu-devel <address@hidden> on behalf of Igor Mammedov <address@hidden>
> Sent: Tuesday, June 12, 2018 2:55:46 PM
> To: Junyan He
> Cc: Haozhong Zhang; address@hidden; address@hidden; address@hidden; 
> address@hidden; address@hidden; address@hidden; address@hidden; Junyan He; 
> address@hidden; address@hidden; address@hidden
> Subject: Re: [Qemu-devel] [PATCH V6 0/7] nvdimm: guarantee persistence of 
> QEMU writes to persistent memory
> 
> On Tue, 12 Jun 2018 13:38:08 +0000
> Junyan He <address@hidden> wrote:
> 
> > He have pmem_persist and pmem_memcpy_persist stub functions.
> >
> > If no libpmem and user really specify pmem=on, we just do nothing or just 
> > memcpy.
> >
> > Real persistent memory always require libpmem support its load/save.
> >
> > If pmem=on and without libpmem, we can think that user want to imitate
> >
> > pmem=on while the HW environment is without real persistent memory existing.
> >
> > It may help debug on some machine without real pmem.  
> 
> For unaware user it would be easy to misconfigure and think that feature
> works while it isn't, which cloud lead to data loss.
> 
> (1) Maybe we should error out if pmem=on but compiled without libpmem
> and add 3rd state pmem=force for testing purposes.
> 
> Also can we detect that backing file is located on pmem storage and do [1] if 
> it's not?
> 
> >
> > ________________________________
> > From: Qemu-devel <address@hidden> on behalf of Igor Mammedov 
> > <address@hidden>
> > Sent: Tuesday, June 12, 2018 12:06:43 PM
> > To: address@hidden
> > Cc: Haozhong Zhang; address@hidden; address@hidden; address@hidden; 
> > address@hidden; address@hidden; address@hidden; Junyan He; address@hidden; 
> > address@hidden; address@hidden; address@hidden
> > Subject: Re: [Qemu-devel] [PATCH V6 0/7] nvdimm: guarantee persistence of 
> > QEMU writes to persistent memory
> >
> > On Fri,  1 Jun 2018 16:10:22 +0800
> > address@hidden wrote:
> >  
> > > From: Junyan He <address@hidden>
> > >
> > > 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.  
> >
> > extra question, what are expected behavior when QEMU is built without
> > libpmem and user specifies pmem=on for backend?
> >  
> > >
> > > 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
> > > v5: https://lists.gnu.org/archive/html/qemu-devel/2018-05/msg02258.html
> > > V4: https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg06993.html
> > > 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 v6:
> > > * (Patch 1) Expose all ram block flags rather than redefine the flags.
> > > * (Patch 4) Use pkg-config rather the hard check when configure.
> > > * (Patch 7) Sync and flush all the pmem data when migration completes,
> > > rather than sync pages one by one in previous version.
> > >
> > > Changes in v5:
> > > * (Patch 9) Add post copy check and output some messages for nvdimm.
> > >
> > > 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.
> > >
> > >
> > > Junyan:
> > > [1/7] memory, exec: Expose all memory block related flags.
> > > [6/7] migration/ram: Add check and info message to nvdimm post copy.
> > > [7/7] migration/ram: ensure write persistence on loading all date to PMEM.
> > >
> > > Haozhong:
> > > [5/7] mem/nvdimm: ensure write persistence to PMEM in label emulation
> > >
> > > Haozhong & Junyan:
> > > [2/7] memory, exec: switch file ram allocation functions to 'flags' 
> > > parameters
> > > [3/7] hostmem-file: add the 'pmem' option
> > > [4/7] configure: add libpmem support
> > >
> > >
> > > Signed-off-by: Haozhong Zhang <address@hidden>
> > > Signed-off-by: Junyan He <address@hidden>
> > >
> > > ---
> > > backends/hostmem-file.c | 28 +++++++++++++++++++++++++++-
> > > configure               | 29 +++++++++++++++++++++++++++++
> > > docs/nvdimm.txt         | 14 ++++++++++++++
> > > exec.c                  | 36 ++++++++++++++----------------------
> > > hw/mem/nvdimm.c         |  9 ++++++++-
> > > include/exec/memory.h   | 31 +++++++++++++++++++++++++++++--
> > > include/exec/ram_addr.h | 28 ++++++++++++++++++++++++++--
> > > include/qemu/pmem.h     | 24 ++++++++++++++++++++++++
> > > memory.c                |  8 +++++---
> > > migration/ram.c         | 18 ++++++++++++++++++
> > > numa.c                  |  2 +-
> > > qemu-options.hx         |  7 +++++++
> > > stubs/Makefile.objs     |  1 +
> > > stubs/pmem.c            | 23 +++++++++++++++++++++++
> > > 14 files changed, 226 insertions(+), 32 deletions(-)  
> >
> >  
> 
>