[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] hostmem-file: add a property 'notrunc' to avoid
From: |
Eduardo Habkost |
Subject: |
Re: [Qemu-devel] [PATCH] hostmem-file: add a property 'notrunc' to avoid data corruption |
Date: |
Fri, 21 Oct 2016 09:53:43 -0200 |
User-agent: |
Mutt/1.7.0 (2016-08-17) |
On Fri, Oct 21, 2016 at 11:31:41AM +0200, Igor Mammedov wrote:
> On Thu, 20 Oct 2016 14:56:58 -0200
> Eduardo Habkost <address@hidden> wrote:
>
> > On Thu, Oct 20, 2016 at 05:35:38PM +0200, Igor Mammedov wrote:
> > > On Thu, 20 Oct 2016 12:47:34 -0200
> > > Eduardo Habkost <address@hidden> wrote:
> > >
> > > > On Thu, Oct 20, 2016 at 04:15:21PM +0200, Igor Mammedov wrote:
> > > > > On Thu, 20 Oct 2016 11:56:10 -0200
> > > > > Eduardo Habkost <address@hidden> wrote:
> > > > >
> > > > > > On Thu, Oct 20, 2016 at 03:42:15PM +0200, Igor Mammedov wrote:
> > > > > > > On Thu, 20 Oct 2016 21:11:38 +0800
> > > > > > > Haozhong Zhang <address@hidden> wrote:
> > > > > > >
> > > > > > > > On 10/20/16 14:34 +0200, Igor Mammedov wrote:
> > > > > > > > >On Thu, 20 Oct 2016 14:13:01 +0800
> > > > > > > > >Haozhong Zhang <address@hidden> wrote:
> > > > > > > > >
> > > > > > > > >> If a file is used as the backend of memory-backend-file and
> > > > > > > > >> its size is
> > > > > > > > >> not identical to the property 'size', the file will be
> > > > > > > > >> truncated. For a
> > > > > > > > >> file used as the backend of vNVDIMM, its data is expected to
> > > > > > > > >> be
> > > > > > > > >> persistent and the truncation may corrupt the existing data.
> > > > > > > > >>
> > > > > > > > >I wonder if it's possible just skip 'size' property in your
> > > > > > > > >case instead
> > > > > > > > >'notrunc' property. That way if size is not present one'd get
> > > > > > > > >actual size
> > > > > > > > >using get_file_size() and set 'size' to it?
> > > > > > > > >And if 'size' is provided and 'size' != file_size then error
> > > > > > > > >out.
> > > > > > > > >
> > > > > > > >
> > > > > > > > I don't know how this can be implemented in QEMU. Specially,
> > > > > > > > how does
> > > > > > > > the memory-backend-file know it's used for vNVDIMM, so that it
> > > > > > > > can
> > > > > > > > skip the 'size' property?
> > > > > > > Does memory-backend-file needs to know that it's used by NVDIMM?
> > > > > > > Looking at nvdimm_realize it doesn't as it's assumes
> > > > > > > hostemem_size == pmem_size + label_size
> > > > > > >
> > > > > > > I'd make hostmem_file.size optional and take size from file
> > > > > > > and if 'size' is specified explictly require it to mach file size.
> > > > > > > It's generic and has nothing to do with nvdimm.
> > > > > >
> > > > > > We can take size from file, or take size from the
> > > > > > host_memory_backend_get_memory() callers.
> > > > > >
> > > > > > Enumerating all sizes that QEMU can use as input:
> > > > > >
> > > > > > A) Backend file size
> > > > > > B) memory backend "size" option
> > > > > > C) frontend-provided size (-numa size, -m, or pc-dimm "size"
> > > > > > property)
> > > > > -numa size affect only anon memory not backend backed one, for
> > > > > backend baked memory we use memdev where size comes from backend
> > > > >
> > > > > pc-dimm.size is readonly and isn't supposed to influence backend.size
> > > > >
> > > > > I'd drop C option
> > > >
> > > > If C is not present, it should be, as it affects the guest ABI
> > > > (and the ABI must never depend on the host you are running or
> > > > backend configuration, only on the frontend configuration).
> > > I've meant that C should not affect behavior of backend.
> > >
> > > > If we are dropping -numa size in favor of the
> > > > memory-backend-provided size, that's a bug.
> > > -numa size is not applicable here as it's not using backends,
> > > when backends are used it's -numa memdev instead in which case
> > > numa_info[nodenr].node_mem = object_property_get_int(o, "size", NULL);
> > >
> >
> > OK. My suggestion is to change this to not ignore the size
> > option, but to validate it and/or do whatever necessary to get a
> > MemoryRegion of the right size (your suggestion below to split
> > the memory region would work too).
> >
> > In other words, this should work:
> >
> > $ qemu -object memory-backend-file,id=mem0,mem-path=/tmp/mempath,size=2G \
> > -numa node,size=2G,memdev=mem0 -m 2G
> >
> > this must error out:
> >
> > $ qemu -object memory-backend-file,id=mem0,mem-path=/tmp/mempath,size=2G \
> > -numa node,size=4G,memdev=mem0 -m 2G
> >
> > and this should either error out, or result in a 1GB NUMA node
> > (but never in a 2G NUMA node):
> >
> > $ qemu -object memory-backend-file,id=mem0,mem-path=/tmp/mempath,size=2G \
> > -numa node,size=1G,memdev=mem0 -m 1G
> Looks like a bit of over-engineering and mixing together backend with
> frontend,
> I don't see a compelling reason to support
> -numa size=X,memdev=foo
> as just memdev=foo is sufficient
>
> so I'd rather error out if user adds 'size' to -numa memdev=foo
Erroring out would be good enough. I won't argue to allow people
to use only a portion of the backend-provided memory unless we
have a real use case for it.
>
> [...]
>
> > Probably there's only one case where behavior would be different
> > than what I was thinking. I would like to be possible to specify
> > only C (frontend size), and omit both A and B. For example:
> >
> > $ mkdir /tmp/mempath
> > $ qemu -object memory-backend-file,id=mem0,mem-path=/tmp/mempath \
> > -numa node,size=1G,memdev=mem0 -m 1G
> >
> > I would like this command to create a 1G file inside /tmp/mempath
> > automatically, without the need for an explicit size=1G argument
> > to memory-backend-file.
> This is still C where frontend manages allocation of backend
> (i.e. changes backend's 'size' property) vs being just consumer
> of whatever backend provides.
I don't want it to change the 'size' property of the backend, I
just want to allow the backend to allocate memory on demand in
case size was not specified, to make the command-line interface
simpler and less error-prone.
> Purpose of backend is to create complete/initialized hostside
> object regardless of which frontend uses it.
> I see all properties of backend (including sizes) as belonging
> to hostside which shouldn't be influenced by whatever frontend
> it's used. And these properties are specified/managed only by mgmt
> side explicitly when creating a backend (either via CLI or monitor/QMP).
I agree completely with the above paragraph, but we probably have
different definitions of "complete/initialized". To me,
"complete" doesn't need to include allocation (unless prealloc is
required), and it's OK to let the backend allocate memory on
demand to simplify the command-line interface.
>
> In your example I'd error out with:
> "memory-backend-file: requires 'size' to create file in /tmp/mempath"
This is what should happen today, yes. But I would like to make
'size' optional later, to make the interface simpler.
Anyway, let's wait until I have an actual RFC (which may take a
while as it's not high priority to me), otherwise it would be
just a theoretical discussion.
In the meantime, we can change the code to error out on the
examples I gave.
--
Eduardo
- Re: [Qemu-devel] [PATCH] hostmem-file: add a property 'notrunc' to avoid data corruption, (continued)
- Re: [Qemu-devel] [PATCH] hostmem-file: add a property 'notrunc' to avoid data corruption, Haozhong Zhang, 2016/10/20
- Re: [Qemu-devel] [PATCH] hostmem-file: add a property 'notrunc' to avoid data corruption, Igor Mammedov, 2016/10/20
- Re: [Qemu-devel] [PATCH] hostmem-file: add a property 'notrunc' to avoid data corruption, Eduardo Habkost, 2016/10/20
- Re: [Qemu-devel] [PATCH] hostmem-file: add a property 'notrunc' to avoid data corruption, Igor Mammedov, 2016/10/20
- Re: [Qemu-devel] [PATCH] hostmem-file: add a property 'notrunc' to avoid data corruption, Eduardo Habkost, 2016/10/20
- Re: [Qemu-devel] [PATCH] hostmem-file: add a property 'notrunc' to avoid data corruption, Igor Mammedov, 2016/10/20
- Re: [Qemu-devel] [PATCH] hostmem-file: add a property 'notrunc' to avoid data corruption, Eduardo Habkost, 2016/10/20
- Re: [Qemu-devel] [PATCH] hostmem-file: add a property 'notrunc' to avoid data corruption, Igor Mammedov, 2016/10/21
- Re: [Qemu-devel] [PATCH] hostmem-file: add a property 'notrunc' to avoid data corruption,
Eduardo Habkost <=
- Re: [Qemu-devel] [PATCH] hostmem-file: add a property 'notrunc' to avoid data corruption, Igor Mammedov, 2016/10/21
- Re: [Qemu-devel] [PATCH] hostmem-file: add a property 'notrunc' to avoid data corruption, Haozhong Zhang, 2016/10/21
- Re: [Qemu-devel] [PATCH] hostmem-file: add a property 'notrunc' to avoid data corruption, Igor Mammedov, 2016/10/21
- Re: [Qemu-devel] [PATCH] hostmem-file: add a property 'notrunc' to avoid data corruption, Haozhong Zhang, 2016/10/21
- Re: [Qemu-devel] [PATCH] hostmem-file: add a property 'notrunc' to avoid data corruption, Eduardo Habkost, 2016/10/21
- Re: [Qemu-devel] [PATCH] hostmem-file: add a property 'notrunc' to avoid data corruption, Haozhong Zhang, 2016/10/20
- Re: [Qemu-devel] [PATCH] hostmem-file: add a property 'notrunc' to avoid data corruption, Eduardo Habkost, 2016/10/20
- Re: [Qemu-devel] [PATCH] hostmem-file: add a property 'notrunc' to avoid data corruption, Haozhong Zhang, 2016/10/20
Re: [Qemu-devel] [PATCH] hostmem-file: add a property 'notrunc' to avoid data corruption, Eduardo Habkost, 2016/10/20