linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH] locks: skip posix unlock when there are no posix locks
@ 2011-08-18 20:38 David Teigland
  2011-08-19  9:40 ` Steven Whitehouse
  0 siblings, 1 reply; 3+ messages in thread
From: David Teigland @ 2011-08-18 20:38 UTC (permalink / raw)
  To: linux-fsdevel

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


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [RFC PATCH] locks: skip posix unlock when there are no posix locks
  2011-08-18 20:38 [RFC PATCH] locks: skip posix unlock when there are no posix locks David Teigland
@ 2011-08-19  9:40 ` Steven Whitehouse
  2011-08-19 15:07   ` David Teigland
  0 siblings, 1 reply; 3+ messages in thread
From: Steven Whitehouse @ 2011-08-19  9:40 UTC (permalink / raw)
  To: David Teigland; +Cc: linux-fsdevel

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;



^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [RFC PATCH] locks: skip posix unlock when there are no posix locks
  2011-08-19  9:40 ` Steven Whitehouse
@ 2011-08-19 15:07   ` David Teigland
  0 siblings, 0 replies; 3+ messages in thread
From: David Teigland @ 2011-08-19 15:07 UTC (permalink / raw)
  To: Steven Whitehouse; +Cc: linux-fsdevel

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2011-08-19 15:07 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-08-18 20:38 [RFC PATCH] locks: skip posix unlock when there are no posix locks David Teigland
2011-08-19  9:40 ` Steven Whitehouse
2011-08-19 15:07   ` David Teigland

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).