Re: [Qemu-devel] [PATCH] hostmem-file: add a property 'notrunc' to avoid data corruption

[Eduardo Habkost]

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

I will add that to my TODO-list.

> 
> > 
> > >   
> > > > 
> > > > My suggestion is:
> > > > * B should be optional.
> > > > * If B is omitted, we should never truncate the file to a smaller
> > > >   size.  
> > > i.e. derive backend.size from filesize if possible (i.e. not hugepages)
> > >   
> > > > * If B is omitted, we can use C as the size when mapping the
> > > >   file.  
> > > frontend size is the size that's mapped into guest address space.
> > > it should not influence backend's size in backward direction.
> > > You may notice pc-dimm.size is not user settable (readonly) property.  
> > 
> > Frontend size will not influence backend size, it will just
> > affect the size of the memory region the frontend code will ask
> > the backend to provide.
> > 
> > In other words: I believe host_memory_backend_get_memory() needs
> > a 'size' argument, and that memory allocation could be optionally
> > delayed to the host_memory_backend_get_memory() call. This way,
> > we don't need a backend size at all, unless we want the backend
> > to truncate files or preallocate memory for us.
> To not complicate things I'd keep current behavior of
>   host_memory_backend_get_memory()
> i.e return MR for all the baked memory and then frontend
> can split and map it into guest address space as it sees fit
> using aliases for non trivial cases taking in account frontend's
> own size/label_size/whatnot properties.
> That's what NVDIMM does now, it gets MR for whole file and
> the splits it on data and label areas and maps into GA only
> data part using memory_region_init_alias().

Good idea. That would probably work too.

> 
> > 
> > >   
> > > > * If B is omitted, and C > A, maybe we could use ftruncate() to
> > > >   extend the file to make users happy. But I'm not sure we
> > > >   should (I think B should be the only option that cause
> > > >   truncation).
> > > > * If we want to make C optional on some cases, we could use A if
> > > >   B is omitted.  
> > > we shouldn't use C to manage backends behavior  
> > 
> > I don't think this would be "managing backend behavior". I
> > believe the memory backend is a memory mapper/allocator, but the
> > exact size of the memory it provide is up to the caller that's
> > asking for a MemoryRegion.
> caller in backend case is QEMU's CLI and backend options,
> and yes it's a simple allocator/mapper. If frontend needs
> to partition allocated region in some way it should do it
> itself in some stable manner as it own layout.
> That would keep things simple and consistent with which
> and where from different sizes were originated.

If the MemoryRegion API lets us do that, that's an even better
solution.

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.

But this would be a new feature, anyway. No need to worry about
it right now. I will add it to my TODO-list and try to send a RFC
later.

-- 
Eduardo