sergiotarxz/nuttx
sergiotarxz/nuttx
1
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

fs/inode: improve the performance of get file pointer

Browse Source 
Remove file locks and enter critical sections to protect file lists

Signed-off-by: chao an <anchao@xiaomi.com>
This commit is contained in:
chao an 2023-11-08 17:23:44 +08:00 committed by Xiang Xiao
parent 66ccaed5ce
commit 0a567bbae4
2 changed files with 116 additions and 162 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

275
fs/inode/fs_files.c
View File

@ -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 -EBADF;
}
filep = files_fget(list, fd);
/* If the file was properly opened, there should be an inode assigned */
if (filep->f_inode == NULL)
{
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)
{
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);
}

3
include/nuttx/fs/fs.h
View File

@ -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 */
};