[RFC PATCH] locks: skip posix unlock when there are no posix locks

[RFC PATCH] locks: skip posix unlock when there are no posix locks

[David Teigland]

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?

Signed-off-by: David Teigland <teigland@redhat.com>
---
 fs/locks.c |   14 +++++++++++++-
 1 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index 703f545..4507401 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -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;
-- 
1.7.6

Re: [RFC PATCH] locks: skip posix unlock when there are no posix locks

[Steven Whitehouse]

Hi,

On Thu, 2011-08-18 at 16:38 -0400, David Teigland wrote:
> 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?
> 
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.

> Signed-off-by: David Teigland <teigland@redhat.com>
> ---
>  fs/locks.c |   14 +++++++++++++-
>  1 files changed, 13 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/locks.c b/fs/locks.c
> index 703f545..4507401 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -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;
>  
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.

> +	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;

Re: [RFC PATCH] locks: skip posix unlock when there are no posix locks

[David Teigland]

On Fri, Aug 19, 2011 at 10:40:39AM +0100, Steven Whitehouse wrote:
> > -	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.

> > +	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;
>