You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by xi...@apache.org on 2023/11/13 01:01:19 UTC
(nuttx) 01/03: fs/inode: improve the performance of get file pointer
This is an automated email from the ASF dual-hosted git repository.
xiaoxiang pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/nuttx.git
commit 0a567bbae407d382ace7439977a4151b3c11d80d
Author: chao an <an...@xiaomi.com>
AuthorDate: Wed Nov 8 17:23:44 2023 +0800
fs/inode: improve the performance of get file pointer
Remove file locks and enter critical sections to protect file lists
Signed-off-by: chao an <an...@xiaomi.com>
---
fs/inode/fs_files.c | 265 +++++++++++++++++++++-----------------------------
include/nuttx/fs/fs.h | 3 +-
2 files changed, 111 insertions(+), 157 deletions(-)
diff --git a/fs/inode/fs_files.c b/fs/inode/fs_files.c
index bcb7638288..51b8437e23 100644
--- a/fs/inode/fs_files.c
+++ b/fs/inode/fs_files.c
@@ -51,14 +51,47 @@
* Private Functions
****************************************************************************/
+/****************************************************************************
+ * Name: files_fget_by_index
+ ****************************************************************************/
+
+static FAR struct file *files_fget_by_index(FAR struct filelist *list,
+ int l1, int l2)
+{
+ FAR struct file *filep;
+ irqstate_t flags;
+
+ flags = spin_lock_irqsave(&list->fl_lock);
+
+ filep = &list->fl_files[l1][l2];
+
+ spin_unlock_irqrestore(&list->fl_lock, flags);
+
+ return filep;
+}
+
+/****************************************************************************
+ * Name: files_fget
+ ****************************************************************************/
+
+static FAR struct file *files_fget(FAR struct filelist *list, int fd)
+{
+ return files_fget_by_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 = spin_lock_irqsave(&list->fl_lock);
- /* 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)
+ {
+ spin_unlock_irqrestore(&list->fl_lock, 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;
+
+ spin_unlock_irqrestore(&list->fl_lock, 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_by_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_by_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_by_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_by_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_by_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_by_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,23 @@ 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)
+ if (fd < 0 || fd >= list->fl_rows * CONFIG_NFILE_DESCRIPTORS_PER_BLOCK)
{
- return ret;
+ return -EBADF;
}
+ filep = files_fget(list, fd);
+
/* 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)
+ if (filep->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];
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 be7c122be2..f944c22d99 100644
--- a/include/nuttx/fs/fs.h
+++ b/include/nuttx/fs/fs.h
@@ -37,6 +37,7 @@
#include <nuttx/mutex.h>
#include <nuttx/semaphore.h>
+#include <nuttx/spinlock.h>
#include <nuttx/mm/map.h>
/****************************************************************************
@@ -478,7 +479,7 @@ struct file
struct filelist
{
- mutex_t fl_lock; /* Manage access to the file list */
+ spinlock_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 */
};