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

[Eduardo Habkost <ehabkost@redhat.com>]

On Thu, Oct 20, 2016 at 10:22:53PM +0800, Haozhong Zhang wrote:
> 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 <haozhong.zhang@intel.com> wrote:
> > > 
> > > > On 10/20/16 14:34 +0200, Igor Mammedov wrote:
> > > > >On Thu, 20 Oct 2016 14:13:01 +0800
> > > > >Haozhong Zhang <haozhong.zhang@intel.com> 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.

The only use case I see for B is to force allocation and
ftruncate to a larger size. All other uses should be covered by
C. But note that Igor and I are having an ongoing discussion in
this thread about using C vs A/B.

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

We don't have an option meaning "please touch only this specific
region of the file, leave the rest alone". In this case, not
truncating even when "size" is explicitly set would make sense.

If we wanted that "please don't touch the rest of the file" mode
to be supported, this doesn't seem to be the use case we have in
mind right now, does it? If we did, I believe we would have an
"offset" parameter too.

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

I would say it's still (1), but we don't need to truncate the
file if B < A, so we can avoid it because it's safer.

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

-- 
Eduardo