Skip to content

Conversation

@crafcat7
Copy link
Contributor

@crafcat7 crafcat7 commented Sep 17, 2024

Summary

Why is this change necessary
In some external test cases (such as FIO), they use the msync interface provided by the system to ensure that the rammap data can actually be written to the file system. Currently, mmap in NuttX does not provide this interface, so msync is implemented.

Changes
This mainly adds fs_msync.c under fs/mmap. And makes some adjustments to rammap.c and anonmap.c

Implementation details
Each mmap corresponds to an mm_map entry. The priv.p of each entry is used to save filep + type (the lower three bits of each priv.p represent type).
At each msync, the lower three bits of type need to be restored to filep and the data written

Refer to #5997 and implement an msync based on the current mmap
Follow https://man7.org/linux/man-pages/man2/msync.2.html

Impact

  • Is a new feature added? YES, the msync() system call is now supported.
  • Impact on user: YES, users can now utilize msync() to synchronize memory-mapped files with their corresponding files on disk. This is essential for applications that require data consistency between memory and storage.
  • Impact on build: NO, the build process remains unchanged. No new build options or dependencies are introduced.
  • Impact on hardware: NO, this change does not specifically target any particular hardware architectures or boards.
  • Impact on documentation: YES, documentation updates are required to describe the new msync() functionality and its usage. These updates are included in this PR.
  • Impact on security: The addition of msync() itself doesn't introduce new security vulnerabilities, however, improper usage of msync() by application developers could potentially lead to data corruption or inconsistencies.
  • Impact on compatibility: This implementation aims to be backward compatible. No existing functionality is expected to be broken.

Testing

Build Host(s): Linux x86
Target(s): sim/nsh

Based on anonymous mapping and rammap demo is OK.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/mman.h>
#include <unistd.h>

int main() {
    const char *text = "This is a demo of anonymous mmap!";
    size_t length = strlen(text) + 1; 

    char *map = mmap(NULL, length, PROT_READ | PROT_WRITE, MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
    if (map == MAP_FAILED) {
        perror("mmap");
        return 1;
    }

    memcpy(map, text, length);

    printf("Mapped memory contains: %s\n", map);

    if (munmap(map, length) == -1) {
        perror("munmap");
        return 1;
    }

    return 0;
}
#include <stdio.h>
#include <stdlib.h>
#include <fcntl.h>
#include <unistd.h>
#include <sys/mman.h>
#include <sys/stat.h>
#include <string.h>

int main(int argc, char *argv[]) {
    const char *filepath = "example.txt";
    const char *text = "This is a demo of mmap!";
    
    int fd = open(filepath, O_RDWR | O_CREAT, S_IRUSR | S_IWUSR);
    if (fd == -1) {
        perror("open");
        return 1;
    }

    size_t filesize = strlen(text);
    if (ftruncate(fd, filesize) == -1) {
        perror("ftruncate");
        close(fd);
        return 1;
    }

    char *map = mmap(NULL, filesize, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
    if (map == MAP_FAILED) {
        perror("mmap");
        close(fd);
        return 1;
    }

    memcpy(map, text, filesize);

    if (msync(map, filesize, MS_SYNC) == -1) {
        perror("msync");
    }

    if (munmap(map, filesize) == -1) {
        perror("munmap");
    }

    close(fd);
    
    printf("File mapped and modified successfully.\n");

    return 0;
}

Signed-off-by: wanggang26 <wanggang26@xiaomi.com>
@lupyuen
Copy link
Member

lupyuen commented Sep 18, 2024

[Experimental Bot, please feedback here]

The provided PR description does not fully meet the NuttX requirements. Here's a breakdown and suggestions for improvement:

Strengths:

  • Summary: Briefly explains the change and provides a link to the source of the requirement.
  • Testing: Includes code examples demonstrating the functionality before and after the change.

Areas for Improvement:

  • Summary:

    • Why is this change necessary? Elaborate on the benefits of having msync. Why is it important to implement it based on the current mmap implementation?
    • What functional part of the code is being changed? Be specific about which files and functions are modified.
    • How does the change exactly work? Provide a more technical explanation of how msync interacts with the existing mmap logic.
  • Impact: The current response is too brief. Address all the points:

    • Is a new feature added? (YES - msync)
    • Impact on the user: YES - Users can now utilize msync. Describe how they would use it and what benefits it offers.
    • Impact on build: Are any new build options introduced? Any dependencies added?
    • Impact on hardware: Specify if this change affects any specific architectures or boards.
    • Impact on documentation: YES - You need to document the new msync functionality. State if this documentation is included in the PR.
    • Impact on security: Are there any potential security implications of adding msync? Explain and mitigate if so.
    • Impact on compatibility: Does this change affect backward compatibility?
  • Testing:

    • Build Host(s): Specify the operating system, CPU architecture, and compiler used for testing.
    • Target(s): Be specific about the target architecture(s) and board configurations used for testing.
    • Testing logs: While code examples are good, include actual output logs from running the code before and after the change.

Revised Impact Section Example:

## Impact

* **Is a new feature added?** YES, the `msync()` system call is now supported.
* **Impact on user:** YES, users can now utilize `msync()` to synchronize memory-mapped files with their corresponding files on disk. This is essential for applications that require data consistency between memory and storage.
* **Impact on build:** NO, the build process remains unchanged. No new build options or dependencies are introduced.
* **Impact on hardware:** NO, this change does not specifically target any particular hardware architectures or boards. 
* **Impact on documentation:** YES, documentation updates are required to describe the new `msync()` functionality and its usage. These updates are included in this PR.
* **Impact on security:**  The addition of `msync()` itself doesn't introduce new security vulnerabilities, however, improper usage of `msync()` by application developers could potentially lead to data corruption or inconsistencies. 
* **Impact on compatibility:**  This implementation aims to be backward compatible. No existing functionality is expected to be broken.

Additional Tips:

  • Clarity: Use clear and concise language throughout the description.
  • Technical Details: Don't be afraid to get technical, especially when explaining the implementation and potential impact.
  • Thoroughness: Address all the required points in the template.

By improving the PR description, you make it easier for reviewers to understand your changes and ensure a smoother review process.

Summary:
  1.Added msync callback in struct mm_map_entry_s
  2.Added msync API in fs_msync.c
  3.Added static msync_rammap for rammap.

Signed-off-by: chenrun1 <chenrun1@xiaomi.com>
Summary:
 Limitations of using fs putfile

Signed-off-by: chenrun1 <chenrun1@xiaomi.com>
Summary:
  In rammap:
   1.0 - User
   2.1 - Kernel
   3.2 - XIP
  Therefore we need to use 2 bits to represent the type

Signed-off-by: chenrun1 <chenrun1@xiaomi.com>
mmap/fs_rammap.c:81:39: error: expected ‘)’ before ‘PRIdOFF’
   81 |       ferr("ERRORL Seek to position %"PRIdOFF" failed\n", fpos);
      |                                       ^~~~~~~
mmap/fs_rammap.c:81:12: warning: spurious trailing ‘%’ in format [-Wformat=]
   81 |       ferr("ERRORL Seek to position %"PRIdOFF" failed\n", fpos);
      |            ^~~~~~~~~~~~~~~~~~~~~~~~~~~
mmap/fs_rammap.c:81:37: note: format string is defined here
   81 |       ferr("ERRORL Seek to position %"PRIdOFF" failed\n", fpos);
      |                                     ^
In file included from mmap/fs_rammap.c:30:
mmap/fs_rammap.c:98:51: error: expected ‘)’ before ‘PRIdOFF’
   98 |               ferr("ERROR: Write failed: offset=%"PRIdOFF" nwrite=%zd\n",
      |                                                   ^~~~~~~
mmap/fs_rammap.c:98:20: warning: spurious trailing ‘%’ in format [-Wformat=]
   98 |               ferr("ERROR: Write failed: offset=%"PRIdOFF" nwrite=%zd\n",
      |                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
mmap/fs_rammap.c:98:49: note: format string is defined here
   98 |               ferr("ERROR: Write failed: offset=%"PRIdOFF" nwrite=%zd\n",

Signed-off-by: zhangshoukui <zhangshoukui@xiaomi.com>
@acassis
Copy link
Contributor

acassis commented Sep 18, 2024

@crafcat7 please include this testing example to apps/testing, maybe inside ostest or other place

@xiaoxiang781216
Copy link
Contributor

@crafcat7 please include this testing example to apps/testing, maybe inside ostest or other place

@acassis it's a standard api, let's reuse ltp test case:
https://github.com/linux-test-project/ltp/tree/master/testcases/kernel/syscalls/msync

@xiaoxiang781216 xiaoxiang781216 merged commit 3fb6b4c into apache:master Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants