[PATCH] Nfs lost locks

[PATCH] Nfs lost locks

[Philippe Troin]

[-- Attachment #1: Type: text/plain, Size: 470 bytes --]

The NFS client is prone to loose locks on filesystems when the locking
process is killed with a signal. This has been discussed on the nfs
mailing list in these threads:

  http://sourceforge.net/mailarchive/forum.php?thread_id=3213117&forum_id=4930

  http://marc.theaimsgroup.com/?l=linux-nfs&m=107074045907620&w=2

Marcelo, if the above links are not sufficient, please email back for
more details.

The enclosed patch is from Trond, and it fixes the problem.

Phil.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: linux-2.4.25-nfs-lock-race.patch --]
[-- Type: text/x-patch, Size: 4142 bytes --]

diff -ruN linux-2.4.25.orig/fs/nfs/file.c linux-2.4.25/fs/nfs/file.c
--- linux-2.4.25.orig/fs/nfs/file.c	Mon Aug 25 04:44:43 2003
+++ linux-2.4.25/fs/nfs/file.c	Wed Feb 18 13:32:05 2004
@@ -241,6 +241,93 @@
 	goto out;
 }
 
+static int
+do_getlk(struct inode *inode, int cmd, struct file_lock *fl)
+{
+	int status;
+
+	lock_kernel();
+	status = nlmclnt_proc(inode, cmd, fl);
+	unlock_kernel();
+	return status;
+}
+
+static int
+do_unlk(struct inode *inode, int cmd, struct file_lock *fl)
+{
+	sigset_t oldset;
+	int status;
+
+	rpc_clnt_sigmask(NFS_CLIENT(inode), &oldset);
+	/*
+	 * Flush all pending writes before doing anything
+	 * with locks..
+	 */
+	filemap_fdatasync(inode->i_mapping);
+	down(&inode->i_sem);
+	nfs_wb_all(inode);
+	up(&inode->i_sem);
+	filemap_fdatawait(inode->i_mapping);
+
+	/* NOTE: special case
+	 *	If we're signalled while cleaning up locks on process exit, we
+	 *	still need to complete the unlock.
+	 */
+	lock_kernel();
+	status = nlmclnt_proc(inode, cmd, fl);
+	unlock_kernel();
+	rpc_clnt_sigunmask(NFS_CLIENT(inode), &oldset);
+	return status;
+}
+
+static int
+do_setlk(struct file *filp, int cmd, struct file_lock *fl)
+{
+	struct inode * inode = filp->f_dentry->d_inode;
+	int status;
+
+	/*
+	 * Flush all pending writes before doing anything
+	 * with locks..
+	 */
+	status = filemap_fdatasync(inode->i_mapping);
+	if (status == 0) {
+		down(&inode->i_sem);
+		status = nfs_wb_all(inode);
+		up(&inode->i_sem);
+		if (status == 0)
+			status = filemap_fdatawait(inode->i_mapping);
+	}
+	if (status < 0)
+		return status;
+
+	lock_kernel();
+	status = nlmclnt_proc(inode, cmd, fl);
+	/* If we were signalled we still need to ensure that
+	 * we clean up any state on the server. We therefore
+	 * record the lock call as having succeeded in order to
+	 * ensure that locks_remove_posix() cleans it out when
+	 * the process exits.
+	 */
+	if (status == -EINTR || status == -ERESTARTSYS)
+		posix_lock_file(filp, fl, 0);
+	unlock_kernel();
+	if (status < 0)
+		return status;
+
+	/*
+	 * Make sure we clear the cache whenever we try to get the lock.
+	 * This makes locking act as a cache coherency point.
+	 */
+	filemap_fdatasync(inode->i_mapping);
+	down(&inode->i_sem);
+	nfs_wb_all(inode);      /* we may have slept */
+	up(&inode->i_sem);
+	filemap_fdatawait(inode->i_mapping);
+	nfs_zap_caches(inode);
+	return 0;
+}
+
 /*
  * Lock a (portion of) a file
  */
@@ -248,8 +335,6 @@
 nfs_lock(struct file *filp, int cmd, struct file_lock *fl)
 {
 	struct inode * inode = filp->f_dentry->d_inode;
-	int	status = 0;
-	int	status2;
 
 	dprintk("NFS: nfs_lock(f=%4x/%ld, t=%x, fl=%x, r=%Ld:%Ld)\n",
 			inode->i_dev, inode->i_ino,
@@ -266,8 +351,8 @@
 	/* Fake OK code if mounted without NLM support */
 	if (NFS_SERVER(inode)->flags & NFS_MOUNT_NONLM) {
 		if (IS_GETLK(cmd))
-			status = LOCK_USE_CLNT;
-		goto out_ok;
+			return LOCK_USE_CLNT;
+		return 0;
 	}
 
 	/*
@@ -280,42 +365,9 @@
 	if (!fl->fl_owner || (fl->fl_flags & (FL_POSIX|FL_BROKEN)) != FL_POSIX)
 		return -ENOLCK;
 
-	/*
-	 * Flush all pending writes before doing anything
-	 * with locks..
-	 */
-	status = filemap_fdatasync(inode->i_mapping);
-	down(&inode->i_sem);
-	status2 = nfs_wb_all(inode);
-	if (status2 && !status)
-		status = status2;
-	up(&inode->i_sem);
-	status2 = filemap_fdatawait(inode->i_mapping);
-	if (status2 && !status)
-		status = status2;
-	if (status < 0)
-		return status;
-
-	lock_kernel();
-	status = nlmclnt_proc(inode, cmd, fl);
-	unlock_kernel();
-	if (status < 0)
-		return status;
-	
-	status = 0;
-
-	/*
-	 * Make sure we clear the cache whenever we try to get the lock.
-	 * This makes locking act as a cache coherency point.
-	 */
- out_ok:
-	if ((IS_SETLK(cmd) || IS_SETLKW(cmd)) && fl->fl_type != F_UNLCK) {
-		filemap_fdatasync(inode->i_mapping);
-		down(&inode->i_sem);
-		nfs_wb_all(inode);      /* we may have slept */
-		up(&inode->i_sem);
-		filemap_fdatawait(inode->i_mapping);
-		nfs_zap_caches(inode);
-	}
-	return status;
+	if (IS_GETLK(cmd))
+		return do_getlk(inode, cmd, fl);
+	if (fl->fl_type == F_UNLCK)
+		return do_unlk(inode, cmd, fl);
+	return do_setlk(filp, cmd, fl);
 }

Re: [PATCH] Nfs lost locks

[Marcelo Tosatti]

On Fri, 20 Feb 2004, Philippe Troin wrote:

> The NFS client is prone to loose locks on filesystems when the locking
> process is killed with a signal. This has been discussed on the nfs
> mailing list in these threads:
>
>   http://sourceforge.net/mailarchive/forum.php?thread_id=3213117&forum_id=4930
>
>   http://marc.theaimsgroup.com/?l=linux-nfs&m=107074045907620&w=2
>
> Marcelo, if the above links are not sufficient, please email back for
> more details.
>
> The enclosed patch is from Trond, and it fixes the problem.

Hi Philippe,

It might be wise to wait for the patch to be in 2.6 first?

Trond, what do you think?

Re: [PATCH] Nfs lost locks

[Philippe Troin]

Marcelo Tosatti <marcelo.tosatti@cyclades.com> writes:

> On Fri, 20 Feb 2004, Philippe Troin wrote:
> 
> > The NFS client is prone to loose locks on filesystems when the locking
> > process is killed with a signal. This has been discussed on the nfs
> > mailing list in these threads:
> >
> >   http://sourceforge.net/mailarchive/forum.php?thread_id=3213117&forum_id=4930
> >
> >   http://marc.theaimsgroup.com/?l=linux-nfs&m=107074045907620&w=2
> >
> > Marcelo, if the above links are not sufficient, please email back for
> > more details.
> >
> > The enclosed patch is from Trond, and it fixes the problem.
> 
> Hi Philippe,
> 
> It might be wise to wait for the patch to be in 2.6 first?
> 
> Trond, what do you think?

I do not know about the 2.6.x status, but Trond requested help with
pushing this patch to the kernel, mentionning he was very busy with
NFSv4.

I personnaly think it fixes a serious problem with file locking on
NFS, but that's my assessment.

Phil.

Re: [PATCH] Nfs lost locks

[Marcelo Tosatti]

On Tue, 24 Feb 2004, Philippe Troin wrote:

> Marcelo Tosatti <marcelo.tosatti@cyclades.com> writes:
>
> > On Fri, 20 Feb 2004, Philippe Troin wrote:
> >
> > > The NFS client is prone to loose locks on filesystems when the locking
> > > process is killed with a signal. This has been discussed on the nfs
> > > mailing list in these threads:
> > >
> > >   http://sourceforge.net/mailarchive/forum.php?thread_id=3213117&forum_id=4930
> > >
> > >   http://marc.theaimsgroup.com/?l=linux-nfs&m=107074045907620&w=2
> > >
> > > Marcelo, if the above links are not sufficient, please email back for
> > > more details.
> > >
> > > The enclosed patch is from Trond, and it fixes the problem.
> >
> > Hi Philippe,
> >
> > It might be wise to wait for the patch to be in 2.6 first?
> >
> > Trond, what do you think?
>
> I do not know about the 2.6.x status, but Trond requested help with
> pushing this patch to the kernel, mentionning he was very busy with
> NFSv4.
>
> I personnaly think it fixes a serious problem with file locking on
> NFS, but that's my assessment.

I also think it fixes a serious problem with file locking on NFS. What I
dont think is that it has been extensively tested (I seen you stressed
file locking with it for a couple of days, but thats not enough).

I feel that it needs to get tested on different setups.

If Trond is confident it works reliably, I will apply it right away.

Re: [PATCH] Nfs lost locks

[Philippe Troin]

Marcelo Tosatti <marcelo.tosatti@cyclades.com> writes:

> On Tue, 24 Feb 2004, Philippe Troin wrote:
> 
> > Marcelo Tosatti <marcelo.tosatti@cyclades.com> writes:
> >
> > > On Fri, 20 Feb 2004, Philippe Troin wrote:
> > >
> > > > The NFS client is prone to loose locks on filesystems when the locking
> > > > process is killed with a signal. This has been discussed on the nfs
> > > > mailing list in these threads:
> > > >
> > > >   http://sourceforge.net/mailarchive/forum.php?thread_id=3213117&forum_id=4930
> > > >
> > > >   http://marc.theaimsgroup.com/?l=linux-nfs&m=107074045907620&w=2
> > > >
> > > > Marcelo, if the above links are not sufficient, please email back for
> > > > more details.
> > > >
> > > > The enclosed patch is from Trond, and it fixes the problem.
> > >
> > > Hi Philippe,
> > >
> > > It might be wise to wait for the patch to be in 2.6 first?
> > >
> > > Trond, what do you think?
> >
> > I do not know about the 2.6.x status, but Trond requested help with
> > pushing this patch to the kernel, mentionning he was very busy with
> > NFSv4.
> >
> > I personnaly think it fixes a serious problem with file locking on
> > NFS, but that's my assessment.
> 
> I also think it fixes a serious problem with file locking on NFS. What I
> dont think is that it has been extensively tested (I seen you stressed
> file locking with it for a couple of days, but thats not enough).
> 
> I feel that it needs to get tested on different setups.
> 
> If Trond is confident it works reliably, I will apply it right away.

For reference and posterity, 2.4.26 includes this patch.

Thanks to Marcelo and Trond.

Phil.