Signed-off-by: OGAWA Hirofumi <[email protected]>
---
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);
_
This calls d_move(), so fsnotify_d_instantiate() is unnecessary like
rename path.
Signed-off-by: OGAWA Hirofumi <[email protected]>
---
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);
_
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 <[email protected]>
---
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);
_
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 <[email protected]>
---
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;
}
_
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 <[email protected]>
---
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);
_
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 <[email protected]>
---
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 *);
_
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.
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.
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..)
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.
Looks correct, but I'm not entirely sure it's a worthwhile cleanup,
noth version are about the same level of readability to me.
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!
Christoph Hellwig <[email protected]> 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 <[email protected]>
[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 <[email protected]>
---
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 *);
_
Christoph Hellwig <[email protected]> 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 <[email protected]>
On Thu, Oct 16, 2008 at 05:16:58AM +0900, OGAWA Hirofumi wrote:
> Christoph Hellwig <[email protected]> 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.
Christoph Hellwig <[email protected]> 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 <[email protected]>
Christoph Hellwig <[email protected]> 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 <[email protected]>
On Thu, 2008-10-16 at 05:39 +0900, OGAWA Hirofumi wrote:
> Christoph Hellwig <[email protected]> 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
[email protected]
http://www.netapp.com
Trond Myklebust <[email protected]> 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 <[email protected]>
Christoph Hellwig <[email protected]> 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 <[email protected]>
Christoph Hellwig <[email protected]> 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 <[email protected]>