From 8e60029f403781b8a63b7ffdb7dc1faff6ca651e Mon Sep 17 00:00:00 2001 From: Jeff Layton Date: Mon, 11 Feb 2008 10:00:20 -0500 Subject: NFS: fix reference counting for NFSv4 callback thread The reference counting for the NFSv4 callback thread stays artificially high. When this thread comes down, it doesn't properly tear down the svc_serv, causing a memory leak. In my testing on an older kernel on x86_64, memory would leak out of the 8k kmalloc slab. So, we're leaking at least a page of memory every time the thread comes down. svc_create() creates the svc_serv with a sv_nrthreads count of 1, and then svc_create_thread() increments that count. Whenever the callback thread is started it has a sv_nrthreads count of 2. When coming down, it calls svc_exit_thread() which decrements that count and if it hits 0, it tears everything down. That never happens here since the count is always at 2 when the thread exits. The problem is that nfs_callback_up() should be calling svc_destroy() on the svc_serv on both success and failure. This is how lockd_up_proto() handles the reference counting, and doing that here fixes the leak. Signed-off-by: Jeff Layton Signed-off-by: Trond Myklebust --- fs/nfs/callback.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) (limited to 'fs/nfs') diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c index bd185a572a2..ecc06c61949 100644 --- a/fs/nfs/callback.c +++ b/fs/nfs/callback.c @@ -105,7 +105,7 @@ static void nfs_callback_svc(struct svc_rqst *rqstp) */ int nfs_callback_up(void) { - struct svc_serv *serv; + struct svc_serv *serv = NULL; int ret = 0; lock_kernel(); @@ -122,24 +122,30 @@ int nfs_callback_up(void) ret = svc_create_xprt(serv, "tcp", nfs_callback_set_tcpport, SVC_SOCK_ANONYMOUS); if (ret <= 0) - goto out_destroy; + goto out_err; nfs_callback_tcpport = ret; dprintk("Callback port = 0x%x\n", nfs_callback_tcpport); ret = svc_create_thread(nfs_callback_svc, serv); if (ret < 0) - goto out_destroy; + goto out_err; nfs_callback_info.serv = serv; wait_for_completion(&nfs_callback_info.started); out: + /* + * svc_create creates the svc_serv with sv_nrthreads == 1, and then + * svc_create_thread increments that. So we need to call svc_destroy + * on both success and failure so that the refcount is 1 when the + * thread exits. + */ + if (serv) + svc_destroy(serv); mutex_unlock(&nfs_callback_mutex); unlock_kernel(); return ret; -out_destroy: +out_err: dprintk("Couldn't create callback socket or server thread; err = %d\n", ret); - svc_destroy(serv); -out_err: nfs_callback_info.users--; goto out; } -- cgit v1.2.3 From 497799e7c0ac7e82164a510ebf8beed7b3635e34 Mon Sep 17 00:00:00 2001 From: Dan Muntz Date: Wed, 13 Feb 2008 13:09:35 -0800 Subject: NFS: missing spaces in KERN_WARNING The warning message for a v4 server returning various bad sequence-ids is missing spaces. Signed-off-by: Dan Muntz Signed-off-by: Trond Myklebust --- fs/nfs/nfs4state.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'fs/nfs') diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c index f9c7432471d..6233eb5e98c 100644 --- a/fs/nfs/nfs4state.c +++ b/fs/nfs/nfs4state.c @@ -682,8 +682,8 @@ static void nfs_increment_seqid(int status, struct nfs_seqid *seqid) if (seqid->sequence->flags & NFS_SEQID_CONFIRMED) return; printk(KERN_WARNING "NFS: v4 server returned a bad" - "sequence-id error on an" - "unconfirmed sequence %p!\n", + " sequence-id error on an" + " unconfirmed sequence %p!\n", seqid->sequence); case -NFS4ERR_STALE_CLIENTID: case -NFS4ERR_STALE_STATEID: -- cgit v1.2.3 From 25606656b19a38bbece914c4c67101f674908f49 Mon Sep 17 00:00:00 2001 From: Jeff Layton Date: Tue, 12 Feb 2008 06:49:01 -0500 Subject: NFS: remove error field from nfs_readdir_descriptor_t The error field in nfs_readdir_descriptor_t is never used outside of the function in which it is set. Remove the field and change the place that does use it to use an existing local variable. Signed-off-by: Jeff Layton Signed-off-by: Trond Myklebust --- fs/nfs/dir.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) (limited to 'fs/nfs') diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c index 476cb0f837f..ae04892a5e5 100644 --- a/fs/nfs/dir.c +++ b/fs/nfs/dir.c @@ -154,7 +154,6 @@ typedef struct { struct nfs_entry *entry; decode_dirent_t decode; int plus; - int error; unsigned long timestamp; int timestamp_valid; } nfs_readdir_descriptor_t; @@ -213,7 +212,6 @@ int nfs_readdir_filler(nfs_readdir_descriptor_t *desc, struct page *page) return 0; error: unlock_page(page); - desc->error = error; return -EIO; } @@ -483,13 +481,13 @@ int uncached_readdir(nfs_readdir_descriptor_t *desc, void *dirent, goto out; } timestamp = jiffies; - desc->error = NFS_PROTO(inode)->readdir(file->f_path.dentry, cred, *desc->dir_cookie, - page, + status = NFS_PROTO(inode)->readdir(file->f_path.dentry, cred, + *desc->dir_cookie, page, NFS_SERVER(inode)->dtsize, desc->plus); desc->page = page; desc->ptr = kmap(page); /* matching kunmap in nfs_do_filldir */ - if (desc->error >= 0) { + if (status >= 0) { desc->timestamp = timestamp; desc->timestamp_valid = 1; if ((status = dir_decode(desc)) == 0) -- cgit v1.2.3 From 8d042218b075de3cdbe066198515b3521553746e Mon Sep 17 00:00:00 2001 From: Olga Kornievskaia Date: Wed, 13 Feb 2008 16:47:06 -0500 Subject: NFS: add missing spkm3 strings to mount option parser This patch adds previous missing spkm3 string values that are needed to parse mount options in the kernel. --- fs/nfs/super.c | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'fs/nfs') diff --git a/fs/nfs/super.c b/fs/nfs/super.c index 7f4505f6ac6..1fb38184365 100644 --- a/fs/nfs/super.c +++ b/fs/nfs/super.c @@ -190,6 +190,10 @@ static match_table_t nfs_secflavor_tokens = { { Opt_sec_lkeyi, "lkeyi" }, { Opt_sec_lkeyp, "lkeyp" }, + { Opt_sec_spkm, "spkm3" }, + { Opt_sec_spkmi, "spkm3i" }, + { Opt_sec_spkmp, "spkm3p" }, + { Opt_sec_err, NULL } }; -- cgit v1.2.3 From 4ac9137858e08a19f29feac4e1f4df7c268b0ba5 Mon Sep 17 00:00:00 2001 From: Jan Blunck Date: Thu, 14 Feb 2008 19:34:32 -0800 Subject: Embed a struct path into struct nameidata instead of nd->{dentry,mnt} This is the central patch of a cleanup series. In most cases there is no good reason why someone would want to use a dentry for itself. This series reflects that fact and embeds a struct path into nameidata. Together with the other patches of this series - it enforced the correct order of getting/releasing the reference count on pairs - it prepares the VFS for stacking support since it is essential to have a struct path in every place where the stack can be traversed - it reduces the overall code size: without patch series: text data bss dec hex filename 5321639 858418 715768 6895825 6938d1 vmlinux with patch series: text data bss dec hex filename 5320026 858418 715768 6894212 693284 vmlinux This patch: Switch from nd->{dentry,mnt} to nd->path.{dentry,mnt} everywhere. [akpm@linux-foundation.org: coding-style fixes] [akpm@linux-foundation.org: fix cifs] [akpm@linux-foundation.org: fix smack] Signed-off-by: Jan Blunck Signed-off-by: Andreas Gruenbacher Acked-by: Christoph Hellwig Cc: Al Viro Cc: Casey Schaufler Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- fs/nfs/namespace.c | 27 +++++++++++++++------------ fs/nfs/nfs4proc.c | 8 ++++---- 2 files changed, 19 insertions(+), 16 deletions(-) (limited to 'fs/nfs') diff --git a/fs/nfs/namespace.c b/fs/nfs/namespace.c index be4ce1c3a3d..3b6d83dc98a 100644 --- a/fs/nfs/namespace.c +++ b/fs/nfs/namespace.c @@ -107,38 +107,40 @@ static void * nfs_follow_mountpoint(struct dentry *dentry, struct nameidata *nd) BUG_ON(IS_ROOT(dentry)); dprintk("%s: enter\n", __FUNCTION__); - dput(nd->dentry); - nd->dentry = dget(dentry); + dput(nd->path.dentry); + nd->path.dentry = dget(dentry); /* Look it up again */ - parent = dget_parent(nd->dentry); + parent = dget_parent(nd->path.dentry); err = server->nfs_client->rpc_ops->lookup(parent->d_inode, - &nd->dentry->d_name, + &nd->path.dentry->d_name, &fh, &fattr); dput(parent); if (err != 0) goto out_err; if (fattr.valid & NFS_ATTR_FATTR_V4_REFERRAL) - mnt = nfs_do_refmount(nd->mnt, nd->dentry); + mnt = nfs_do_refmount(nd->path.mnt, nd->path.dentry); else - mnt = nfs_do_submount(nd->mnt, nd->dentry, &fh, &fattr); + mnt = nfs_do_submount(nd->path.mnt, nd->path.dentry, &fh, + &fattr); err = PTR_ERR(mnt); if (IS_ERR(mnt)) goto out_err; mntget(mnt); - err = do_add_mount(mnt, nd, nd->mnt->mnt_flags|MNT_SHRINKABLE, &nfs_automount_list); + err = do_add_mount(mnt, nd, nd->path.mnt->mnt_flags|MNT_SHRINKABLE, + &nfs_automount_list); if (err < 0) { mntput(mnt); if (err == -EBUSY) goto out_follow; goto out_err; } - mntput(nd->mnt); - dput(nd->dentry); - nd->mnt = mnt; - nd->dentry = dget(mnt->mnt_root); + mntput(nd->path.mnt); + dput(nd->path.dentry); + nd->path.mnt = mnt; + nd->path.dentry = dget(mnt->mnt_root); schedule_delayed_work(&nfs_automount_task, nfs_mountpoint_expiry_timeout); out: dprintk("%s: done, returned %d\n", __FUNCTION__, err); @@ -149,7 +151,8 @@ out_err: path_release(nd); goto out; out_follow: - while(d_mountpoint(nd->dentry) && follow_down(&nd->mnt, &nd->dentry)) + while (d_mountpoint(nd->path.dentry) && + follow_down(&nd->path.mnt, &nd->path.dentry)) ; err = 0; goto out; diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c index 027e1095256..7ce07862c2f 100644 --- a/fs/nfs/nfs4proc.c +++ b/fs/nfs/nfs4proc.c @@ -1384,11 +1384,11 @@ out_close: struct dentry * nfs4_atomic_open(struct inode *dir, struct dentry *dentry, struct nameidata *nd) { - struct dentry *parent; struct path path = { - .mnt = nd->mnt, + .mnt = nd->path.mnt, .dentry = dentry, }; + struct dentry *parent; struct iattr attr; struct rpc_cred *cred; struct nfs4_state *state; @@ -1433,7 +1433,7 @@ int nfs4_open_revalidate(struct inode *dir, struct dentry *dentry, int openflags, struct nameidata *nd) { struct path path = { - .mnt = nd->mnt, + .mnt = nd->path.mnt, .dentry = dentry, }; struct rpc_cred *cred; @@ -1885,7 +1885,7 @@ nfs4_proc_create(struct inode *dir, struct dentry *dentry, struct iattr *sattr, int flags, struct nameidata *nd) { struct path path = { - .mnt = nd->mnt, + .mnt = nd->path.mnt, .dentry = dentry, }; struct nfs4_state *state; -- cgit v1.2.3 From 1d957f9bf87da74f420424d16ece005202bbebd3 Mon Sep 17 00:00:00 2001 From: Jan Blunck Date: Thu, 14 Feb 2008 19:34:35 -0800 Subject: Introduce path_put() * Add path_put() functions for releasing a reference to the dentry and vfsmount of a struct path in the right order * Switch from path_release(nd) to path_put(&nd->path) * Rename dput_path() to path_put_conditional() [akpm@linux-foundation.org: fix cifs] Signed-off-by: Jan Blunck Signed-off-by: Andreas Gruenbacher Acked-by: Christoph Hellwig Cc: Cc: Al Viro Cc: Steven French Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- fs/nfs/namespace.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'fs/nfs') diff --git a/fs/nfs/namespace.c b/fs/nfs/namespace.c index 3b6d83dc98a..607f6eb9cdb 100644 --- a/fs/nfs/namespace.c +++ b/fs/nfs/namespace.c @@ -148,7 +148,7 @@ out: dprintk("<-- nfs_follow_mountpoint() = %d\n", err); return ERR_PTR(err); out_err: - path_release(nd); + path_put(&nd->path); goto out; out_follow: while (d_mountpoint(nd->path.dentry) && -- cgit v1.2.3 From 1227a74e2e0217a4ca155d1677bdbf5f69e32bed Mon Sep 17 00:00:00 2001 From: Jeff Layton Date: Tue, 19 Feb 2008 12:51:35 -0500 Subject: NFS: flush signals before taking down callback thread Now that the reference counting on the callback thread is working as expected, it uncovers another problem. Peter Staubach noticed while testing that patch on an older kernel that he would occasionally see this printk in rpc_register fire: "RPC: failed to contact portmap (errno -512). The NFSv4 callback thread is signaled by nfs_callback_down(), but never flushes that signal. All of the shutdown processing is done with that signal pending. This makes it fail the call to unregister the port with the portmapper. In actuality, this rpc_register call isn't necessary at all since the port isn't actually registered with the portmapper anymore. Regardless, there doesn't seem to be any reason to leave the signal pending while the thread is being shut down and flushing it should generally silence that printk. Signed-off-by: Jeff Layton Signed-off-by: Trond Myklebust --- fs/nfs/callback.c | 1 + 1 file changed, 1 insertion(+) (limited to 'fs/nfs') diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c index ecc06c61949..0d784f6f609 100644 --- a/fs/nfs/callback.c +++ b/fs/nfs/callback.c @@ -93,6 +93,7 @@ static void nfs_callback_svc(struct svc_rqst *rqstp) svc_process(rqstp); } + flush_signals(current); svc_exit_thread(rqstp); nfs_callback_info.pid = 0; complete(&nfs_callback_info.stopped); -- cgit v1.2.3 From 90dc7d2796edf94a9eaa838321a9734c8513e717 Mon Sep 17 00:00:00 2001 From: Harvey Harrison Date: Wed, 20 Feb 2008 13:03:05 -0800 Subject: nfs: fix sparse warnings fs/nfs/nfs4state.c:788:34: warning: Using plain integer as NULL pointer fs/nfs/delegation.c:52:34: warning: Using plain integer as NULL pointer fs/nfs/idmap.c:312:12: warning: Using plain integer as NULL pointer fs/nfs/callback_xdr.c:257:6: warning: Using plain integer as NULL pointer fs/nfs/callback_xdr.c:270:6: warning: Using plain integer as NULL pointer fs/nfs/callback_xdr.c:281:6: warning: Using plain integer as NULL pointer Signed-off-by: Harvey Harrison Signed-off-by: Trond Myklebust --- fs/nfs/callback_xdr.c | 6 +++--- fs/nfs/delegation.c | 2 +- fs/nfs/idmap.c | 2 +- fs/nfs/nfs4state.c | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) (limited to 'fs/nfs') diff --git a/fs/nfs/callback_xdr.c b/fs/nfs/callback_xdr.c index c63eb720b68..13619d24f02 100644 --- a/fs/nfs/callback_xdr.c +++ b/fs/nfs/callback_xdr.c @@ -254,7 +254,7 @@ static __be32 encode_attr_change(struct xdr_stream *xdr, const uint32_t *bitmap, if (!(bitmap[0] & FATTR4_WORD0_CHANGE)) return 0; p = xdr_reserve_space(xdr, 8); - if (unlikely(p == 0)) + if (unlikely(!p)) return htonl(NFS4ERR_RESOURCE); p = xdr_encode_hyper(p, change); return 0; @@ -267,7 +267,7 @@ static __be32 encode_attr_size(struct xdr_stream *xdr, const uint32_t *bitmap, u if (!(bitmap[0] & FATTR4_WORD0_SIZE)) return 0; p = xdr_reserve_space(xdr, 8); - if (unlikely(p == 0)) + if (unlikely(!p)) return htonl(NFS4ERR_RESOURCE); p = xdr_encode_hyper(p, size); return 0; @@ -278,7 +278,7 @@ static __be32 encode_attr_time(struct xdr_stream *xdr, const struct timespec *ti __be32 *p; p = xdr_reserve_space(xdr, 12); - if (unlikely(p == 0)) + if (unlikely(!p)) return htonl(NFS4ERR_RESOURCE); p = xdr_encode_hyper(p, time->tv_sec); *p = htonl(time->tv_nsec); diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c index b9eadd18ba7..00a5e4405e1 100644 --- a/fs/nfs/delegation.c +++ b/fs/nfs/delegation.c @@ -49,7 +49,7 @@ static int nfs_delegation_claim_locks(struct nfs_open_context *ctx, struct nfs4_ struct file_lock *fl; int status; - for (fl = inode->i_flock; fl != 0; fl = fl->fl_next) { + for (fl = inode->i_flock; fl != NULL; fl = fl->fl_next) { if (!(fl->fl_flags & (FL_POSIX|FL_FLOCK))) continue; if (nfs_file_open_context(fl->fl_file) != ctx) diff --git a/fs/nfs/idmap.c b/fs/nfs/idmap.c index 8ae5dba2d4e..86147b0ab2c 100644 --- a/fs/nfs/idmap.c +++ b/fs/nfs/idmap.c @@ -309,7 +309,7 @@ nfs_idmap_name(struct idmap *idmap, struct idmap_hashtable *h, mutex_lock(&idmap->idmap_im_lock); he = idmap_lookup_id(h, id); - if (he != 0) { + if (he) { memcpy(name, he->ih_name, he->ih_namelen); ret = he->ih_namelen; goto out; diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c index 6233eb5e98c..b962397004c 100644 --- a/fs/nfs/nfs4state.c +++ b/fs/nfs/nfs4state.c @@ -785,7 +785,7 @@ static int nfs4_reclaim_locks(struct nfs4_state_recovery_ops *ops, struct nfs4_s struct file_lock *fl; int status = 0; - for (fl = inode->i_flock; fl != 0; fl = fl->fl_next) { + for (fl = inode->i_flock; fl != NULL; fl = fl->fl_next) { if (!(fl->fl_flags & (FL_POSIX|FL_FLOCK))) continue; if (nfs_file_open_context(fl->fl_file)->state != state) -- cgit v1.2.3 From 5216a8e70e25b01cbd2915cd0442fb96deb2c262 Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Thu, 21 Feb 2008 10:57:45 +0300 Subject: Wrap buffers used for rpc debug printks into RPC_IFDEBUG Sorry for the noise, but here's the v3 of this compilation fix :) There are some places, which declare the char buf[...] on the stack to push it later into dprintk(). Since the dprintk sometimes (if the CONFIG_SYSCTL=n) becomes an empty do { } while (0) stub, these buffers cause gcc to produce appropriate warnings. Wrap these buffers with RPC_IFDEBUG macro, as Trond proposed, to compile them out when not needed. Signed-off-by: Pavel Emelyanov Acked-by: J. Bruce Fields Signed-off-by: Trond Myklebust --- fs/nfs/callback.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'fs/nfs') diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c index 0d784f6f609..66648dd92d9 100644 --- a/fs/nfs/callback.c +++ b/fs/nfs/callback.c @@ -172,7 +172,7 @@ void nfs_callback_down(void) static int nfs_callback_authenticate(struct svc_rqst *rqstp) { struct nfs_client *clp; - char buf[RPC_MAX_ADDRBUFLEN]; + RPC_IFDEBUG(char buf[RPC_MAX_ADDRBUFLEN]); /* Don't talk to strangers */ clp = nfs_find_client(svc_addr(rqstp), 4); -- cgit v1.2.3