| Message ID | 1446455617-129562-10-git-send-email-guangrong.xiao@linux.intel.com |
|---|---|
| State | New |
| 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.
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
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
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) {
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!
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
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. :)
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(-)