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

[Haozhong Zhang]

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?

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.

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

> > +    }
> > +
> > + out_fclose:
> > +    close(fd);
> > + out:
> > +    error_propagate(errp, local_err);
> > +    return size;
> > +}
> > +
> [...]

Thank you for the review!

Haozhong