Messages in this thread Patch in this message | | | From | hooanon05@yahoo ... | Subject | Re: [patch 4/4] nfs: clean up sillyrenaming in nfs_rename() | Date | Sat, 26 Dec 2009 15:24:11 +0900 |
| |
Miklos,
Don't we need to make the 'dentry' hashed, in sillyrename case? The d_alloc()ed dentry is unhashed, and it was d_rehash()ed later (before the patch).
new_dentry = rehash = dentry; ::: out: if (rehash) d_rehash(rehash);
Applying your patch, the 'rehash' is not re-assigned and the 'dentry' left unhashed. Such dentry which is visible by readdir but its d_hash is empty, will be a problem in nfs readdir (which calls d_lookup()), I am afraid.
J. R. Okajima
Miklos Szeredi: > The d_instantiate(new_dentry, NULL) is superfluous, the dentry is > already negative. Rehashing this dummy dentry isn't needed either, > d_move() works fine on an unhashed target. > > The re-checking for busy after a failed nfs_sillyrename() is bogus > too: new_dentry->d_count < 2 would be a bug here. > > Signed-off-by: Miklos Szeredi <mszeredi@suse.cz> > --- > fs/nfs/dir.c | 11 ++++------- > 1 file changed, 4 insertions(+), 7 deletions(-) > > Index: linux-2.6/fs/nfs/dir.c > =================================================================== > --- linux-2.6.orig/fs/nfs/dir.c 2009-12-02 15:11:19.000000000 +0100 > +++ linux-2.6/fs/nfs/dir.c 2009-12-02 15:11:30.000000000 +0100 > @@ -1611,14 +1611,11 @@ static int nfs_rename(struct inode *old_ > > /* silly-rename the existing target ... */ > err = nfs_sillyrename(new_dir, new_dentry); > - if (!err) { > - new_dentry = rehash = dentry; > - new_inode = NULL; > - /* instantiate the replacement target */ > - d_instantiate(new_dentry, NULL); > - } else if (atomic_read(&new_dentry->d_count) > 1) > - /* dentry still busy? */ > + if (err) > goto out; > + > + new_dentry = dentry; > + new_inode = NULL; > } > }
| |