[PATCH] NFS nolock broken in 2.6.9-rc4

[PATCH] NFS nolock broken in 2.6.9-rc4

[Olaf Kirch]

Hi,

2.6.9-rc2 and later contain the following hunks in fs/locks.c:

@@ -1489,8 +1534,7 @@ int fcntl_setlk(struct file *filp, unsig

        if (filp->f_op && filp->f_op->lock != NULL) {
                error = filp->f_op->lock(filp, cmd, file_lock);
-               if (error < 0)
-                       goto out;
+               goto out;
        }

        for (;;) {
@@ -1624,8 +1668,7 @@ int fcntl_setlk64(struct file *filp, uns

        if (filp->f_op && filp->f_op->lock != NULL) {
                error = filp->f_op->lock(filp, cmd, file_lock);
-               if (error < 0)
-                       goto out;
+               goto out;
        }

        for (;;) {

For an NFS file system mounted with -o nolock, this means filp->f_op->lock
will return 0 but do nothing. Previously, this would cause local locking
to be used. With the change above, we just return. 

I don't know why this change was made. The patch below simply reverts
these two hunks; maybe there's a better solution involving LOCK_USE_CLNT
the way it's used for F_GETLK at the moment.

Olaf
-- 
Olaf Kirch     | Things that make Monday morning interesting, #1:
okir@suse.de   |        "I want to use NFS over AX25, can you help me?"
---------------+ 


-------------------------------------------------------
This SF.net email is sponsored by: IT Product Guide on ITManagersJournal
Use IT products in your business? Tell us what you think of them. Give us
Your Opinions, Get Free ThinkGeek Gift Certificates! Click to find out more
http://productguide.itmanagersjournal.com/guidepromo.tmpl
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH] NFS nolock broken in 2.6.9-rc4

[Olaf Kirch]

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

On Fri, Oct 15, 2004 at 11:58:40AM +0200, Olaf Kirch wrote:
> I don't know why this change was made. The patch below simply reverts
> these two hunks; maybe there's a better solution involving LOCK_USE_CLNT
> the way it's used for F_GETLK at the moment.

I forgot to add the patch. Below there are two possible fixes.

 fix1: just revert the changes

 fix2: use LOCK_USE_CLNT for all locking operations if mounted nolock.

The second one is probably better...

Olaf
-- 
Olaf Kirch     | Things that make Monday morning interesting, #1:
okir@suse.de   |        "I want to use NFS over AX25, can you help me?"
---------------+ 

[-- Attachment #2: nfs-nolock-fix1 --]
[-- Type: text/plain, Size: 929 bytes --]

Index: linux-2.6.9-rc4/fs/locks.c
===================================================================
--- linux-2.6.9-rc4.orig/fs/locks.c
+++ linux-2.6.9-rc4/fs/locks.c
@@ -1534,7 +1534,8 @@ int fcntl_setlk(struct file *filp, unsig
 
 	if (filp->f_op && filp->f_op->lock != NULL) {
 		error = filp->f_op->lock(filp, cmd, file_lock);
-		goto out;
+		if (error < 0)
+			goto out;
 	}
 
 	for (;;) {
@@ -1668,7 +1669,8 @@ int fcntl_setlk64(struct file *filp, uns
 
 	if (filp->f_op && filp->f_op->lock != NULL) {
 		error = filp->f_op->lock(filp, cmd, file_lock);
-		goto out;
+		if (error < 0)
+			goto out;
 	}
 
 	for (;;) {
@@ -1720,7 +1722,7 @@ void locks_remove_posix(struct file *fil
 
 	if (filp->f_op && filp->f_op->lock != NULL) {
 		filp->f_op->lock(filp, F_SETLK, &lock);
-		goto out;
+		/* Ignore any error -- we must remove the locks anyway */
 	}
 
 	/* Can't use posix_lock_file here; we need to remove it no matter

[-- Attachment #3: nfs-nolock-fix2 --]
[-- Type: text/plain, Size: 1836 bytes --]


A previous change broke locking on NFS file systems mounted -o nolock.
This patch fixes the problem by making all NFS lock calls return
LOCK_USE_CLNT in this case, and treating it the way we did previously.

Signed-off-by: Olaf Kirch <okir@suse.de>

Index: linux-2.6.9-rc4/fs/locks.c
===================================================================
--- linux-2.6.9-rc4.orig/fs/locks.c
+++ linux-2.6.9-rc4/fs/locks.c
@@ -1534,7 +1534,8 @@ int fcntl_setlk(struct file *filp, unsig
 
 	if (filp->f_op && filp->f_op->lock != NULL) {
 		error = filp->f_op->lock(filp, cmd, file_lock);
-		goto out;
+		if (error != LOCK_USE_CLNT)
+			goto out;
 	}
 
 	for (;;) {
@@ -1668,7 +1669,8 @@ int fcntl_setlk64(struct file *filp, uns
 
 	if (filp->f_op && filp->f_op->lock != NULL) {
 		error = filp->f_op->lock(filp, cmd, file_lock);
-		goto out;
+		if (error != LOCK_USE_CLNT)
+			goto out;
 	}
 
 	for (;;) {
@@ -1719,8 +1721,9 @@ void locks_remove_posix(struct file *fil
 	lock.fl_lmops = NULL;
 
 	if (filp->f_op && filp->f_op->lock != NULL) {
-		filp->f_op->lock(filp, F_SETLK, &lock);
-		goto out;
+		error = filp->f_op->lock(filp, F_SETLK, &lock);
+		if (error != LOCK_USE_CLNT)
+			goto out;
 	}
 
 	/* Can't use posix_lock_file here; we need to remove it no matter
Index: linux-2.6.9-rc4/fs/nfs/file.c
===================================================================
--- linux-2.6.9-rc4.orig/fs/nfs/file.c
+++ linux-2.6.9-rc4/fs/nfs/file.c
@@ -397,11 +397,9 @@ nfs_lock(struct file *filp, int cmd, str
 		return -ENOLCK;
 
 	if (NFS_PROTO(inode)->version != 4) {
-		/* Fake OK code if mounted without NLM support */
+		/* If mounted NONLM, tell VFS to use local locking only. */
 		if (NFS_SERVER(inode)->flags & NFS_MOUNT_NONLM) {
-			if (IS_GETLK(cmd))
-				return LOCK_USE_CLNT;
-			return 0;
+			return LOCK_USE_CLNT;
 		}
 	}
 

Re: Re: [PATCH] NFS nolock broken in 2.6.9-rc4

[Trond Myklebust]

P=E5 fr , 15/10/2004 klokka 12:07, skreiv Olaf Kirch:
> On Fri, Oct 15, 2004 at 11:58:40AM +0200, Olaf Kirch wrote:
> > I don't know why this change was made. The patch below simply reverts
> > these two hunks; maybe there's a better solution involving LOCK_USE_CLN=
T
> > the way it's used for F_GETLK at the moment.
>=20
> I forgot to add the patch. Below there are two possible fixes.
>=20
>  fix1: just revert the changes
>=20
>  fix2: use LOCK_USE_CLNT for all locking operations if mounted nolock.
>
> The second one is probably better...

LOCK_USE_CLNT is better than "fix1" in that it actually does the right
thing. That said, it is still a hack, and since there is no reason to
keep it around any longer, it really should go.

Cheers,
  Trond



-------------------------------------------------------
This SF.net email is sponsored by: IT Product Guide on ITManagersJournal
Use IT products in your business? Tell us what you think of them. Give us
Your Opinions, Get Free ThinkGeek Gift Certificates! Click to find out more
http://productguide.itmanagersjournal.com/guidepromo.tmpl
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: Re: [PATCH] NFS nolock broken in 2.6.9-rc4

[Trond Myklebust]

  NFS: when we mount with the "nolock" flag we need to use local locking.

  Signed-off-by: Trond Myklebust <trond.myklebust@fys.uio.no>
---

 file.c |   43 +++++++++++++++++++++++--------------------
 1 files changed, 23 insertions(+), 20 deletions(-)

Index: linux-2.6.9-rc4-up/fs/nfs/file.c
===================================================================
--- linux-2.6.9-rc4-up.orig/fs/nfs/file.c	2004-10-15 12:03:43.000000000 +0200
+++ linux-2.6.9-rc4-up/fs/nfs/file.c	2004-10-15 16:56:59.804503229 +0200
@@ -298,7 +298,11 @@ static int do_getlk(struct file *filp, i
 	int status;
 
 	lock_kernel();
-	status = NFS_PROTO(inode)->lock(filp, cmd, fl);
+	/* Use local locking if mounted with "-onolock" */
+	if (!(NFS_SERVER(inode)->flags & NFS_MOUNT_NONLM))
+		status = NFS_PROTO(inode)->lock(filp, cmd, fl);
+	else
+		status = posix_test_lock(filp, fl);
 	unlock_kernel();
 	return status;
 }
@@ -325,7 +329,11 @@ static int do_unlk(struct file *filp, in
 	 * 	still need to complete the unlock.
 	 */
 	lock_kernel();
-	status = NFS_PROTO(inode)->lock(filp, cmd, fl);
+	/* Use local locking if mounted with "-onolock" */
+	if (!(NFS_SERVER(inode)->flags & NFS_MOUNT_NONLM))
+		status = NFS_PROTO(inode)->lock(filp, cmd, fl);
+	else
+		status = posix_lock_file_wait(filp, fl);
 	rpc_clnt_sigunmask(NFS_CLIENT(inode), &oldset);
 	return status;
 }
@@ -351,15 +359,19 @@ static int do_setlk(struct file *filp, i
 		return status;
 
 	lock_kernel();
-	status = NFS_PROTO(inode)->lock(filp, 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);
+	/* Use local locking if mounted with "-onolock" */
+	if (!(NFS_SERVER(inode)->flags & NFS_MOUNT_NONLM)) {
+		status = NFS_PROTO(inode)->lock(filp, 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);
+	} else
+		status = posix_lock_file_wait(filp, fl);
 	unlock_kernel();
 	if (status < 0)
 		return status;
@@ -396,15 +408,6 @@ nfs_lock(struct file *filp, int cmd, str
 	if ((inode->i_mode & (S_ISGID | S_IXGRP)) == S_ISGID)
 		return -ENOLCK;
 
-	if (NFS_PROTO(inode)->version != 4) {
-		/* Fake OK code if mounted without NLM support */
-		if (NFS_SERVER(inode)->flags & NFS_MOUNT_NONLM) {
-			if (IS_GETLK(cmd))
-				return LOCK_USE_CLNT;
-			return 0;
-		}
-	}
-
 	/*
 	 * No BSD flocks over NFS allowed.
 	 * Note: we could try to fake a POSIX lock request here by




-------------------------------------------------------
This SF.net email is sponsored by: IT Product Guide on ITManagersJournal
Use IT products in your business? Tell us what you think of them. Give us
Your Opinions, Get Free ThinkGeek Gift Certificates! Click to find out more
http://productguide.itmanagersjournal.com/guidepromo.tmpl
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: Re: [PATCH] NFS nolock broken in 2.6.9-rc4

[Trond Myklebust]

  VFS: Remove remnants of LOCK_USE_CLNT. It should no longer be necessary.

  Signed-off-by: Trond Myklebust <trond.myklebust@fys.uio.no>
---
 fs/locks.c         |    6 ------
 include/linux/fs.h |    5 -----
 2 files changed, 11 deletions(-)

Index: linux-2.6.9-rc4-up/include/linux/fs.h
===================================================================
--- linux-2.6.9-rc4-up.orig/include/linux/fs.h	2004-10-15 12:03:03.000000000 +0200
+++ linux-2.6.9-rc4-up/include/linux/fs.h	2004-10-15 17:04:10.645827518 +0200
@@ -1167,11 +1167,6 @@ extern long do_mount(char *, char *, cha
 
 extern int vfs_statfs(struct super_block *, struct kstatfs *);
 
-/* Return value for VFS lock functions - tells locks.c to lock conventionally
- * REALLY kosha for root NFS and nfs_lock
- */ 
-#define LOCK_USE_CLNT 1
-
 #define FLOCK_VERIFY_READ  1
 #define FLOCK_VERIFY_WRITE 2
 
Index: linux-2.6.9-rc4-up/fs/locks.c
===================================================================
--- linux-2.6.9-rc4-up.orig/fs/locks.c	2004-10-15 12:04:18.000000000 +0200
+++ linux-2.6.9-rc4-up/fs/locks.c	2004-10-15 17:03:16.376966402 +0200
@@ -1435,9 +1435,6 @@ int fcntl_getlk(struct file *filp, struc
 		error = filp->f_op->lock(filp, F_GETLK, &file_lock);
 		if (error < 0)
 			goto out;
-		else if (error == LOCK_USE_CLNT)
-		  /* Bypass for NFS with no locking - 2.0.36 compat */
-		  fl = posix_test_lock(filp, &file_lock);
 		else
 		  fl = (file_lock.fl_type == F_UNLCK ? NULL : &file_lock);
 	} else {
@@ -1580,9 +1577,6 @@ int fcntl_getlk64(struct file *filp, str
 		error = filp->f_op->lock(filp, F_GETLK, &file_lock);
 		if (error < 0)
 			goto out;
-		else if (error == LOCK_USE_CLNT)
-		  /* Bypass for NFS with no locking - 2.0.36 compat */
-		  fl = posix_test_lock(filp, &file_lock);
 		else
 		  fl = (file_lock.fl_type == F_UNLCK ? NULL : &file_lock);
 	} else {




-------------------------------------------------------
This SF.net email is sponsored by: IT Product Guide on ITManagersJournal
Use IT products in your business? Tell us what you think of them. Give us
Your Opinions, Get Free ThinkGeek Gift Certificates! Click to find out more
http://productguide.itmanagersjournal.com/guidepromo.tmpl
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH] NFS nolock broken in 2.6.9-rc4

[Olaf Kirch]

On Sat, Oct 16, 2004 at 12:49:57AM -0700, Andrew Morton wrote:
> Did we come to a decision as to which patch to use?
> 
> When we do, someone please send me the patch.

Trond's the one with the maintainer hat.

I like a separate file_operations_nolock better since it's KISS, but
if he prefers the long and winding road of locks.c calling NFS calling
locks.c, I'm fine with that, too.

Olaf
-- 
Olaf Kirch     | Things that make Monday morning interesting, #1:
okir@suse.de   |        "I want to use NFS over AX25, can you help me?"
---------------+ 


-------------------------------------------------------
This SF.net email is sponsored by: IT Product Guide on ITManagersJournal
Use IT products in your business? Tell us what you think of them. Give us
Your Opinions, Get Free ThinkGeek Gift Certificates! Click to find out more
http://productguide.itmanagersjournal.com/guidepromo.tmpl
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH] NFS nolock broken in 2.6.9-rc4

[Trond Myklebust]

P=E5 fr , 15/10/2004 klokka 11:58, skreiv Olaf Kirch:

> I don't know why this change was made. The patch below simply reverts
> these two hunks; maybe there's a better solution involving LOCK_USE_CLNT
> the way it's used for F_GETLK at the moment.

NO! The correct way to do this is to handle it with a call to
posix_lock_file() in the NFS code for the particular case of "nolock".

See

http://linux.bkbits.net:8080/linux-2.6/cset@412a5e26rbgf-d7ofSTJKn9DIGumag

for details on why the changes were made (and that should make it
obvious why your patch is wrong). The VFS must NOT be allowed to
interfere if a filesystem has its own ->lock() method: that will violate
the ability of filesystems to impose rigid locking rules.

Cheers,
  Trond



-------------------------------------------------------
This SF.net email is sponsored by: IT Product Guide on ITManagersJournal
Use IT products in your business? Tell us what you think of them. Give us
Your Opinions, Get Free ThinkGeek Gift Certificates! Click to find out more
http://productguide.itmanagersjournal.com/guidepromo.tmpl
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH] NFS nolock broken in 2.6.9-rc4

[Olaf Kirch]

On Fri, Oct 15, 2004 at 04:44:31PM +0200, Trond Myklebust wrote:
> NO! The correct way to do this is to handle it with a call to
> posix_lock_file() in the NFS code for the particular case of "nolock".

Urrrgh.. this looks complicated and duplicates code. Then I'd rather
have two struct file_operations, one for normal operations with a .lock
function, and a second one for nolock without.

> for details on why the changes were made (and that should make it
> obvious why your patch is wrong). The VFS must NOT be allowed to
> interfere if a filesystem has its own ->lock() method: that will violate
> the ability of filesystems to impose rigid locking rules.

My patch doesn't interfere; it just falls back to the original code if
the fs routine returns LOCK_USE_CLNT. In that respect, it doesn't
interfere with the new code.

Olaf
-- 
Olaf Kirch     | Things that make Monday morning interesting, #1:
okir@suse.de   |        "I want to use NFS over AX25, can you help me?"
---------------+ 


-------------------------------------------------------
This SF.net email is sponsored by: IT Product Guide on ITManagersJournal
Use IT products in your business? Tell us what you think of them. Give us
Your Opinions, Get Free ThinkGeek Gift Certificates! Click to find out more
http://productguide.itmanagersjournal.com/guidepromo.tmpl
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH] NFS nolock broken in 2.6.9-rc4

[Olaf Kirch]

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

> Urrrgh.. this looks complicated and duplicates code. Then I'd rather
> have two struct file_operations, one for normal operations with a .lock
> function, and a second one for nolock without.

Please find attached proposed fix3 that does just this.

Olaf
-- 
Olaf Kirch     | Things that make Monday morning interesting, #1:
okir@suse.de   |        "I want to use NFS over AX25, can you help me?"
---------------+ 

[-- Attachment #2: nfs-nolock-fix3 --]
[-- Type: text/plain, Size: 2034 bytes --]

Index: linux-2.6.9-rc4/fs/nfs/inode.c
===================================================================
--- linux-2.6.9-rc4.orig/fs/nfs/inode.c
+++ linux-2.6.9-rc4/fs/nfs/inode.c
@@ -666,7 +666,10 @@ nfs_fhget(struct super_block *sb, struct
 		 */
 		inode->i_op = &nfs_file_inode_operations;
 		if (S_ISREG(inode->i_mode)) {
-			inode->i_fop = &nfs_file_operations;
+			if (NFS_SERVER(inode)->flags & NFS_MOUNT_NONLM)
+				inode->i_fop = &nfs_file_operations_nolock;
+			else
+				inode->i_fop = &nfs_file_operations;
 			inode->i_data.a_ops = &nfs_file_aops;
 			inode->i_data.backing_dev_info = &NFS_SB(sb)->backing_dev_info;
 		} else if (S_ISDIR(inode->i_mode)) {
Index: linux-2.6.9-rc4/fs/nfs/file.c
===================================================================
--- linux-2.6.9-rc4.orig/fs/nfs/file.c
+++ linux-2.6.9-rc4/fs/nfs/file.c
@@ -60,6 +60,20 @@ struct file_operations nfs_file_operatio
 	.sendfile	= nfs_file_sendfile,
 	.check_flags	= nfs_check_flags,
 };
+struct file_operations nfs_file_operations_nolock = {
+	.llseek		= remote_llseek,
+	.read		= do_sync_read,
+	.write		= do_sync_write,
+	.aio_read		= nfs_file_read,
+	.aio_write		= nfs_file_write,
+	.mmap		= nfs_file_mmap,
+	.open		= nfs_file_open,
+	.flush		= nfs_file_flush,
+	.release	= nfs_file_release,
+	.fsync		= nfs_fsync,
+	.sendfile	= nfs_file_sendfile,
+	.check_flags	= nfs_check_flags,
+};
 
 struct inode_operations nfs_file_inode_operations = {
 	.permission	= nfs_permission,
Index: linux-2.6.9-rc4/include/linux/nfs_fs.h
===================================================================
--- linux-2.6.9-rc4.orig/include/linux/nfs_fs.h
+++ linux-2.6.9-rc4/include/linux/nfs_fs.h
@@ -319,6 +319,7 @@ extern u32 root_nfs_parse_addr(char *nam
  */
 extern struct inode_operations nfs_file_inode_operations;
 extern struct file_operations nfs_file_operations;
+extern struct file_operations nfs_file_operations_nolock;
 extern struct address_space_operations nfs_file_aops;
 
 static inline struct rpc_cred *nfs_file_cred(struct file *file)

Re: [PATCH] NFS nolock broken in 2.6.9-rc4

[Trond Myklebust]

P=E5 fr , 15/10/2004 klokka 17:21, skreiv Olaf Kirch:
> > Urrrgh.. this looks complicated and duplicates code. Then I'd rather
> > have two struct file_operations, one for normal operations with a .lock
> > function, and a second one for nolock without.

Huh? Those 3 tests are exactly what we had prior to 2.6.9-rc2. I've just
moved the LOCK_USE_CLNT option into the filesystem. In fact it does the
same thing with LESS code than what used to be the case (the 2 combined
patches remove 31 lines, and insert 20).

> Please find attached proposed fix3 that does just this.

Hmm... I agree that is slightly more efficient. The worry I have is that
the "nolock" option is a mountpoint option; it's not really a file
property.
If we ever want to be able to move options like "-onolock" to the
vfsmount, so that people can have the same underlying filesystem mounted
in different places with several different sets of mount options, then
we should not mix mount options and file properties.

Cheers,
  Trond



-------------------------------------------------------
This SF.net email is sponsored by: IT Product Guide on ITManagersJournal
Use IT products in your business? Tell us what you think of them. Give us
Your Opinions, Get Free ThinkGeek Gift Certificates! Click to find out more
http://productguide.itmanagersjournal.com/guidepromo.tmpl
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs