While working on something unrelated I noticed nfsd tends to ignore a
lot of memory allocation failures on initialization. I don't think
we'eve seen a bug report here--such failures are probably nearly
nonexistant. But still it seems a minor bug in most cases not to catch
such failures.
My guess is that one of the motivations for the current behavior, in the
case of failures to create /proc entries, was to continue to allow nfsd
to function in kernels with no proc support whatsoever.
However in kernels that do have proc support failure to create such
entries should be treated as fatal. So, I add some #ifdef's to handle
this case explicitly.
I also add "select PROC_FS" when nfsv4 or gss are selected, since those
newer features don't function at all without proc.
Look reasonable?
--b.
-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
On Fri, Nov 16, 2007 at 10:38:47AM -0500, Peter Staubach wrote:
> J. Bruce Fields wrote:
>> On Fri, Nov 16, 2007 at 09:29:21AM -0500, Peter Staubach wrote:
>>
>>> J. Bruce Fields wrote:
>>>
>>>> + printk(KERN_ERR "nfsd: failed to allocate reply cache\n");
>>>>
>>> I was thinking that it might be nice to have something which
>>> better explains what the ramifications of this failure might
>>> be. This explains _precisely_ what happened, but not what
>>> will happen in the future.
>>>
>>
>> The module will fail to load (or, I suppose, the kernel will fail to
>> boot?). So the failure will be pretty obvious.
>>
>>
>
> The module will fail to load, but I suspect that that won't cause
> the system to fail to boot.
OK, but I was thinking of the case where nfsd was built in.
> The only way to notice that the module
> didn't load is to run lsmod or some such and to look for the module.
Typical distro init scripts probably emit a pretty loud warning in this
case, don't they?
> The admin may notice that the NFS server fails to start, but I
> think that it would be nice to better connect this memory allocation
> failure to the NFS server not running.
Well, send in a patch if you'd like, but it should probably add a
printk() to the end of init_nfsd() rather than fooling with the message
here, so it can catch failures that occur elsewhere in the
initialization process.
--b.
-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
_______________________________________________
Please note that [email protected] is being discontinued.
Please subscribe to [email protected] instead.
http://vger.kernel.org/vger-lists.html#linux-nfs
J. Bruce Fields wrote:
> On Fri, Nov 16, 2007 at 10:38:47AM -0500, Peter Staubach wrote:
>
>> J. Bruce Fields wrote:
>>
>>> On Fri, Nov 16, 2007 at 09:29:21AM -0500, Peter Staubach wrote:
>>>
>>>
>>>> J. Bruce Fields wrote:
>>>>
>>>>
>>>>> + printk(KERN_ERR "nfsd: failed to allocate reply cache\n");
>>>>>
>>>>>
>>>> I was thinking that it might be nice to have something which
>>>> better explains what the ramifications of this failure might
>>>> be. This explains _precisely_ what happened, but not what
>>>> will happen in the future.
>>>>
>>>>
>>> The module will fail to load (or, I suppose, the kernel will fail to
>>> boot?). So the failure will be pretty obvious.
>>>
>>>
>>>
>> The module will fail to load, but I suspect that that won't cause
>> the system to fail to boot.
>>
>
> OK, but I was thinking of the case where nfsd was built in.
>
>
Ahh. Sorry, didn't think about that.
This seems a little strong, doesn't it? To cause the system
to fail to boot?
>> The only way to notice that the module
>> didn't load is to run lsmod or some such and to look for the module.
>>
>
> Typical distro init scripts probably emit a pretty loud warning in this
> case, don't they?
>
>
Well, they note that the NFS service failed to start, yes.
>> The admin may notice that the NFS server fails to start, but I
>> think that it would be nice to better connect this memory allocation
>> failure to the NFS server not running.
>>
>
> Well, send in a patch if you'd like, but it should probably add a
> printk() to the end of init_nfsd() rather than fooling with the message
> here, so it can catch failures that occur elsewhere in the
> initialization process.
Yes, I think that you have hit it on the head. Do we care
whether any particular allocation failed or just that the
initialization failed?
Thanx...
ps
-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
_______________________________________________
Please note that [email protected] is being discontinued.
Please subscribe to [email protected] instead.
http://vger.kernel.org/vger-lists.html#linux-nfs
On Fri, Nov 16, 2007 at 11:22:24AM -0500, Peter Staubach wrote:
> J. Bruce Fields wrote:
>> On Fri, Nov 16, 2007 at 10:38:47AM -0500, Peter Staubach wrote:
>>
>>> J. Bruce Fields wrote:
>>>
>>>> On Fri, Nov 16, 2007 at 09:29:21AM -0500, Peter Staubach wrote:
>>>>
>>>>> J. Bruce Fields wrote:
>>>>>
>>>>>> + printk(KERN_ERR "nfsd: failed to allocate reply cache\n");
>>>>>>
>>>>> I was thinking that it might be nice to have something which
>>>>> better explains what the ramifications of this failure might
>>>>> be. This explains _precisely_ what happened, but not what
>>>>> will happen in the future.
>>>>>
>>>> The module will fail to load (or, I suppose, the kernel will fail to
>>>> boot?). So the failure will be pretty obvious.
>>>>
>>>>
>>> The module will fail to load, but I suspect that that won't cause
>>> the system to fail to boot.
>>>
>>
>> OK, but I was thinking of the case where nfsd was built in.
>>
>>
>
> Ahh. Sorry, didn't think about that.
>
> This seems a little strong, doesn't it? To cause the system
> to fail to boot?
Yeah, and I was confused: it's init/main.c:do_initcalls() that does
this, and it just keeps going regardless (and doesn't even log the error
unless debugging is turned on). That makes more sense.
--b.
-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
_______________________________________________
Please note that [email protected] is being discontinued.
Please subscribe to [email protected] instead.
http://vger.kernel.org/vger-lists.html#linux-nfs
If the reply cache initialization fails due to a kmalloc failure,
currently we try to soldier on with a reduced (or nonexistant) reply
cache.
Better to just fail immediately: the failure is then much easier to
understand and debug, and it could save us complexity in some later
code. (But actually, it doesn't help currently because the cache is
also turned off in some odd failure cases; we should probably find a
better way to handle those failure cases some day.)
Fix some minor style problems while we're at it, and rename
nfsd_cache_init() to remove the need for a comment describing it.
Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/nfsd/nfscache.c | 28 +++++++++++++---------------
fs/nfsd/nfsctl.c | 11 +++++++----
include/linux/nfsd/cache.h | 4 ++--
3 files changed, 22 insertions(+), 21 deletions(-)
diff --git a/fs/nfsd/nfscache.c b/fs/nfsd/nfscache.c
index 578f2c9..92cb5ae 100644
--- a/fs/nfsd/nfscache.c
+++ b/fs/nfsd/nfscache.c
@@ -44,17 +44,18 @@ static int nfsd_cache_append(struct svc_rqst *rqstp, struct kvec *vec);
*/
static DEFINE_SPINLOCK(cache_lock);
-void
-nfsd_cache_init(void)
+int
+nfsd_reply_cache_init(void)
{
struct svc_cacherep *rp;
int i;
INIT_LIST_HEAD(&lru_head);
i = CACHESIZE;
- while(i) {
+ while (i) {
rp = kmalloc(sizeof(*rp), GFP_KERNEL);
- if (!rp) break;
+ if (!rp)
+ goto out_nomem;
list_add(&rp->c_lru, &lru_head);
rp->c_state = RC_UNUSED;
rp->c_type = RC_NOCACHE;
@@ -62,23 +63,20 @@ nfsd_cache_init(void)
i--;
}
- if (i)
- printk (KERN_ERR "nfsd: cannot allocate all %d cache entries, only got %d\n",
- CACHESIZE, CACHESIZE-i);
-
hash_list = kcalloc (HASHSIZE, sizeof(struct hlist_head), GFP_KERNEL);
- if (!hash_list) {
- nfsd_cache_shutdown();
- printk (KERN_ERR "nfsd: cannot allocate %Zd bytes for hash list\n",
- HASHSIZE * sizeof(struct hlist_head));
- return;
- }
+ if (!hash_list)
+ goto out_nomem;
cache_disabled = 0;
+ return 0;
+out_nomem:
+ printk(KERN_ERR "nfsd: failed to allocate reply cache\n");
+ nfsd_reply_cache_shutdown();
+ return -ENOMEM;
}
void
-nfsd_cache_shutdown(void)
+nfsd_reply_cache_shutdown(void)
{
struct svc_cacherep *rp;
diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index ecf3779..2bfda9b 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -683,7 +683,9 @@ static int __init init_nfsd(void)
if (retval)
return retval;
nfsd_stat_init(); /* Statistics */
- nfsd_cache_init(); /* RPC reply cache */
+ retval = nfsd_reply_cache_init();
+ if (retval)
+ goto out_free_stat;
nfsd_export_init(); /* Exports table */
nfsd_lockd_init(); /* lockd->nfsd callbacks */
nfsd_idmap_init(); /* Name to ID mapping */
@@ -700,11 +702,12 @@ static int __init init_nfsd(void)
out_free_all:
nfsd_idmap_shutdown();
nfsd_export_shutdown();
- nfsd_cache_shutdown();
+ nfsd_reply_cache_shutdown();
remove_proc_entry("fs/nfs/exports", NULL);
remove_proc_entry("fs/nfs", NULL);
- nfsd_stat_shutdown();
nfsd_lockd_shutdown();
+out_free_stat:
+ nfsd_stat_shutdown();
nfsd4_free_slabs();
return retval;
}
@@ -712,7 +715,7 @@ out_free_all:
static void __exit exit_nfsd(void)
{
nfsd_export_shutdown();
- nfsd_cache_shutdown();
+ nfsd_reply_cache_shutdown();
remove_proc_entry("fs/nfs/exports", NULL);
remove_proc_entry("fs/nfs", NULL);
nfsd_stat_shutdown();
diff --git a/include/linux/nfsd/cache.h b/include/linux/nfsd/cache.h
index 007480c..7b5d784 100644
--- a/include/linux/nfsd/cache.h
+++ b/include/linux/nfsd/cache.h
@@ -72,8 +72,8 @@ enum {
*/
#define RC_DELAY (HZ/5)
-void nfsd_cache_init(void);
-void nfsd_cache_shutdown(void);
+int nfsd_reply_cache_init(void);
+void nfsd_reply_cache_shutdown(void);
int nfsd_cache_lookup(struct svc_rqst *, int);
void nfsd_cache_update(struct svc_rqst *, int, __be32 *);
--
1.5.3.5.561.g140d
-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
Newer server features such as nfsv4 and gss depend on proc to work, so a
failure to initialize the proc files they need should be treated as
fatal.
Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/nfsd/export.c | 12 +++++++++---
fs/nfsd/nfs4idmap.c | 13 ++++++++++---
fs/nfsd/nfsctl.c | 12 +++++++++---
include/linux/nfsd/export.h | 2 +-
include/linux/nfsd_idmap.h | 4 ++--
include/linux/sunrpc/cache.h | 2 +-
net/sunrpc/auth_gss/svcauth_gss.c | 17 +++++++++++++----
net/sunrpc/cache.c | 30 +++++++++++++++++++++++-------
8 files changed, 68 insertions(+), 24 deletions(-)
diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
index d29b70a..cbbc594 100644
--- a/fs/nfsd/export.c
+++ b/fs/nfsd/export.c
@@ -1637,13 +1637,19 @@ exp_verify_string(char *cp, int max)
/*
* Initialize the exports module.
*/
-void
+int
nfsd_export_init(void)
{
+ int rv;
dprintk("nfsd: initializing export module.\n");
- cache_register(&svc_export_cache);
- cache_register(&svc_expkey_cache);
+ rv = cache_register(&svc_export_cache);
+ if (rv)
+ return rv;
+ rv = cache_register(&svc_expkey_cache);
+ if (rv)
+ cache_unregister(&svc_export_cache);
+ return rv;
}
diff --git a/fs/nfsd/nfs4idmap.c b/fs/nfsd/nfs4idmap.c
index ef22179..996bd88 100644
--- a/fs/nfsd/nfs4idmap.c
+++ b/fs/nfsd/nfs4idmap.c
@@ -464,11 +464,18 @@ nametoid_update(struct ent *new, struct ent *old)
* Exported API
*/
-void
+int
nfsd_idmap_init(void)
{
- cache_register(&idtoname_cache);
- cache_register(&nametoid_cache);
+ int rv;
+
+ rv = cache_register(&idtoname_cache);
+ if (rv)
+ return rv;
+ rv = cache_register(&nametoid_cache);
+ if (rv)
+ cache_unregister(&idtoname_cache);
+ return rv;
}
void
diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index 63d8075..e307972 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -707,9 +707,13 @@ static int __init init_nfsd(void)
retval = nfsd_reply_cache_init();
if (retval)
goto out_free_stat;
- nfsd_export_init(); /* Exports table */
+ retval = nfsd_export_init();
+ if (retval)
+ goto out_free_cache;
nfsd_lockd_init(); /* lockd->nfsd callbacks */
- nfsd_idmap_init(); /* Name to ID mapping */
+ retval = nfsd_idmap_init();
+ if (retval)
+ goto out_free_lockd;
retval = create_proc_exports_entry();
if (retval)
goto out_free_idmap;
@@ -720,10 +724,12 @@ static int __init init_nfsd(void)
out_free_all:
remove_proc_entry("fs/nfs/exports", NULL);
remove_proc_entry("fs/nfs", NULL);
- nfsd_idmap_shutdown();
out_free_idmap:
+ nfsd_idmap_shutdown();
+out_free_lockd:
nfsd_lockd_shutdown();
nfsd_export_shutdown();
+out_free_cache:
nfsd_reply_cache_shutdown();
out_free_stat:
nfsd_stat_shutdown();
diff --git a/include/linux/nfsd/export.h b/include/linux/nfsd/export.h
index bcb7aba..3a16872 100644
--- a/include/linux/nfsd/export.h
+++ b/include/linux/nfsd/export.h
@@ -122,7 +122,7 @@ __be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp);
/*
* Function declarations
*/
-void nfsd_export_init(void);
+int nfsd_export_init(void);
void nfsd_export_shutdown(void);
void nfsd_export_flush(void);
void exp_readlock(void);
diff --git a/include/linux/nfsd_idmap.h b/include/linux/nfsd_idmap.h
index e82746f..f5dd037 100644
--- a/include/linux/nfsd_idmap.h
+++ b/include/linux/nfsd_idmap.h
@@ -44,10 +44,10 @@
#define IDMAP_NAMESZ 128
#ifdef CONFIG_NFSD_V4
-void nfsd_idmap_init(void);
+int nfsd_idmap_init(void);
void nfsd_idmap_shutdown(void);
#else
-static inline void nfsd_idmap_init(void) {};
+static inline int nfsd_idmap_init(void) {};
static inline void nfsd_idmap_shutdown(void) {};
#endif
diff --git a/include/linux/sunrpc/cache.h b/include/linux/sunrpc/cache.h
index b683b5d..03547d6 100644
--- a/include/linux/sunrpc/cache.h
+++ b/include/linux/sunrpc/cache.h
@@ -169,7 +169,7 @@ extern int cache_check(struct cache_detail *detail,
extern void cache_flush(void);
extern void cache_purge(struct cache_detail *detail);
#define NEVER (0x7FFFFFFF)
-extern void cache_register(struct cache_detail *cd);
+extern int cache_register(struct cache_detail *cd);
extern void cache_unregister(struct cache_detail *cd);
extern void qword_add(char **bpp, int *lp, char *str);
diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index d329a12..aa790bb 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -1386,10 +1386,19 @@ int
gss_svc_init(void)
{
int rv = svc_auth_register(RPC_AUTH_GSS, &svcauthops_gss);
- if (rv == 0) {
- cache_register(&rsc_cache);
- cache_register(&rsi_cache);
- }
+ if (rv)
+ return rv;
+ rv = cache_register(&rsc_cache);
+ if (rv)
+ goto out1;
+ rv = cache_register(&rsi_cache);
+ if (rv)
+ goto out2;
+ return 0;
+out2:
+ cache_unregister(&rsc_cache);
+out1:
+ svc_auth_unregister(RPC_AUTH_GSS);
return rv;
}
diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c
index 504b4e8..d41fe3c 100644
--- a/net/sunrpc/cache.c
+++ b/net/sunrpc/cache.c
@@ -304,20 +304,21 @@ void remove_cache_proc_entries(struct cache_detail *cd)
remove_proc_entry(cd->name, proc_net_rpc);
}
-void create_cache_proc_entries(struct cache_detail *cd)
+#ifdef CONFIG_PROC_FS
+int create_cache_proc_entries(struct cache_detail *cd)
{
struct proc_dir_entry *p;
cd->proc_ent = proc_mkdir(cd->name, proc_net_rpc);
if (cd->proc_ent == NULL)
- return;
+ goto out_nomem;
cd->proc_ent->owner = cd->owner;
cd->channel_ent = cd->content_ent = NULL;
p = create_proc_entry("flush", S_IFREG|S_IRUSR|S_IWUSR, cd->proc_ent);
cd->flush_ent = p;
if (p == NULL)
- return;
+ goto out_nomem;
p->proc_fops = &cache_flush_operations;
p->owner = cd->owner;
p->data = cd;
@@ -327,7 +328,7 @@ void create_cache_proc_entries(struct cache_detail *cd)
cd->proc_ent);
cd->channel_ent = p;
if (p == NULL)
- return;
+ goto out_nomem;
p->proc_fops = &cache_file_operations;
p->owner = cd->owner;
p->data = cd;
@@ -337,16 +338,30 @@ void create_cache_proc_entries(struct cache_detail *cd)
cd->proc_ent);
cd->content_ent = p;
if (p == NULL)
- return;
+ goto out_nomem;
p->proc_fops = &content_file_operations;
p->owner = cd->owner;
p->data = cd;
}
+ return 0;
+out_nomem:
+ remove_cache_proc_entries(cd);
+ return -ENOMEM;
}
+#else /* CONFIG_PROC_FS */
+int create_cache_proc_entries(struct cache_detail *cd)
+{
+ return 0;
+}
+#endif
-void cache_register(struct cache_detail *cd)
+int cache_register(struct cache_detail *cd)
{
- create_cache_proc_entries(cd);
+ int ret;
+
+ ret = create_cache_proc_entries(cd);
+ if (ret)
+ return ret;
rwlock_init(&cd->hash_lock);
INIT_LIST_HEAD(&cd->queue);
spin_lock(&cache_list_lock);
@@ -360,6 +375,7 @@ void cache_register(struct cache_detail *cd)
/* start the cleaning process */
schedule_delayed_work(&cache_cleaner, 0);
+ return 0;
}
void cache_unregister(struct cache_detail *cd)
--
1.5.3.5.561.g140d
-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
Just some minor cleanup.
Also I don't see much point in trying to register further proc entries
if initial entries fail; so just stop trying in that case.
Signed-off-by: J. Bruce Fields <[email protected]>
---
net/sunrpc/cache.c | 99 ++++++++++++++++++++++++++++-----------------------
1 files changed, 54 insertions(+), 45 deletions(-)
diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c
index d05ea16..504b4e8 100644
--- a/net/sunrpc/cache.c
+++ b/net/sunrpc/cache.c
@@ -290,44 +290,63 @@ static const struct file_operations cache_flush_operations;
static void do_cache_clean(struct work_struct *work);
static DECLARE_DELAYED_WORK(cache_cleaner, do_cache_clean);
-void cache_register(struct cache_detail *cd)
+void remove_cache_proc_entries(struct cache_detail *cd)
{
- cd->proc_ent = proc_mkdir(cd->name, proc_net_rpc);
- if (cd->proc_ent) {
- struct proc_dir_entry *p;
- cd->proc_ent->owner = cd->owner;
- cd->channel_ent = cd->content_ent = NULL;
+ if (cd->proc_ent == NULL)
+ return;
+ if (cd->flush_ent)
+ remove_proc_entry("flush", cd->proc_ent);
+ if (cd->channel_ent)
+ remove_proc_entry("channel", cd->proc_ent);
+ if (cd->content_ent)
+ remove_proc_entry("content", cd->proc_ent);
+ cd->proc_ent = NULL;
+ remove_proc_entry(cd->name, proc_net_rpc);
+}
- p = create_proc_entry("flush", S_IFREG|S_IRUSR|S_IWUSR,
- cd->proc_ent);
- cd->flush_ent = p;
- if (p) {
- p->proc_fops = &cache_flush_operations;
- p->owner = cd->owner;
- p->data = cd;
- }
+void create_cache_proc_entries(struct cache_detail *cd)
+{
+ struct proc_dir_entry *p;
- if (cd->cache_request || cd->cache_parse) {
- p = create_proc_entry("channel", S_IFREG|S_IRUSR|S_IWUSR,
- cd->proc_ent);
- cd->channel_ent = p;
- if (p) {
- p->proc_fops = &cache_file_operations;
- p->owner = cd->owner;
- p->data = cd;
- }
- }
- if (cd->cache_show) {
- p = create_proc_entry("content", S_IFREG|S_IRUSR|S_IWUSR,
- cd->proc_ent);
- cd->content_ent = p;
- if (p) {
- p->proc_fops = &content_file_operations;
- p->owner = cd->owner;
- p->data = cd;
- }
- }
+ cd->proc_ent = proc_mkdir(cd->name, proc_net_rpc);
+ if (cd->proc_ent == NULL)
+ return;
+ cd->proc_ent->owner = cd->owner;
+ cd->channel_ent = cd->content_ent = NULL;
+
+ p = create_proc_entry("flush", S_IFREG|S_IRUSR|S_IWUSR, cd->proc_ent);
+ cd->flush_ent = p;
+ if (p == NULL)
+ return;
+ p->proc_fops = &cache_flush_operations;
+ p->owner = cd->owner;
+ p->data = cd;
+
+ if (cd->cache_request || cd->cache_parse) {
+ p = create_proc_entry("channel", S_IFREG|S_IRUSR|S_IWUSR,
+ cd->proc_ent);
+ cd->channel_ent = p;
+ if (p == NULL)
+ return;
+ p->proc_fops = &cache_file_operations;
+ p->owner = cd->owner;
+ p->data = cd;
+ }
+ if (cd->cache_show) {
+ p = create_proc_entry("content", S_IFREG|S_IRUSR|S_IWUSR,
+ cd->proc_ent);
+ cd->content_ent = p;
+ if (p == NULL)
+ return;
+ p->proc_fops = &content_file_operations;
+ p->owner = cd->owner;
+ p->data = cd;
}
+}
+
+void cache_register(struct cache_detail *cd)
+{
+ create_cache_proc_entries(cd);
rwlock_init(&cd->hash_lock);
INIT_LIST_HEAD(&cd->queue);
spin_lock(&cache_list_lock);
@@ -358,17 +377,7 @@ void cache_unregister(struct cache_detail *cd)
list_del_init(&cd->others);
write_unlock(&cd->hash_lock);
spin_unlock(&cache_list_lock);
- if (cd->proc_ent) {
- if (cd->flush_ent)
- remove_proc_entry("flush", cd->proc_ent);
- if (cd->channel_ent)
- remove_proc_entry("channel", cd->proc_ent);
- if (cd->content_ent)
- remove_proc_entry("content", cd->proc_ent);
-
- cd->proc_ent = NULL;
- remove_proc_entry(cd->name, proc_net_rpc);
- }
+ remove_cache_proc_entries(cd);
if (list_empty(&cache_list)) {
/* module must be being unloaded so its safe to kill the worker */
cancel_delayed_work_sync(&cache_cleaner);
--
1.5.3.5.561.g140d
-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
Handle the failure case here with something closer to the standard
kernel style.
Doesn't really matter for now, but I'd like to add a few more failure
cases, and then this'll help.
Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/nfsd/nfsctl.c | 22 ++++++++++++----------
1 files changed, 12 insertions(+), 10 deletions(-)
diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index d8d50a7..ecf3779 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -694,16 +694,18 @@ static int __init init_nfsd(void)
entry->proc_fops = &exports_operations;
}
retval = register_filesystem(&nfsd_fs_type);
- if (retval) {
- nfsd_idmap_shutdown();
- nfsd_export_shutdown();
- nfsd_cache_shutdown();
- remove_proc_entry("fs/nfs/exports", NULL);
- remove_proc_entry("fs/nfs", NULL);
- nfsd_stat_shutdown();
- nfsd_lockd_shutdown();
- nfsd4_free_slabs();
- }
+ if (retval)
+ goto out_free_all;
+ return 0;
+out_free_all:
+ nfsd_idmap_shutdown();
+ nfsd_export_shutdown();
+ nfsd_cache_shutdown();
+ remove_proc_entry("fs/nfs/exports", NULL);
+ remove_proc_entry("fs/nfs", NULL);
+ nfsd_stat_shutdown();
+ nfsd_lockd_shutdown();
+ nfsd4_free_slabs();
return retval;
}
--
1.5.3.5.561.g140d
-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
The server depends on upcalls under /proc to support nfsv4 and gss.
Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/Kconfig | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/fs/Kconfig b/fs/Kconfig
index 429a002..340b233 100644
--- a/fs/Kconfig
+++ b/fs/Kconfig
@@ -1670,6 +1670,8 @@ config NFSD
select CRYPTO_MD5 if NFSD_V4
select CRYPTO if NFSD_V4
select FS_POSIX_ACL if NFSD_V4
+ select PROC_FS if NFSD_V4
+ select PROC_FS if SUNRPC_GSS
help
If you want your Linux box to act as an NFS *server*, so that other
computers on your local network which support NFS can access certain
--
1.5.3.5.561.g140d
-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
I assume the reason failure of creation was ignored here was just to
continue support embedded systems that want nfsd but not proc.
However, in cases where proc is supported it would be clearer to fail
entirely than to come up with some features disabled.
Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/nfsd/nfsctl.c | 37 ++++++++++++++++++++++++++++---------
1 files changed, 28 insertions(+), 9 deletions(-)
diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index 2bfda9b..63d8075 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -674,6 +674,27 @@ static struct file_system_type nfsd_fs_type = {
.kill_sb = kill_litter_super,
};
+#ifdef CONFIG_PROC_FS
+static inline int create_proc_exports_entry(void)
+{
+ struct proc_dir_entry *entry;
+
+ entry = proc_mkdir("fs/nfs", NULL);
+ if (!entry)
+ return -ENOMEM;
+ entry = create_proc_entry("fs/nfs/exports", 0, NULL);
+ if (!entry)
+ return -ENOMEM;
+ entry->proc_fops = &exports_operations;
+ return 0;
+}
+#else /* CONFIG_PROC_FS */
+static inline int create_proc_exports_entry(void)
+{
+ return 0;
+}
+#endif
+
static int __init init_nfsd(void)
{
int retval;
@@ -689,23 +710,21 @@ static int __init init_nfsd(void)
nfsd_export_init(); /* Exports table */
nfsd_lockd_init(); /* lockd->nfsd callbacks */
nfsd_idmap_init(); /* Name to ID mapping */
- if (proc_mkdir("fs/nfs", NULL)) {
- struct proc_dir_entry *entry;
- entry = create_proc_entry("fs/nfs/exports", 0, NULL);
- if (entry)
- entry->proc_fops = &exports_operations;
- }
+ retval = create_proc_exports_entry();
+ if (retval)
+ goto out_free_idmap;
retval = register_filesystem(&nfsd_fs_type);
if (retval)
goto out_free_all;
return 0;
out_free_all:
- nfsd_idmap_shutdown();
- nfsd_export_shutdown();
- nfsd_reply_cache_shutdown();
remove_proc_entry("fs/nfs/exports", NULL);
remove_proc_entry("fs/nfs", NULL);
+ nfsd_idmap_shutdown();
+out_free_idmap:
nfsd_lockd_shutdown();
+ nfsd_export_shutdown();
+ nfsd_reply_cache_shutdown();
out_free_stat:
nfsd_stat_shutdown();
nfsd4_free_slabs();
--
1.5.3.5.561.g140d
-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
There's really nothing much the caller can do if cache unregistration
fails. And indeed, all any caller does in this case is print an error
and continue. So just return void and move the printk's inside
cache_unregister.
Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/nfsd/export.c | 6 ++----
fs/nfsd/nfs4idmap.c | 6 ++----
include/linux/sunrpc/cache.h | 2 +-
net/sunrpc/auth_gss/svcauth_gss.c | 6 ++----
net/sunrpc/cache.c | 8 +++++---
net/sunrpc/sunrpc_syms.c | 6 ++----
6 files changed, 14 insertions(+), 20 deletions(-)
diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
index 66d0aeb..d29b70a 100644
--- a/fs/nfsd/export.c
+++ b/fs/nfsd/export.c
@@ -1670,10 +1670,8 @@ nfsd_export_shutdown(void)
exp_writelock();
- if (cache_unregister(&svc_expkey_cache))
- printk(KERN_ERR "nfsd: failed to unregister expkey cache\n");
- if (cache_unregister(&svc_export_cache))
- printk(KERN_ERR "nfsd: failed to unregister export cache\n");
+ cache_unregister(&svc_expkey_cache);
+ cache_unregister(&svc_export_cache);
svcauth_unix_purge();
exp_writeunlock();
diff --git a/fs/nfsd/nfs4idmap.c b/fs/nfsd/nfs4idmap.c
index 5b56c77..ef22179 100644
--- a/fs/nfsd/nfs4idmap.c
+++ b/fs/nfsd/nfs4idmap.c
@@ -474,10 +474,8 @@ nfsd_idmap_init(void)
void
nfsd_idmap_shutdown(void)
{
- if (cache_unregister(&idtoname_cache))
- printk(KERN_ERR "nfsd: failed to unregister idtoname cache\n");
- if (cache_unregister(&nametoid_cache))
- printk(KERN_ERR "nfsd: failed to unregister nametoid cache\n");
+ cache_unregister(&idtoname_cache);
+ cache_unregister(&nametoid_cache);
}
/*
diff --git a/include/linux/sunrpc/cache.h b/include/linux/sunrpc/cache.h
index bd7a6b0..b683b5d 100644
--- a/include/linux/sunrpc/cache.h
+++ b/include/linux/sunrpc/cache.h
@@ -170,7 +170,7 @@ extern void cache_flush(void);
extern void cache_purge(struct cache_detail *detail);
#define NEVER (0x7FFFFFFF)
extern void cache_register(struct cache_detail *cd);
-extern int cache_unregister(struct cache_detail *cd);
+extern void cache_unregister(struct cache_detail *cd);
extern void qword_add(char **bpp, int *lp, char *str);
extern void qword_addhex(char **bpp, int *lp, char *buf, int blen);
diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index 73940df..d329a12 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -1396,9 +1396,7 @@ gss_svc_init(void)
void
gss_svc_shutdown(void)
{
- if (cache_unregister(&rsc_cache))
- printk(KERN_ERR "auth_rpcgss: failed to unregister rsc cache\n");
- if (cache_unregister(&rsi_cache))
- printk(KERN_ERR "auth_rpcgss: failed to unregister rsi cache\n");
+ cache_unregister(&rsc_cache);
+ cache_unregister(&rsi_cache);
svc_auth_unregister(RPC_AUTH_GSS);
}
diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c
index 0d747e2..d05ea16 100644
--- a/net/sunrpc/cache.c
+++ b/net/sunrpc/cache.c
@@ -343,7 +343,7 @@ void cache_register(struct cache_detail *cd)
schedule_delayed_work(&cache_cleaner, 0);
}
-int cache_unregister(struct cache_detail *cd)
+void cache_unregister(struct cache_detail *cd)
{
cache_purge(cd);
spin_lock(&cache_list_lock);
@@ -351,7 +351,7 @@ int cache_unregister(struct cache_detail *cd)
if (cd->entries || atomic_read(&cd->inuse)) {
write_unlock(&cd->hash_lock);
spin_unlock(&cache_list_lock);
- return -EBUSY;
+ goto out;
}
if (current_detail == cd)
current_detail = NULL;
@@ -373,7 +373,9 @@ int cache_unregister(struct cache_detail *cd)
/* module must be being unloaded so its safe to kill the worker */
cancel_delayed_work_sync(&cache_cleaner);
}
- return 0;
+ return;
+out:
+ printk(KERN_ERR "nfsd: failed to unregister %s cache\n", cd->name);
}
/* clean cache tries to find something to clean
diff --git a/net/sunrpc/sunrpc_syms.c b/net/sunrpc/sunrpc_syms.c
index 33d89e8..5793e00 100644
--- a/net/sunrpc/sunrpc_syms.c
+++ b/net/sunrpc/sunrpc_syms.c
@@ -164,10 +164,8 @@ cleanup_sunrpc(void)
cleanup_socket_xprt();
unregister_rpc_pipefs();
rpc_destroy_mempool();
- if (cache_unregister(&ip_map_cache))
- printk(KERN_ERR "sunrpc: failed to unregister ip_map cache\n");
- if (cache_unregister(&unix_gid_cache))
- printk(KERN_ERR "sunrpc: failed to unregister unix_gid cache\n");
+ cache_unregister(&ip_map_cache);
+ cache_unregister(&unix_gid_cache);
#ifdef RPC_DEBUG
rpc_unregister_sysctl();
#endif
--
1.5.3.5.561.g140d
-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
We forgot to shut down the nfs4 state and idmapping code in this case.
Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/nfsd/nfsctl.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index 77dc989..d8d50a7 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -695,12 +695,14 @@ static int __init init_nfsd(void)
}
retval = register_filesystem(&nfsd_fs_type);
if (retval) {
+ nfsd_idmap_shutdown();
nfsd_export_shutdown();
nfsd_cache_shutdown();
remove_proc_entry("fs/nfs/exports", NULL);
remove_proc_entry("fs/nfs", NULL);
nfsd_stat_shutdown();
nfsd_lockd_shutdown();
+ nfsd4_free_slabs();
}
return retval;
}
--
1.5.3.5.561.g140d
-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
On Thu, 2007-11-15 at 16:57 -0500, J. Bruce Fields wrote:
> The server depends on upcalls under /proc to support nfsv4 and gss.
>
> Signed-off-by: J. Bruce Fields <[email protected]>
> ---
> fs/Kconfig | 2 ++
> 1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/fs/Kconfig b/fs/Kconfig
> index 429a002..340b233 100644
> --- a/fs/Kconfig
> +++ b/fs/Kconfig
> @@ -1670,6 +1670,8 @@ config NFSD
> select CRYPTO_MD5 if NFSD_V4
> select CRYPTO if NFSD_V4
> select FS_POSIX_ACL if NFSD_V4
> + select PROC_FS if NFSD_V4
> + select PROC_FS if SUNRPC_GSS
> help
> If you want your Linux box to act as an NFS *server*, so that other
> computers on your local network which support NFS can access certain
What if you just want it to act as a client? No need for PROC_FS then...
Trond
-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
On Thu, Nov 15, 2007 at 05:06:36PM -0500, Trond Myklebust wrote:
>
> On Thu, 2007-11-15 at 16:57 -0500, J. Bruce Fields wrote:
> > The server depends on upcalls under /proc to support nfsv4 and gss.
> >
> > Signed-off-by: J. Bruce Fields <[email protected]>
> > ---
> > fs/Kconfig | 2 ++
> > 1 files changed, 2 insertions(+), 0 deletions(-)
> >
> > diff --git a/fs/Kconfig b/fs/Kconfig
> > index 429a002..340b233 100644
> > --- a/fs/Kconfig
> > +++ b/fs/Kconfig
> > @@ -1670,6 +1670,8 @@ config NFSD
> > select CRYPTO_MD5 if NFSD_V4
> > select CRYPTO if NFSD_V4
> > select FS_POSIX_ACL if NFSD_V4
> > + select PROC_FS if NFSD_V4
> > + select PROC_FS if SUNRPC_GSS
> > help
> > If you want your Linux box to act as an NFS *server*, so that other
> > computers on your local network which support NFS can access certain
>
> What if you just want it to act as a client? No need for PROC_FS then...
We're inside the config NFSD clause, so if you really want *just* a
client, then this doesn't change anything.
So the problematic case would be if you want it to be both client and
server, and want to use GSS on the client, but don't want to use GSS on
the server, and don't want to compile in proc.
Is that an important case?
If so, OK, we can remove the "select PROC_FS if SUNRPC_GSS". But it
might help to at least keep some documentation here though so people
know they need proc if they expect GSS to work on the server.
I suppose we could just add another "server-side gss support" config
entry whose only reason for existance is to turn on PROC_FS.
--b.
-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
On Thu, 2007-11-15 at 17:20 -0500, J. Bruce Fields wrote:
> On Thu, Nov 15, 2007 at 05:06:36PM -0500, Trond Myklebust wrote:
> >
> > On Thu, 2007-11-15 at 16:57 -0500, J. Bruce Fields wrote:
> > > The server depends on upcalls under /proc to support nfsv4 and gss.
> > >
> > > Signed-off-by: J. Bruce Fields <[email protected]>
> > > ---
> > > fs/Kconfig | 2 ++
> > > 1 files changed, 2 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/fs/Kconfig b/fs/Kconfig
> > > index 429a002..340b233 100644
> > > --- a/fs/Kconfig
> > > +++ b/fs/Kconfig
> > > @@ -1670,6 +1670,8 @@ config NFSD
> > > select CRYPTO_MD5 if NFSD_V4
> > > select CRYPTO if NFSD_V4
> > > select FS_POSIX_ACL if NFSD_V4
> > > + select PROC_FS if NFSD_V4
> > > + select PROC_FS if SUNRPC_GSS
> > > help
> > > If you want your Linux box to act as an NFS *server*, so that other
> > > computers on your local network which support NFS can access certain
> >
> > What if you just want it to act as a client? No need for PROC_FS then...
>
> We're inside the config NFSD clause, so if you really want *just* a
> client, then this doesn't change anything.
Fair enough. I missed that.
> So the problematic case would be if you want it to be both client and
> server, and want to use GSS on the client, but don't want to use GSS on
> the server, and don't want to compile in proc.
>
> Is that an important case?
>
> If so, OK, we can remove the "select PROC_FS if SUNRPC_GSS". But it
> might help to at least keep some documentation here though so people
> know they need proc if they expect GSS to work on the server.
>
> I suppose we could just add another "server-side gss support" config
> entry whose only reason for existance is to turn on PROC_FS.
No need.
Trond
-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
On Thu, Nov 15, 2007 at 05:25:52PM -0500, Trond Myklebust wrote:
>
> On Thu, 2007-11-15 at 17:20 -0500, J. Bruce Fields wrote:
> > On Thu, Nov 15, 2007 at 05:06:36PM -0500, Trond Myklebust wrote:
> > >
> > > On Thu, 2007-11-15 at 16:57 -0500, J. Bruce Fields wrote:
> > > > The server depends on upcalls under /proc to support nfsv4 and gss.
> > > >
> > > > Signed-off-by: J. Bruce Fields <[email protected]>
> > > > ---
> > > > fs/Kconfig | 2 ++
> > > > 1 files changed, 2 insertions(+), 0 deletions(-)
> > > >
> > > > diff --git a/fs/Kconfig b/fs/Kconfig
> > > > index 429a002..340b233 100644
> > > > --- a/fs/Kconfig
> > > > +++ b/fs/Kconfig
> > > > @@ -1670,6 +1670,8 @@ config NFSD
> > > > select CRYPTO_MD5 if NFSD_V4
> > > > select CRYPTO if NFSD_V4
> > > > select FS_POSIX_ACL if NFSD_V4
> > > > + select PROC_FS if NFSD_V4
> > > > + select PROC_FS if SUNRPC_GSS
> > > > help
> > > > If you want your Linux box to act as an NFS *server*, so that other
> > > > computers on your local network which support NFS can access certain
> > >
> > > What if you just want it to act as a client? No need for PROC_FS then...
> >
> > We're inside the config NFSD clause, so if you really want *just* a
> > client, then this doesn't change anything.
>
> Fair enough. I missed that.
>
> > So the problematic case would be if you want it to be both client and
> > server, and want to use GSS on the client, but don't want to use GSS on
> > the server, and don't want to compile in proc.
> >
> > Is that an important case?
> >
> > If so, OK, we can remove the "select PROC_FS if SUNRPC_GSS". But it
> > might help to at least keep some documentation here though so people
> > know they need proc if they expect GSS to work on the server.
> >
> > I suppose we could just add another "server-side gss support" config
> > entry whose only reason for existance is to turn on PROC_FS.
>
> No need.
OK, thanks for clarifying.--b.
-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
On Thursday November 15, [email protected] wrote:
> While working on something unrelated I noticed nfsd tends to ignore a
> lot of memory allocation failures on initialization. I don't think
> we'eve seen a bug report here--such failures are probably nearly
> nonexistant. But still it seems a minor bug in most cases not to catch
> such failures.
>
> My guess is that one of the motivations for the current behavior, in the
> case of failures to create /proc entries, was to continue to allow nfsd
> to function in kernels with no proc support whatsoever.
>
> However in kernels that do have proc support failure to create such
> entries should be treated as fatal. So, I add some #ifdef's to handle
> this case explicitly.
>
> I also add "select PROC_FS" when nfsv4 or gss are selected, since those
> newer features don't function at all without proc.
>
> Look reasonable?
Yes, all Acked-by: NeilBrown <[email protected]>
The cleanup of the cache_unregister call sites is very nice. It is a
pity that the cache_register sites become more ugly, but I think it is
the right thing to do.
Thanks,
NeilBrown
-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
On Fri, Nov 16, 2007 at 11:41:09AM +1100, Neil Brown wrote:
> On Thursday November 15, [email protected] wrote:
> > While working on something unrelated I noticed nfsd tends to ignore a
> > lot of memory allocation failures on initialization. I don't think
> > we'eve seen a bug report here--such failures are probably nearly
> > nonexistant. But still it seems a minor bug in most cases not to catch
> > such failures.
> >
> > My guess is that one of the motivations for the current behavior, in the
> > case of failures to create /proc entries, was to continue to allow nfsd
> > to function in kernels with no proc support whatsoever.
> >
> > However in kernels that do have proc support failure to create such
> > entries should be treated as fatal. So, I add some #ifdef's to handle
> > this case explicitly.
> >
> > I also add "select PROC_FS" when nfsv4 or gss are selected, since those
> > newer features don't function at all without proc.
> >
> > Look reasonable?
>
> Yes, all Acked-by: NeilBrown <[email protected]>
>
> The cleanup of the cache_unregister call sites is very nice. It is a
> pity that the cache_register sites become more ugly, but I think it is
> the right thing to do.
OK, thanks.
--b.
-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
J. Bruce Fields wrote:
> If the reply cache initialization fails due to a kmalloc failure,
> currently we try to soldier on with a reduced (or nonexistant) reply
> cache.
>
> Better to just fail immediately: the failure is then much easier to
> understand and debug, and it could save us complexity in some later
> code. (But actually, it doesn't help currently because the cache is
> also turned off in some odd failure cases; we should probably find a
> better way to handle those failure cases some day.)
>
> Fix some minor style problems while we're at it, and rename
> nfsd_cache_init() to remove the need for a comment describing it.
>
> Signed-off-by: J. Bruce Fields <[email protected]>
> ---
> fs/nfsd/nfscache.c | 28 +++++++++++++---------------
> fs/nfsd/nfsctl.c | 11 +++++++----
> include/linux/nfsd/cache.h | 4 ++--
> 3 files changed, 22 insertions(+), 21 deletions(-)
>
> diff --git a/fs/nfsd/nfscache.c b/fs/nfsd/nfscache.c
> index 578f2c9..92cb5ae 100644
> --- a/fs/nfsd/nfscache.c
> +++ b/fs/nfsd/nfscache.c
> @@ -44,17 +44,18 @@ static int nfsd_cache_append(struct svc_rqst *rqstp, struct kvec *vec);
> */
> static DEFINE_SPINLOCK(cache_lock);
>
> -void
> -nfsd_cache_init(void)
> +int
> +nfsd_reply_cache_init(void)
> {
> struct svc_cacherep *rp;
> int i;
>
> INIT_LIST_HEAD(&lru_head);
> i = CACHESIZE;
> - while(i) {
> + while (i) {
> rp = kmalloc(sizeof(*rp), GFP_KERNEL);
> - if (!rp) break;
> + if (!rp)
> + goto out_nomem;
> list_add(&rp->c_lru, &lru_head);
> rp->c_state = RC_UNUSED;
> rp->c_type = RC_NOCACHE;
> @@ -62,23 +63,20 @@ nfsd_cache_init(void)
> i--;
> }
>
> - if (i)
> - printk (KERN_ERR "nfsd: cannot allocate all %d cache entries, only got %d\n",
> - CACHESIZE, CACHESIZE-i);
> -
> hash_list = kcalloc (HASHSIZE, sizeof(struct hlist_head), GFP_KERNEL);
> - if (!hash_list) {
> - nfsd_cache_shutdown();
> - printk (KERN_ERR "nfsd: cannot allocate %Zd bytes for hash list\n",
> - HASHSIZE * sizeof(struct hlist_head));
> - return;
> - }
> + if (!hash_list)
> + goto out_nomem;
>
> cache_disabled = 0;
> + return 0;
> +out_nomem:
> + printk(KERN_ERR "nfsd: failed to allocate reply cache\n");
>
I was thinking that it might be nice to have something which
better explains what the ramifications of this failure might
be. This explains _precisely_ what happened, but not what
will happen in the future.
Or can we get this documented in some fashion that admins
will know how to find and translate to what they need to
understand and do?
Thanx...
ps
> + nfsd_reply_cache_shutdown();
> + return -ENOMEM;
> }
>
> void
> -nfsd_cache_shutdown(void)
> +nfsd_reply_cache_shutdown(void)
> {
> struct svc_cacherep *rp;
>
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index ecf3779..2bfda9b 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -683,7 +683,9 @@ static int __init init_nfsd(void)
> if (retval)
> return retval;
> nfsd_stat_init(); /* Statistics */
> - nfsd_cache_init(); /* RPC reply cache */
> + retval = nfsd_reply_cache_init();
> + if (retval)
> + goto out_free_stat;
> nfsd_export_init(); /* Exports table */
> nfsd_lockd_init(); /* lockd->nfsd callbacks */
> nfsd_idmap_init(); /* Name to ID mapping */
> @@ -700,11 +702,12 @@ static int __init init_nfsd(void)
> out_free_all:
> nfsd_idmap_shutdown();
> nfsd_export_shutdown();
> - nfsd_cache_shutdown();
> + nfsd_reply_cache_shutdown();
> remove_proc_entry("fs/nfs/exports", NULL);
> remove_proc_entry("fs/nfs", NULL);
> - nfsd_stat_shutdown();
> nfsd_lockd_shutdown();
> +out_free_stat:
> + nfsd_stat_shutdown();
> nfsd4_free_slabs();
> return retval;
> }
> @@ -712,7 +715,7 @@ out_free_all:
> static void __exit exit_nfsd(void)
> {
> nfsd_export_shutdown();
> - nfsd_cache_shutdown();
> + nfsd_reply_cache_shutdown();
> remove_proc_entry("fs/nfs/exports", NULL);
> remove_proc_entry("fs/nfs", NULL);
> nfsd_stat_shutdown();
> diff --git a/include/linux/nfsd/cache.h b/include/linux/nfsd/cache.h
> index 007480c..7b5d784 100644
> --- a/include/linux/nfsd/cache.h
> +++ b/include/linux/nfsd/cache.h
> @@ -72,8 +72,8 @@ enum {
> */
> #define RC_DELAY (HZ/5)
>
> -void nfsd_cache_init(void);
> -void nfsd_cache_shutdown(void);
> +int nfsd_reply_cache_init(void);
> +void nfsd_reply_cache_shutdown(void);
> int nfsd_cache_lookup(struct svc_rqst *, int);
> void nfsd_cache_update(struct svc_rqst *, int, __be32 *);
>
>
-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
_______________________________________________
Please note that [email protected] is being discontinued.
Please subscribe to [email protected] instead.
http://vger.kernel.org/vger-lists.html#linux-nfs
On Fri, Nov 16, 2007 at 09:29:21AM -0500, Peter Staubach wrote:
> J. Bruce Fields wrote:
>> + printk(KERN_ERR "nfsd: failed to allocate reply cache\n");
>>
>
> I was thinking that it might be nice to have something which
> better explains what the ramifications of this failure might
> be. This explains _precisely_ what happened, but not what
> will happen in the future.
The module will fail to load (or, I suppose, the kernel will fail to
boot?). So the failure will be pretty obvious.
>
> Or can we get this documented in some fashion that admins
> will know how to find and translate to what they need to
> understand and do?
Would a more explicit statement of the cause (like "nfsd: out of memory
setting up reply cache") be helpful?
Obviously this should only happen in a rather extreme low-memory
condition.
--b.
-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
_______________________________________________
Please note that [email protected] is being discontinued.
Please subscribe to [email protected] instead.
http://vger.kernel.org/vger-lists.html#linux-nfs
J. Bruce Fields wrote:
> On Fri, Nov 16, 2007 at 09:29:21AM -0500, Peter Staubach wrote:
>
>> J. Bruce Fields wrote:
>>
>>> + printk(KERN_ERR "nfsd: failed to allocate reply cache\n");
>>>
>>>
>> I was thinking that it might be nice to have something which
>> better explains what the ramifications of this failure might
>> be. This explains _precisely_ what happened, but not what
>> will happen in the future.
>>
>
> The module will fail to load (or, I suppose, the kernel will fail to
> boot?). So the failure will be pretty obvious.
>
>
The module will fail to load, but I suspect that that won't cause
the system to fail to boot. The only way to notice that the module
didn't load is to run lsmod or some such and to look for the module.
The admin may notice that the NFS server fails to start, but I
think that it would be nice to better connect this memory allocation
failure to the NFS server not running.
>> Or can we get this documented in some fashion that admins
>> will know how to find and translate to what they need to
>> understand and do?
>>
>
> Would a more explicit statement of the cause (like "nfsd: out of memory
> setting up reply cache") be helpful?
>
>
No, I don't think that this adds anything that one couldn't reason
out of the first proposed message.
> Obviously this should only happen in a rather extreme low-memory
> condition.
>
Yes, and may never happen in nature, but if we are preparing
for the possibility, we may as well make it as easy for the
unsuspecting admin to figure out.
Otherwise, the message is only useful for someone who understands
the implementation and what the ramification of this failure
might be.
Thanx...
ps
-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
_______________________________________________
Please note that [email protected] is being discontinued.
Please subscribe to [email protected] instead.
http://vger.kernel.org/vger-lists.html#linux-nfs