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

[Tingmao Wang]

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.

> [...]
>> /*
>>  * 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;
 }

> 
> I like this new version.
>