[v7,09/35] exec: allow file_ram_alloc to work on file
diff mbox

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

Commit Message

Xiao Guangrong Nov. 2, 2015, 9:13 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 Nov. 2, 2015, 3:12 p.m. UTC | #1
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>
Xiao Guangrong Nov. 2, 2015, 3:25 p.m. UTC | #2
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
Vladimir Sementsov-Ogievskiy Nov. 2, 2015, 3:58 p.m. UTC | #3
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.
>
>
Paolo Bonzini Nov. 2, 2015, 9:12 p.m. UTC | #4
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
Xiao Guangrong Nov. 3, 2015, 3:56 a.m. UTC | #5
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
Igor Mammedov Nov. 3, 2015, 12:34 p.m. UTC | #6
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
Xiao Guangrong Nov. 3, 2015, 1:32 p.m. UTC | #7
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
Paolo Bonzini Nov. 3, 2015, 1:55 p.m. UTC | #8
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
Xiao Guangrong Nov. 3, 2015, 2:26 p.m. UTC | #9
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

Patch
diff mbox

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