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

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

[David Teigland]

When i_flock contains only flocks, the posix unlock is
unnecessary, but called anyway.  On gfs2, ocfs2, and
possibly others with f_op->lock, the posix unlock can
be costly, so only do it if posix locks exist.

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

diff --git a/fs/locks.c b/fs/locks.c
index 703f545..d47f497 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1982,15 +1982,30 @@ 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;
 
+	lock_flocks();
+	for_each_lock(inode, before) {
+		struct file_lock *fl = *before;
+		if (IS_POSIX(fl))
+			goto do_unlock;
+	}
+	unlock_flocks();
+	return;
+
+do_unlock:
+	unlock_flocks();
 	lock.fl_type = F_UNLCK;
 	lock.fl_flags = FL_POSIX | FL_CLOSE;
 	lock.fl_start = 0;
-- 
1.7.6

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

[Trond Myklebust]

On Fri, 2011-08-19 at 12:56 -0400, David Teigland wrote: 
> When i_flock contains only flocks, the posix unlock is
> unnecessary, but called anyway.  On gfs2, ocfs2, and
> possibly others with f_op->lock, the posix unlock can
> be costly, so only do it if posix locks exist.
> 
> Signed-off-by: David Teigland <teigland@redhat.com>
> ---
>  fs/locks.c |   17 ++++++++++++++++-
>  1 files changed, 16 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/locks.c b/fs/locks.c
> index 703f545..d47f497 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -1982,15 +1982,30 @@ 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;
>  
> +	lock_flocks();
> +	for_each_lock(inode, before) {
> +		struct file_lock *fl = *before;
> +		if (IS_POSIX(fl))
> +			goto do_unlock;
> +	}
> +	unlock_flocks();
> +	return;
> +
> +do_unlock:
> +	unlock_flocks();
>  	lock.fl_type = F_UNLCK;
>  	lock.fl_flags = FL_POSIX | FL_CLOSE;
>  	lock.fl_start = 0;

This assumes that all locks are mirrored in inode->i_flock, which may
not be the case if the filesystem implements its own f_op->lock(). The
right place for this optimisation would be in your filesystem callback.

Cheers
  Trond
-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

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

[David Teigland]

On Fri, Aug 19, 2011 at 01:09:36PM -0400, Trond Myklebust wrote:
> >  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;
> >  
> > +	lock_flocks();
> > +	for_each_lock(inode, before) {
> > +		struct file_lock *fl = *before;
> > +		if (IS_POSIX(fl))
> > +			goto do_unlock;
> > +	}
> > +	unlock_flocks();
> > +	return;
> > +
> > +do_unlock:
> > +	unlock_flocks();
> >  	lock.fl_type = F_UNLCK;
> >  	lock.fl_flags = FL_POSIX | FL_CLOSE;
> >  	lock.fl_start = 0;
> 
> This assumes that all locks are mirrored in inode->i_flock, which may
> not be the case if the filesystem implements its own f_op->lock(). The
> right place for this optimisation would be in your filesystem callback.

I suppose if i_flock is being faked at the same time to avoid the null
check.  I once looked at not doing lock mirroring in vfs and couldn't for
reasons I don't remember... may the null i_flock check was the only
reason, not sure.

Dave