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

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

0001-BZ-446396-NFS4-handle-incomplete-opens-more-compre.patch (text/plain), 13.92 KB, created by Jeff Layton on 2008-09-23 15:50:51 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Jeff Layton

Created: 2008-09-23 15:50:51 UTC

Size: 13.92 KB

patch

obsolete

>From af8dd386e074c85d9f487d6fadbabaffb4d84b5f Mon Sep 17 00:00:00 2001
>From: Jeff Layton <jlayton@redhat.com>
>Date: Tue, 23 Sep 2008 11:38:19 -0400
>Subject: [PATCH] BZ#446396: NFS4: handle incomplete opens more comprehensively
>
>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 have to catch them all. 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 a file open is not done after
>a successful lookup with an intent to open, then it calls a new inode
>operation that does an extra nfs4_close_state to fix up the refcounting.
>
>While I don't like having to poke around in the generic VFS layer to fix
>this, I really don't see any other option. The way this was fixed
>upstream is a kABI-breaker (we'd have to add a field to the open_intent
>struct and hence the nameidata struct).
>
>This patch is slightly different from the last patch:
>
>- leaves the INO64 flags alone and declares a new flag for the
>  new inode op
>
>- adds an extra sanity check to the nfs4 lookup_undo function to
>  make sure that it's undoing a lookup with a valid open intent
>
>- renames the new inode op to "lookup_undo"
>
>- moves the prototype for nfs4_lookup_undo to a header file
>
>- some other minor formatting changes
>
>This seems to fix the reproducer I have. An earlier, similar patch was
>confirmed by one customer to fix the reproducer they had. I also have
>another panic reported against this BZ. I believe this patch will fix
>that as well, based on analysis of the core.
>---
> fs/exec.c              |    2 +
> fs/namei.c             |   17 +++++++++-
> fs/nfs/dir.c           |    3 +-
> fs/nfs/file.c          |    1 +
> fs/nfs/inode.c         |    2 +-
> fs/nfs/nfs4proc.c      |   85 +++++++++++++++--------------------------------
> fs/nfs/nfs4state.c     |    1 -
> include/linux/fs.h     |    3 ++
> include/linux/namei.h  |    1 +
> include/linux/nfs_fs.h |   10 +----
> 10 files changed, 55 insertions(+), 70 deletions(-)
>
>diff --git a/fs/exec.c b/fs/exec.c
>index 00fe192..3b0d83b 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_undo(&nd);
> 	path_release(&nd);
> 	goto out;
> }
>@@ -510,6 +511,7 @@ out:
> 				return file;
> 			}
> 		}
>+		do_lookup_undo(&nd);
> 		path_release(&nd);
> 	}
> 	goto out;
>diff --git a/fs/namei.c b/fs/namei.c
>index f5cfd97..51f49c3 100644
>--- a/fs/namei.c
>+++ b/fs/namei.c
>@@ -1398,6 +1398,18 @@ int may_open(struct nameidata *nd, int acc_mode, int flag)
> 	return 0;
> }
> 
>+void
>+do_lookup_undo(struct nameidata *nd)
>+{
>+	struct inode *inode = nd->dentry->d_inode;
>+	struct inode_operations_ext *ixop;
>+
>+	if (inode && IS_LOOKUP_UNDO(inode)) {
>+		ixop = (struct inode_operations_ext *) inode->i_op;
>+		ixop->lookup_undo(nd);
>+	}
>+}
>+
> /*
>  *	open_namei()
>  *
>@@ -1524,6 +1536,7 @@ ok:
> exit_dput:
> 	dput(dentry);
> exit:
>+	do_lookup_undo(nd);
> 	path_release(nd);
> 	return error;
> 
>@@ -1559,8 +1572,10 @@ do_link:
> 	dput(dentry);
> 	mntput(mnt);
> 
>-	if (error)
>+	if (error) {
>+		do_lookup_undo(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..57549b9 100644
>--- a/fs/nfs/dir.c
>+++ b/fs/nfs/dir.c
>@@ -146,6 +146,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_undo		= nfs4_lookup_undo,
> };
> 
> #endif /* CONFIG_NFS_V4 */
>@@ -1021,7 +1022,7 @@ struct dentry_operations nfs4_dentry_operations = {
> 	.d_iput		= nfs_dentry_iput,
> };
> 
>-static int is_atomic_open(struct inode *dir, struct nameidata *nd)
>+int is_atomic_open(struct inode *dir, struct nameidata *nd)
> {
> 	if (!nd)
> 		return 0;
>diff --git a/fs/nfs/file.c b/fs/nfs/file.c
>index da0008a..d6a03c7 100644
>--- a/fs/nfs/file.c
>+++ b/fs/nfs/file.c
>@@ -88,6 +88,7 @@ struct inode_operations_ext nfs4_file_inode_operations = {
> 	.i_op_orig.getattr	= nfs_getattr,
> 	.i_op_orig.setattr	= nfs_setattr,
> 	.getattr64		= nfs_getattr64,
>+	.lookup_undo		= nfs4_lookup_undo,
> };
> #endif	/* CONFIG_NFS_v4 */
> 
>diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
>index e5a39d5..f7dcf53 100644
>--- a/fs/nfs/inode.c
>+++ b/fs/nfs/inode.c
>@@ -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_HAS_INO64 | MS_LOOKUP_UNDO;
> 	err = nfs_sb_init(sb, authflavour);
> 	if (err == 0)
> 		return 0;
>diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>index 6259730..135b1dc 100644
>--- a/fs/nfs/nfs4proc.c
>+++ b/fs/nfs/nfs4proc.c
>@@ -639,35 +639,32 @@ 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_undo(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);
>+
>+	/* sanity check: don't do anything if not atomic open */
>+	if (!is_atomic_open(nd->dentry->d_parent->d_inode, nd))
>+		return;
>+
>+	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,
>@@ -836,7 +833,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;
>@@ -848,22 +844,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;
> }
> 
>@@ -1089,14 +1074,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,
>@@ -1107,7 +1089,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);
>@@ -1123,20 +1104,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);
> 	}
>@@ -2135,7 +2105,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 17de877..cc5de53 100644
>--- a/fs/nfs/nfs4state.c
>+++ b/fs/nfs/nfs4state.c
>@@ -404,7 +404,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/include/linux/fs.h b/include/linux/fs.h
>index 28baf1f..4b01df6 100644
>--- a/include/linux/fs.h
>+++ b/include/linux/fs.h
>@@ -128,6 +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_LOOKUP_UNDO	(1<<28)	/* must cleanup after open-intent lookup */
> #define MS_HAS_INO64	(1<<29)	/* has 64-bit inode numbers */
> #define MS_ACTIVE	(1<<30)
> #define MS_NOUSER	(1<<31)
>@@ -189,6 +190,7 @@ extern int leases_enable, dir_notify_enable, lease_break_time;
> #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_LOOKUP_UNDO(inode)	__IS_FLG(inode, MS_LOOKUP_UNDO)
> 
> #define IS_DEADDIR(inode)	((inode)->i_flags & S_DEAD)
> #define IS_NOCMTIME(inode)	((inode)->i_flags & S_NOCMTIME)
>@@ -997,6 +999,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_undo)(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..2e33d58 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_undo(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 32fc9fe..6899f7a 100644
>--- a/include/linux/nfs_fs.h
>+++ b/include/linux/nfs_fs.h
>@@ -420,6 +420,7 @@ extern struct file_operations_ext nfs3_dir_file_operations;
> extern struct dentry_operations nfs_dentry_operations;
> 
> extern int nfs_instantiate(struct dentry *dentry, struct nfs_fh *fh, struct nfs_fattr *fattr);
>+extern int is_atomic_open(struct inode *dir, struct nameidata *nd);
> 
> /*
>  * linux/fs/nfs/symlink.c
>@@ -775,18 +776,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 */
>@@ -828,6 +821,7 @@ extern struct inode *nfs4_atomic_open(struct inode *, struct dentry *, struct na
> extern int nfs4_open_revalidate(struct inode *, struct dentry *, int);
> extern int nfs4_handle_exception(struct nfs_server *, int, struct nfs4_exception *);
> extern int nfs4_lock_reclaim(struct nfs4_state *state, struct file_lock *request);
>+extern void nfs4_lookup_undo(struct nameidata *nd);
> 
> /* nfs4renewd.c */
> extern void nfs4_schedule_state_renewal(struct nfs4_client *);
>-- 
>1.5.5.1
>

Actions: View | Diff

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