Messages in this thread Patch in this message | | | Date | Fri, 9 May 2003 15:24:10 -0700 | From | Andrew Morton <> | Subject | Re: PATCH - Don't remove inode from hash until filesystem has deleted it. |
| |
Neil Brown <neilb@cse.unsw.edu.au> wrote: > > > There is a small race with knfsd using iget to get an inode > that is currently being deleted. This is because it is removed > from the hash table *before* the filesystem gets to delete it. > If nfsd does an iget in this window it will cause a read_inode which > will return an apparently valid inode. However that inode will > shortly be deleted from disc without knfsd noticing... until > it is too late.
Cannot nfsd use igrab()?
> With this patch, the inode being deleted is left on the hash table, > and if a lookup find an inode being freed in the hashtable, it waits > in the inode waitqueue for the inode to be fully deleted.
Few things:
- Why the tests for I_CLEAR as well?
- There are lots of paths which set I_FREEING, and lots of paths which unhash inodes. But only one path in which a waiter on __wait_on_freeing_inode() gets woken up.
Are you sure there is sufficient coverage here? That there are no paths by which someone goes to sleep on a freeing inode but never gets woken up?
- wart: when a task gets woken in __wait_on_freeing_inode(), it doesn't know that it got woken on behalf of the correct inode (hash collision). So the inode can still be in state I_FREEING when __wait_on_freeing_inode() returns.
Yeah, it happens that this is OK because the caller will just repeat the search, but that sort of subtlety needs to be covered in commentary.
- Cleanups:
25-akpm/fs/inode.c | 10 ++++------ 1 files changed, 4 insertions(+), 6 deletions(-)
diff -puN fs/inode.c~inode-unhashing-fix-cleanups fs/inode.c --- 25/fs/inode.c~inode-unhashing-fix-cleanups Fri May 9 15:19:22 2003 +++ 25-akpm/fs/inode.c Fri May 9 15:20:20 2003 @@ -478,6 +478,7 @@ static struct inode * find_inode(struct struct hlist_node *node; struct inode * inode = NULL; +repeat: hlist_for_each (node, head) { prefetch(node->next); inode = hlist_entry(node, struct inode, i_hash); @@ -487,8 +488,7 @@ static struct inode * find_inode(struct continue; if (inode->i_state & (I_FREEING|I_CLEAR)) { __wait_on_freeing_inode(inode); - node = head->first; - continue; + goto repeat; } break; } @@ -504,6 +504,7 @@ static struct inode * find_inode_fast(st struct hlist_node *node; struct inode * inode = NULL; +repeat: hlist_for_each (node, head) { prefetch(node->next); inode = list_entry(node, struct inode, i_hash); @@ -513,8 +514,7 @@ static struct inode * find_inode_fast(st continue; if (inode->i_state & (I_FREEING|I_CLEAR)) { __wait_on_freeing_inode(inode); - node = head->first; - continue; + goto repeat; } break; } @@ -1253,11 +1253,9 @@ void __wait_on_freeing_inode(struct inod spin_unlock(&inode_lock); schedule(); remove_wait_queue(wq, &wait); - current->state = TASK_RUNNING; spin_lock(&inode_lock); } - void wake_up_inode(struct inode *inode) { wait_queue_head_t *wq = i_waitq_head(inode); _
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |