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

[Eduardo Habkost]

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).

If we are dropping -numa size in favor of the
memory-backend-provided size, that's a bug.

> 
> > 
> > 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.

> 
> > * 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.

> 
> But I have a question why do we have truncation at all in place now.

Probably because ftruncate() needs to be run by QEMU if memory is
supposed to be allocated by QEMU (e.g. if using tmpfs or
hugetlbfs).

Even if we decide we don't want it anymore, we can't drop the
ability to extend files without breaking existing setups. We can
probably drop the ability to truncate files to smaller sizes,
though.

> 
> > 
> > Does that make sense?
> > 
> 

-- 
Eduardo