On Thu, 20 Oct 2016 21:11:38 +0800
Haozhong Zhang <haozhong.zh...@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.zh...@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.
> 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
> >> + }
> >> +
> >> + out_fclose:
> >> + close(fd);
> >> + out:
> >> + error_propagate(errp, local_err);
> >> + return size;
> >> +}
> >> +
> >[...]
> >
>
> Thank you for the review!
>
> Haozhong
>
