[PATCH] -- filesystems.c::sys_nfsservctl

[PATCH] -- filesystems.c::sys_nfsservctl

[Craig Christophel]

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

Ok guys get ready to flame me....  

	The attached patch removes the lock/unlock in this function.   Now I am 80% 
sure of this one, but would like a word from the kmod maintainer about 
whether request_module needs the BKL or not.   do_nfsservctl already takes 
the BKL inside the function so as long as request_module is safe this pair 
can be removed -- effectively making do_nfsservctl responsible for it's own 
locking scheme.

	So whoever knows for SURE about request_module, please reply.

========NOT A PATCH --- filesystems.c::sys_nfsservctl================
long
asmlinkage sys_nfsservctl(int cmd, void *argp, void *resp)
{
	int ret = -ENOSYS;
	
	lock_kernel();

	if (nfsd_linkage ||
	    (request_module ("nfsd") == 0 && nfsd_linkage))
		ret = nfsd_linkage->do_nfsservctl(cmd, argp, resp);

	unlock_kernel();
	return ret;
}


==================PATCH ATTACHED==========================

[-- Attachment #2: filesystems-remove-lock_kernel-nfsd.diff --]
[-- Type: text/x-diff, Size: 411 bytes --]

===== fs/filesystems.c 1.4 vs edited =====
--- 1.4/fs/filesystems.c	Fri Feb  8 22:10:55 2002
+++ edited/fs/filesystems.c	Wed Feb 13 15:30:20 2002
@@ -22,13 +22,11 @@
 {
 	int ret = -ENOSYS;
 	
-	lock_kernel();
 
 	if (nfsd_linkage ||
 	    (request_module ("nfsd") == 0 && nfsd_linkage))
 		ret = nfsd_linkage->do_nfsservctl(cmd, argp, resp);
 
-	unlock_kernel();
 	return ret;
 }
 EXPORT_SYMBOL(nfsd_linkage);

Re: [PATCH] -- filesystems.c::sys_nfsservctl

[Trond Myklebust]

>>>>> " " == Craig Christophel <merlin@transgeek.com> writes:

     > Ok guys get ready to flame me....
     > 	The attached patch removes the lock/unlock in this function.
     > 	Now I am 80%
     > sure of this one, but would like a word from the kmod
     > maintainer about whether request_module needs the BKL or not.
     > do_nfsservctl already takes the BKL inside the function so as
     > long as request_module is safe this pair can be removed --
     > effectively making do_nfsservctl responsible for it's own
     > locking scheme.

What would remain to protect 'nfsd_linkage' if you removed the BKL?

Cheers,
  Trond

Re: [PATCH] -- filesystems.c::sys_nfsservctl 2.5.5-pre1

[Craig Christophel]

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

> What would remain to protect 'nfsd_linkage' if you removed the BKL?


ok here is a patch with that changed. inlined for commenting and attached for 
patching.  (kmail screws up my inlines occasionally) 
All of this extra checking is only in place if nfsd is a module.  rw_sems 
shouldn't impost that much overhead in the normal case.  

	The while is there to catch extremely unlikely cases of the module unloading 
during this loop, ie:  the cleanup_module section of nfsd is waiting for the 
semaphore.

===== fs/filesystems.c 1.4 vs edited =====
--- 1.4/fs/filesystems.c	Fri Feb  8 22:10:55 2002
+++ edited/fs/filesystems.c	Wed Feb 13 17:43:41 2002
@@ -16,19 +16,28 @@
 
 #if defined(CONFIG_NFSD_MODULE)
 struct nfsd_linkage *nfsd_linkage = NULL;
+DECLARE_RWSEM(nfsd_linkage_lock);
 
 long
 asmlinkage sys_nfsservctl(int cmd, void *argp, void *resp)
 {
 	int ret = -ENOSYS;
 	
-	lock_kernel();
-
-	if (nfsd_linkage ||
-	    (request_module ("nfsd") == 0 && nfsd_linkage))
-		ret = nfsd_linkage->do_nfsservctl(cmd, argp, resp);
-
-	unlock_kernel();
+	down_read(&nfsd_linkage_lock);
+	while(!nfsd_linkage) {
+		up_read(&nfsd_linkage_lock);
+		down_write(&nfsd_linkage_lock);
+		if (!nfsd_linkage)
+			if (request_module ("nfsd") != 0) {
+				up_write(&nfsd_linkage_lock);
+				goto out;
+			}
+		up_write(&nfsd_linkage_lock);
+		down_read(&nfsd_linkage_lock);
+	}
+	ret = nfsd_linkage->do_nfsservctl(cmd, argp, resp);
+	up_read(&nfsd_linkage_lock);
+out:
 	return ret;
 }
 EXPORT_SYMBOL(nfsd_linkage);
===== fs/nfsd/nfsctl.c 1.10 vs edited =====
--- 1.10/fs/nfsd/nfsctl.c	Fri Feb  8 22:10:55 2002
+++ edited/fs/nfsd/nfsctl.c	Wed Feb 13 17:33:17 2002
@@ -290,6 +290,7 @@
 	do_nfsservctl: handle_sys_nfsservctl,
 };
 
+extern struct nfsd_linkage_lock;
 /*
  * Initialize the module
  */
@@ -307,7 +308,9 @@
 void
 cleanup_module(void)
 {
+	down_write(&nfsd_linkage_lock);
 	nfsd_linkage = NULL;
+	up_write(&nfsd_linkage_lock);
 	nfsd_export_shutdown();
 	nfsd_cache_shutdown();
 	remove_proc_entry("fs/nfs/exports", NULL);

[-- Attachment #2: filesystems-remove-lock_kernel-nfsd.diff --]
[-- Type: text/x-diff, Size: 1516 bytes --]

===== fs/filesystems.c 1.4 vs edited =====
--- 1.4/fs/filesystems.c	Fri Feb  8 22:10:55 2002
+++ edited/fs/filesystems.c	Wed Feb 13 17:43:41 2002
@@ -16,19 +16,28 @@
 
 #if defined(CONFIG_NFSD_MODULE)
 struct nfsd_linkage *nfsd_linkage = NULL;
+DECLARE_RWSEM(nfsd_linkage_lock);
 
 long
 asmlinkage sys_nfsservctl(int cmd, void *argp, void *resp)
 {
 	int ret = -ENOSYS;
 	
-	lock_kernel();
-
-	if (nfsd_linkage ||
-	    (request_module ("nfsd") == 0 && nfsd_linkage))
-		ret = nfsd_linkage->do_nfsservctl(cmd, argp, resp);
-
-	unlock_kernel();
+	down_read(&nfsd_linkage_lock);
+	while(!nfsd_linkage) {
+		up_read(&nfsd_linkage_lock);
+		down_write(&nfsd_linkage_lock);
+		if (!nfsd_linkage)
+			if (request_module ("nfsd") != 0) {
+				up_write(&nfsd_linkage_lock);
+				goto out;
+			}
+		up_write(&nfsd_linkage_lock);
+		down_read(&nfsd_linkage_lock);
+	}
+	ret = nfsd_linkage->do_nfsservctl(cmd, argp, resp);
+	up_read(&nfsd_linkage_lock);
+out:
 	return ret;
 }
 EXPORT_SYMBOL(nfsd_linkage);
===== fs/nfsd/nfsctl.c 1.10 vs edited =====
--- 1.10/fs/nfsd/nfsctl.c	Fri Feb  8 22:10:55 2002
+++ edited/fs/nfsd/nfsctl.c	Wed Feb 13 17:33:17 2002
@@ -290,6 +290,7 @@
 	do_nfsservctl: handle_sys_nfsservctl,
 };
 
+extern struct nfsd_linkage_lock;
 /*
  * Initialize the module
  */
@@ -307,7 +308,9 @@
 void
 cleanup_module(void)
 {
+	down_write(&nfsd_linkage_lock);
 	nfsd_linkage = NULL;
+	up_write(&nfsd_linkage_lock);
 	nfsd_export_shutdown();
 	nfsd_cache_shutdown();
 	remove_proc_entry("fs/nfs/exports", NULL);

Re: [PATCH] -- filesystems.c::sys_nfsservctl

[Neil Brown]

On Wednesday February 13, merlin@transgeek.com wrote:
> Ok guys get ready to flame me....  
> 
> 	The attached patch removes the lock/unlock in this function.   Now I am 80% 
> sure of this one, but would like a word from the kmod maintainer about 
> whether request_module needs the BKL or not.   do_nfsservctl already takes 
> the BKL inside the function so as long as request_module is safe this pair 
> can be removed -- effectively making do_nfsservctl responsible for it's own 
> locking scheme.
> 
> 	So whoever knows for SURE about request_module, please reply.

Please see
  http://www.cse.unsw.edu.au/~neilb/patches/linux-devel/2.5.4-pre5/
for current nfsd patches.
  patch-E-NfsdSyscallCleanup  
might be of particular interest.

When I have finished testing these (sometime next week I hope) I will
be submitting them to Linux for 2.5, and then backporting them to 2.4,
and hopefully submitting the least intrusive ones to Marcello shortly
after 2.4.18 comes out.

These patches:
  remove most of the BKL
  enable TCP support
  plus assorted other things.

Comments, and testing, most welcome.

NeilBrown

how to avoid stale NFS filehandles?

[Matt Bernstein]

Hi,

I'm wondering what encoding is used for NFSv{2,3} file handles under
knfsd. In particular, can I upgrade kernel from 2.4.x to 2.4.y without
my clients accumulating stale filehandles?

Cheers,

Matt

Re: how to avoid stale NFS filehandles?

[Neil Brown]

On Friday February 15, matt@theBachChoir.org.uk wrote:
> Hi,
> 
> I'm wondering what encoding is used for NFSv{2,3} file handles under
> knfsd. In particular, can I upgrade kernel from 2.4.x to 2.4.y without
> my clients accumulating stale filehandles?

In general, you should be able to upgrade a minor version (e.g. 2.2 to
2.4) and up or down grade a patch leverl (e.g. 2.4.2 to 2.4.10 and
back to 2.4.2) without and filehandle compatability problems.
This only applies from 2.2.16 or there abouts.  Before that it was a
bit of a mess.

In particular, 2.4 introduced a new file handle format, but still
recognises the old file handle format.  Further, in response to a
request that contains an old style file handle, it generates an old
style file handle so that the client can compare filehandles and so
that thos filehandles can still be used in the server is down graded.

Some time in 2.4 I will probably introduce a new export option
(fs=nnnn) which will cause the filehandles to look slightly different
(i.e. not have any device numbers in them).  This will be optional
(options are like that...) but even if you do change a filesystem to
be exported with the new option, clients that currently have it
mounted will still work without getting stale file handles.

So, you should be safe (but if something does go wrong, let me know
... there could always be a bug in there somewhere).

NeilBrown