Attachment 317001 Details for Bug 446396 – patch -- handle nfs4 incomplete opens more comprehensively

[patch] patch -- handle nfs4 incomplete opens more comprehensively

27-bz-446396-nfs4-handle-incomple.patch (text/plain), 14.16 KB, created by Jeff Layton on 2008-09-17 19:04:33 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Jeff Layton

Created: 2008-09-17 19:04:33 UTC

Size: 14.16 KB

patch

obsolete

>BZ#446396: NFS4: handle incomplete opens more comprehensively
>
>From: Jeff Layton <jlayton@redhat.com>
>
>RHEL4.6 had a patch for NFSv4 that tracked incomplete opens and cleaned
>them up (doing a nfs4_close_state) if a failed setattr call would cause
>open_namei to fail.
>
>This solution turns out not to be complete. I've gotten one core that
>shows a "busy inodes" problem, where there was still a dangling
>incomplete open. There are a lot of different reasons that open_namei
>can return error, and we need to try to catch more of them. Also,
>we weren't tracking incomplete opens in all of the places that they
>could be generated. In particular, the d_revalidate path was not
>covered.
>
>This patch takes a different approach. If open_namei fails after a
>successful lookup, then it calls a new inode operation that does an
>extra nfs4_close_state to fix up the refcounting. This seems to fix
>the problems I've seen.
>
>This patch piggybacks on the work that Peter Staubach did to add
>the extended inode infrastructure to RHEL4. It also also changes the
>S_INO64 macro to something named a bit more generically since the new
>extended inode op doesn't have anything to do with 64-bit inodes.
>---
>
> fs/exec.c              |    2 +
> fs/namei.c             |   18 ++++++++++-
> fs/nfs/dir.c           |    2 +
> fs/nfs/file.c          |    2 +
> fs/nfs/inode.c         |    4 +-
> fs/nfs/nfs4proc.c      |   82 ++++++++++++++----------------------------------
> fs/nfs/nfs4state.c     |    1 -
> fs/readdir.c           |    4 ++
> fs/stat.c              |    6 ++--
> include/linux/fs.h     |    5 ++-
> include/linux/namei.h  |    1 +
> include/linux/nfs_fs.h |    8 -----
> 12 files changed, 59 insertions(+), 76 deletions(-)
>
>
>diff --git a/fs/exec.c b/fs/exec.c
>index 00fe192..5b24301 100644
>--- a/fs/exec.c
>+++ b/fs/exec.c
>@@ -168,6 +168,7 @@ asmlinkage long sys_uselib(const char __user * library)
> out:
>   	return error;
> exit:
>+	do_lookup_cleanup(&nd);
> 	path_release(&nd);
> 	goto out;
> }
>@@ -510,6 +511,7 @@ out:
> 				return file;
> 			}
> 		}
>+		do_lookup_cleanup(&nd);
> 		path_release(&nd);
> 	}
> 	goto out;
>diff --git a/fs/namei.c b/fs/namei.c
>index f5cfd97..e7beadb 100644
>--- a/fs/namei.c
>+++ b/fs/namei.c
>@@ -1398,6 +1398,19 @@ int may_open(struct nameidata *nd, int acc_mode, int flag)
> 	return 0;
> }
> 
>+void
>+do_lookup_cleanup(struct nameidata *nd)
>+{
>+	struct inode *inode = nd->dentry->d_inode;
>+	struct inode_operations_ext *ixop;
>+
>+	if (inode && IS_INO_OP_EXT(inode)) {
>+		ixop = (struct inode_operations_ext *) inode->i_op;
>+		if (ixop->lookup_cleanup)
>+			ixop->lookup_cleanup(nd);
>+	}
>+}
>+
> /*
>  *	open_namei()
>  *
>@@ -1524,6 +1537,7 @@ ok:
> exit_dput:
> 	dput(dentry);
> exit:
>+	do_lookup_cleanup(nd);
> 	path_release(nd);
> 	return error;
> 
>@@ -1559,8 +1573,10 @@ do_link:
> 	dput(dentry);
> 	mntput(mnt);
> 
>-	if (error)
>+	if (error) {
>+		do_lookup_cleanup(nd);
> 		return error;
>+	}
> 	nd->flags &= ~LOOKUP_PARENT;
> 	if (nd->last_type == LAST_BIND) {
> 		dentry = nd->dentry;
>diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
>index 80ed2f5..78ed9fb 100644
>--- a/fs/nfs/dir.c
>+++ b/fs/nfs/dir.c
>@@ -132,6 +132,7 @@ struct file_operations_ext nfs4_dir_file_operations = {
> };
> 
> static struct dentry *nfs_atomic_lookup(struct inode *, struct dentry *, struct nameidata *);
>+void nfs4_lookup_cleanup(struct nameidata *);
> struct inode_operations_ext nfs4_dir_inode_operations = {
> 	.i_op_orig.create	= nfs_create,
> 	.i_op_orig.lookup	= nfs_atomic_lookup,
>@@ -146,6 +147,7 @@ struct inode_operations_ext nfs4_dir_inode_operations = {
> 	.i_op_orig.getattr	= nfs_getattr,
> 	.i_op_orig.setattr	= nfs_setattr,
> 	.getattr64		= nfs_getattr64,
>+	.lookup_cleanup		= nfs4_lookup_cleanup,
> };
> 
> #endif /* CONFIG_NFS_V4 */
>diff --git a/fs/nfs/file.c b/fs/nfs/file.c
>index da0008a..6c6f12a 100644
>--- a/fs/nfs/file.c
>+++ b/fs/nfs/file.c
>@@ -83,11 +83,13 @@ struct inode_operations_ext nfs3_file_inode_operations = {
> #endif  /* CONFIG_NFS_v3 */
> 
> #ifdef CONFIG_NFS_V4
>+void nfs4_lookup_cleanup(struct nameidata *);
> struct inode_operations_ext nfs4_file_inode_operations = {
> 	.i_op_orig.permission	= nfs_permission,
> 	.i_op_orig.getattr	= nfs_getattr,
> 	.i_op_orig.setattr	= nfs_setattr,
> 	.getattr64		= nfs_getattr64,
>+	.lookup_cleanup		= nfs4_lookup_cleanup,
> };
> #endif	/* CONFIG_NFS_v4 */
> 
>diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
>index e5a39d5..80bd783 100644
>--- a/fs/nfs/inode.c
>+++ b/fs/nfs/inode.c
>@@ -481,7 +481,7 @@ nfs_fill_super(struct super_block *sb, struct nfs_mount_data *data, int silent)
> 			printk(KERN_NOTICE "NFS: NFSv3 not supported by mount program.\n");
> 			return -EIO;
> 		}
>-		sb->s_flags |= MS_HAS_INO64;
>+		sb->s_flags |= MS_INO_OP_EXT;
> #else
> 		printk(KERN_NOTICE "NFS: NFSv3 not supported.\n");
> 		return -EIO;
>@@ -1961,7 +1961,7 @@ static int nfs4_fill_super(struct super_block *sb, struct nfs4_mount_data *data,
> 	}
> 
> 	sb->s_op = &nfs4_sops;
>-	sb->s_flags |= MS_HAS_INO64;
>+	sb->s_flags |= MS_INO_OP_EXT;
> 	err = nfs_sb_init(sb, authflavour);
> 	if (err == 0)
> 		return 0;
>diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>index fb7d986..78b796f 100644
>--- a/fs/nfs/nfs4proc.c
>+++ b/fs/nfs/nfs4proc.c
>@@ -638,35 +638,29 @@ struct nfs4_state *nfs4_do_open(struct inode *dir, struct qstr *name, int flags,
> }
> 
> /*
>- * NFSv4 has a complex open scheme where the actual OPEN call is done in the
>- * in the lookup, and the file pointer is initialized in the open. If the
>- * lookup completes successfully, but open_namei returns an error, then the
>- * nfs4_state is left "dangling". The obvious way for this to occur is if
>- * a setattr that's eventually called from open_namei returns an error. We
>- * start tracking this info in nfs4_atomic_open, and then remove the tracking
>- * here. If this returns true then a dangling open was found and removed. In
>- * error condition we'll want to do an extra nfs4_close_state based on this
>- * return value to clean up the dangling open.
>+ * open_namei is going to fail but we have a successful lookup. We need to
>+ * clean up the state so that we don't leave the inode dangling.
>  */
>-static int nfs4_remove_incomplete_open(struct nfs4_state *state,
>-					unsigned long *flags) {
>-	struct list_head	*entry;
>-	struct nfs4_inc_open	*inc_open;
>-
>-	spin_lock(&state->inode->i_lock);
>-	list_for_each(entry, &state->inc_open) {
>-		inc_open = list_entry(entry, struct nfs4_inc_open, state);
>-		if (inc_open->task == current) {
>-			list_del(&inc_open->state);
>-			spin_unlock(&state->inode->i_lock);
>-			if (flags)
>-				*flags = inc_open->flags;
>-			kfree(inc_open);
>-			return 1;
>-		}
>+void
>+nfs4_lookup_cleanup(struct nameidata *nd)
>+{
>+	struct inode *inode = nd->dentry->d_inode;
>+	struct rpc_cred	*cred;
>+	struct nfs4_state *state;
>+	int flags = nd->intent.open.flags & (FMODE_READ|FMODE_WRITE);
>+
>+	cred = rpcauth_lookupcred(NFS_SERVER(inode)->client->cl_auth, 0);
>+	state = nfs4_find_state(inode, cred, flags);
>+	put_rpccred(cred);
>+
>+	/*
>+	 * one close for the dangling open and one for the reference we
>+	 * just picked up from nfs4_find_state
>+	 */
>+	if (state) {
>+		nfs4_close_state(state, flags);
>+		nfs4_close_state(state, flags);
> 	}
>-	spin_unlock(&state->inode->i_lock);
>-	return 0;
> }
> 
> static int _nfs4_do_setattr(struct nfs_server *server, struct nfs_fattr *fattr,
>@@ -835,7 +829,6 @@ nfs4_atomic_open(struct inode *dir, struct dentry *dentry, struct nameidata *nd)
> 	struct iattr attr;
> 	struct rpc_cred *cred;
> 	struct nfs4_state *state;
>-	struct nfs4_inc_open *inc_open;
> 
> 	if (nd->flags & LOOKUP_CREATE) {
> 		attr.ia_mode = nd->intent.open.create_mode;
>@@ -847,22 +840,11 @@ nfs4_atomic_open(struct inode *dir, struct dentry *dentry, struct nameidata *nd)
> 		BUG_ON(nd->intent.open.flags & O_CREAT);
> 	}
> 
>-	/* track info in case the open never completes */
>-	if (!(inc_open = kmalloc(sizeof(*inc_open), GFP_KERNEL)))
>-		return ERR_PTR(-ENOMEM);
> 	cred = rpcauth_lookupcred(NFS_SERVER(dir)->client->cl_auth, 0);
> 	state = nfs4_do_open(dir, &dentry->d_name, nd->intent.open.flags, &attr, cred);
> 	put_rpccred(cred);
>-	if (IS_ERR(state)) {
>-		kfree(inc_open);
>+	if (IS_ERR(state))
> 		return (struct inode *)state;
>-	}
>-	inc_open->task = current;
>-	inc_open->flags = nd->intent.open.flags;
>-	INIT_LIST_HEAD(&inc_open->state);
>-	spin_lock(&state->inode->i_lock);
>-	list_add(&inc_open->state, &state->inc_open);
>-	spin_unlock(&state->inode->i_lock);
> 	return state->inode;
> }
> 
>@@ -1088,14 +1070,11 @@ nfs4_proc_setattr(struct dentry *dentry, struct nfs_fattr *fattr,
> 	struct nfs4_state	*state = NULL;
> 	int need_iput = 0;
> 	int status;
>-	unsigned long 		flags;
>-	struct rpc_cred 	*cred;
>-	mode_t			mode = 0;
> 
> 	nfs_fattr_init(fattr);
> 	
> 	if (size_change) {
>-		cred = rpcauth_lookupcred(NFS_SERVER(inode)->client->cl_auth, 0);
>+		struct rpc_cred *cred = rpcauth_lookupcred(NFS_SERVER(inode)->client->cl_auth, 0);
> 		state = nfs4_find_state(inode, cred, FMODE_WRITE);
> 		if (state == NULL) {
> 			state = nfs4_open_delegated(dentry->d_inode,
>@@ -1106,7 +1085,6 @@ nfs4_proc_setattr(struct dentry *dentry, struct nfs_fattr *fattr,
> 						NULL, cred);
> 			need_iput = 1;
> 		}
>-		mode = FMODE_WRITE;
> 		put_rpccred(cred);
> 		if (IS_ERR(state))
> 			return PTR_ERR(state);
>@@ -1122,20 +1100,9 @@ nfs4_proc_setattr(struct dentry *dentry, struct nfs_fattr *fattr,
> 	if (status == 0)
> 		nfs_setattr_update_inode(inode, sattr);
> out:
>-	/* if we're erroring out, clean up incomplete open (if any) */
>-	if (status != 0) {
>-		if (state == NULL) {
>-			cred = rpcauth_lookupcred(NFS_SERVER(inode)->client->cl_auth, 0);
>-			state = nfs4_find_state(inode, cred, 0);
>-			put_rpccred(cred);
>-		}
>-		if (state && nfs4_remove_incomplete_open(state, &flags))
>-			nfs4_close_state(state, flags);
>-	}
>-
> 	if (state) {
> 		inode = state->inode;
>-		nfs4_close_state(state, mode);
>+		nfs4_close_state(state, FMODE_WRITE);
> 		if (need_iput)
> 			iput(inode);
> 	}
>@@ -2134,7 +2101,6 @@ nfs4_proc_file_open(struct inode *inode, struct file *filp)
> 	state = nfs4_find_state(inode, cred, filp->f_mode);
> 	if (unlikely(state == NULL))
> 		goto no_state;
>-	nfs4_remove_incomplete_open(state, NULL);
> 	ctx->state = state;
> 	nfs4_close_state(state, filp->f_mode);
> 	ctx->mode = filp->f_mode;
>diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
>index a29f8e1..3057764 100644
>--- a/fs/nfs/nfs4state.c
>+++ b/fs/nfs/nfs4state.c
>@@ -403,7 +403,6 @@ nfs4_alloc_open_state(void)
> 	memset(state->stateid.data, 0, sizeof(state->stateid.data));
> 	atomic_set(&state->count, 1);
> 	INIT_LIST_HEAD(&state->lock_states);
>-	INIT_LIST_HEAD(&state->inc_open);
> 	init_MUTEX(&state->lock_sema);
> 	rwlock_init(&state->state_lock);
> 	return state;
>diff --git a/fs/readdir.c b/fs/readdir.c
>index d5e7909..bd81386 100644
>--- a/fs/readdir.c
>+++ b/fs/readdir.c
>@@ -53,7 +53,7 @@ int vfs_readdir64(struct file *file, filldir64_t filler, void *buf)
> 		goto out;
> 
> 	res = -ENOSYS;
>-	if (!IS_INO64(inode))
>+	if (!IS_INO_OP_EXT(inode))
> 		goto out;
> 
> 	res = security_file_permission(file, MAY_READ);
>@@ -61,6 +61,8 @@ int vfs_readdir64(struct file *file, filldir64_t filler, void *buf)
> 		goto out;
> 
> 	fxops = (struct file_operations_ext *) file->f_op;
>+	if (!fxops->readdir64)
>+		goto out;
> 
> 	down(&inode->i_sem);
> 	res = -ENOENT;
>diff --git a/fs/stat.c b/fs/stat.c
>index cd7e80c..630c0ea 100644
>--- a/fs/stat.c
>+++ b/fs/stat.c
>@@ -72,11 +72,11 @@ int vfs_getattr64(struct vfsmount *mnt, struct dentry *dentry, struct kstat64 *s
> 		return retval;
> 
> 	/* retrieve a 64-bit inode number if possible */
>-	if (IS_INO64(inode)) {
>+	if (IS_INO_OP_EXT(inode)) {
> 		struct inode_operations_ext *ixop =
> 			(struct inode_operations_ext *) inode->i_op;
>-
>-		return ixop->getattr64(mnt, dentry, stat);
>+		if (ixop->getattr64)
>+			return ixop->getattr64(mnt, dentry, stat);
> 	}
> 
> 	if (inode->i_op->getattr) {
>diff --git a/include/linux/fs.h b/include/linux/fs.h
>index 28baf1f..8ffce47 100644
>--- a/include/linux/fs.h
>+++ b/include/linux/fs.h
>@@ -128,7 +128,7 @@ extern int leases_enable, dir_notify_enable, lease_break_time;
> #define MS_ONE_SECOND	(1<<17)	/* fs has 1 sec time resolution (obsolete) */
> #define MS_TIME_GRAN	(1<<18)	/* fs has s_time_gran field */
> #define MS_NO_LEASES	(1<<19)	/* fs does not support leases */
>-#define MS_HAS_INO64	(1<<29)	/* has 64-bit inode numbers */
>+#define MS_INO_OP_EXT	(1<<29)	/* has extended inode operations */
> #define MS_ACTIVE	(1<<30)
> #define MS_NOUSER	(1<<31)
> 
>@@ -188,7 +188,7 @@ extern int leases_enable, dir_notify_enable, lease_break_time;
> #define IS_NODIRATIME(inode)	__IS_FLG(inode, MS_NODIRATIME)
> #define IS_POSIXACL(inode)	__IS_FLG(inode, MS_POSIXACL)
> #define IS_ONE_SECOND(inode)	__IS_FLG(inode, MS_ONE_SECOND)
>-#define IS_INO64(inode)		__IS_FLG(inode, MS_HAS_INO64)
>+#define IS_INO_OP_EXT(inode)	__IS_FLG(inode, MS_INO_OP_EXT)
> 
> #define IS_DEADDIR(inode)	((inode)->i_flags & S_DEAD)
> #define IS_NOCMTIME(inode)	((inode)->i_flags & S_NOCMTIME)
>@@ -997,6 +997,7 @@ struct inode_operations_ext {
> 	
> 	/* get attributes with 64-bit inode number if MS_HAS_INO64 is set */
> 	int (*getattr64)(struct vfsmount *mnt, struct dentry *, struct kstat64 *);
>+	void (*lookup_cleanup)(struct nameidata *);
> };
> 
> extern ssize_t vfs_read(struct file *, char __user *, size_t, loff_t *);
>diff --git a/include/linux/namei.h b/include/linux/namei.h
>index e1ce8dd..ed72471 100644
>--- a/include/linux/namei.h
>+++ b/include/linux/namei.h
>@@ -65,6 +65,7 @@ extern int FASTCALL(path_walk(const char *, struct nameidata *));
> extern int FASTCALL(link_path_walk(const char *, struct nameidata *));
> extern void path_release(struct nameidata *);
> extern void path_release_on_umount(struct nameidata *);
>+extern void do_lookup_cleanup(struct nameidata *nd);
> 
> extern struct dentry * lookup_one_len(const char *, struct dentry *, int);
> extern struct dentry * lookup_hash(struct qstr *, struct dentry *);
>diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
>index 2a6f3d7..20fd931 100644
>--- a/include/linux/nfs_fs.h
>+++ b/include/linux/nfs_fs.h
>@@ -771,18 +771,10 @@ enum {
> 	NFS_DELEGATED_STATE,
> };
> 
>-/* struct for tracking incomplete opens */
>-struct nfs4_inc_open {
>-	struct list_head	state;
>-	struct task_struct	*task;
>-	unsigned long		flags;
>-};
>-
> struct nfs4_state {
> 	struct list_head open_states;	/* List of states for the same state_owner */
> 	struct list_head inode_states;	/* List of states for the same inode */
> 	struct list_head lock_states;	/* List of subservient lock stateids */
>-	struct list_head inc_open;	/* List of incomplete opens */
> 
> 	struct nfs4_state_owner *owner;	/* Pointer to the open owner */
> 	struct inode *inode;		/* Pointer to the inode */

Actions: View | Diff

Attachments on bug 446396: 309745 | 315673 | 317001 | 317484