locks: skip posix unlock when there are no posix locks
Subsystems (2)
RFC PATCH 1/1 locks: skip posix unlock when there are no posix locks
When i_flock contains only flocks, the posix unlock is extraneous. On gfs2, ocfs2, and possibly others, the posix unlock can be costly, and something to avoid if possible.
As the comment implies, there will be races here. The question is, do those races become harmful with this new loop?
--- fs/locks.c | 14 +++++++++++++- 1 files changed, 13 insertions(+), 1 deletions(-)
fs/locks.c +13 -1
@@ -1982,15 +1982,27 @@ out:void locks_remove_posix (struct file *filp,fl_owner_t owner) {struct file_lock lock; +struct file_lock **before; +struct inode *inode;/* * If there are no locks held on this file, we don't need to call * posix_lock_file(). Another process could be setting a lock on this * file at the same time, but we wouldn't remove that lock anyway. */ -if (!filp->f_path.dentry->d_inode->i_flock) + inode = filp->f_path.dentry->d_inode; + +if (!inode->i_flock)return ; +for_each_lock (inode, before) { +struct file_lock *fl = *before; +if (IS_POSIX (fl)) + goto do_unlock; + } +return ; + +do_unlock: lock.fl_type =F_UNLCK ; lock.fl_flags =FL_POSIX |FL_CLOSE ; lock.fl_start =0 ;
Signature · 1 line
1.7.6
Hi,
Quoted 9 lines · David Teigland
When
i_flockcontains only flocks, the posix unlock is extraneous. On gfs2, ocfs2, and possibly others, the posix unlock can be costly, and something to avoid if possible.As the comment implies, there will be races here. The question is, do those races become harmful with this new loop?
The problem is that you don't hold the right lock (file_lock_lock) at this point, so its ok to test the i_flock pointer for being NULL, but its not ok to dereference it.
Since this check is made for every file that is closed, taking a global spinlock in this code path is frowned upon from a performance PoV.
Quoted 17 lines
Signed-off-by David Teigland <[email protected]>--- fs/locks.c | 14 +++++++++++++- 1 files changed, 13 insertions(+), 1 deletions(-)fs/locks.c +2
-0@@ -1982,15 +1982,27 @@ out:void locks_remove_posix (struct file *filp,fl_owner_t owner) {struct file_lock lock; +struct file_lock **before; +struct inode *inode;/*
fs/locks.c out: · line 1997
Quoted 10 lines
fs/locks.c +3
-1* If there are no locks held on this file, we don't need to call * posix_lock_file(). Another process could be setting a lock on this * file at the same time, but we wouldn't remove that lock anyway. */ -if (!filp->f_path.dentry->d_inode->i_flock) + inode = filp->f_path.dentry->d_inode; + +if (!inode->i_flock)return ;
So if you take the lock at this point, and drop it after the loop, that should resolve the issue without incurring the extra overhead on every single close,
Steve.
Quoted 11 lines
fs/locks.c +8
-0+for_each_lock (inode, before) { +struct file_lock *fl = *before; +if (IS_POSIX (fl)) + goto do_unlock; + } +return ; + +do_unlock: lock.fl_type =F_UNLCK ; lock.fl_flags =FL_POSIX |FL_CLOSE ; lock.fl_start =0 ;
Quoted 9 lines · Steven Whitehouse
fs/locks.c out: · line 1996
Quoted 6 lines
fs/locks.c +3
-1-if (!filp->f_path.dentry->d_inode->i_flock) + inode = filp->f_path.dentry->d_inode; + +if (!inode->i_flock)return ;So if you take the lock at this point, and drop it after the loop, that should resolve the issue without incurring the extra overhead on every single close,
Yep, thanks.
Quoted 12 lines
Quoted 11 lines
fs/locks.c +8
-0+for_each_lock (inode, before) { +struct file_lock *fl = *before; +if (IS_POSIX (fl)) + goto do_unlock; + } +return ; + +do_unlock: lock.fl_type =F_UNLCK ; lock.fl_flags =FL_POSIX |FL_CLOSE ; lock.fl_start =0 ;