| Message ID | 1446184587-142784-10-git-send-email-guangrong.xiao@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 30.10.2015 08:56, 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 3ca7e50..f219010 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_file_path(RAMBlock *block, const char *path, size_t size) I think the name should be more descriptive, as it is very special function in common exec.c and it doesn't "just open file path'.. May be open_ram_file_path > +{ > + 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); > + } Was not there old scenarios when path is file? statfs will success for any file withing mounted filesystem. Also, may be we shouldn't warn about "Memory is not allocated from HugeTlbfs.\n" in case of !path_is_dir ? > + > + /* 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; > @@ -1211,38 +1257,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_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) {
On 10/30/2015 10:25 PM, Vladimir Sementsov-Ogievskiy wrote: > On 30.10.2015 08:56, 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 3ca7e50..f219010 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_file_path(RAMBlock *block, const char *path, size_t size) > > I think the name should be more descriptive, as it is very special function in common exec.c and it > doesn't "just open file path'.. May be open_ram_file_path Okay, good to me. :) > > >> +{ >> + 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); >> + } > > Was not there old scenarios when path is file? statfs will success for any file withing mounted > filesystem. > Yes. The old scenarios will create a temporary file under the @path that stop regular file's work. > Also, may be we shouldn't warn about "Memory is not allocated from HugeTlbfs.\n" in case of > !path_is_dir ? > A file from hugetlbfs and attach it as backend memory is a valid case to users. -- 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 Fri, Oct 30, 2015 at 01:56:03PM +0800, 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 3ca7e50..f219010 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); This means file doesn't exist is treated as a file. Can't figure out if that's intentional, should be documented in any case. > +} > + > +static int open_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; Why does this make sense? > + > + flags |= O_EXCL; And why does this makes sense? > + 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; > @@ -1211,38 +1257,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_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) { > -- > 1.8.3.1 -- 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/09/2015 06:13 PM, Michael S. Tsirkin wrote: > On Fri, Oct 30, 2015 at 01:56:03PM +0800, 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 3ca7e50..f219010 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); > > This means file doesn't exist is treated as a file. > Can't figure out if that's intentional, should > be documented in any case. The path is dir only if it passes S_ISDIR test. Otherwise, it is treated as regular file. Any error on the file will be figured out by open() and mmap() later. > >> +} >> + >> +static int open_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; > > Why does this make sense? RAM_SHARED means its write is invisible to others also it does not actual change the content of the file. Readonly permission is enough for this case. It is also help us to pass a readonly file to the guess but discard its change after guest shutdown. > >> + >> + flags |= O_EXCL; > > And why does this makes sense? 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 BTW, the similar logic has already been in upsteam QEMU which is introduced by commit 8d31d6b6 (backends/hostmem-file: Allow to specify full pathname for backing file). I will drop these patches: util: introduce qemu_file_get_page_size() exec: allow memory to be allocated from any kind of path exec: allow file_ram_alloc to work on file -- 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 3ca7e50..f219010 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_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; @@ -1211,38 +1257,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_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(-)