From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54354) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bxD8O-00088n-MB for qemu-devel@nongnu.org; Thu, 20 Oct 2016 09:11:52 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bxD8J-0000sA-Ng for qemu-devel@nongnu.org; Thu, 20 Oct 2016 09:11:48 -0400 Received: from mga06.intel.com ([134.134.136.31]:47891) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1bxD8J-0000r1-FV for qemu-devel@nongnu.org; Thu, 20 Oct 2016 09:11:43 -0400 Date: Thu, 20 Oct 2016 21:11:38 +0800 From: Haozhong Zhang Message-ID: <20161020131138.4gzxk5ekltkiqtq2@hz-desktop> References: <20161020061301.31372-1-haozhong.zhang@intel.com> <20161020143412.5ea6b564@nial.brq.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline In-Reply-To: <20161020143412.5ea6b564@nial.brq.redhat.com> 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: Igor Mammedov Cc: qemu-devel@nongnu.org, Eduardo Habkost , Paolo Bonzini , Peter Crosthwaite , Richard Henderson , Xiao Guangrong , kwolf@redhat.com, mreitz@redhat.com 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? 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