Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add file flush in sys_close and sys_exit #80

Closed
wants to merge 1 commit into from

Conversation

lhw2002426
Copy link
Contributor

No description provided.

Copy link
Contributor

@coolyjg coolyjg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix bugs and some code.

api/ruxos_posix_api/src/imp/fd_ops.rs Outdated Show resolved Hide resolved
api/ruxos_posix_api/src/imp/fd_ops.rs Outdated Show resolved Hide resolved
api/ruxos_posix_api/src/imp/net.rs Show resolved Hide resolved
api/ruxos_posix_api/src/imp/pthread/mod.rs Show resolved Hide resolved
api/ruxos_posix_api/src/imp/stdio.rs Show resolved Hide resolved
api/ruxos_posix_api/src/imp/task.rs Show resolved Hide resolved
modules/ruxfs/src/dev.rs Outdated Show resolved Hide resolved
modules/ruxfs/src/fs/fatfs.rs Outdated Show resolved Hide resolved
modules/ruxfs/src/fs/fatfs.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@coolyjg coolyjg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there are concurrency bugs for the sys_exit and sys_pthread_exit:
Checking fd_table_count() in every while loop seems to be unsafe for multi-thread situation..... I'll take more consideration..... @ken4647
By the way, ramfs should implement a fake flush as well.

api/ruxos_posix_api/src/imp/fd_ops.rs Show resolved Hide resolved
@@ -184,7 +192,7 @@ fn flags_to_options(flags: c_int, _mode: ctypes::mode_t) -> OpenOptions {
/// has the maximum number of files open.
pub fn sys_open(filename: *const c_char, flags: c_int, mode: ctypes::mode_t) -> c_int {
let filename = char_ptr_to_str(filename);
debug!("sys_open <= {:?} {:#o} {:#o}", filename, flags, mode);
info!("sys_open <= {:?} {:#o} {:#o}", filename, flags, mode);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use debug

@@ -196,7 +204,7 @@ pub fn sys_open(filename: *const c_char, flags: c_int, mode: ctypes::mode_t) ->
pub fn sys_openat(fd: usize, path: *const c_char, flags: c_int, mode: ctypes::mode_t) -> c_int {
let path = char_ptr_to_str(path);
let fd: c_int = fd as c_int;
debug!("sys_openat <= {}, {:?}, {:#o} {:#o}", fd, path, flags, mode);
info!("sys_openat <= {}, {:?}, {:#o} {:#o}", fd, path, flags, mode);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use debug

@@ -8,6 +8,7 @@
*/

use alloc::sync::Arc;
use axio::Error;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused?

@coolyjg
Copy link
Contributor

coolyjg commented Apr 3, 2024

For example:
If thread A exit, two files are opened with fd=3, fd=4.
FIRST loop: flush fd=3, get fd_table_count=2, now_fd_count=1
SECOND loop: flush fd=4
(thread B open another file fd=5, then fd_table_count -> 3)
get fd_table_count=3, now_fd_count=2, continue loop.
THIRD loop: flush fd=5
(thread B now close fd=5, then fd_table_count -> 2)
get fd_table_count=2, now_fd_count=3, still continue loop. And loop for 1024 times which caused
very bad performance.
This example might be too extreme, but I think the situation is not rare. @ken4647
There are some of my thoughts:

  • sync operation should always be idempotent(need to check current implementation).
  • A big fd_table lock when exiting. (I think it's unacceptable)
  • Gather all opened fd one time when exiting.

@coolyjg
Copy link
Contributor

coolyjg commented Apr 3, 2024

#include <stdio.h>
#include <unistd.h>
#include <stdlib.h>
#include <fcntl.h>
#include <string.h>
#include <pthread.h>


char* setFileName(char* s, int i){
    s[4] = i + '0';
    return s;
}


void* threadA(void* _args){
    char* s = (char*)malloc(sizeof(char)*6);
    strcpy(s, "file0");
    s[5] = '\0';

    for(int i = 0; i<10; i++) {
        s = setFileName(s, i);
        int fd = open(s, O_RDWR | O_CREAT);
        printf("fd = %d, name: %s\n", fd, s);
        close(fd);
    }
}

void* threadB(void* _args){
    char* s = (char*)malloc(sizeof(char)*6);
    strcpy(s, "Fale0");
    s[5] = '\0';

    for(int i = 0; i<3; i++) {
        s = setFileName(s, i);
        int fd = open(s, O_RDWR | O_CREAT);
        printf("fd = %d, name: %s\n", fd, s);
    }
}


int main()
{
    pthread_t p1, p2;
    pthread_create(&p1, NULL, threadA, NULL);
    pthread_create(&p2, NULL, threadB, NULL);
    pthread_join(p1, NULL);
    pthread_join(p2, NULL);

    return 0;
}

make A=apps/c/helloworld/ MUSL=y FEATURES=fs,multitask,blkfs,fd BLK=y LOG=debug ARCH=aarch64 run SMP=4

@coolyjg
Copy link
Contributor

coolyjg commented Apr 8, 2024

I think this PR should be merged after #81 is fixed.

@coolyjg
Copy link
Contributor

coolyjg commented Apr 9, 2024

In rust-fatfs, when closing file, files would be called flush automatically, in rust-fats/src/file.rs line 235

@lhw2002426 lhw2002426 closed this Apr 15, 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.

2 participants