Login
[x]
Log in using an account from:
Fedora Account System
Red Hat Associate
Red Hat Customer
Or login using a Red Hat Bugzilla account
Forgot Password
Login:
Hide Forgot
Create an Account
Red Hat Bugzilla – Attachment 317484 Details for
Bug 446396
crm #1790828 Kernel 2.6.9-67.ELsmp panics in nfs4_free_client
[?]
New
Simple Search
Advanced Search
My Links
Browse
Requests
Reports
Current State
Search
Tabular reports
Graphical reports
Duplicates
Other Reports
User Changes
Plotly Reports
Bug Status
Bug Severity
Non-Defaults
|
Product Dashboard
Help
Page Help!
Bug Writing Guidelines
What's new
Browser Support Policy
5.0.4.rh83 Release notes
FAQ
Guides index
User guide
Web Services
Contact
Legal
This site requires JavaScript to be enabled to function correctly, please enable it.
[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:
updated patch -- handle nfs4 incomplete opens more comprehensively
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 >
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
Attachments on
bug 446396
:
309745
|
315673
|
317001
| 317484