diff mbox series

[2/3] hostmem-file: add readonly=on|off option

Message ID 20200804101244.1283503-3-stefanha@redhat.com
State New
Headers show
Series nvdimm: read-only file support | expand

Commit Message

Stefan Hajnoczi Aug. 4, 2020, 10:12 a.m. UTC
Let -object memory-backend-file work on read-only files when the
readonly=on option is given. This can be used to share the contents of a
file between multiple guests while preventing them from consuming
Copy-on-Write memory if guests dirty the pages, for example.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 backends/hostmem-file.c | 26 +++++++++++++++++++++++++-
 qemu-options.hx         |  5 ++++-
 2 files changed, 29 insertions(+), 2 deletions(-)

Comments

Philippe Mathieu-Daudé Aug. 21, 2020, 12:50 p.m. UTC | #1
On 8/4/20 12:12 PM, Stefan Hajnoczi wrote:
> Let -object memory-backend-file work on read-only files when the
> readonly=on option is given. This can be used to share the contents of a
> file between multiple guests while preventing them from consuming
> Copy-on-Write memory if guests dirty the pages, for example.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  backends/hostmem-file.c | 26 +++++++++++++++++++++++++-
>  qemu-options.hx         |  5 ++++-
>  2 files changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
> index 37c70acfe2..6bd5bf9b91 100644
> --- a/backends/hostmem-file.c
> +++ b/backends/hostmem-file.c
> @@ -30,6 +30,7 @@ struct HostMemoryBackendFile {
>      uint64_t align;
>      bool discard_data;
>      bool is_pmem;
> +    bool readonly;
>  };
>  
>  static void
> @@ -57,7 +58,7 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
>                                       backend->size, fb->align,
>                                       (backend->share ? RAM_SHARED : 0) |
>                                       (fb->is_pmem ? RAM_PMEM : 0),
> -                                     fb->mem_path, false, errp);
> +                                     fb->mem_path, fb->readonly, errp);
>      g_free(name);
>  #endif
>  }
> @@ -152,6 +153,26 @@ static void file_memory_backend_set_pmem(Object *o, bool value, Error **errp)
>      fb->is_pmem = value;
>  }
>  
> +static bool file_memory_backend_get_readonly(Object *o, Error **errp)
> +{
> +    return MEMORY_BACKEND_FILE(o)->readonly;
> +}
> +
> +static void file_memory_backend_set_readonly(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 'readonly' of %s.",
> +                   object_get_typename(o));


The 'host_memory_backend_mr_inited()' function is not documented;
my understanding is a backend is considered initialized once it has
a MemoryRegion assigned to it.

So this error message is not very helpful, it doesn't explain the
reason. I see all other setters in this file use the same error,
so it is almost a predating issue.

Still I'd rather use a different message, something like:
"'%s' already initialized, can not set it 'readonly'".

Preferably with the error message reworded:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> +        return;
> +    }
> +
> +    fb->readonly = value;
> +}
> +
>  static void file_backend_unparent(Object *obj)
>  {
>      HostMemoryBackend *backend = MEMORY_BACKEND(obj);
> @@ -183,6 +204,9 @@ file_backend_class_init(ObjectClass *oc, void *data)
>          NULL, NULL);
>      object_class_property_add_bool(oc, "pmem",
>          file_memory_backend_get_pmem, file_memory_backend_set_pmem);
> +    object_class_property_add_bool(oc, "readonly",
> +        file_memory_backend_get_readonly,
> +        file_memory_backend_set_readonly);
>  }
>  
>  static void file_backend_instance_finalize(Object *o)
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 708583b4ce..d834e00b0d 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -4369,7 +4369,7 @@ SRST
>      they are specified. Note that the 'id' property must be set. These
>      objects are placed in the '/objects' path.
>  
> -    ``-object memory-backend-file,id=id,size=size,mem-path=dir,share=on|off,discard-data=on|off,merge=on|off,dump=on|off,prealloc=on|off,host-nodes=host-nodes,policy=default|preferred|bind|interleave,align=align``
> +    ``-object memory-backend-file,id=id,size=size,mem-path=dir,share=on|off,discard-data=on|off,merge=on|off,dump=on|off,prealloc=on|off,host-nodes=host-nodes,policy=default|preferred|bind|interleave,align=align,readonly=on|off``
>          Creates a memory file backend object, which can be used to back
>          the guest RAM with huge pages.
>  
> @@ -4452,6 +4452,9 @@ SRST
>          4.15) and the filesystem of ``mem-path`` mounted with DAX
>          option.
>  
> +        The ``readonly`` option specifies whether the backing file is opened
> +        read-only or read-write (default).
> +
>      ``-object memory-backend-ram,id=id,merge=on|off,dump=on|off,share=on|off,prealloc=on|off,size=size,host-nodes=host-nodes,policy=default|preferred|bind|interleave``
>          Creates a memory backend object, which can be used to back the
>          guest RAM. Memory backend objects offer more control than the
>
Stefan Hajnoczi Sept. 16, 2020, 9:31 a.m. UTC | #2
On Fri, Aug 21, 2020 at 02:50:42PM +0200, Philippe Mathieu-Daudé wrote:
> On 8/4/20 12:12 PM, Stefan Hajnoczi wrote:
> > Let -object memory-backend-file work on read-only files when the
> > readonly=on option is given. This can be used to share the contents of a
> > file between multiple guests while preventing them from consuming
> > Copy-on-Write memory if guests dirty the pages, for example.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  backends/hostmem-file.c | 26 +++++++++++++++++++++++++-
> >  qemu-options.hx         |  5 ++++-
> >  2 files changed, 29 insertions(+), 2 deletions(-)
> > 
> > diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
> > index 37c70acfe2..6bd5bf9b91 100644
> > --- a/backends/hostmem-file.c
> > +++ b/backends/hostmem-file.c
> > @@ -30,6 +30,7 @@ struct HostMemoryBackendFile {
> >      uint64_t align;
> >      bool discard_data;
> >      bool is_pmem;
> > +    bool readonly;
> >  };
> >  
> >  static void
> > @@ -57,7 +58,7 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
> >                                       backend->size, fb->align,
> >                                       (backend->share ? RAM_SHARED : 0) |
> >                                       (fb->is_pmem ? RAM_PMEM : 0),
> > -                                     fb->mem_path, false, errp);
> > +                                     fb->mem_path, fb->readonly, errp);
> >      g_free(name);
> >  #endif
> >  }
> > @@ -152,6 +153,26 @@ static void file_memory_backend_set_pmem(Object *o, bool value, Error **errp)
> >      fb->is_pmem = value;
> >  }
> >  
> > +static bool file_memory_backend_get_readonly(Object *o, Error **errp)
> > +{
> > +    return MEMORY_BACKEND_FILE(o)->readonly;
> > +}
> > +
> > +static void file_memory_backend_set_readonly(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 'readonly' of %s.",
> > +                   object_get_typename(o));
> 
> 
> The 'host_memory_backend_mr_inited()' function is not documented;
> my understanding is a backend is considered initialized once it has
> a MemoryRegion assigned to it.
> 
> So this error message is not very helpful, it doesn't explain the
> reason. I see all other setters in this file use the same error,
> so it is almost a predating issue.
> 
> Still I'd rather use a different message, something like:
> "'%s' already initialized, can not set it 'readonly'".
> 
> Preferably with the error message reworded:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

I haven't reworded the error message because it's used in
hostmem-file.c, hostmem-memfd.c, and hostmem.c. A separate patch would
need to change the error messages across these files.

There is no time when users can actually change these QOM properties, so
"cannot change FOO" is a reasonable wording form the user perspective.
Telling the user that there is a pre-initialization state when the
property can be change isn't useful because they cannot observe that
state (the object is created and ->complete is called in a single step).
Philippe Mathieu-Daudé Sept. 16, 2020, 10:17 a.m. UTC | #3
On 9/16/20 11:31 AM, Stefan Hajnoczi wrote:
> On Fri, Aug 21, 2020 at 02:50:42PM +0200, Philippe Mathieu-Daudé wrote:
>> On 8/4/20 12:12 PM, Stefan Hajnoczi wrote:
>>> Let -object memory-backend-file work on read-only files when the
>>> readonly=on option is given. This can be used to share the contents of a
>>> file between multiple guests while preventing them from consuming
>>> Copy-on-Write memory if guests dirty the pages, for example.
>>>
[...]

>>> +static bool file_memory_backend_get_readonly(Object *o, Error **errp)
>>> +{
>>> +    return MEMORY_BACKEND_FILE(o)->readonly;
>>> +}
>>> +
>>> +static void file_memory_backend_set_readonly(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 'readonly' of %s.",
>>> +                   object_get_typename(o));
>>
>>
>> The 'host_memory_backend_mr_inited()' function is not documented;
>> my understanding is a backend is considered initialized once it has
>> a MemoryRegion assigned to it.
>>
>> So this error message is not very helpful, it doesn't explain the
>> reason. I see all other setters in this file use the same error,
>> so it is almost a predating issue.
>>
>> Still I'd rather use a different message, something like:
>> "'%s' already initialized, can not set it 'readonly'".
>>
>> Preferably with the error message reworded:
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> I haven't reworded the error message because it's used in
> hostmem-file.c, hostmem-memfd.c, and hostmem.c. A separate patch would
> need to change the error messages across these files.
> 
> There is no time when users can actually change these QOM properties, so
> "cannot change FOO" is a reasonable wording form the user perspective.
> Telling the user that there is a pre-initialization state when the
> property can be change isn't useful because they cannot observe that
> state (the object is created and ->complete is called in a single step).

OK, understood.
diff mbox series

Patch

diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
index 37c70acfe2..6bd5bf9b91 100644
--- a/backends/hostmem-file.c
+++ b/backends/hostmem-file.c
@@ -30,6 +30,7 @@  struct HostMemoryBackendFile {
     uint64_t align;
     bool discard_data;
     bool is_pmem;
+    bool readonly;
 };
 
 static void
@@ -57,7 +58,7 @@  file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
                                      backend->size, fb->align,
                                      (backend->share ? RAM_SHARED : 0) |
                                      (fb->is_pmem ? RAM_PMEM : 0),
-                                     fb->mem_path, false, errp);
+                                     fb->mem_path, fb->readonly, errp);
     g_free(name);
 #endif
 }
@@ -152,6 +153,26 @@  static void file_memory_backend_set_pmem(Object *o, bool value, Error **errp)
     fb->is_pmem = value;
 }
 
+static bool file_memory_backend_get_readonly(Object *o, Error **errp)
+{
+    return MEMORY_BACKEND_FILE(o)->readonly;
+}
+
+static void file_memory_backend_set_readonly(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 'readonly' of %s.",
+                   object_get_typename(o));
+        return;
+    }
+
+    fb->readonly = value;
+}
+
 static void file_backend_unparent(Object *obj)
 {
     HostMemoryBackend *backend = MEMORY_BACKEND(obj);
@@ -183,6 +204,9 @@  file_backend_class_init(ObjectClass *oc, void *data)
         NULL, NULL);
     object_class_property_add_bool(oc, "pmem",
         file_memory_backend_get_pmem, file_memory_backend_set_pmem);
+    object_class_property_add_bool(oc, "readonly",
+        file_memory_backend_get_readonly,
+        file_memory_backend_set_readonly);
 }
 
 static void file_backend_instance_finalize(Object *o)
diff --git a/qemu-options.hx b/qemu-options.hx
index 708583b4ce..d834e00b0d 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -4369,7 +4369,7 @@  SRST
     they are specified. Note that the 'id' property must be set. These
     objects are placed in the '/objects' path.
 
-    ``-object memory-backend-file,id=id,size=size,mem-path=dir,share=on|off,discard-data=on|off,merge=on|off,dump=on|off,prealloc=on|off,host-nodes=host-nodes,policy=default|preferred|bind|interleave,align=align``
+    ``-object memory-backend-file,id=id,size=size,mem-path=dir,share=on|off,discard-data=on|off,merge=on|off,dump=on|off,prealloc=on|off,host-nodes=host-nodes,policy=default|preferred|bind|interleave,align=align,readonly=on|off``
         Creates a memory file backend object, which can be used to back
         the guest RAM with huge pages.
 
@@ -4452,6 +4452,9 @@  SRST
         4.15) and the filesystem of ``mem-path`` mounted with DAX
         option.
 
+        The ``readonly`` option specifies whether the backing file is opened
+        read-only or read-write (default).
+
     ``-object memory-backend-ram,id=id,merge=on|off,dump=on|off,share=on|off,prealloc=on|off,size=size,host-nodes=host-nodes,policy=default|preferred|bind|interleave``
         Creates a memory backend object, which can be used to back the
         guest RAM. Memory backend objects offer more control than the