diff --git a/fs/inode/fs_files.c b/fs/inode/fs_files.c index bcb7638288166..af9120b852f5d 100644 --- a/fs/inode/fs_files.c +++ b/fs/inode/fs_files.c @@ -51,14 +51,47 @@ * Private Functions ****************************************************************************/ +/**************************************************************************** + * Name: files_fget_index + ****************************************************************************/ + +static FAR struct file *files_fget_index(FAR struct filelist *list, + int l1, int l2) +{ + FAR struct file *filep; + irqstate_t flags; + + flags = enter_critical_section(); + + filep = &list->fl_files[l1][l2]; + + leave_critical_section(flags); + + return filep; +} + +/**************************************************************************** + * Name: files_fget + ****************************************************************************/ + +static FAR struct file *files_fget(FAR struct filelist *list, int fd) +{ + return files_fget_index(list, fd / CONFIG_NFILE_DESCRIPTORS_PER_BLOCK, + fd % CONFIG_NFILE_DESCRIPTORS_PER_BLOCK); +} + /**************************************************************************** * Name: files_extend ****************************************************************************/ static int files_extend(FAR struct filelist *list, size_t row) { - FAR struct file **tmp; + FAR struct file **files; + uint8_t orig_rows; + FAR void *tmp; + int flags; int i; + int j; if (row <= list->fl_rows) { @@ -70,9 +103,11 @@ static int files_extend(FAR struct filelist *list, size_t row) return -EMFILE; } - tmp = kmm_realloc(list->fl_files, sizeof(FAR struct file *) * row); - DEBUGASSERT(tmp); - if (tmp == NULL) + orig_rows = list->fl_rows; + + files = kmm_malloc(sizeof(FAR struct file *) * row); + DEBUGASSERT(files); + if (files == NULL) { return -ENFILE; } @@ -80,30 +115,54 @@ static int files_extend(FAR struct filelist *list, size_t row) i = list->fl_rows; do { - tmp[i] = kmm_zalloc(sizeof(struct file) * - CONFIG_NFILE_DESCRIPTORS_PER_BLOCK); - if (tmp[i] == NULL) + files[i] = kmm_zalloc(sizeof(struct file) * + CONFIG_NFILE_DESCRIPTORS_PER_BLOCK); + if (files[i] == NULL) { while (--i >= list->fl_rows) { - kmm_free(tmp[i]); + kmm_free(files[i]); } - kmm_free(tmp); + kmm_free(files); return -ENFILE; } } while (++i < row); - list->fl_files = tmp; - list->fl_rows = row; + flags = enter_critical_section(); - /* Note: If assertion occurs, the fl_rows has a overflow. - * And there may be file descriptors leak in system. + /* To avoid race condition, if the file list is updated by other threads + * and list rows is greater or equal than temp list, + * release the obsolete buffers */ - DEBUGASSERT(list->fl_rows == row); - return 0; + if (orig_rows != list->fl_rows && list->fl_rows >= row) + { + leave_critical_section(flags); + + for (j = orig_rows; j < i; j++) + { + kmm_free(files[i]); + } + + kmm_free(files); + + return OK; + } + + memcpy(files, list->fl_files, + list->fl_rows * sizeof(FAR struct file *)); + + tmp = list->fl_files; + list->fl_files = files; + list->fl_rows = row; + + leave_critical_section(flags); + + kmm_free(tmp); + + return OK; } static void task_fssync(FAR struct tcb_s *tcb, FAR void *arg) @@ -113,10 +172,6 @@ static void task_fssync(FAR struct tcb_s *tcb, FAR void *arg) int j; list = &tcb->group->tg_filelist; - if (nxmutex_lock(&list->fl_lock) < 0) - { - return; - } for (i = 0; i < list->fl_rows; i++) { @@ -124,15 +179,13 @@ static void task_fssync(FAR struct tcb_s *tcb, FAR void *arg) { FAR struct file *filep; - filep = &list->fl_files[i][j]; - if (filep != NULL && filep->f_inode != NULL) + filep = files_fget_index(list, i, j); + if (filep->f_inode != NULL) { file_fsync(filep); } } } - - nxmutex_unlock(&list->fl_lock); } /**************************************************************************** @@ -183,41 +236,27 @@ static int nx_dup3_from_tcb(FAR struct tcb_s *tcb, int fd1, int fd2, return -EBADF; } - ret = nxmutex_lock(&list->fl_lock); - if (ret < 0) - { - /* Probably canceled */ - - return ret; - } - if (fd2 >= CONFIG_NFILE_DESCRIPTORS_PER_BLOCK * list->fl_rows) { ret = files_extend(list, fd2 / CONFIG_NFILE_DESCRIPTORS_PER_BLOCK + 1); if (ret < 0) { - nxmutex_unlock(&list->fl_lock); return ret; } } - filep = &list->fl_files[fd2 / CONFIG_NFILE_DESCRIPTORS_PER_BLOCK] - [fd2 % CONFIG_NFILE_DESCRIPTORS_PER_BLOCK]; + filep = files_fget(list, fd2); memcpy(&file, filep, sizeof(struct file)); memset(filep, 0, sizeof(struct file)); /* Perform the dup3 operation */ - ret = file_dup3(&list->fl_files[fd1 / CONFIG_NFILE_DESCRIPTORS_PER_BLOCK] - [fd1 % CONFIG_NFILE_DESCRIPTORS_PER_BLOCK], - filep, flags); + ret = file_dup3(files_fget(list, fd1), filep, flags); #ifdef CONFIG_FDSAN filep->f_tag = file.f_tag; #endif - nxmutex_unlock(&list->fl_lock); - file_close(&file); #ifdef CONFIG_FDCHECK @@ -241,10 +280,6 @@ static int nx_dup3_from_tcb(FAR struct tcb_s *tcb, int fd1, int fd2, void files_initlist(FAR struct filelist *list) { DEBUGASSERT(list); - - /* Initialize the list access mutex */ - - nxmutex_init(&list->fl_lock); } /**************************************************************************** @@ -278,10 +313,6 @@ void files_releaselist(FAR struct filelist *list) } kmm_free(list->fl_files); - - /* Destroy the mutex */ - - nxmutex_destroy(&list->fl_lock); } /**************************************************************************** @@ -302,6 +333,7 @@ int file_allocate_from_tcb(FAR struct tcb_s *tcb, FAR struct inode *inode, bool addref) { FAR struct filelist *list; + FAR struct file *filep; int ret; int i; int j; @@ -309,15 +341,6 @@ int file_allocate_from_tcb(FAR struct tcb_s *tcb, FAR struct inode *inode, /* Get the file descriptor list. It should not be NULL in this context. */ list = nxsched_get_files_from_tcb(tcb); - DEBUGASSERT(list != NULL); - - ret = nxmutex_lock(&list->fl_lock); - if (ret < 0) - { - /* Probably canceled */ - - return ret; - } /* Calculate minfd whether is in list->fl_files. * if not, allocate a new filechunk. @@ -329,7 +352,6 @@ int file_allocate_from_tcb(FAR struct tcb_s *tcb, FAR struct inode *inode, ret = files_extend(list, i + 1); if (ret < 0) { - nxmutex_unlock(&list->fl_lock); return ret; } } @@ -341,25 +363,10 @@ int file_allocate_from_tcb(FAR struct tcb_s *tcb, FAR struct inode *inode, { do { - if (!list->fl_files[i][j].f_inode) + filep = files_fget_index(list, i, j); + if (filep->f_inode == NULL) { - list->fl_files[i][j].f_oflags = oflags; - list->fl_files[i][j].f_pos = pos; - list->fl_files[i][j].f_inode = inode; - list->fl_files[i][j].f_priv = priv; - nxmutex_unlock(&list->fl_lock); - - if (addref) - { - inode_addref(inode); - } - -#ifdef CONFIG_FDCHECK - return - fdcheck_protect(i * CONFIG_NFILE_DESCRIPTORS_PER_BLOCK + j); -#else - return i * CONFIG_NFILE_DESCRIPTORS_PER_BLOCK + j; -#endif + goto found; } } while (++j < CONFIG_NFILE_DESCRIPTORS_PER_BLOCK); @@ -373,15 +380,16 @@ int file_allocate_from_tcb(FAR struct tcb_s *tcb, FAR struct inode *inode, ret = files_extend(list, i + 1); if (ret < 0) { - nxmutex_unlock(&list->fl_lock); return ret; } - list->fl_files[i][0].f_oflags = oflags; - list->fl_files[i][0].f_pos = pos; - list->fl_files[i][0].f_inode = inode; - list->fl_files[i][0].f_priv = priv; - nxmutex_unlock(&list->fl_lock); + filep = files_fget_index(list, i, 0); +found: + + filep->f_oflags = oflags; + filep->f_pos = pos; + filep->f_inode = inode; + filep->f_priv = priv; if (addref) { @@ -389,9 +397,9 @@ int file_allocate_from_tcb(FAR struct tcb_s *tcb, FAR struct inode *inode, } #ifdef CONFIG_FDCHECK - return fdcheck_protect(i * CONFIG_NFILE_DESCRIPTORS_PER_BLOCK); + return fdcheck_protect(i * CONFIG_NFILE_DESCRIPTORS_PER_BLOCK + j); #else - return i * CONFIG_NFILE_DESCRIPTORS_PER_BLOCK; + return i * CONFIG_NFILE_DESCRIPTORS_PER_BLOCK + j; #endif } @@ -432,14 +440,6 @@ int files_duplist(FAR struct filelist *plist, FAR struct filelist *clist) DEBUGASSERT(clist); DEBUGASSERT(plist); - ret = nxmutex_lock(&plist->fl_lock); - if (ret < 0) - { - /* Probably canceled */ - - return ret; - } - for (i = 0; i < plist->fl_rows; i++) { for (j = 0; j < CONFIG_NFILE_DESCRIPTORS_PER_BLOCK; j++) @@ -456,13 +456,12 @@ int files_duplist(FAR struct filelist *plist, FAR struct filelist *clist) if (i * CONFIG_NFILE_DESCRIPTORS_PER_BLOCK + j >= 3) { - goto out; + return OK; } #endif - filep = &plist->fl_files[i][j]; - - if (filep && filep->f_inode == NULL) + filep = files_fget_index(plist, i, j); + if (filep->f_inode == NULL) { continue; } @@ -470,23 +469,21 @@ int files_duplist(FAR struct filelist *plist, FAR struct filelist *clist) ret = files_extend(clist, i + 1); if (ret < 0) { - goto out; + return ret; } /* Yes... duplicate it for the child, include O_CLOEXEC flag. */ - ret = file_dup3(filep, &clist->fl_files[i][j], + ret = file_dup3(filep, files_fget_index(clist, i, j), filep->f_oflags & O_CLOEXEC ? O_CLOEXEC : 0); if (ret < 0) { - goto out; + return ret; } } } -out: - nxmutex_unlock(&plist->fl_lock); - return ret; + return OK; } /**************************************************************************** @@ -508,23 +505,20 @@ void files_close_onexec(FAR struct tcb_s *tcb) list = nxsched_get_files_from_tcb(tcb); DEBUGASSERT(list != NULL); - nxmutex_lock(&list->fl_lock); for (i = 0; i < list->fl_rows; i++) { for (j = 0; j < CONFIG_NFILE_DESCRIPTORS_PER_BLOCK; j++) { FAR struct file *filep; - filep = &list->fl_files[i][j]; - if (filep && filep->f_inode != NULL && + filep = files_fget_index(list, i, j); + if (filep->f_inode != NULL && (filep->f_oflags & O_CLOEXEC) != 0) { file_close(filep); } } } - - nxmutex_unlock(&list->fl_lock); } /**************************************************************************** @@ -547,8 +541,6 @@ void files_close_onexec(FAR struct tcb_s *tcb) int fs_getfilep(int fd, FAR struct file **filep) { FAR struct filelist *list; - irqstate_t flags; - int ret; #ifdef CONFIG_FDCHECK fd = fdcheck_restore(fd); @@ -575,54 +567,21 @@ int fs_getfilep(int fd, FAR struct file **filep) return -EBADF; } - /* Protect this part with a critical section to make sure that we won't - * interrupt the mutex lock-unclock sequence below which may lead to the - * priority inversion. The case is as follows: - * - * We have two threads: low-priority thread A and high-priority thread B, - * both threads share the same task group data. - * - * Thread A performs IO on files periodically. Thread B is woken up by a - * high-frequency interrupts, and performs IO on files periodically. - * - * There is a chance that thread B wakes up exactly when thread A holds - * the mutex below, and consequently the file access in thread B will be - * delayed due to thread A holding the list->fl_lock mutex and execution - * will be returned to a thread with lower priority. - * - * The correct solution to this problem is to use the read-write lock, - * which is currently not supported by NuttX. - */ - - flags = enter_critical_section(); - /* The descriptor is in a valid range to file descriptor... Get the * thread-specific file list. */ - /* And return the file pointer from the list */ - - ret = nxmutex_lock(&list->fl_lock); - if (ret < 0) - { - leave_critical_section(flags); - return ret; - } - - *filep = &list->fl_files[fd / CONFIG_NFILE_DESCRIPTORS_PER_BLOCK] - [fd % CONFIG_NFILE_DESCRIPTORS_PER_BLOCK]; + *filep = files_fget(list, fd); /* if f_inode is NULL, fd was closed */ - if (!(*filep)->f_inode) + if ((*filep)->f_inode == NULL) { *filep = NULL; - ret = -EBADF; + return -EBADF; } - nxmutex_unlock(&list->fl_lock); - leave_critical_section(flags); - return ret; + return OK; } /**************************************************************************** @@ -746,7 +705,6 @@ int nx_close_from_tcb(FAR struct tcb_s *tcb, int fd) FAR struct file *filep; FAR struct file file; FAR struct filelist *list; - int ret; #ifdef CONFIG_FDCHECK fd = fdcheck_restore(fd); @@ -756,28 +714,18 @@ int nx_close_from_tcb(FAR struct tcb_s *tcb, int fd) /* Perform the protected close operation */ - ret = nxmutex_lock(&list->fl_lock); - if (ret < 0) - { - return ret; - } - /* If the file was properly opened, there should be an inode assigned */ if (fd < 0 || fd >= list->fl_rows * CONFIG_NFILE_DESCRIPTORS_PER_BLOCK || - !list->fl_files[fd / CONFIG_NFILE_DESCRIPTORS_PER_BLOCK] - [fd % CONFIG_NFILE_DESCRIPTORS_PER_BLOCK].f_inode) + files_fget(list, fd)->f_inode == NULL) { - nxmutex_unlock(&list->fl_lock); return -EBADF; } - filep = &list->fl_files[fd / CONFIG_NFILE_DESCRIPTORS_PER_BLOCK] - [fd % CONFIG_NFILE_DESCRIPTORS_PER_BLOCK]; + filep = files_fget(list, fd); memcpy(&file, filep, sizeof(struct file)); memset(filep, 0, sizeof(struct file)); - nxmutex_unlock(&list->fl_lock); return file_close(&file); } diff --git a/include/nuttx/fs/fs.h b/include/nuttx/fs/fs.h index be7c122be2837..8ec6ff0e9e5c1 100644 --- a/include/nuttx/fs/fs.h +++ b/include/nuttx/fs/fs.h @@ -478,7 +478,6 @@ struct file struct filelist { - mutex_t fl_lock; /* Manage access to the file list */ uint8_t fl_rows; /* The number of rows of fl_files array */ FAR struct file **fl_files; /* The pointer of two layer file descriptors array */ };