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

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

0001-BZ-446396-NFS4-handle-incomplete-opens-more-compre.patch (text/plain), 10.49 KB, created by Jeff Layton on 2008-06-18 15:21:44 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Jeff Layton

Created: 2008-06-18 15:21:44 UTC

Size: 10.49 KB

patch

obsolete

>From 51c360d16b4814068bc80ec63afcdca277f31e08 Mon Sep 17 00:00:00 2001
>From: Jeff Layton <jlayton@redhat.com>
>Date: Wed, 18 Jun 2008 11:13:39 -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 need to try to catch more of them
>
>This patch attempts to do this in a more complete fashion. It adds a
>new inode_operation_ext field, and uses that to implement a routine
>to clean up an incomplete open call.
>
>The patch 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/namei.c             |   18 ++++++++++++++++-
> fs/nfs/dir.c           |    2 +
> fs/nfs/file.c          |    2 +
> fs/nfs/inode.c         |    4 +-
> fs/nfs/nfs4proc.c      |   49 +++++++++++++++++++++++++----------------------
> fs/readdir.c           |    2 +-
> fs/stat.c              |    2 +-
> include/linux/fs.h     |    5 ++-
> include/linux/nfs_fs.h |    1 -
> 9 files changed, 54 insertions(+), 31 deletions(-)
>
>diff --git a/fs/namei.c b/fs/namei.c
>index 479ca22..a8fe110 100644
>--- a/fs/namei.c
>+++ b/fs/namei.c
>@@ -1396,6 +1396,19 @@ int may_open(struct nameidata *nd, int acc_mode, int flag)
> 	return 0;
> }
> 
>+static 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()
>  *
>@@ -1522,6 +1535,7 @@ ok:
> exit_dput:
> 	dput(dentry);
> exit:
>+	do_lookup_cleanup(nd);
> 	path_release(nd);
> 	return error;
> 
>@@ -1557,8 +1571,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 bb60b33..9e3b0e0 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 5c33400..bff810b 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;
>@@ -2044,7 +2044,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 321fb37..5029099 100644
>--- a/fs/nfs/nfs4proc.c
>+++ b/fs/nfs/nfs4proc.c
>@@ -648,8 +648,7 @@ struct nfs4_state *nfs4_do_open(struct inode *dir, struct qstr *name, int flags,
>  * error condition we'll want to do an extra nfs4_close_state based on this
>  * return value to clean up the dangling open.
>  */
>-static int nfs4_remove_incomplete_open(struct nfs4_state *state,
>-					unsigned long *flags) {
>+static int nfs4_remove_incomplete_open(struct nfs4_state *state) {
> 	struct list_head	*entry;
> 	struct nfs4_inc_open	*inc_open;
> 
>@@ -659,8 +658,6 @@ static int nfs4_remove_incomplete_open(struct nfs4_state *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;
> 		}
>@@ -669,6 +666,28 @@ static int nfs4_remove_incomplete_open(struct nfs4_state *state,
> 	return 0;
> }
> 
>+/*
>+ * 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.
>+ */
>+void
>+nfs4_lookup_cleanup(struct nameidata *nd)
>+{
>+	struct inode *inode = nd->dentry->d_inode;
>+	struct rpc_cred	*cred;
>+	struct nfs4_state *state;
>+
>+	cred = rpcauth_lookupcred(NFS_SERVER(inode)->client->cl_auth, 0);
>+	state = nfs4_find_state(inode, cred, nd->intent.open.flags);
>+	put_rpccred(cred);
>+
>+	if (state) {
>+		if (nfs4_remove_incomplete_open(state))
>+			nfs4_close_state(state, nd->intent.open.flags);
>+		nfs4_close_state(state, nd->intent.open.flags);
>+	}
>+}
>+
> static int _nfs4_do_setattr(struct nfs_server *server, struct nfs_fattr *fattr,
>                 struct nfs_fh *fhandle, struct iattr *sattr,
>                 struct nfs4_state *state)
>@@ -858,7 +877,6 @@ nfs4_atomic_open(struct inode *dir, struct dentry *dentry, struct nameidata *nd)
> 		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);
>@@ -1088,14 +1106,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 +1121,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 +1136,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);
> 	}
>@@ -2147,7 +2150,7 @@ 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);
>+	nfs4_remove_incomplete_open(state);
> 	ctx->state = state;
> 	nfs4_close_state(state, filp->f_mode);
> 	ctx->mode = filp->f_mode;
>diff --git a/fs/readdir.c b/fs/readdir.c
>index d5e7909..9c441c6 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);
>diff --git a/fs/stat.c b/fs/stat.c
>index cd7e80c..860e263 100644
>--- a/fs/stat.c
>+++ b/fs/stat.c
>@@ -72,7 +72,7 @@ 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;
> 
>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/nfs_fs.h b/include/linux/nfs_fs.h
>index 870dc16..26b566c 100644
>--- a/include/linux/nfs_fs.h
>+++ b/include/linux/nfs_fs.h
>@@ -777,7 +777,6 @@ enum {
> struct nfs4_inc_open {
> 	struct list_head	state;
> 	struct task_struct	*task;
>-	unsigned long		flags;
> };
> 
> struct nfs4_state {
>-- 
>1.5.5.1
>

Actions: View | Diff

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