Messages in this thread | | | Date | Wed, 15 Oct 2008 15:53:18 -0400 | From | Christoph Hellwig <> | Subject | Re: [PATCH 2/6] vfs: add d_ancestor() |
| |
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!
| |