From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60718) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bxDbz-00044F-9i for qemu-devel@nongnu.org; Thu, 20 Oct 2016 09:42:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bxDbw-0002vF-5N for qemu-devel@nongnu.org; Thu, 20 Oct 2016 09:42:23 -0400 Received: from mx1.redhat.com ([209.132.183.28]:57086) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1bxDbv-0002uz-Ta for qemu-devel@nongnu.org; Thu, 20 Oct 2016 09:42:20 -0400 Date: Thu, 20 Oct 2016 15:42:15 +0200 From: Igor Mammedov Message-ID: <20161020154215.4b765284@nial.brq.redhat.com> In-Reply-To: <20161020131138.4gzxk5ekltkiqtq2@hz-desktop> References: <20161020061301.31372-1-haozhong.zhang@intel.com> <20161020143412.5ea6b564@nial.brq.redhat.com> <20161020131138.4gzxk5ekltkiqtq2@hz-desktop> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] hostmem-file: add a property 'notrunc' to avoid data corruption List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Haozhong Zhang Cc: kwolf@redhat.com, Xiao Guangrong , Eduardo Habkost , Peter Crosthwaite , qemu-devel@nongnu.org, mreitz@redhat.com, Paolo Bonzini , Richard Henderson On Thu, 20 Oct 2016 21:11:38 +0800 Haozhong Zhang wrote: > On 10/20/16 14:34 +0200, Igor Mammedov wrote: > >On Thu, 20 Oct 2016 14:13:01 +0800 > >Haozhong Zhang 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 >