[PATCH 1/6] vfs: replace parent == dentry->d_parent by IS_ROOT()

[PATCH 1/6] vfs: replace parent == dentry->d_parent by IS_ROOT()

[OGAWA Hirofumi]

Signed-off-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
---

 fs/dcache.c |   21 ++++++++++++---------
 fs/namei.c  |    4 ++--
 2 files changed, 14 insertions(+), 11 deletions(-)

diff -puN fs/dcache.c~dcache-cleanup-1 fs/dcache.c
--- linux-2.6/fs/dcache.c~dcache-cleanup-1	2008-09-30 05:24:32.000000000 +0900
+++ linux-2.6-hirofumi/fs/dcache.c	2008-09-30 05:24:32.000000000 +0900
@@ -174,9 +174,12 @@ static struct dentry *d_kill(struct dent
 	dentry_stat.nr_dentry--;	/* For d_free, below */
 	/*drops the locks, at that point nobody can reach this dentry */
 	dentry_iput(dentry);
-	parent = dentry->d_parent;
+	if (IS_ROOT(dentry))
+		parent = NULL;
+	else
+		parent = dentry->d_parent;
 	d_free(dentry);
-	return dentry == parent ? NULL : parent;
+	return parent;
 }
 
 /* 
@@ -666,11 +669,12 @@ static void shrink_dcache_for_umount_sub
 				BUG();
 			}
 
-			parent = dentry->d_parent;
-			if (parent == dentry)
+			if (IS_ROOT(dentry))
 				parent = NULL;
-			else
+			else {
+				parent = dentry->d_parent;
 				atomic_dec(&parent->d_count);
+			}
 
 			list_del(&dentry->d_u.d_child);
 			detached++;
@@ -1721,7 +1725,7 @@ static int d_isparent(struct dentry *p1,
 {
 	struct dentry *p;
 
-	for (p = p2; p->d_parent != p; p = p->d_parent) {
+	for (p = p2; !IS_ROOT(p); p = p->d_parent) {
 		if (p->d_parent == p1)
 			return 1;
 	}
@@ -2166,10 +2170,9 @@ int is_subdir(struct dentry * new_dentry
 		seq = read_seqbegin(&rename_lock);
 		for (;;) {
 			if (new_dentry != old_dentry) {
-				struct dentry * parent = new_dentry->d_parent;
-				if (parent == new_dentry)
+				if (IS_ROOT(new_dentry))
 					break;
-				new_dentry = parent;
+				new_dentry = new_dentry->d_parent;
 				continue;
 			}
 			result = 1;
diff -puN fs/namei.c~dcache-cleanup-1 fs/namei.c
--- linux-2.6/fs/namei.c~dcache-cleanup-1	2008-09-30 05:24:32.000000000 +0900
+++ linux-2.6-hirofumi/fs/namei.c	2008-09-30 05:24:32.000000000 +0900
@@ -1470,7 +1470,7 @@ struct dentry *lock_rename(struct dentry
 
 	mutex_lock(&p1->d_inode->i_sb->s_vfs_rename_mutex);
 
-	for (p = p1; p->d_parent != p; p = p->d_parent) {
+	for (p = p1; !IS_ROOT(p); p = p->d_parent) {
 		if (p->d_parent == p2) {
 			mutex_lock_nested(&p2->d_inode->i_mutex, I_MUTEX_PARENT);
 			mutex_lock_nested(&p1->d_inode->i_mutex, I_MUTEX_CHILD);
@@ -1478,7 +1478,7 @@ struct dentry *lock_rename(struct dentry
 		}
 	}
 
-	for (p = p2; p->d_parent != p; p = p->d_parent) {
+	for (p = p2; !IS_ROOT(p); p = p->d_parent) {
 		if (p->d_parent == p1) {
 			mutex_lock_nested(&p1->d_inode->i_mutex, I_MUTEX_PARENT);
 			mutex_lock_nested(&p2->d_inode->i_mutex, I_MUTEX_CHILD);
_

[PATCH 2/6] vfs: add d_ancestor()

[OGAWA Hirofumi]

This adds d_ancestor() instead of d_isparent(), then use it.

If new_dentry == old_dentry, is_subdir() returns 1, looks strange. But
I'm not checking callers for now, so this keeps current behavior.

Signed-off-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
---

 fs/dcache.c            |   28 ++++++++++------------------
 fs/namei.c             |   22 ++++++++++------------
 include/linux/dcache.h |    1 +
 3 files changed, 21 insertions(+), 30 deletions(-)

diff -puN fs/dcache.c~dcache-cleanup-2 fs/dcache.c
--- linux-2.6/fs/dcache.c~dcache-cleanup-2	2008-10-13 02:26:23.000000000 +0900
+++ linux-2.6-hirofumi/fs/dcache.c	2008-10-13 02:26:55.000000000 +0900
@@ -1719,17 +1719,18 @@ void d_move(struct dentry * dentry, stru
 }
 
 /*
- * Helper that returns 1 if p1 is a parent of p2, else 0
+ * Helper that returns the ancestor dentry of p2 which is a child of
+ * p1, if p1 is an ancestor of p2, else NULL.
  */
-static int d_isparent(struct dentry *p1, struct dentry *p2)
+struct dentry *d_ancestor(struct dentry *p1, struct dentry *p2)
 {
 	struct dentry *p;
 
 	for (p = p2; !IS_ROOT(p); p = p->d_parent) {
 		if (p->d_parent == p1)
-			return 1;
+			return p;
 	}
-	return 0;
+	return NULL;
 }
 
 /*
@@ -1753,7 +1754,7 @@ static struct dentry *__d_unalias(struct
 
 	/* Check for loops */
 	ret = ERR_PTR(-ELOOP);
-	if (d_isparent(alias, dentry))
+	if (d_ancestor(alias, dentry))
 		goto out_err;
 
 	/* See lock_rename() */
@@ -2156,28 +2157,19 @@ out:
 int is_subdir(struct dentry * new_dentry, struct dentry * old_dentry)
 {
 	int result;
-	struct dentry * saved = new_dentry;
 	unsigned long seq;
 
+	if (new_dentry == old_dentry)
+		return 1;
+
 	/* need rcu_readlock to protect against the d_parent trashing due to
 	 * d_move
 	 */
 	rcu_read_lock();
         do {
 		/* for restarting inner loop in case of seq retry */
-		new_dentry = saved;
-		result = 0;
 		seq = read_seqbegin(&rename_lock);
-		for (;;) {
-			if (new_dentry != old_dentry) {
-				if (IS_ROOT(new_dentry))
-					break;
-				new_dentry = new_dentry->d_parent;
-				continue;
-			}
-			result = 1;
-			break;
-		}
+		result = (d_ancestor(old_dentry, new_dentry) != NULL);
 	} while (read_seqretry(&rename_lock, seq));
 	rcu_read_unlock();
 
diff -puN include/linux/dcache.h~dcache-cleanup-2 include/linux/dcache.h
--- linux-2.6/include/linux/dcache.h~dcache-cleanup-2	2008-10-13 02:26:23.000000000 +0900
+++ linux-2.6-hirofumi/include/linux/dcache.h	2008-10-13 02:26:23.000000000 +0900
@@ -287,6 +287,7 @@ static inline struct dentry *d_add_uniqu
 
 /* used for rename() and baskets */
 extern void d_move(struct dentry *, struct dentry *);
+extern struct dentry *d_ancestor(struct dentry *, struct dentry *);
 
 /* appendix may either be NULL or be used for transname suffixes */
 extern struct dentry * d_lookup(struct dentry *, struct qstr *);
diff -puN fs/namei.c~dcache-cleanup-2 fs/namei.c
--- linux-2.6/fs/namei.c~dcache-cleanup-2	2008-10-13 02:26:23.000000000 +0900
+++ linux-2.6-hirofumi/fs/namei.c	2008-10-13 02:26:23.000000000 +0900
@@ -1470,20 +1470,18 @@ struct dentry *lock_rename(struct dentry
 
 	mutex_lock(&p1->d_inode->i_sb->s_vfs_rename_mutex);
 
-	for (p = p1; !IS_ROOT(p); p = p->d_parent) {
-		if (p->d_parent == p2) {
-			mutex_lock_nested(&p2->d_inode->i_mutex, I_MUTEX_PARENT);
-			mutex_lock_nested(&p1->d_inode->i_mutex, I_MUTEX_CHILD);
-			return p;
-		}
+	p = d_ancestor(p2, p1);
+	if (p) {
+		mutex_lock_nested(&p2->d_inode->i_mutex, I_MUTEX_PARENT);
+		mutex_lock_nested(&p1->d_inode->i_mutex, I_MUTEX_CHILD);
+		return p;
 	}
 
-	for (p = p2; !IS_ROOT(p); p = p->d_parent) {
-		if (p->d_parent == p1) {
-			mutex_lock_nested(&p1->d_inode->i_mutex, I_MUTEX_PARENT);
-			mutex_lock_nested(&p2->d_inode->i_mutex, I_MUTEX_CHILD);
-			return p;
-		}
+	p = d_ancestor(p1, p2);
+	if (p) {
+		mutex_lock_nested(&p1->d_inode->i_mutex, I_MUTEX_PARENT);
+		mutex_lock_nested(&p2->d_inode->i_mutex, I_MUTEX_CHILD);
+		return p;
 	}
 
 	mutex_lock_nested(&p1->d_inode->i_mutex, I_MUTEX_PARENT);
_

[PATCH 3/6] vfs: add __d_instantiate() helper

[OGAWA Hirofumi]

This adds __d_instantiate() for users which is already taking
dcache_lock, and replace with it.

The part of d_add_ci() isn't equivalent. But it should be needed
fsnotify_d_instantiate() actually, because the path is to add the
inode to negative dentry.  fsnotify_d_instantiate() should be called
after change from negative to positive.

__d_instantiate_unique() and d_materialise_unique() does opencoded
optimized version. From history, it seems a intent, so just add comment.

Signed-off-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
---

 fs/dcache.c |   29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff -puN fs/dcache.c~dcache-cleanup-3 fs/dcache.c
--- linux-2.6/fs/dcache.c~dcache-cleanup-3	2008-09-30 05:24:37.000000000 +0900
+++ linux-2.6-hirofumi/fs/dcache.c	2008-09-30 05:24:37.000000000 +0900
@@ -981,6 +981,15 @@ struct dentry *d_alloc_name(struct dentr
 	return d_alloc(parent, &q);
 }
 
+/* the caller must hold dcache_lock */
+static void __d_instantiate(struct dentry *dentry, struct inode *inode)
+{
+	if (inode)
+		list_add(&dentry->d_alias, &inode->i_dentry);
+	dentry->d_inode = inode;
+	fsnotify_d_instantiate(dentry, inode);
+}
+
 /**
  * d_instantiate - fill in inode information for a dentry
  * @entry: dentry to complete
@@ -1000,10 +1009,7 @@ void d_instantiate(struct dentry *entry,
 {
 	BUG_ON(!list_empty(&entry->d_alias));
 	spin_lock(&dcache_lock);
-	if (inode)
-		list_add(&entry->d_alias, &inode->i_dentry);
-	entry->d_inode = inode;
-	fsnotify_d_instantiate(entry, inode);
+	__d_instantiate(entry, inode);
 	spin_unlock(&dcache_lock);
 	security_d_instantiate(entry, inode);
 }
@@ -1033,6 +1039,7 @@ static struct dentry *__d_instantiate_un
 	unsigned int hash = entry->d_name.hash;
 
 	if (!inode) {
+		/* __d_instantiate() by hand */
 		entry->d_inode = NULL;
 		return NULL;
 	}
@@ -1052,9 +1059,7 @@ static struct dentry *__d_instantiate_un
 		return alias;
 	}
 
-	list_add(&entry->d_alias, &inode->i_dentry);
-	entry->d_inode = inode;
-	fsnotify_d_instantiate(entry, inode);
+	__d_instantiate(entry, inode);
 	return NULL;
 }
 
@@ -1211,10 +1216,8 @@ struct dentry *d_splice_alias(struct ino
 			d_move(new, dentry);
 			iput(inode);
 		} else {
-			/* d_instantiate takes dcache_lock, so we do it by hand */
-			list_add(&dentry->d_alias, &inode->i_dentry);
-			dentry->d_inode = inode;
-			fsnotify_d_instantiate(dentry, inode);
+			/* already taking dcache_lock, so d_add() by hand */
+			__d_instantiate(dentry, inode);
 			spin_unlock(&dcache_lock);
 			security_d_instantiate(dentry, inode);
 			d_rehash(dentry);
@@ -1297,8 +1300,7 @@ struct dentry *d_add_ci(struct dentry *d
 		 * d_instantiate() by hand because it takes dcache_lock which
 		 * we already hold.
 		 */
-		list_add(&found->d_alias, &inode->i_dentry);
-		found->d_inode = inode;
+		__d_instantiate(found, inode);
 		spin_unlock(&dcache_lock);
 		security_d_instantiate(found, inode);
 		return found;
@@ -1827,6 +1829,7 @@ struct dentry *d_materialise_unique(stru
 
 	if (!inode) {
 		actual = dentry;
+		/* __d_instantiate() by hand */
 		dentry->d_inode = NULL;
 		goto found_lock;
 	}
_

[PATCH 4/6] vfs: remove unnecessary fsnotify_d_instantiate()

[OGAWA Hirofumi]

This calls d_move(), so fsnotify_d_instantiate() is unnecessary like
rename path.

Signed-off-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
---

 fs/dcache.c |    1 -
 1 file changed, 1 deletion(-)

diff -puN fs/dcache.c~dcache-cleanup-4 fs/dcache.c
--- linux-2.6/fs/dcache.c~dcache-cleanup-4	2008-08-26 11:13:29.000000000 +0900
+++ linux-2.6-hirofumi/fs/dcache.c	2008-08-26 11:13:29.000000000 +0900
@@ -1209,7 +1209,6 @@ struct dentry *d_splice_alias(struct ino
 		new = __d_find_alias(inode, 1);
 		if (new) {
 			BUG_ON(!(new->d_flags & DCACHE_DISCONNECTED));
-			fsnotify_d_instantiate(new, inode);
 			spin_unlock(&dcache_lock);
 			security_d_instantiate(new, inode);
 			d_rehash(dentry);
_

[PATCH 5/6] vfs: remove LOOKUP_PARENT from non LOOKUP_PARENT lookup

[OGAWA Hirofumi]

lookup_hash() with LOOKUP_PARENT is bogus. And this prepares to add
new intent on those path.

The user of LOOKUP_PARENT intent is nfs only, and it checks whether
nd->flags has LOOKUP_CREATE or LOOKUP_OPEN, so the result is same.

Signed-off-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
---

 fs/namei.c |   27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff -puN fs/namei.c~dcache-remove-parent fs/namei.c
--- linux-2.6/fs/namei.c~dcache-remove-parent	2008-08-26 11:13:30.000000000 +0900
+++ linux-2.6-hirofumi/fs/namei.c	2008-08-26 11:13:30.000000000 +0900
@@ -2176,16 +2176,19 @@ static long do_rmdir(int dfd, const char
 		return error;
 
 	switch(nd.last_type) {
-		case LAST_DOTDOT:
-			error = -ENOTEMPTY;
-			goto exit1;
-		case LAST_DOT:
-			error = -EINVAL;
-			goto exit1;
-		case LAST_ROOT:
-			error = -EBUSY;
-			goto exit1;
+	case LAST_DOTDOT:
+		error = -ENOTEMPTY;
+		goto exit1;
+	case LAST_DOT:
+		error = -EINVAL;
+		goto exit1;
+	case LAST_ROOT:
+		error = -EBUSY;
+		goto exit1;
 	}
+
+	nd.flags &= ~LOOKUP_PARENT;
+
 	mutex_lock_nested(&nd.path.dentry->d_inode->i_mutex, I_MUTEX_PARENT);
 	dentry = lookup_hash(&nd);
 	error = PTR_ERR(dentry);
@@ -2263,6 +2266,9 @@ static long do_unlinkat(int dfd, const c
 	error = -EISDIR;
 	if (nd.last_type != LAST_NORM)
 		goto exit1;
+
+	nd.flags &= ~LOOKUP_PARENT;
+
 	mutex_lock_nested(&nd.path.dentry->d_inode->i_mutex, I_MUTEX_PARENT);
 	dentry = lookup_hash(&nd);
 	error = PTR_ERR(dentry);
@@ -2652,6 +2658,9 @@ asmlinkage long sys_renameat(int olddfd,
 	if (newnd.last_type != LAST_NORM)
 		goto exit2;
 
+	oldnd.flags &= ~LOOKUP_PARENT;
+	newnd.flags &= ~LOOKUP_PARENT;
+
 	trap = lock_rename(new_dir, old_dir);
 
 	old_dentry = lookup_hash(&oldnd);
_

[PATCH 6/6] vfs: add LOOKUP_RENAME_NEW intent

[OGAWA Hirofumi]

This adds LOOKUP_RENAME_NEW intent for lookup of rename destination.

LOOKUP_RENAME_NEW is going to be used like LOOKUP_CREATE. But since
the destination of rename() can be existing directory entry, so it has a
difference. Although that difference doesn't matter in my usage, this
tells it to user of this intent.

Signed-off-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
---

 fs/namei.c            |    1 +
 include/linux/namei.h |    1 +
 2 files changed, 2 insertions(+)

diff -puN fs/namei.c~dcache-add-rename-intent fs/namei.c
--- linux-2.6/fs/namei.c~dcache-add-rename-intent	2008-08-26 11:13:32.000000000 +0900
+++ linux-2.6-hirofumi/fs/namei.c	2008-08-26 11:13:32.000000000 +0900
@@ -2660,6 +2660,7 @@ asmlinkage long sys_renameat(int olddfd,
 
 	oldnd.flags &= ~LOOKUP_PARENT;
 	newnd.flags &= ~LOOKUP_PARENT;
+	newnd.flags |= LOOKUP_RENAME_NEW;
 
 	trap = lock_rename(new_dir, old_dir);
 
diff -puN include/linux/namei.h~dcache-add-rename-intent include/linux/namei.h
--- linux-2.6/include/linux/namei.h~dcache-add-rename-intent	2008-08-26 11:13:32.000000000 +0900
+++ linux-2.6-hirofumi/include/linux/namei.h	2008-08-26 11:13:32.000000000 +0900
@@ -53,6 +53,7 @@ enum {LAST_NORM, LAST_ROOT, LAST_DOT, LA
  */
 #define LOOKUP_OPEN		(0x0100)
 #define LOOKUP_CREATE		(0x0200)
+#define LOOKUP_RENAME_NEW	(0x0400)
 
 extern int user_path_at(int, const char __user *, unsigned, struct path *);
 
_

Re: [PATCH 6/6] vfs: add LOOKUP_RENAME_NEW intent

[Christoph Hellwig]

On Wed, Oct 15, 2008 at 10:58:11PM +0900, OGAWA Hirofumi wrote:
> 
> This adds LOOKUP_RENAME_NEW intent for lookup of rename destination.
> 
> LOOKUP_RENAME_NEW is going to be used like LOOKUP_CREATE. But since
> the destination of rename() can be existing directory entry, so it has a
> difference. Although that difference doesn't matter in my usage, this
> tells it to user of this intent.

Is this for handling CI rename properly?  Barry was looking into this,
but i told him to hold off for a while - the lookup code is changing
quite a bit because Al is trying to sort out the lookup intent mess and
we hopefully will stop passing the nameidata to ->lookup soon.

Also I think LOOKUP_RENAME_TARGET might be a little more descriptive
than LOOKUP_RENAME_NEW.

Re: [PATCH 6/6] vfs: add LOOKUP_RENAME_NEW intent

[OGAWA Hirofumi]

Christoph Hellwig <hch@infradead.org> writes:

> On Wed, Oct 15, 2008 at 10:58:11PM +0900, OGAWA Hirofumi wrote:
>> 
>> This adds LOOKUP_RENAME_NEW intent for lookup of rename destination.
>> 
>> LOOKUP_RENAME_NEW is going to be used like LOOKUP_CREATE. But since
>> the destination of rename() can be existing directory entry, so it has a
>> difference. Although that difference doesn't matter in my usage, this
>> tells it to user of this intent.
>
> Is this for handling CI rename properly?  Barry was looking into this,

Exactly, it is for some kind of CI rename workaround.

> but i told him to hold off for a while - the lookup code is changing
> quite a bit because Al is trying to sort out the lookup intent mess and
> we hopefully will stop passing the nameidata to ->lookup soon.

Umm.. however the intent for ->lookup() is useful for optimization in
some case?

> Also I think LOOKUP_RENAME_TARGET might be a little more descriptive
> than LOOKUP_RENAME_NEW.

It is the name which namei.c is using, anyway I don't care at all.
I'll take it.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>


[PATCH] vfs: add LOOKUP_RENAME_TARGET intent

This adds LOOKUP_RENAME_TARGET intent for lookup of rename destination.

LOOKUP_RENAME_TARGET is going to be used like LOOKUP_CREATE. But since
the destination of rename() can be existing directory entry, so it has a
difference. Although that difference doesn't matter in my usage, this
tells it to user of this intent.

Signed-off-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
---

 fs/namei.c            |    1 +
 include/linux/namei.h |    1 +
 2 files changed, 2 insertions(+)

diff -puN fs/namei.c~dcache-add-rename-intent fs/namei.c
--- linux-2.6/fs/namei.c~dcache-add-rename-intent	2008-10-15 20:35:24.000000000 +0900
+++ linux-2.6-hirofumi/fs/namei.c	2008-10-16 05:06:53.000000000 +0900
@@ -2660,6 +2660,7 @@ asmlinkage long sys_renameat(int olddfd,
 
 	oldnd.flags &= ~LOOKUP_PARENT;
 	newnd.flags &= ~LOOKUP_PARENT;
+	newnd.flags |= LOOKUP_RENAME_TARGET;
 
 	trap = lock_rename(new_dir, old_dir);
 
diff -puN include/linux/namei.h~dcache-add-rename-intent include/linux/namei.h
--- linux-2.6/include/linux/namei.h~dcache-add-rename-intent	2008-10-15 20:35:24.000000000 +0900
+++ linux-2.6-hirofumi/include/linux/namei.h	2008-10-16 05:07:15.000000000 +0900
@@ -53,6 +53,7 @@ enum {LAST_NORM, LAST_ROOT, LAST_DOT, LA
  */
 #define LOOKUP_OPEN		(0x0100)
 #define LOOKUP_CREATE		(0x0200)
+#define LOOKUP_RENAME_TARGET	(0x0400)
 
 extern int user_path_at(int, const char __user *, unsigned, struct path *);
 
_

Re: [PATCH 5/6] vfs: remove LOOKUP_PARENT from non LOOKUP_PARENT lookup

[Christoph Hellwig]

On Wed, Oct 15, 2008 at 10:58:11PM +0900, OGAWA Hirofumi wrote:
> 
> lookup_hash() with LOOKUP_PARENT is bogus. And this prepares to add
> new intent on those path.
> 
> The user of LOOKUP_PARENT intent is nfs only, and it checks whether
> nd->flags has LOOKUP_CREATE or LOOKUP_OPEN, so the result is same.

Makes sense to me, but in this area you really need to work against
Al's vfs.git tree as there's quite some work going on.

Re: [PATCH 5/6] vfs: remove LOOKUP_PARENT from non LOOKUP_PARENT lookup

[OGAWA Hirofumi]

Christoph Hellwig <hch@infradead.org> writes:

> Makes sense to me, but in this area you really need to work against
> Al's vfs.git tree as there's quite some work going on.

I'll send updated patchset soon for vfs-2.6. Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH 4/6] vfs: remove unnecessary fsnotify_d_instantiate()

[Christoph Hellwig]

On Wed, Oct 15, 2008 at 10:58:11PM +0900, OGAWA Hirofumi wrote:
> 
> This calls d_move(), so fsnotify_d_instantiate() is unnecessary like
> rename path.

fsnotify_d_instantiate and fsnotify_d_move call into different inotify
functions.  Did you verify that this really does the right thing?
(Sorry, not in a mood to look into idiotify code..)

Re: [PATCH 4/6] vfs: remove unnecessary fsnotify_d_instantiate()

[OGAWA Hirofumi]

Christoph Hellwig <hch@infradead.org> writes:

> On Wed, Oct 15, 2008 at 10:58:11PM +0900, OGAWA Hirofumi wrote:
>> 
>> This calls d_move(), so fsnotify_d_instantiate() is unnecessary like
>> rename path.
>
> fsnotify_d_instantiate and fsnotify_d_move call into different inotify
> functions.  Did you verify that this really does the right thing?
> (Sorry, not in a mood to look into idiotify code..)

Yes. I checked it.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH 4/6] vfs: remove unnecessary fsnotify_d_instantiate()

[Christoph Hellwig]

On Thu, Oct 16, 2008 at 05:16:58AM +0900, OGAWA Hirofumi wrote:
> Christoph Hellwig <hch@infradead.org> writes:
> 
> > On Wed, Oct 15, 2008 at 10:58:11PM +0900, OGAWA Hirofumi wrote:
> >> 
> >> This calls d_move(), so fsnotify_d_instantiate() is unnecessary like
> >> rename path.
> >
> > fsnotify_d_instantiate and fsnotify_d_move call into different inotify
> > functions.  Did you verify that this really does the right thing?
> > (Sorry, not in a mood to look into idiotify code..)
> 
> Yes. I checked it.

Then the patch looks good to me.

Re: [PATCH 3/6] vfs: add __d_instantiate() helper

[Christoph Hellwig]

On Wed, Oct 15, 2008 at 10:58:10PM +0900, OGAWA Hirofumi wrote:
> 
> This adds __d_instantiate() for users which is already taking
> dcache_lock, and replace with it.
> 
> The part of d_add_ci() isn't equivalent. But it should be needed
> fsnotify_d_instantiate() actually, because the path is to add the
> inode to negative dentry.  fsnotify_d_instantiate() should be called
> after change from negative to positive.
> 
> __d_instantiate_unique() and d_materialise_unique() does opencoded
> optimized version. From history, it seems a intent, so just add comment.

Looks good, but I think those "optimized' version should also be
converted, a single if shouldn't matter with todays branch predictors.

Re: [PATCH 3/6] vfs: add __d_instantiate() helper

[OGAWA Hirofumi]

Christoph Hellwig <hch@infradead.org> writes:

> On Wed, Oct 15, 2008 at 10:58:10PM +0900, OGAWA Hirofumi wrote:
>> 
>> This adds __d_instantiate() for users which is already taking
>> dcache_lock, and replace with it.
>> 
>> The part of d_add_ci() isn't equivalent. But it should be needed
>> fsnotify_d_instantiate() actually, because the path is to add the
>> inode to negative dentry.  fsnotify_d_instantiate() should be called
>> after change from negative to positive.
>> 
>> __d_instantiate_unique() and d_materialise_unique() does opencoded
>> optimized version. From history, it seems a intent, so just add comment.
>
> Looks good, but I think those "optimized' version should also be
> converted, a single if shouldn't matter with todays branch predictors.

Me too.

Trond, do you care the following convert? E.g.

in d_materialise_unique():

from
		dentry->d_inode = NULL;
		goto found_lock;
to
		__d_instantiate(dentry, NULL);
		goto found_lock;
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH 3/6] vfs: add __d_instantiate() helper

[Trond Myklebust]

On Thu, 2008-10-16 at 05:39 +0900, OGAWA Hirofumi wrote:
> Christoph Hellwig <hch@infradead.org> writes:
> 
> > On Wed, Oct 15, 2008 at 10:58:10PM +0900, OGAWA Hirofumi wrote:
> >> 
> >> This adds __d_instantiate() for users which is already taking
> >> dcache_lock, and replace with it.
> >> 
> >> The part of d_add_ci() isn't equivalent. But it should be needed
> >> fsnotify_d_instantiate() actually, because the path is to add the
> >> inode to negative dentry.  fsnotify_d_instantiate() should be called
> >> after change from negative to positive.
> >> 
> >> __d_instantiate_unique() and d_materialise_unique() does opencoded
> >> optimized version. From history, it seems a intent, so just add comment.
> >
> > Looks good, but I think those "optimized' version should also be
> > converted, a single if shouldn't matter with todays branch predictors.
> 
> Me too.
> 
> Trond, do you care the following convert? E.g.
> 
> in d_materialise_unique():
> 
> from
> 		dentry->d_inode = NULL;
> 		goto found_lock;
> to
> 		__d_instantiate(dentry, NULL);
> 		goto found_lock;

That would be fine by me.

Cheers
  Trond

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

Re: [PATCH 3/6] vfs: add __d_instantiate() helper

[OGAWA Hirofumi]

Trond Myklebust <Trond.Myklebust@netapp.com> writes:

>> Trond, do you care the following convert? E.g.
>> 
>> in d_materialise_unique():
>> 
>> from
>> 		dentry->d_inode = NULL;
>> 		goto found_lock;
>> to
>> 		__d_instantiate(dentry, NULL);
>> 		goto found_lock;
>
> That would be fine by me.

Thanks. I'll replace those.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH 2/6] vfs: add d_ancestor()

[Christoph Hellwig]

On Wed, Oct 15, 2008 at 10:58:10PM +0900, OGAWA Hirofumi wrote:
> 
> 
> This adds d_ancestor() instead of d_isparent(), then use it.
> 
> If new_dentry == old_dentry, is_subdir() returns 1, looks strange. But
> I'm not checking callers for now, so this keeps current behavior.

This change looks good, but a slightly more detailed changelog would be
good.

>  
>  /*
> - * Helper that returns 1 if p1 is a parent of p2, else 0
> + * Helper that returns the ancestor dentry of p2 which is a child of
> + * p1, if p1 is an ancestor of p2, else NULL.
>   */
> -static int d_isparent(struct dentry *p1, struct dentry *p2)
> +struct dentry *d_ancestor(struct dentry *p1, struct dentry *p2)

Given that d_ancestor is non-static a kerneldoc comment describing it
instead of a plain one would be good.

>  int is_subdir(struct dentry * new_dentry, struct dentry * old_dentry)
>  {
>  	int result;
> -	struct dentry * saved = new_dentry;
>  	unsigned long seq;
>  
> +	if (new_dentry == old_dentry)
> +		return 1;

Maybe a comment like in your commit description here would be good?

> +
>  	/* need rcu_readlock to protect against the d_parent trashing due to
>  	 * d_move
>  	 */
>  	rcu_read_lock();
>          do {
>  		/* for restarting inner loop in case of seq retry */
> -		new_dentry = saved;
> -		result = 0;
>  		seq = read_seqbegin(&rename_lock);
> -		for (;;) {
> -			if (new_dentry != old_dentry) {
> -				if (IS_ROOT(new_dentry))
> -					break;
> -				new_dentry = new_dentry->d_parent;
> -				continue;
> -			}
> -			result = 1;
> -			break;
> -		}
> +		result = (d_ancestor(old_dentry, new_dentry) != NULL);
>  	} while (read_seqretry(&rename_lock, seq));

That's a big improvement, but with a better loop and fixing all the
formatting mess in the old code it could become even better:

	/*
	 * Need rcu_readlock to protect against the d_parent trashing due to
	 * d_move.
	 */
	rcu_read_lock();
	do {
		/* for restarting inner loop in case of seq retry */
 		seq = read_seqbegin(&rename_lock);
		if (d_ancestor(old_dentry, new_dentry))
			result = 1;
		else
			result = 0;
	} while (read_seqretry(&rename_lock, seq));
 	rcu_read_unlock();


Nice set of patches!

Re: [PATCH 2/6] vfs: add d_ancestor()

[OGAWA Hirofumi]

Christoph Hellwig <hch@infradead.org> writes:

> This change looks good, but a slightly more detailed changelog would be
> good.

tweaked...

>>  /*
>> - * Helper that returns 1 if p1 is a parent of p2, else 0
>> + * Helper that returns the ancestor dentry of p2 which is a child of
>> + * p1, if p1 is an ancestor of p2, else NULL.
>>   */
>> -static int d_isparent(struct dentry *p1, struct dentry *p2)
>> +struct dentry *d_ancestor(struct dentry *p1, struct dentry *p2)
>
> Given that d_ancestor is non-static a kerneldoc comment describing it
> instead of a plain one would be good.

Yes.

>>  int is_subdir(struct dentry * new_dentry, struct dentry * old_dentry)
>>  {
>>  	int result;
>> -	struct dentry * saved = new_dentry;
>>  	unsigned long seq;
>>  
>> +	if (new_dentry == old_dentry)
>> +		return 1;
>
> Maybe a comment like in your commit description here would be good?

Sure, thanks.

> 	/*
> 	 * Need rcu_readlock to protect against the d_parent trashing due to
> 	 * d_move.
> 	 */
> 	rcu_read_lock();
> 	do {
> 		/* for restarting inner loop in case of seq retry */
>  		seq = read_seqbegin(&rename_lock);
> 		if (d_ancestor(old_dentry, new_dentry))
> 			result = 1;
> 		else
> 			result = 0;
> 	} while (read_seqretry(&rename_lock, seq));
>  	rcu_read_unlock();

I'll borrow your version. Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH 1/6] vfs: replace parent == dentry->d_parent by IS_ROOT()

[Christoph Hellwig]

Looks correct, but I'm not entirely sure it's a worthwhile cleanup,
noth version are about the same level of readability to me.

Re: [PATCH 1/6] vfs: replace parent == dentry->d_parent by IS_ROOT()

[OGAWA Hirofumi]

Christoph Hellwig <hch@infradead.org> writes:

> Looks correct, but I'm not entirely sure it's a worthwhile cleanup,
> noth version are about the same level of readability to me.

Yes. However, I think mix is not readable (and not grep friendly).
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>