From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vladimir Sementsov-Ogievskiy Subject: Re: [Qemu-devel] [PATCH v7 09/35] exec: allow file_ram_alloc to work on file Date: Mon, 2 Nov 2015 18:58:57 +0300 Message-ID: <56378841.8070804@virtuozzo.com> References: <1446455617-129562-1-git-send-email-guangrong.xiao@linux.intel.com> <1446455617-129562-10-git-send-email-guangrong.xiao@linux.intel.com> <56377D56.6050301@virtuozzo.com> <56378054.6050100@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: , , , , , , , , To: Xiao Guangrong , , Return-path: Received: from relay.parallels.com ([195.214.232.42]:36711 "EHLO relay.parallels.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753762AbbKBP7N (ORCPT ); Mon, 2 Nov 2015 10:59:13 -0500 In-Reply-To: <56378054.6050100@linux.intel.com> Sender: kvm-owner@vger.kernel.org List-ID: On 02.11.2015 18:25, Xiao Guangrong wrote: > > > On 11/02/2015 11:12 PM, Vladimir Sementsov-Ogievskiy wrote: >> On 02.11.2015 12:13, Xiao Guangrong wrote: >>> Currently, file_ram_alloc() only works on directory - it creates a = file >>> under @path and do mmap on it >>> >>> This patch tries to allow it to work on file directly, if @path is = a >> >> It isn't try to allow, it allows, as I understand)... > > Err... Sorry for my English, but what is the different between: > =E2=80=9DThis patch tries to allow it to work on file directly=E2=80=9C= and > "This patch allows it to work on file directly" > > :( Not sure that everyone is interested in this nit-picking discussion.. A allows B: if A then B A tries to allow B: if A then _may be_ B In any way it doesn't matter. > >> >>> directory it works as before, otherwise it treats @path as the targ= et >>> file then directly allocate memory from it >>> >>> Signed-off-by: Xiao Guangrong >>> --- >>> exec.c | 80=20 >>> ++++++++++++++++++++++++++++++++++++++++++------------------------ >>> 1 file changed, 51 insertions(+), 29 deletions(-) >>> >>> diff --git a/exec.c b/exec.c >>> index 9075f4d..db0fdaf 100644 >>> --- a/exec.c >>> +++ b/exec.c >>> @@ -1174,14 +1174,60 @@ void qemu_mutex_unlock_ramlist(void) >>> } >>> #ifdef __linux__ >>> +static bool path_is_dir(const char *path) >>> +{ >>> + struct stat fs; >>> + >>> + return stat(path, &fs) =3D=3D 0 && S_ISDIR(fs.st_mode); >>> +} >>> + >>> +static int open_ram_file_path(RAMBlock *block, const char *path,=20 >>> size_t size) >>> +{ >>> + char *filename; >>> + char *sanitized_name; >>> + char *c; >>> + int fd; >>> + >>> + if (!path_is_dir(path)) { >>> + int flags =3D (block->flags & RAM_SHARED) ? O_RDWR : O_RDO= NLY; >>> + >>> + flags |=3D O_EXCL; >>> + return open(path, flags); >>> + } >>> + >>> + /* Make name safe to use with mkstemp by replacing '/' with=20 >>> '_'. */ >>> + sanitized_name =3D g_strdup(memory_region_name(block->mr)); >>> + for (c =3D sanitized_name; *c !=3D '\0'; c++) { >>> + if (*c =3D=3D '/') { >>> + *c =3D '_'; >>> + } >>> + } >>> + filename =3D g_strdup_printf("%s/qemu_back_mem.%s.XXXXXX", pat= h, >>> + sanitized_name); >>> + g_free(sanitized_name); >> >> one empty line will be very nice here, and it was in master branch >> >>> + fd =3D mkstemp(filename); >>> + if (fd >=3D 0) { >>> + unlink(filename); >>> + /* >>> + * ftruncate is not supported by hugetlbfs in older >>> + * hosts, so don't bother bailing out on errors. >>> + * If anything goes wrong with it under other filesystems, >>> + * mmap will fail. >>> + */ >>> + if (ftruncate(fd, size)) { >>> + perror("ftruncate"); >>> + } >>> + } >>> + g_free(filename); >>> + >>> + return fd; >>> +} >>> + >>> static void *file_ram_alloc(RAMBlock *block, >>> ram_addr_t memory, >>> const char *path, >>> Error **errp) >>> { >>> - char *filename; >>> - char *sanitized_name; >>> - char *c; >>> void *area; >>> int fd; >>> uint64_t pagesize; >>> @@ -1212,38 +1258,14 @@ static void *file_ram_alloc(RAMBlock *block= , >>> goto error; >>> } >>> - /* Make name safe to use with mkstemp by replacing '/' with=20 >>> '_'. */ >>> - sanitized_name =3D g_strdup(memory_region_name(block->mr)); >>> - for (c =3D sanitized_name; *c !=3D '\0'; c++) { >>> - if (*c =3D=3D '/') >>> - *c =3D '_'; >>> - } >>> - >>> - filename =3D g_strdup_printf("%s/qemu_back_mem.%s.XXXXXX", pat= h, >>> - sanitized_name); >>> - g_free(sanitized_name); >>> + memory =3D ROUND_UP(memory, pagesize); >>> - fd =3D mkstemp(filename); >>> + fd =3D open_ram_file_path(block, path, memory); >>> if (fd < 0) { >>> error_setg_errno(errp, errno, >>> "unable to create backing store for path= =20 >>> %s", path); >>> - g_free(filename); >>> goto error; >>> } >>> - unlink(filename); >>> - g_free(filename); >>> - >>> - memory =3D ROUND_UP(memory, pagesize); >>> - >>> - /* >>> - * ftruncate is not supported by hugetlbfs in older >>> - * hosts, so don't bother bailing out on errors. >>> - * If anything goes wrong with it under other filesystems, >>> - * mmap will fail. >>> - */ >>> - if (ftruncate(fd, memory)) { >>> - perror("ftruncate"); >>> - } >>> area =3D qemu_ram_mmap(fd, memory, pagesize, block->flags &=20 >>> RAM_SHARED); >>> if (area =3D=3D MAP_FAILED) { >> >> >> Reviewed-by: Vladimir Sementsov-Ogievskiy > > Thanks for your review. > > --=20 Best regards, Vladimir * now, @virtuozzo.com instead of @parallels.com. Sorry for this inconve= nience. From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39626) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZtHW1-0001mT-DI for qemu-devel@nongnu.org; Mon, 02 Nov 2015 10:59:26 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZtHVx-0004Ko-8e for qemu-devel@nongnu.org; Mon, 02 Nov 2015 10:59:25 -0500 Received: from relay.parallels.com ([195.214.232.42]:46323) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZtHVw-0004KW-Qe for qemu-devel@nongnu.org; Mon, 02 Nov 2015 10:59:21 -0500 Message-ID: <56378841.8070804@virtuozzo.com> Date: Mon, 2 Nov 2015 18:58:57 +0300 From: Vladimir Sementsov-Ogievskiy MIME-Version: 1.0 References: <1446455617-129562-1-git-send-email-guangrong.xiao@linux.intel.com> <1446455617-129562-10-git-send-email-guangrong.xiao@linux.intel.com> <56377D56.6050301@virtuozzo.com> <56378054.6050100@linux.intel.com> In-Reply-To: <56378054.6050100@linux.intel.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v7 09/35] exec: allow file_ram_alloc to work on file List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Xiao Guangrong , pbonzini@redhat.com, imammedo@redhat.com Cc: ehabkost@redhat.com, kvm@vger.kernel.org, mst@redhat.com, gleb@kernel.org, mtosatti@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, dan.j.williams@intel.com, rth@twiddle.net On 02.11.2015 18:25, Xiao Guangrong wrote: > > > On 11/02/2015 11:12 PM, Vladimir Sementsov-Ogievskiy wrote: >> On 02.11.2015 12:13, Xiao Guangrong wrote: >>> Currently, file_ram_alloc() only works on directory - it creates a file >>> under @path and do mmap on it >>> >>> This patch tries to allow it to work on file directly, if @path is a >> >> It isn't try to allow, it allows, as I understand)... > > Err... Sorry for my English, but what is the different between: > ”This patch tries to allow it to work on file directly“ and > "This patch allows it to work on file directly" > > :( Not sure that everyone is interested in this nit-picking discussion.. A allows B: if A then B A tries to allow B: if A then _may be_ B In any way it doesn't matter. > >> >>> directory it works as before, otherwise it treats @path as the target >>> file then directly allocate memory from it >>> >>> Signed-off-by: Xiao Guangrong >>> --- >>> exec.c | 80 >>> ++++++++++++++++++++++++++++++++++++++++++------------------------ >>> 1 file changed, 51 insertions(+), 29 deletions(-) >>> >>> diff --git a/exec.c b/exec.c >>> index 9075f4d..db0fdaf 100644 >>> --- a/exec.c >>> +++ b/exec.c >>> @@ -1174,14 +1174,60 @@ void qemu_mutex_unlock_ramlist(void) >>> } >>> #ifdef __linux__ >>> +static bool path_is_dir(const char *path) >>> +{ >>> + struct stat fs; >>> + >>> + return stat(path, &fs) == 0 && S_ISDIR(fs.st_mode); >>> +} >>> + >>> +static int open_ram_file_path(RAMBlock *block, const char *path, >>> size_t size) >>> +{ >>> + char *filename; >>> + char *sanitized_name; >>> + char *c; >>> + int fd; >>> + >>> + if (!path_is_dir(path)) { >>> + int flags = (block->flags & RAM_SHARED) ? O_RDWR : O_RDONLY; >>> + >>> + flags |= O_EXCL; >>> + return open(path, flags); >>> + } >>> + >>> + /* Make name safe to use with mkstemp by replacing '/' with >>> '_'. */ >>> + sanitized_name = g_strdup(memory_region_name(block->mr)); >>> + for (c = sanitized_name; *c != '\0'; c++) { >>> + if (*c == '/') { >>> + *c = '_'; >>> + } >>> + } >>> + filename = g_strdup_printf("%s/qemu_back_mem.%s.XXXXXX", path, >>> + sanitized_name); >>> + g_free(sanitized_name); >> >> one empty line will be very nice here, and it was in master branch >> >>> + fd = mkstemp(filename); >>> + if (fd >= 0) { >>> + unlink(filename); >>> + /* >>> + * ftruncate is not supported by hugetlbfs in older >>> + * hosts, so don't bother bailing out on errors. >>> + * If anything goes wrong with it under other filesystems, >>> + * mmap will fail. >>> + */ >>> + if (ftruncate(fd, size)) { >>> + perror("ftruncate"); >>> + } >>> + } >>> + g_free(filename); >>> + >>> + return fd; >>> +} >>> + >>> static void *file_ram_alloc(RAMBlock *block, >>> ram_addr_t memory, >>> const char *path, >>> Error **errp) >>> { >>> - char *filename; >>> - char *sanitized_name; >>> - char *c; >>> void *area; >>> int fd; >>> uint64_t pagesize; >>> @@ -1212,38 +1258,14 @@ static void *file_ram_alloc(RAMBlock *block, >>> goto error; >>> } >>> - /* Make name safe to use with mkstemp by replacing '/' with >>> '_'. */ >>> - sanitized_name = g_strdup(memory_region_name(block->mr)); >>> - for (c = sanitized_name; *c != '\0'; c++) { >>> - if (*c == '/') >>> - *c = '_'; >>> - } >>> - >>> - filename = g_strdup_printf("%s/qemu_back_mem.%s.XXXXXX", path, >>> - sanitized_name); >>> - g_free(sanitized_name); >>> + memory = ROUND_UP(memory, pagesize); >>> - fd = mkstemp(filename); >>> + fd = open_ram_file_path(block, path, memory); >>> if (fd < 0) { >>> error_setg_errno(errp, errno, >>> "unable to create backing store for path >>> %s", path); >>> - g_free(filename); >>> goto error; >>> } >>> - unlink(filename); >>> - g_free(filename); >>> - >>> - memory = ROUND_UP(memory, pagesize); >>> - >>> - /* >>> - * ftruncate is not supported by hugetlbfs in older >>> - * hosts, so don't bother bailing out on errors. >>> - * If anything goes wrong with it under other filesystems, >>> - * mmap will fail. >>> - */ >>> - if (ftruncate(fd, memory)) { >>> - perror("ftruncate"); >>> - } >>> area = qemu_ram_mmap(fd, memory, pagesize, block->flags & >>> RAM_SHARED); >>> if (area == MAP_FAILED) { >> >> >> Reviewed-by: Vladimir Sementsov-Ogievskiy > > Thanks for your review. > > -- Best regards, Vladimir * now, @virtuozzo.com instead of @parallels.com. Sorry for this inconvenience.