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

[Steven Whitehouse <swhiteho@redhat.com>]

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;