lkml.org 
[lkml]   [2008]   [Oct]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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!


\
 
 \ /
  Last update: 2008-10-15 21:57    [W:0.236 / U:0.196 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site