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

[Haozhong Zhang]

On 10/20/16 15:42 +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.

I thought you meant to make 'size' optional only if it's used for
vNVDIMM. Now I understand it's a general change regardless of the
frontend.

> > Besides, it cannot cover the case that only a part of file is used as
> > the backend of vNVDIMM, which is allowed by the current implementation.
> and which hasn't ever worked due to truncation this patch tries to fix.
>
>
> > >>
> > >> A property 'notrunc' is added to memory-backend-file to allow users to
> > >> control the truncation. If 'notrunc' is on, QEMU will not truncate the
> > >> backend file and require the property 'size' is not larger than the file
> > >> size. If 'notrunc' is off, QEMU will behave as before.
> > >[...]
> > >
> > >>
> > >>  #ifdef __linux__
> > >> +static uint64_t get_file_size(const char *path, Error **errp)
> > >Maybe QEMU laredy has an utility to do it that could be shared,
> > >CCing block maintainers.
> > >
> > >> +{
> > >> +    int fd;
> > >> +    struct stat st;
> > >> +    uint64_t size = 0;
> > >> +    Error *local_err = NULL;
> > >> +
> > >> +    fd = qemu_open(path, O_RDONLY);
> > >> +    if (fd < 0) {
> > >> +        error_setg(&local_err, "cannot open file");
> > >error_setg_errno
> > >> +        goto out;
> > >> +    }
> > >> +
> > >> +    if (stat(path, &st)) {
> > >fstat()
> >
> > will change
> >
> > >
> > >> +        error_setg(&local_err, "cannot get file stat");
> > >error_setg_errno
> >
> > ditto
> >
> > >> +        goto out_fclose;
> > >> +    }
> > >> +
> > >> +    switch (st.st_mode & S_IFMT) {
> > >> +    case S_IFREG:
> > >> +        size = st.st_size;
> > >> +        break;
> > >> +
> > >> +    case S_IFBLK:
> > >> +        if (ioctl(fd, BLKGETSIZE64, &size)) {
> > >compilation might fail on platforms without  BLKGETSIZE64,
> > >
> >
> > BLKGETSIZE64 was first introduced by linux kernel 2.4.10. For these
> > linux specific code, does QEMU have any assumption for the least
> > kernel version? If no, I'll fallback to BLKGETSIZE if BLKGETSIZE64
> > fails.
> >
> > >
> > >> +            error_setg(&local_err, "cannot get size of block device");
> > >error_setg_errno
> > >> +            size = 0;
> > >> +        }
> > >> +        break;
> > >> +
> > >> +    default:
> > >> +        error_setg(&local_err,
> > >> +                   "only block device on Linux and regular file are 
> > supported");
> > >what about huge page usecase, would it fall right here fail?
> > >
> >
> > Yes, it will fail. notrunc is not necessary for the huge page case. I
> > could report more messages here, like "remove notrunc if hugetlbfs is used".
> it's better to skip this check in case of hugetlbfs so user won't have to do 
> anything

Agree, it's better to not confuse users.

Thanks,
Haozhong

> > >> +    }
> > >> +
> > >> + out_fclose:
> > >> +    close(fd);
> > >> + out:
> > >> +    error_propagate(errp, local_err);
> > >> +    return size;
> > >> +}
> > >> +
> > >[...]
> > >
> >
> > Thank you for the review!
> >
> > Haozhong