Attachment 152181 Details for Bug 234587 – patch -- track incomplete opens and clean up in setattr

[patch] patch -- track incomplete opens and clean up in setattr

03-nfs4-incomplete-open.patch (text/plain), 7.86 KB, created by Jeff Layton on 2007-04-10 21:06:05 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Jeff Layton

Created: 2007-04-10 21:06:05 UTC

Size: 7.86 KB

patch

obsolete

>From: Jeff Layton <jlayton@redhat.com>
>Subject: [RHEL4.6 PATCH] NFSv4: prevent "busy inodes" left when process is signalled in the middle of an open call (bz 234587)
>
>This is the respin of the patch I posted earlier and retracted.
>
>When I posted the patch for bz 209419, I mentioned that it had traded the
>BUG() in nfs4_clear_inode for a "VFS: busy inodes at umount" problem. The
>issue is this:
>
>Because of the lack of an atomic open in the VFS layer, RHEL4's NFSv4 has a
>somewhat odd scheme for handling file opens. The actual NFSv4 OPEN call is
>done via the lookup dir inode_operation, and the actual open file_operation
>just hooks up the already opened file to a file descriptor and cleans up the
>reference counts. The lookup is done via a call chain something like this:
>
>filp_open
>  open_namei
>    __lookup_hash
>      i_op->lookup (nfs_atomic_lookup)
>         nfs4_atomic_open
>           nfs4_do_open
>              _nfs4_do_open
>                   nfs4_get_open_state
>
>...and the open call chain is something like this:
>
>filp_open
>  __dentry_open
>     f_op->open  (nfs_file_open)
>       rpc_ops->file_open (nfs4_proc_file_open)
>          nfs4_close_state
>             nfs4_put_open_state
>
>Both call chains above need to be done, or we leave a dangling reference
>count on the nfs4_state. If the file is opened with O_TRUNC, then there's
>another call chain to do a SETATTR to resize the file and set the mtime. That
>call chain is intiated in open_namei after the lookup. If the process is
>signalled while the SETATTR RPC call is in progress, then it will pass an
>error (-ERESTARTSYS) all the way back to filp_open and __dentry_open never
>gets called.
>
>This was fixed upstream by adding atomic open capability to the VFS, but that's
>probably too invasive a change for RHEL4. The patch below fixes this by
>tracking incomplete NFSv4 open calls and cleaning them up if the SETATTR
>returns an error. This hinges on the idea is that a task will only call a 
>SETATTR on an incomplete open state in order to handle the truncate.
>
>It's very difficult to trigger this problem without the patch for BZ 228292,
>but I believe that's simply because it's hard to have a long RPC queue with
>a lot of stateful operations without that patch.
>
>This patch seems to prevent the most common type of "busy inodes" problem that
>I've been able to reproduce on nfs4. There is another variety as well, and I'll
>be posting a patch for that too. With both patches in place, I've not been able
>to reproduce a "busy inodes" problem at all on nfs4. With neither patch or
>just one of them, I can reproduce the problem reliably.
>
>diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>index f21139e..4bccf18 100644
>--- a/fs/nfs/nfs4proc.c
>+++ b/fs/nfs/nfs4proc.c
>@@ -607,6 +607,38 @@ struct nfs4_state *nfs4_do_open(struct inode *dir, struct qstr *name, int flags,
> 	return res;
> }
> 
>+/*
>+ * 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.
>+ */
>+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;
>+		}
>+	}
>+	spin_unlock(&state->inode->i_lock);
>+	return 0;
>+}
>+
> static int _nfs4_do_setattr(struct nfs_server *server, struct nfs_fattr *fattr,
>                 struct nfs_fh *fhandle, struct iattr *sattr,
>                 struct nfs4_state *state)
>@@ -769,6 +801,7 @@ 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;
>@@ -780,11 +813,22 @@ 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))
>+	if (IS_ERR(state)) {
>+		kfree(inc_open);
> 		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;
> }
> 
>@@ -1010,11 +1054,13 @@ 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;
> 
> 	nfs_fattr_init(fattr);
> 	
> 	if (size_change) {
>-		struct rpc_cred *cred = rpcauth_lookupcred(NFS_SERVER(inode)->client->cl_auth, 0);
>+		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,
>@@ -1040,6 +1086,17 @@ 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, FMODE_WRITE);
>@@ -2027,6 +2084,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);
> 	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 b0bbb73..fc80be1 100644
>--- a/fs/nfs/nfs4state.c
>+++ b/fs/nfs/nfs4state.c
>@@ -358,6 +358,7 @@ 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/nfs_fs.h b/include/linux/nfs_fs.h
>index 5bb2406..7a906ab 100644
>--- a/include/linux/nfs_fs.h
>+++ b/include/linux/nfs_fs.h
>@@ -766,10 +766,18 @@ 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 234587: 151296 | 151557 | 151576 | 152181 | 152182