NFS nolock broken in 2.6.9-rc4

PATCH In review Olaf Kirch 2 participants 10 messages Oct 15, 2004 – Oct 18, 2004 1 message not in the archive

Subsystems (4)

VFS NFS client NFS server File locking (flock() and fcntl()/lockf())

lore

Share & tools

lore

Olaf Kirch

author

2004-10-15 10:00 UTC #0 lore raw

PATCH 1/1 NFS nolock broken in 2.6.9-rc4

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

Signature · 13 lines

Olaf Kirch     | Things that make Monday morning interesting, #1:
[email protected]   |        "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  -  [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

Olaf Kirch

author

2004-10-15 10:11 UTC #1 lore raw

Quoted 3 lines · 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.

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

Signature · 3 lines

Olaf Kirch     | Things that make Monday morning interesting, #1:
[email protected]   |        "I want to use NFS over AX25, can you help me?"
---------------+

fs/locks.c +5 -3

Index: 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

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 <[email protected]>

fs/locks.c +7 -4

Index: 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

fs/nfs/file.c +2 -4

Index: 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;
 		}
 	}

Trond Myklebust

2004-10-15 14:47 UTC #2 lore raw

P=E5 fr , 15/10/2004 klokka 12:07, skreiv Olaf Kirch:

Quoted 3 lines · Olaf Kirch

Quoted 2 lines
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=

Quoted 9 lines

Quoted 1 line
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 - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

Trond Myklebust

2004-10-15 15:12 UTC #3 lore raw

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

Signed-off-by Trond Myklebust <[email protected]>

---

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

fs/nfs/file.c +23 ~~-20~~

Index: linux-2.6.9-rc4-up/fs/nfs/file.c
===================================================================
@@ -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

Trond Myklebust

2004-10-15 15:14 UTC #4 lore raw

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

Signed-off-by Trond Myklebust <[email protected]>

---
 fs/locks.c         |    6 ------
 include/linux/fs.h |    5 -----
 2 files changed, 11 deletions(-)

include/linux/fs.h +0 -5

Index: linux-2.6.9-rc4-up/include/linux/fs.h
===================================================================
@@ -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

fs/locks.c +0 -6

Index: linux-2.6.9-rc4-up/fs/locks.c
===================================================================
@@ -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 {

A message that is not in the archive (sent off-list or not yet received): [email protected]

Olaf Kirch

author

2004-10-18 07:39 UTC #6 lore raw

Quoted 3 lines · Andrew Morton

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

Signature · 13 lines

Olaf Kirch     | Things that make Monday morning interesting, #1:
[email protected]   |        "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  -  [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

Trond Myklebust

2004-10-15 14:44 UTC #7 lore raw

In reply to Olaf Kirch

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

Quoted 3 lines

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

Olaf Kirch

author

2004-10-15 15:25 UTC #8 lore raw

Quoted 2 lines · Trond Myklebust

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.

Quoted 4 lines

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

Signature · 13 lines

Olaf Kirch     | Things that make Monday morning interesting, #1:
[email protected]   |        "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  -  [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

Olaf Kirch

author

2004-10-15 15:30 UTC #9 lore raw

Quoted 3 lines

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

Signature · 3 lines

Olaf Kirch     | Things that make Monday morning interesting, #1:
[email protected]   |        "I want to use NFS over AX25, can you help me?"
---------------+

fs/nfs/inode.c +4 -1

Index: 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)) {

fs/nfs/file.c +14 -0

Index: 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,

include/linux/nfs_fs.h +1 -0

Index: 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)

Trond Myklebust

2004-10-15 16:49 UTC #10 lore raw

P=E5 fr , 15/10/2004 klokka 17:21, skreiv Olaf Kirch:

Quoted 3 lines

Quoted 3 lines
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).

Quoted 1 line

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