Re: [RFC PATCH 1/6] fs/9p: Add ability to identify inode by path for .L

["Mickaël Salaün" <mic@digikod.net>]

On Wed, Aug 13, 2025 at 12:57:49AM +0100, Tingmao Wang wrote:
> Thanks for the review :)  I will try to send a v2 in the coming weeks with
> the two changes you suggested and the changes to cached mode as suggested
> by Dominique (plus rename handling).  (will also try to figure out how to
> test with xfstests)
> 
> On 8/8/25 09:32, Mickaël Salaün wrote:
> > [...]
> >> On 7/5/25 01:25, Al Viro wrote:
> >>> On Sun, Apr 06, 2025 at 09:43:02PM +0100, Tingmao Wang wrote:
> >>>> +bool ino_path_compare(struct v9fs_ino_path *ino_path,
> >>>> +			     struct dentry *dentry)
> >>>> +{
> >>>> +	struct dentry *curr = dentry;
> >>>> +	struct qstr *curr_name;
> >>>> +	struct name_snapshot *compare;
> >>>> +	ssize_t i;
> >>>> +
> >>>> +	lockdep_assert_held_read(&v9fs_dentry2v9ses(dentry)->rename_sem);
> >>>> +
> >>>> +	rcu_read_lock();
> >>>> +	for (i = ino_path->nr_components - 1; i >= 0; i--) {
> >>>> +		if (curr->d_parent == curr) {
> >>>> +			/* We're supposed to have more components to walk */
> >>>> +			rcu_read_unlock();
> >>>> +			return false;
> >>>> +		}
> >>>> +		curr_name = &curr->d_name;
> >>>> +		compare = &ino_path->names[i];
> >>>> +		/*
> >>>> +		 * We can't use hash_len because it is salted with the parent
> >>>> +		 * dentry pointer.  We could make this faster by pre-computing our
> >>>> +		 * own hashlen for compare and ino_path outside, probably.
> >>>> +		 */
> >>>> +		if (curr_name->len != compare->name.len) {
> >>>> +			rcu_read_unlock();
> >>>> +			return false;
> >>>> +		}
> >>>> +		if (strncmp(curr_name->name, compare->name.name,
> >>>> +			    curr_name->len) != 0) {
> >>>
> >>> ... without any kind of protection for curr_name.  Incidentally,
> >>> what about rename()?  Not a cross-directory one, just one that
> >>> changes the name of a subdirectory within the same parent?
> >>
> >> As far as I can tell, in v9fs_vfs_rename, v9ses->rename_sem is taken for
> >> both same-parent and different parent renames, so I think we're safe here
> >> (and hopefully for any v9fs dentries, nobody should be causing d_name to
> >> change except for ourselves when we call d_move in v9fs_vfs_rename?  If
> >> yes then because we also take v9ses->rename_sem, in theory we should be
> >> fine here...?)
> > 
> > A lockdep_assert_held() or similar and a comment would make this clear.
> 
> I can add a comment, but there is already a lockdep_assert_held_read of
> the v9fs rename sem at the top of this function.

I wrote this comment before reading your new version beneath, which
already have this lockdep, so no need to change anything. :)

> 
> > [...]
> >> /*
> >>  * Must hold rename_sem due to traversing parents
> >>  */
> >> bool ino_path_compare(struct v9fs_ino_path *ino_path, struct dentry *dentry)
> >> {
> >> 	struct dentry *curr = dentry;
> >> 	struct name_snapshot *compare;
> >> 	ssize_t i;
> >>
> >> 	lockdep_assert_held_read(&v9fs_dentry2v9ses(dentry)->rename_sem);
> >>
> >> 	rcu_read_lock();
> >> 	for (i = ino_path->nr_components - 1; i >= 0; i--) {
> >> 		if (curr->d_parent == curr) {
> >> 			/* We're supposed to have more components to walk */
> >> 			rcu_read_unlock();
> >> 			return false;
> >> 		}
> >> 		compare = &ino_path->names[i];
> >> 		if (!d_same_name(curr, curr->d_parent, &compare->name)) {
> >> 			rcu_read_unlock();
> >> 			return false;
> >> 		}
> >> 		curr = curr->d_parent;
> >> 	}
> >> 	rcu_read_unlock();
> >> 	if (curr != curr->d_parent) {
> 
> Looking at this again I think this check probably needs to be done inside
> RCU, will fix as below:
> 
> >> 		/* dentry is deeper than ino_path */
> >> 		return false;
> >> 	}
> >> 	return true;
> >> }
> 
> diff --git a/fs/9p/ino_path.c b/fs/9p/ino_path.c
> index 0000b4964df0..7264003cb087 100644
> --- a/fs/9p/ino_path.c
> +++ b/fs/9p/ino_path.c
> @@ -77,13 +77,15 @@ void free_ino_path(struct v9fs_ino_path *path)
>  }
>  
>  /*
> - * Must hold rename_sem due to traversing parents
> + * Must hold rename_sem due to traversing parents.  Returns whether
> + * ino_path matches with the path of a v9fs dentry.
>   */
>  bool ino_path_compare(struct v9fs_ino_path *ino_path, struct dentry *dentry)
>  {
>  	struct dentry *curr = dentry;
>  	struct name_snapshot *compare;
>  	ssize_t i;
> +	bool ret;
>  
>  	lockdep_assert_held_read(&v9fs_dentry2v9ses(dentry)->rename_sem);
>  
> @@ -101,10 +103,8 @@ bool ino_path_compare(struct v9fs_ino_path *ino_path, struct dentry *dentry)
>  		}
>  		curr = curr->d_parent;
>  	}
> +	/* Comparison fails if dentry is deeper than ino_path */
> +	ret = (curr == curr->d_parent);
>  	rcu_read_unlock();
> -	if (curr != curr->d_parent) {
> -		/* dentry is deeper than ino_path */
> -		return false;
> -	}
> -	return true;
> +	return ret;
>  }

Looks good

> 
> > 
> > I like this new version.
> > 
>