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

[Haozhong Zhang]

On 10/20/16 11:56 -0200, Eduardo Habkost 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)
>
> My suggestion is:
> * B should be optional.
> * If B is omitted, we should never truncate the file to a smaller
>  size.
> * If B is omitted, we can use C as the size when mapping the
>  file.
> * 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).

For the words in parentheses, I have one question: what does size B mean?
1) The user knows the file size is the one specified (size B),
  and, if it's not, all bad results are user's fault.
or
2) The user wants only the part of size B is used. How the other part
  is used is unspecified, which, I think, implies the user does not
  expect QEMU to touch it.
  
If it's 1), I think it's reasonable to truncate when B is given.
Otherwise, if B is given, QEMU can truncate the file only if B > A.

Haozhong

> * If we want to make C optional on some cases, we could use A if
>  B is omitted.
>
> Does that make sense?
>
> --
> Eduardo