|
From: | Haozhong Zhang |
Subject: | Re: [Qemu-devel] [PATCH] hostmem-file: add a property 'notrunc' to avoid data corruption |
Date: | Thu, 20 Oct 2016 21:56:56 +0800 |
User-agent: | NeoMutt/20160827 (1.7.0) |
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
[Prev in Thread] | Current Thread | [Next in Thread] |