[Qemu-devel] [PATCH v2 0/3] hostmem-file: Add "discard-data" option

[Qemu-devel] [PATCH v2 0/3] hostmem-file: Add "discard-data" option

[Eduardo Habkost]

This series adds a new "discard-data" option to
memory-backend-file.  The new option will be useful if somebody
is sharing RAM contents on a pre-existing file using share=on,
but don't need data to be flushed to disk when QEMU exits.

Internally, it will trigger a madvise(MADV_REMOVE) call when the
memory backend is removed or when QEMU exits.

To make we actually trigger the new code when QEMU exits, the
first patch in the series ensures we destroy all user-created
objects when exiting QEMU.

Changes v1 -> v2:
* Original subject line of v1 was:
  '[PATCH 0/5] hostmem-file: Add "persistent" option'
* Replaced 'persistent=no' with 'discard-data=yes', to make it
  clear that the flag will destroy data on the backing file.
* Use qemu_madvise() instead of madvise()
  * New patch added to series: "osdep: define QEMU_MADV_REMOVE"
* Call qemu_madvise() directly from the backend unparent()
  method, insteead of adding a new flag to the memory API and
  reusing ram_block_discard_range()
  * In addition to simplifying the code a lot, this fixes a bug,
    because v1 relied on getting the memory region reference
    count back to 0, which doesn't happen when QEMU is exiting
    because there's no machine cleanup code to ensure that.

Eduardo Habkost (3):
  vl: Clean up user-creatable objects when exiting
  osdep: Define QEMU_MADV_REMOVE
  hostmem-file: Add "discard-data" option

 include/qemu/osdep.h            |  7 +++++++
 include/qom/object_interfaces.h |  8 ++++++++
 backends/hostmem-file.c         | 29 +++++++++++++++++++++++++++++
 qom/object_interfaces.c         |  5 +++++
 vl.c                            |  1 +
 qemu-options.hx                 |  5 ++++-
 6 files changed, 54 insertions(+), 1 deletion(-)

-- 
2.9.4

[Qemu-devel] [PATCH v2 1/3] vl: Clean up user-creatable objects when exiting

[Eduardo Habkost]

Delete all user-creatable objects in /objects when exiting QEMU, so they
can perform cleanup actions.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Changes v1 -> v2:
* (none)
---
 include/qom/object_interfaces.h | 8 ++++++++
 qom/object_interfaces.c         | 5 +++++
 vl.c                            | 1 +
 3 files changed, 14 insertions(+)

diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
index fdd7603..3f5f206 100644
--- a/include/qom/object_interfaces.h
+++ b/include/qom/object_interfaces.h
@@ -148,4 +148,12 @@ int user_creatable_add_opts_foreach(void *opaque,
  */
 void user_creatable_del(const char *id, Error **errp);
 
+/**
+ * user_creatable_cleanup:
+ *
+ * Delete all user-creatable objects and the user-creatable
+ * objects container.
+ */
+void user_creatable_cleanup(void);
+
 #endif
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index ff27e06..dbf8878 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -193,6 +193,11 @@ void user_creatable_del(const char *id, Error **errp)
     object_unparent(obj);
 }
 
+void user_creatable_cleanup(void)
+{
+    object_unparent(object_get_objects_root());
+}
+
 static void register_types(void)
 {
     static const TypeInfo uc_interface_info = {
diff --git a/vl.c b/vl.c
index 8e247cc..91d0ef0 100644
--- a/vl.c
+++ b/vl.c
@@ -4801,6 +4801,7 @@ int main(int argc, char **argv, char **envp)
     audio_cleanup();
     monitor_cleanup();
     qemu_chr_cleanup();
+    user_creatable_cleanup();
     /* TODO: unref root container, check all devices are ok */
 
     return 0;
-- 
2.9.4

Re: [Qemu-devel] [PATCH v2 1/3] vl: Clean up user-creatable objects when exiting

[Philippe Mathieu-Daudé]

On 08/24/2017 04:23 PM, Eduardo Habkost wrote:
> Delete all user-creatable objects in /objects when exiting QEMU, so they
> can perform cleanup actions.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

Acked-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
> Changes v1 -> v2:
> * (none)
> ---
>   include/qom/object_interfaces.h | 8 ++++++++
>   qom/object_interfaces.c         | 5 +++++
>   vl.c                            | 1 +
>   3 files changed, 14 insertions(+)
> 
> diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
> index fdd7603..3f5f206 100644
> --- a/include/qom/object_interfaces.h
> +++ b/include/qom/object_interfaces.h
> @@ -148,4 +148,12 @@ int user_creatable_add_opts_foreach(void *opaque,
>    */
>   void user_creatable_del(const char *id, Error **errp);
>   
> +/**
> + * user_creatable_cleanup:
> + *
> + * Delete all user-creatable objects and the user-creatable
> + * objects container.
> + */
> +void user_creatable_cleanup(void);
> +
>   #endif
> diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
> index ff27e06..dbf8878 100644
> --- a/qom/object_interfaces.c
> +++ b/qom/object_interfaces.c
> @@ -193,6 +193,11 @@ void user_creatable_del(const char *id, Error **errp)
>       object_unparent(obj);
>   }
>   
> +void user_creatable_cleanup(void)
> +{
> +    object_unparent(object_get_objects_root());
> +}
> +
>   static void register_types(void)
>   {
>       static const TypeInfo uc_interface_info = {
> diff --git a/vl.c b/vl.c
> index 8e247cc..91d0ef0 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4801,6 +4801,7 @@ int main(int argc, char **argv, char **envp)
>       audio_cleanup();
>       monitor_cleanup();
>       qemu_chr_cleanup();
> +    user_creatable_cleanup();
>       /* TODO: unref root container, check all devices are ok */
>   
>       return 0;
> 

[Qemu-devel] [PATCH v2 2/3] osdep: Define QEMU_MADV_REMOVE

[Eduardo Habkost]

Define QEMU_MADV_REMOVE, so we can use it with qemu_madvise().

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Changes v1 -> v2:
* New patch added to series
---
 include/qemu/osdep.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 6855b94..e9fa217 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -257,6 +257,11 @@ void qemu_anon_ram_free(void *ptr, size_t size);
 #else
 #define QEMU_MADV_NOHUGEPAGE QEMU_MADV_INVALID
 #endif
+#ifdef MADV_REMOVE
+#define QEMU_MADV_REMOVE MADV_REMOVE
+#else
+#define QEMU_MADV_REMOVE QEMU_MADV_INVALID
+#endif
 
 #elif defined(CONFIG_POSIX_MADVISE)
 
@@ -269,6 +274,7 @@ void qemu_anon_ram_free(void *ptr, size_t size);
 #define QEMU_MADV_DONTDUMP QEMU_MADV_INVALID
 #define QEMU_MADV_HUGEPAGE  QEMU_MADV_INVALID
 #define QEMU_MADV_NOHUGEPAGE  QEMU_MADV_INVALID
+#define QEMU_MADV_REMOVE QEMU_MADV_INVALID
 
 #else /* no-op */
 
@@ -281,6 +287,7 @@ void qemu_anon_ram_free(void *ptr, size_t size);
 #define QEMU_MADV_DONTDUMP QEMU_MADV_INVALID
 #define QEMU_MADV_HUGEPAGE  QEMU_MADV_INVALID
 #define QEMU_MADV_NOHUGEPAGE  QEMU_MADV_INVALID
+#define QEMU_MADV_REMOVE QEMU_MADV_INVALID
 
 #endif
 
-- 
2.9.4

Re: [Qemu-devel] [PATCH v2 2/3] osdep: Define QEMU_MADV_REMOVE

[Dr. David Alan Gilbert]

* Eduardo Habkost (ehabkost@redhat.com) wrote:
> Define QEMU_MADV_REMOVE, so we can use it with qemu_madvise().
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

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

> ---
> Changes v1 -> v2:
> * New patch added to series
> ---
>  include/qemu/osdep.h | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 6855b94..e9fa217 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -257,6 +257,11 @@ void qemu_anon_ram_free(void *ptr, size_t size);
>  #else
>  #define QEMU_MADV_NOHUGEPAGE QEMU_MADV_INVALID
>  #endif
> +#ifdef MADV_REMOVE
> +#define QEMU_MADV_REMOVE MADV_REMOVE
> +#else
> +#define QEMU_MADV_REMOVE QEMU_MADV_INVALID
> +#endif
>  
>  #elif defined(CONFIG_POSIX_MADVISE)
>  
> @@ -269,6 +274,7 @@ void qemu_anon_ram_free(void *ptr, size_t size);
>  #define QEMU_MADV_DONTDUMP QEMU_MADV_INVALID
>  #define QEMU_MADV_HUGEPAGE  QEMU_MADV_INVALID
>  #define QEMU_MADV_NOHUGEPAGE  QEMU_MADV_INVALID
> +#define QEMU_MADV_REMOVE QEMU_MADV_INVALID
>  
>  #else /* no-op */
>  
> @@ -281,6 +287,7 @@ void qemu_anon_ram_free(void *ptr, size_t size);
>  #define QEMU_MADV_DONTDUMP QEMU_MADV_INVALID
>  #define QEMU_MADV_HUGEPAGE  QEMU_MADV_INVALID
>  #define QEMU_MADV_NOHUGEPAGE  QEMU_MADV_INVALID
> +#define QEMU_MADV_REMOVE QEMU_MADV_INVALID
>  
>  #endif
>  
> -- 
> 2.9.4
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[Qemu-devel] [PATCH v2 3/3] hostmem-file: Add "discard-data" option

[Eduardo Habkost]

The new option can be used to indicate that the file contents can
be destroyed and don't need to be flushed to disk when QEMU exits
or when the memory backend object is removed.

Internally, it will trigger a madvise(MADV_REMOVE) call when the
memory backend is removed.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Changes v1 -> v2:
* Replaced 'persistent=no' with 'discard-data=yes', to make it clear
  that the flag will destroy data on the backing file.
* Call madvise() directly from unparent() method instead of
  relying on low-level memory backend code to call it.
  v1 relied on getting the memory region reference count back to
  0, which doesn't happen when QEMU is exiting because there's no
  machine cleanup code to ensure that.
---
 backends/hostmem-file.c | 29 +++++++++++++++++++++++++++++
 qemu-options.hx         |  5 ++++-
 2 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
index fc4ef46..e44c319 100644
--- a/backends/hostmem-file.c
+++ b/backends/hostmem-file.c
@@ -32,6 +32,7 @@ struct HostMemoryBackendFile {
     HostMemoryBackend parent_obj;
 
     bool share;
+    bool discard_data;
     char *mem_path;
 };
 
@@ -103,16 +104,44 @@ static void file_memory_backend_set_share(Object *o, bool value, Error **errp)
     fb->share = value;
 }
 
+static bool file_memory_backend_get_discard_data(Object *o, Error **errp)
+{
+    return MEMORY_BACKEND_FILE(o)->discard_data;
+}
+
+static void file_memory_backend_set_discard_data(Object *o, bool value,
+                                               Error **errp)
+{
+    MEMORY_BACKEND_FILE(o)->discard_data = value;
+}
+
+static void file_backend_unparent(Object *obj)
+{
+    HostMemoryBackend *backend = MEMORY_BACKEND(obj);
+    HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(obj);
+
+    if (host_memory_backend_mr_inited(backend) && fb->discard_data) {
+        void *ptr = memory_region_get_ram_ptr(&backend->mr);
+        uint64_t sz = memory_region_size(&backend->mr);
+
+        qemu_madvise(ptr, sz, QEMU_MADV_REMOVE);
+    }
+}
+
 static void
 file_backend_class_init(ObjectClass *oc, void *data)
 {
     HostMemoryBackendClass *bc = MEMORY_BACKEND_CLASS(oc);
 
     bc->alloc = file_backend_memory_alloc;
+    oc->unparent = file_backend_unparent;
 
     object_class_property_add_bool(oc, "share",
         file_memory_backend_get_share, file_memory_backend_set_share,
         &error_abort);
+    object_class_property_add_bool(oc, "discard-data",
+        file_memory_backend_get_discard_data, file_memory_backend_set_discard_data,
+        &error_abort);
     object_class_property_add_str(oc, "mem-path",
         get_mem_path, set_mem_path,
         &error_abort);
diff --git a/qemu-options.hx b/qemu-options.hx
index 9f6e2ad..ad985e4 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -4160,7 +4160,7 @@ property must be set.  These objects are placed in the
 
 @table @option
 
-@item -object memory-backend-file,id=@var{id},size=@var{size},mem-path=@var{dir},share=@var{on|off}
+@item -object memory-backend-file,id=@var{id},size=@var{size},mem-path=@var{dir},share=@var{on|off},discard-data=@var{on|off}
 
 Creates a memory file backend object, which can be used to back
 the guest RAM with huge pages. The @option{id} parameter is a
@@ -4172,6 +4172,9 @@ the path to either a shared memory or huge page filesystem mount.
 The @option{share} boolean option determines whether the memory
 region is marked as private to QEMU, or shared. The latter allows
 a co-operating external process to access the QEMU memory region.
+Setting the @option{discard-data} boolean option to @var{on}
+indicates that file contents can be destroyed when QEMU exits,
+to avoid unnecessarily flushing data to the backing file.
 
 @item -object rng-random,id=@var{id},filename=@var{/dev/random}
 
-- 
2.9.4

Re: [Qemu-devel] [PATCH v2 3/3] hostmem-file: Add "discard-data" option

[Daniel P. Berrange]

On Thu, Aug 24, 2017 at 04:23:15PM -0300, Eduardo Habkost wrote:
> The new option can be used to indicate that the file contents can
> be destroyed and don't need to be flushed to disk when QEMU exits
> or when the memory backend object is removed.
> 
> Internally, it will trigger a madvise(MADV_REMOVE) call when the
> memory backend is removed.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Changes v1 -> v2:
> * Replaced 'persistent=no' with 'discard-data=yes', to make it clear
>   that the flag will destroy data on the backing file.
> * Call madvise() directly from unparent() method instead of
>   relying on low-level memory backend code to call it.
>   v1 relied on getting the memory region reference count back to
>   0, which doesn't happen when QEMU is exiting because there's no
>   machine cleanup code to ensure that.
> ---
>  backends/hostmem-file.c | 29 +++++++++++++++++++++++++++++
>  qemu-options.hx         |  5 ++++-
>  2 files changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
> index fc4ef46..e44c319 100644
> --- a/backends/hostmem-file.c
> +++ b/backends/hostmem-file.c
> @@ -32,6 +32,7 @@ struct HostMemoryBackendFile {
>      HostMemoryBackend parent_obj;
>  
>      bool share;
> +    bool discard_data;
>      char *mem_path;
>  };
>  
> @@ -103,16 +104,44 @@ static void file_memory_backend_set_share(Object *o, bool value, Error **errp)
>      fb->share = value;
>  }
>  
> +static bool file_memory_backend_get_discard_data(Object *o, Error **errp)
> +{
> +    return MEMORY_BACKEND_FILE(o)->discard_data;
> +}
> +
> +static void file_memory_backend_set_discard_data(Object *o, bool value,
> +                                               Error **errp)
> +{
> +    MEMORY_BACKEND_FILE(o)->discard_data = value;
> +}
> +
> +static void file_backend_unparent(Object *obj)
> +{
> +    HostMemoryBackend *backend = MEMORY_BACKEND(obj);
> +    HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(obj);
> +
> +    if (host_memory_backend_mr_inited(backend) && fb->discard_data) {
> +        void *ptr = memory_region_get_ram_ptr(&backend->mr);
> +        uint64_t sz = memory_region_size(&backend->mr);
> +
> +        qemu_madvise(ptr, sz, QEMU_MADV_REMOVE);
> +    }
> +}
> +
>  static void
>  file_backend_class_init(ObjectClass *oc, void *data)
>  {
>      HostMemoryBackendClass *bc = MEMORY_BACKEND_CLASS(oc);
>  
>      bc->alloc = file_backend_memory_alloc;
> +    oc->unparent = file_backend_unparent;
>  
>      object_class_property_add_bool(oc, "share",
>          file_memory_backend_get_share, file_memory_backend_set_share,
>          &error_abort);
> +    object_class_property_add_bool(oc, "discard-data",
> +        file_memory_backend_get_discard_data, file_memory_backend_set_discard_data,
> +        &error_abort);
>      object_class_property_add_str(oc, "mem-path",
>          get_mem_path, set_mem_path,
>          &error_abort);
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 9f6e2ad..ad985e4 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -4160,7 +4160,7 @@ property must be set.  These objects are placed in the
>  
>  @table @option
>  
> -@item -object memory-backend-file,id=@var{id},size=@var{size},mem-path=@var{dir},share=@var{on|off}
> +@item -object memory-backend-file,id=@var{id},size=@var{size},mem-path=@var{dir},share=@var{on|off},discard-data=@var{on|off}
>  
>  Creates a memory file backend object, which can be used to back
>  the guest RAM with huge pages. The @option{id} parameter is a
> @@ -4172,6 +4172,9 @@ the path to either a shared memory or huge page filesystem mount.
>  The @option{share} boolean option determines whether the memory
>  region is marked as private to QEMU, or shared. The latter allows
>  a co-operating external process to access the QEMU memory region.
> +Setting the @option{discard-data} boolean option to @var{on}
> +indicates that file contents can be destroyed when QEMU exits,
> +to avoid unnecessarily flushing data to the backing file.

We should note that this only works if QEMU shuts down normally. If QEMU
is aggressively killed (SIGKILL) or aborts for some reason, then we'll
never get a chance to invoke madvise(), so presumably the kernel will
still flush the data

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: [Qemu-devel] [PATCH v2 3/3] hostmem-file: Add "discard-data" option

[Eduardo Habkost]

On Tue, Aug 29, 2017 at 12:13:45PM +0100, Daniel P. Berrange wrote:
> On Thu, Aug 24, 2017 at 04:23:15PM -0300, Eduardo Habkost wrote:
> > The new option can be used to indicate that the file contents can
> > be destroyed and don't need to be flushed to disk when QEMU exits
> > or when the memory backend object is removed.
> > 
> > Internally, it will trigger a madvise(MADV_REMOVE) call when the
> > memory backend is removed.
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> > Changes v1 -> v2:
> > * Replaced 'persistent=no' with 'discard-data=yes', to make it clear
> >   that the flag will destroy data on the backing file.
> > * Call madvise() directly from unparent() method instead of
> >   relying on low-level memory backend code to call it.
> >   v1 relied on getting the memory region reference count back to
> >   0, which doesn't happen when QEMU is exiting because there's no
> >   machine cleanup code to ensure that.
> > ---
> >  backends/hostmem-file.c | 29 +++++++++++++++++++++++++++++
> >  qemu-options.hx         |  5 ++++-
> >  2 files changed, 33 insertions(+), 1 deletion(-)
> > 
> > diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
> > index fc4ef46..e44c319 100644
> > --- a/backends/hostmem-file.c
> > +++ b/backends/hostmem-file.c
> > @@ -32,6 +32,7 @@ struct HostMemoryBackendFile {
> >      HostMemoryBackend parent_obj;
> >  
> >      bool share;
> > +    bool discard_data;
> >      char *mem_path;
> >  };
> >  
> > @@ -103,16 +104,44 @@ static void file_memory_backend_set_share(Object *o, bool value, Error **errp)
> >      fb->share = value;
> >  }
> >  
> > +static bool file_memory_backend_get_discard_data(Object *o, Error **errp)
> > +{
> > +    return MEMORY_BACKEND_FILE(o)->discard_data;
> > +}
> > +
> > +static void file_memory_backend_set_discard_data(Object *o, bool value,
> > +                                               Error **errp)
> > +{
> > +    MEMORY_BACKEND_FILE(o)->discard_data = value;
> > +}
> > +
> > +static void file_backend_unparent(Object *obj)
> > +{
> > +    HostMemoryBackend *backend = MEMORY_BACKEND(obj);
> > +    HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(obj);
> > +
> > +    if (host_memory_backend_mr_inited(backend) && fb->discard_data) {
> > +        void *ptr = memory_region_get_ram_ptr(&backend->mr);
> > +        uint64_t sz = memory_region_size(&backend->mr);
> > +
> > +        qemu_madvise(ptr, sz, QEMU_MADV_REMOVE);
> > +    }
> > +}
> > +
> >  static void
> >  file_backend_class_init(ObjectClass *oc, void *data)
> >  {
> >      HostMemoryBackendClass *bc = MEMORY_BACKEND_CLASS(oc);
> >  
> >      bc->alloc = file_backend_memory_alloc;
> > +    oc->unparent = file_backend_unparent;
> >  
> >      object_class_property_add_bool(oc, "share",
> >          file_memory_backend_get_share, file_memory_backend_set_share,
> >          &error_abort);
> > +    object_class_property_add_bool(oc, "discard-data",
> > +        file_memory_backend_get_discard_data, file_memory_backend_set_discard_data,
> > +        &error_abort);
> >      object_class_property_add_str(oc, "mem-path",
> >          get_mem_path, set_mem_path,
> >          &error_abort);
> > diff --git a/qemu-options.hx b/qemu-options.hx
> > index 9f6e2ad..ad985e4 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -4160,7 +4160,7 @@ property must be set.  These objects are placed in the
> >  
> >  @table @option
> >  
> > -@item -object memory-backend-file,id=@var{id},size=@var{size},mem-path=@var{dir},share=@var{on|off}
> > +@item -object memory-backend-file,id=@var{id},size=@var{size},mem-path=@var{dir},share=@var{on|off},discard-data=@var{on|off}
> >  
> >  Creates a memory file backend object, which can be used to back
> >  the guest RAM with huge pages. The @option{id} parameter is a
> > @@ -4172,6 +4172,9 @@ the path to either a shared memory or huge page filesystem mount.
> >  The @option{share} boolean option determines whether the memory
> >  region is marked as private to QEMU, or shared. The latter allows
> >  a co-operating external process to access the QEMU memory region.
> > +Setting the @option{discard-data} boolean option to @var{on}
> > +indicates that file contents can be destroyed when QEMU exits,
> > +to avoid unnecessarily flushing data to the backing file.
> 
> We should note that this only works if QEMU shuts down normally. If QEMU
> is aggressively killed (SIGKILL) or aborts for some reason, then we'll
> never get a chance to invoke madvise(), so presumably the kernel will
> still flush the data

Good point.  I tried to not give any guarantees by saying
"contents _can_ be destroyed", but users may still have different
expectations.

I will change it to:

  Setting the @option{discard-data} boolean option to @var{on}
  indicates that file contents can be destroyed when QEMU exits,
  to avoid unnecessarily flushing data to the backing file.  Note
  that @option{discard-data} is only an optimization, and QEMU
  might not discard file contents if it aborts unexpectedly or is
  terminated using SIGKILL.

-- 
Eduardo

[Qemu-devel] [PATCH] fixup! hostmem-file: Add "discard-data" option

[Eduardo Habkost]

On Tue, Aug 29, 2017 at 10:12:58AM -0300, Eduardo Habkost wrote:
> On Tue, Aug 29, 2017 at 12:13:45PM +0100, Daniel P. Berrange wrote:
> > On Thu, Aug 24, 2017 at 04:23:15PM -0300, Eduardo Habkost wrote:
[...]
> > > @@ -4172,6 +4172,9 @@ the path to either a shared memory or huge page filesystem mount.
> > >  The @option{share} boolean option determines whether the memory
> > >  region is marked as private to QEMU, or shared. The latter allows
> > >  a co-operating external process to access the QEMU memory region.
> > > +Setting the @option{discard-data} boolean option to @var{on}
> > > +indicates that file contents can be destroyed when QEMU exits,
> > > +to avoid unnecessarily flushing data to the backing file.
> > 
> > We should note that this only works if QEMU shuts down normally. If QEMU
> > is aggressively killed (SIGKILL) or aborts for some reason, then we'll
> > never get a chance to invoke madvise(), so presumably the kernel will
> > still flush the data
> 
> Good point.  I tried to not give any guarantees by saying
> "contents _can_ be destroyed", but users may still have different
> expectations.
> 
> I will change it to:
> 
>   Setting the @option{discard-data} boolean option to @var{on}
>   indicates that file contents can be destroyed when QEMU exits,
>   to avoid unnecessarily flushing data to the backing file.  Note
>   that @option{discard-data} is only an optimization, and QEMU
>   might not discard file contents if it aborts unexpectedly or is
>   terminated using SIGKILL.

Fixup patch:

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 qemu-options.hx | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/qemu-options.hx b/qemu-options.hx
index ad985e4..de9a18a 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -4174,7 +4174,10 @@ region is marked as private to QEMU, or shared. The latter allows
 a co-operating external process to access the QEMU memory region.
 Setting the @option{discard-data} boolean option to @var{on}
 indicates that file contents can be destroyed when QEMU exits,
-to avoid unnecessarily flushing data to the backing file.
+to avoid unnecessarily flushing data to the backing file.  Note
+that @option{discard-data} is only an optimization, and QEMU
+might not discard file contents if it aborts unexpectedly or is
+terminated using SIGKILL.
 
 @item -object rng-random,id=@var{id},filename=@var{/dev/random}
 
-- 
2.9.4

Re: [Qemu-devel] [PATCH] fixup! hostmem-file: Add "discard-data" option

[Daniel P. Berrange]

On Tue, Aug 29, 2017 at 06:36:57PM -0300, Eduardo Habkost wrote:
> On Tue, Aug 29, 2017 at 10:12:58AM -0300, Eduardo Habkost wrote:
> > On Tue, Aug 29, 2017 at 12:13:45PM +0100, Daniel P. Berrange wrote:
> > > On Thu, Aug 24, 2017 at 04:23:15PM -0300, Eduardo Habkost wrote:
> [...]
> > > > @@ -4172,6 +4172,9 @@ the path to either a shared memory or huge page filesystem mount.
> > > >  The @option{share} boolean option determines whether the memory
> > > >  region is marked as private to QEMU, or shared. The latter allows
> > > >  a co-operating external process to access the QEMU memory region.
> > > > +Setting the @option{discard-data} boolean option to @var{on}
> > > > +indicates that file contents can be destroyed when QEMU exits,
> > > > +to avoid unnecessarily flushing data to the backing file.
> > > 
> > > We should note that this only works if QEMU shuts down normally. If QEMU
> > > is aggressively killed (SIGKILL) or aborts for some reason, then we'll
> > > never get a chance to invoke madvise(), so presumably the kernel will
> > > still flush the data
> > 
> > Good point.  I tried to not give any guarantees by saying
> > "contents _can_ be destroyed", but users may still have different
> > expectations.
> > 
> > I will change it to:
> > 
> >   Setting the @option{discard-data} boolean option to @var{on}
> >   indicates that file contents can be destroyed when QEMU exits,
> >   to avoid unnecessarily flushing data to the backing file.  Note
> >   that @option{discard-data} is only an optimization, and QEMU
> >   might not discard file contents if it aborts unexpectedly or is
> >   terminated using SIGKILL.
> 
> Fixup patch:
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  qemu-options.hx | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/qemu-options.hx b/qemu-options.hx
> index ad985e4..de9a18a 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -4174,7 +4174,10 @@ region is marked as private to QEMU, or shared. The latter allows
>  a co-operating external process to access the QEMU memory region.
>  Setting the @option{discard-data} boolean option to @var{on}
>  indicates that file contents can be destroyed when QEMU exits,
> -to avoid unnecessarily flushing data to the backing file.
> +to avoid unnecessarily flushing data to the backing file.  Note
> +that @option{discard-data} is only an optimization, and QEMU
> +might not discard file contents if it aborts unexpectedly or is
> +terminated using SIGKILL.
>  
>  @item -object rng-random,id=@var{id},filename=@var{/dev/random}

For the combined patch:

Reviewed-by: Daniel P. Berrange <berrange@redhat.com>

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: [Qemu-devel] [PATCH v2 0/3] hostmem-file: Add "discard-data" option

[Zack Cornelius]

I've tested this patch set, and it has the expected results.

On a clean exit, with discard-data=on, madvise(MADV_REMOVE) is called, clearing any dirty data from RAM without needing to flush, and removing data from the underlying memory file. These results match our expected operation.


Run with the options:

qemu-system-x86_64 -smp 2 -m 8192 -enable-kvm -name ubuntu -vnc 0.0.0.0:5901 -object memory-backend-file,id=mem,size=4096M,mem-path=$MEM_PATH,share=on,discard-data=on -numa node,memdev=mem -mem-prealloc -hda $HD_PATH

While running (First output is the size on disk):

[root@sr3 qemu]# ls -ls /root/qemu_mem/qemu_memory.raw --block-size 1
8589934592 -rw-r--r--. 1 root root 8589934592 Sep  4 11:34 /root/qemu_mem/qemu_memory.raw

At exit, dirty data was dropped, instead of flushed to disk.

After shutting down, actual size on disk is 0:

[root@sr3 qemu]# ls -ls /root/qemu_mem/qemu_memory.raw 
0 -rw-r--r--. 1 root root 8589934592 Sep  4 11:35 /root/qemu_mem/qemu_memory.raw

--
Zack

----- Original Message -----
> From: "Eduardo Habkost" <ehabkost@redhat.com>
> To: qemu-devel@nongnu.org
> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>, "Zack Cornelius" <zack.cornelius@kove.net>, "Paolo Bonzini"
> <pbonzini@redhat.com>, "Daniel P. Berrange" <berrange@redhat.com>, "Igor Mammedov" <imammedo@redhat.com>
> Sent: Thursday, August 24, 2017 2:23:12 PM
> Subject: [PATCH v2 0/3] hostmem-file: Add "discard-data" option

> This series adds a new "discard-data" option to
> memory-backend-file.  The new option will be useful if somebody
> is sharing RAM contents on a pre-existing file using share=on,
> but don't need data to be flushed to disk when QEMU exits.
> 
> Internally, it will trigger a madvise(MADV_REMOVE) call when the
> memory backend is removed or when QEMU exits.
> 
> To make we actually trigger the new code when QEMU exits, the
> first patch in the series ensures we destroy all user-created
> objects when exiting QEMU.
> 
> Changes v1 -> v2:
> * Original subject line of v1 was:
>  '[PATCH 0/5] hostmem-file: Add "persistent" option'
> * Replaced 'persistent=no' with 'discard-data=yes', to make it
>  clear that the flag will destroy data on the backing file.
> * Use qemu_madvise() instead of madvise()
>  * New patch added to series: "osdep: define QEMU_MADV_REMOVE"
> * Call qemu_madvise() directly from the backend unparent()
>  method, insteead of adding a new flag to the memory API and
>  reusing ram_block_discard_range()
>  * In addition to simplifying the code a lot, this fixes a bug,
>    because v1 relied on getting the memory region reference
>    count back to 0, which doesn't happen when QEMU is exiting
>    because there's no machine cleanup code to ensure that.
> 
> Eduardo Habkost (3):
>  vl: Clean up user-creatable objects when exiting
>  osdep: Define QEMU_MADV_REMOVE
>  hostmem-file: Add "discard-data" option
> 
> include/qemu/osdep.h            |  7 +++++++
> include/qom/object_interfaces.h |  8 ++++++++
> backends/hostmem-file.c         | 29 +++++++++++++++++++++++++++++
> qom/object_interfaces.c         |  5 +++++
> vl.c                            |  1 +
> qemu-options.hx                 |  5 ++++-
> 6 files changed, 54 insertions(+), 1 deletion(-)
> 
> --
> 2.9.4

Re: [Qemu-devel] [PATCH v2 0/3] hostmem-file: Add "discard-data" option

[Eduardo Habkost]

Series queued on machine-next.

On Thu, Aug 24, 2017 at 04:23:12PM -0300, Eduardo Habkost wrote:
> This series adds a new "discard-data" option to
> memory-backend-file.  The new option will be useful if somebody
> is sharing RAM contents on a pre-existing file using share=on,
> but don't need data to be flushed to disk when QEMU exits.
> 
> Internally, it will trigger a madvise(MADV_REMOVE) call when the
> memory backend is removed or when QEMU exits.
> 
> To make we actually trigger the new code when QEMU exits, the
> first patch in the series ensures we destroy all user-created
> objects when exiting QEMU.
> 
> Changes v1 -> v2:
> * Original subject line of v1 was:
>   '[PATCH 0/5] hostmem-file: Add "persistent" option'
> * Replaced 'persistent=no' with 'discard-data=yes', to make it
>   clear that the flag will destroy data on the backing file.
> * Use qemu_madvise() instead of madvise()
>   * New patch added to series: "osdep: define QEMU_MADV_REMOVE"
> * Call qemu_madvise() directly from the backend unparent()
>   method, insteead of adding a new flag to the memory API and
>   reusing ram_block_discard_range()
>   * In addition to simplifying the code a lot, this fixes a bug,
>     because v1 relied on getting the memory region reference
>     count back to 0, which doesn't happen when QEMU is exiting
>     because there's no machine cleanup code to ensure that.
> 
> Eduardo Habkost (3):
>   vl: Clean up user-creatable objects when exiting
>   osdep: Define QEMU_MADV_REMOVE
>   hostmem-file: Add "discard-data" option
> 
>  include/qemu/osdep.h            |  7 +++++++
>  include/qom/object_interfaces.h |  8 ++++++++
>  backends/hostmem-file.c         | 29 +++++++++++++++++++++++++++++
>  qom/object_interfaces.c         |  5 +++++
>  vl.c                            |  1 +
>  qemu-options.hx                 |  5 ++++-
>  6 files changed, 54 insertions(+), 1 deletion(-)
> 
> -- 
> 2.9.4
> 
> 

-- 
Eduardo