[v6,09/33] exec: allow file_ram_alloc to work on file
diff mbox

Message ID 1446184587-142784-10-git-send-email-guangrong.xiao@linux.intel.com
State New
Headers show

Commit Message

Xiao Guangrong Oct. 30, 2015, 5:56 a.m. UTC
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(-)

Comments

Vladimir Sementsov-Ogievskiy Oct. 30, 2015, 2:25 p.m. UTC | #1
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) {
Xiao Guangrong Oct. 31, 2015, 7:53 a.m. UTC | #2
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
Michael S. Tsirkin Nov. 9, 2015, 10:13 a.m. UTC | #3
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
Xiao Guangrong Nov. 9, 2015, 10:48 a.m. UTC | #4
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

Patch
diff mbox

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) {