Re: [V9fs-developer] [GIT PULL] 9p file system bug fixes for 2.6.35-rc2

[Aneesh Kumar K. V]

On Thu, 24 Jun 2010 22:00:10 +0530, "Aneesh Kumar K. V" <aneesh.kumar@xxxxxxxxxxxxxxxxxx> wrote:
> On Wed, 16 Jun 2010 22:12:32 +0530, "Aneesh Kumar K. V" <aneesh.kumar@xxxxxxxxxxxxxxxxxx> wrote:
> > On Tue, 8 Jun 2010 01:41:02 +0100, Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
> > > On Mon, Jun 07, 2010 at 05:08:19PM -0700, Linus Torvalds wrote:
> > > 
> > > > In fact, the other thing that I find doing that whole "dentry->d_parent" 
> > > > thing seems to literally be broken. If you look at v9fs_fid_lookup(), 
> > > > you'll notice how it walks up the d_parent chain, and at that point you do 
> > > > NOT own the directory i_mutex, so at that point d_parent really _can_ be 
> > > > changing wildly due to concurrent renames or whatever.
> > > 
> > > Eh...  It's bogus, all right, but i_mutex is not the correct solution.
> > > You'd have to take it on a lot of inodes along the way to root *and*
> > > you'd violate the ordering in process (ancestors first).
> > > 
> > > I'm not sure what's the right thing to do there, actually - s_vfs_rename_sem
> > > also won't do, since it'll give you ordering problems of its own (it's
> > > taken before i_mutex in VFS, so trying to take it under i_mutex would not
> > > do).
> > 
> > Can we use dcache_lock in v9fs_fid_lookup ? Since we are holding
> > parent directory inode->i_mutex in other places where we refer
> > dentry->d_parent I guess those access are ok ?. And for v9fs_fid_lookup we
> > can hold dcache_lock, walk the parent, build the full path name and use
> > that for TWALK ? 
> > 
> > Another option is we deny a cross directory rename when
> > doing fid_lookup. That is we can introduce a per superblock v9fs
> > specific rwlock that get taken (in write mode) in a cross directory
> > rename and in fid_lookup we take them in read mode ? We will have to set
> > FS_RENAME_DOES_D_MOVE for 9p.
> 
> something like this ?
> 
> commit 86c06ad7506e7e05dd4e1e1b8cee28e19703c4f6
> Author: Aneesh Kumar K.V <aneesh.kumar@xxxxxxxxxxxxxxxxxx>
> Date:   Mon Jun 21 21:50:07 2010 +0530
> 
>     fs/9p: Prevent parallel rename when doing fid_lookup
>     
>     During fid lookup we need to make sure that the dentry->d_parent doesn't
>     change so that we can safely walk the parent dentries. To ensure that
>     we need to prevent cross directory rename during fid_lookup. Add a
>     per superblock rename_lock rwlock to prevent parallel fid lookup and rename.
>     
>     Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@xxxxxxxxxxxxxxxxxx>
> 

To make sure d_name.name remain correct we need something like below ?

diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
index eae89ad..9f9f804 100644
--- a/fs/9p/vfs_inode.c
+++ b/fs/9p/vfs_inode.c
@@ -1050,7 +1050,7 @@ v9fs_vfs_rename(struct inode *old_dir, struct dentry *old_dentry,
 	struct p9_fid *olddirfid;
 	struct p9_fid *newdirfid;
 	struct p9_wstat wstat;
-	int retval, cross_dir_rename = 0;
+	int retval;
 
 	P9_DPRINTK(P9_DEBUG_VFS, "\n");
 	retval = 0;
@@ -1071,17 +1071,15 @@ v9fs_vfs_rename(struct inode *old_dir, struct dentry *old_dentry,
 		retval = PTR_ERR(newdirfid);
 		goto clunk_olddir;
 	}
-	cross_dir_rename = (old_dentry->d_parent != new_dentry->d_parent);
-	if (cross_dir_rename)
-		write_lock(&v9ses->rename_lock);
 
+	write_lock(&v9ses->rename_lock);
 	if (v9fs_proto_dotl(v9ses)) {
 		retval = p9_client_rename(oldfid, newdirfid,
 					(char *) new_dentry->d_name.name);
 		if (retval != -ENOSYS)
 			goto clunk_newdir;
 	}
-	if (cross_dir_rename) {
+	if (old_dentry->d_parent != new_dentry->d_parent) {
 		/*
 		 * 9P .u can only handle file rename in the same directory
 		 */
@@ -1100,8 +1098,7 @@ clunk_newdir:
 	if (!retval)
 		/* successful rename */
 		d_move(old_dentry, new_dentry);
-	if (cross_dir_rename)
-		write_unlock(&v9ses->rename_lock);
+	write_unlock(&v9ses->rename_lock);
 	p9_client_clunk(newdirfid);
 
 clunk_olddir:
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/