| Message ID | 20200804101244.1283503-3-stefanha@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nvdimm: read-only file support | expand |
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 >
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).
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 --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
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(-)