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

[Christoph Hellwig <hch@infradead.org>]

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!