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

[Igor Mammedov]

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.

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