| Message ID | 1446455617-129562-10-git-send-email-guangrong.xiao@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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) > directory it works as before, otherwise it treats @path as the target > file then directly allocate memory from it > > Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com> > --- > 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 <vsementsov@virtuozzo.com>
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" :( > >> directory it works as before, otherwise it treats @path as the target >> file then directly allocate memory from it >> >> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com> >> --- >> 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 <vsementsov@virtuozzo.com> Thanks for your review. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <guangrong.xiao@linux.intel.com> >>> --- >>> 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 <vsementsov@virtuozzo.com> > > Thanks for your review. > >
On 02/11/2015 10: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 > directory it works as before, otherwise it treats @path as the target > file then directly allocate memory from it > > Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com> > --- > 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); > + 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) { > I was going to send tomorrow a pull request for a similar patch, "backends/hostmem-file: Allow to specify full pathname for backing file". The main difference seems to be your usage of O_EXCL. Can you explain why you added it? Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/03/2015 05:12 AM, Paolo Bonzini wrote: > > > On 02/11/2015 10: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 >> directory it works as before, otherwise it treats @path as the target >> file then directly allocate memory from it >> >> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com> >> --- >> 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); >> + 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) { >> > > I was going to send tomorrow a pull request for a similar patch, > "backends/hostmem-file: Allow to specify full pathname for backing file". > > The main difference seems to be your usage of O_EXCL. Can you explain > why you added it? It' used if we pass a block device as a NVDIMM backend memory: O_EXCL can be used without O_CREAT if pathname refers to a block device. If the block device is in use by the system (e.g., mounted), open() fails with the error EBUSY -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2 Nov 2015 17:13:11 +0800 Xiao Guangrong <guangrong.xiao@linux.intel.com> 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 > directory it works as before, otherwise it treats @path as the target > file then directly allocate memory from it Paolo has just queued https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg06513.html perhaps that's what you can reuse here. > > Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com> > --- > 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); > + 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) { -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/03/2015 08:34 PM, Igor Mammedov wrote: > On Mon, 2 Nov 2015 17:13:11 +0800 > Xiao Guangrong <guangrong.xiao@linux.intel.com> 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 >> directory it works as before, otherwise it treats @path as the target >> file then directly allocate memory from it > Paolo has just queued > https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg06513.html > perhaps that's what you can reuse here. Yep, Paolo has told me about that, i will update this patchset after his pull request. BTW, which tree should this patchset be based on in future development? Paolo's or Michael's or even upstream qemu tree? Thanks! -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/11/2015 04:56, Xiao Guangrong wrote: > > > On 11/03/2015 05:12 AM, Paolo Bonzini wrote: >> >> >> On 02/11/2015 10: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 >>> directory it works as before, otherwise it treats @path as the target >>> file then directly allocate memory from it >>> >>> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com> >>> --- >>> 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); >>> + 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) { >>> >> >> I was going to send tomorrow a pull request for a similar patch, >> "backends/hostmem-file: Allow to specify full pathname for backing file". >> >> The main difference seems to be your usage of O_EXCL. Can you explain >> why you added it? > > It' used if we pass a block device as a NVDIMM backend memory: > O_EXCL can be used without O_CREAT if pathname refers to a block > device. If the block device > is in use by the system (e.g., mounted), open() fails with the error EBUSY That makes sense, but I think it's better to be consistent with the handling of block devices. Block devices do not use O_EXCL when QEMU opens them; I guess in principle it would also be possible to share a single pmem backend between multiple guests. Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/03/2015 09:55 PM, Paolo Bonzini wrote: > > > On 03/11/2015 04:56, Xiao Guangrong wrote: >> >> >> On 11/03/2015 05:12 AM, Paolo Bonzini wrote: >>> >>> >>> On 02/11/2015 10: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 >>>> directory it works as before, otherwise it treats @path as the target >>>> file then directly allocate memory from it >>>> >>>> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com> >>>> --- >>>> 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); >>>> + 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) { >>>> >>> >>> I was going to send tomorrow a pull request for a similar patch, >>> "backends/hostmem-file: Allow to specify full pathname for backing file". >>> >>> The main difference seems to be your usage of O_EXCL. Can you explain >>> why you added it? >> >> It' used if we pass a block device as a NVDIMM backend memory: >> O_EXCL can be used without O_CREAT if pathname refers to a block >> device. If the block device >> is in use by the system (e.g., mounted), open() fails with the error EBUSY > > That makes sense, but I think it's better to be consistent with the > handling of block devices. Block devices do not use O_EXCL when QEMU > opens them; I guess in principle it would also be possible to share a > single pmem backend between multiple guests. Yup. Will make a separate patch to do this. :) -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); + 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) {
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 directory it works as before, otherwise it treats @path as the target file then directly allocate memory from it Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com> --- exec.c | 80 ++++++++++++++++++++++++++++++++++++++++++------------------------ 1 file changed, 51 insertions(+), 29 deletions(-)