| Message ID | 20250324113249.3084413-1-chao@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [f2fs-dev] f2fs: add a fast path in finish_preallocate_blocks() | expand |
Chao Yu via Linux-f2fs-devel <linux-f2fs-devel@lists.sourceforge.net> 于2025年3月24日周一 19:36写道： > > This patch uses i_sem to protect access/update on f2fs_inode_info.flag > in finish_preallocate_blocks(), it avoids grabbing inode_lock() in > each open(). > > Signed-off-by: Chao Yu <chao@kernel.org> > --- > fs/f2fs/file.c | 40 +++++++++++++++++++++++----------------- > 1 file changed, 23 insertions(+), 17 deletions(-) > > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c > index abbcbb5865a3..bb6ba3269de0 100644 > --- a/fs/f2fs/file.c > +++ b/fs/f2fs/file.c > @@ -554,19 +554,24 @@ static int f2fs_file_mmap(struct file *file, struct vm_area_struct *vma) > > static int finish_preallocate_blocks(struct inode *inode) > { > - int ret; > + int ret = 0; > + bool opened; > > - inode_lock(inode); > - if (is_inode_flag_set(inode, FI_OPENED_FILE)) { > - inode_unlock(inode); > + f2fs_down_read(&F2FS_I(inode)->i_sem); > + opened = is_inode_flag_set(inode, FI_OPENED_FILE); > + f2fs_up_read(&F2FS_I(inode)->i_sem); > + if (opened) > return 0; > - } > > - if (!file_should_truncate(inode)) { > - set_inode_flag(inode, FI_OPENED_FILE); > - inode_unlock(inode); > - return 0; > - } > + inode_lock(inode); > + f2fs_down_read(&F2FS_I(inode)->i_sem); > + opened = is_inode_flag_set(inode, FI_OPENED_FILE); > + f2fs_up_read(&F2FS_I(inode)->i_sem); Hi Chao, F2FS_I(inode)->i_sem is not needed for this judgment FI_OPENED_FILE area? because inode_lock has been hold and this is a write lock, if process A get inode_lock, other proccesses will be blocked inode_lock until inode_unlock with FI_OPENED_FILE been set? how do you think? thanks！ > + if (opened) > + goto out_unlock; > + > + if (!file_should_truncate(inode)) > + goto out_update; > > f2fs_down_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]); > filemap_invalidate_lock(inode->i_mapping); > @@ -576,16 +581,17 @@ static int finish_preallocate_blocks(struct inode *inode) > > filemap_invalidate_unlock(inode->i_mapping); > f2fs_up_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]); > - > - if (!ret) > - set_inode_flag(inode, FI_OPENED_FILE); > - > - inode_unlock(inode); > if (ret) > - return ret; > + goto out_unlock; > > file_dont_truncate(inode); > - return 0; > +out_update: > + f2fs_down_write(&F2FS_I(inode)->i_sem); > + set_inode_flag(inode, FI_OPENED_FILE); > + f2fs_up_write(&F2FS_I(inode)->i_sem); > +out_unlock: > + inode_unlock(inode); > + return ret; > } > > static int f2fs_file_open(struct inode *inode, struct file *filp) > -- > 2.48.1 > > > > _______________________________________________ > Linux-f2fs-devel mailing list > Linux-f2fs-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
On 3/25/25 14:31, Zhiguo Niu wrote: > Chao Yu via Linux-f2fs-devel <linux-f2fs-devel@lists.sourceforge.net> > 于2025年3月24日周一 19:36写道： >> >> This patch uses i_sem to protect access/update on f2fs_inode_info.flag >> in finish_preallocate_blocks(), it avoids grabbing inode_lock() in >> each open(). >> >> Signed-off-by: Chao Yu <chao@kernel.org> >> --- >> fs/f2fs/file.c | 40 +++++++++++++++++++++++----------------- >> 1 file changed, 23 insertions(+), 17 deletions(-) >> >> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c >> index abbcbb5865a3..bb6ba3269de0 100644 >> --- a/fs/f2fs/file.c >> +++ b/fs/f2fs/file.c >> @@ -554,19 +554,24 @@ static int f2fs_file_mmap(struct file *file, struct vm_area_struct *vma) >> >> static int finish_preallocate_blocks(struct inode *inode) >> { >> - int ret; >> + int ret = 0; >> + bool opened; >> >> - inode_lock(inode); >> - if (is_inode_flag_set(inode, FI_OPENED_FILE)) { >> - inode_unlock(inode); >> + f2fs_down_read(&F2FS_I(inode)->i_sem); >> + opened = is_inode_flag_set(inode, FI_OPENED_FILE); >> + f2fs_up_read(&F2FS_I(inode)->i_sem); >> + if (opened) >> return 0; >> - } >> >> - if (!file_should_truncate(inode)) { >> - set_inode_flag(inode, FI_OPENED_FILE); >> - inode_unlock(inode); >> - return 0; >> - } >> + inode_lock(inode); >> + f2fs_down_read(&F2FS_I(inode)->i_sem); >> + opened = is_inode_flag_set(inode, FI_OPENED_FILE); >> + f2fs_up_read(&F2FS_I(inode)->i_sem); > Hi Chao, > F2FS_I(inode)->i_sem is not needed for this judgment FI_OPENED_FILE area? > because inode_lock has been hold and this is a write lock, if process > A get inode_lock, > other proccesses will be blocked inode_lock until inode_unlock with > FI_OPENED_FILE been set? > how do you think? Zhiguo, Agreed, let me update it. Thank you. Thanks, > thanks！ >> + if (opened) >> + goto out_unlock; >> + >> + if (!file_should_truncate(inode)) >> + goto out_update; >> >> f2fs_down_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]); >> filemap_invalidate_lock(inode->i_mapping); >> @@ -576,16 +581,17 @@ static int finish_preallocate_blocks(struct inode *inode) >> >> filemap_invalidate_unlock(inode->i_mapping); >> f2fs_up_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]); >> - >> - if (!ret) >> - set_inode_flag(inode, FI_OPENED_FILE); >> - >> - inode_unlock(inode); >> if (ret) >> - return ret; >> + goto out_unlock; >> >> file_dont_truncate(inode); >> - return 0; >> +out_update: >> + f2fs_down_write(&F2FS_I(inode)->i_sem); >> + set_inode_flag(inode, FI_OPENED_FILE); >> + f2fs_up_write(&F2FS_I(inode)->i_sem); >> +out_unlock: >> + inode_unlock(inode); >> + return ret; >> } >> >> static int f2fs_file_open(struct inode *inode, struct file *filp) >> -- >> 2.48.1 >> >> >> >> _______________________________________________ >> Linux-f2fs-devel mailing list >> Linux-f2fs-devel@lists.sourceforge.net >> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c index abbcbb5865a3..bb6ba3269de0 100644 --- a/fs/f2fs/file.c +++ b/fs/f2fs/file.c @@ -554,19 +554,24 @@ static int f2fs_file_mmap(struct file *file, struct vm_area_struct *vma) static int finish_preallocate_blocks(struct inode *inode) { - int ret; + int ret = 0; + bool opened; - inode_lock(inode); - if (is_inode_flag_set(inode, FI_OPENED_FILE)) { - inode_unlock(inode); + f2fs_down_read(&F2FS_I(inode)->i_sem); + opened = is_inode_flag_set(inode, FI_OPENED_FILE); + f2fs_up_read(&F2FS_I(inode)->i_sem); + if (opened) return 0; - } - if (!file_should_truncate(inode)) { - set_inode_flag(inode, FI_OPENED_FILE); - inode_unlock(inode); - return 0; - } + inode_lock(inode); + f2fs_down_read(&F2FS_I(inode)->i_sem); + opened = is_inode_flag_set(inode, FI_OPENED_FILE); + f2fs_up_read(&F2FS_I(inode)->i_sem); + if (opened) + goto out_unlock; + + if (!file_should_truncate(inode)) + goto out_update; f2fs_down_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]); filemap_invalidate_lock(inode->i_mapping); @@ -576,16 +581,17 @@ static int finish_preallocate_blocks(struct inode *inode) filemap_invalidate_unlock(inode->i_mapping); f2fs_up_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]); - - if (!ret) - set_inode_flag(inode, FI_OPENED_FILE); - - inode_unlock(inode); if (ret) - return ret; + goto out_unlock; file_dont_truncate(inode); - return 0; +out_update: + f2fs_down_write(&F2FS_I(inode)->i_sem); + set_inode_flag(inode, FI_OPENED_FILE); + f2fs_up_write(&F2FS_I(inode)->i_sem); +out_unlock: + inode_unlock(inode); + return ret; } static int f2fs_file_open(struct inode *inode, struct file *filp)
This patch uses i_sem to protect access/update on f2fs_inode_info.flag in finish_preallocate_blocks(), it avoids grabbing inode_lock() in each open(). Signed-off-by: Chao Yu <chao@kernel.org> --- fs/f2fs/file.c | 40 +++++++++++++++++++++++----------------- 1 file changed, 23 insertions(+), 17 deletions(-)