On 4/6/25 21:43, Tingmao Wang wrote:

Looking back, this function should probably be renamed to something like
"v9fs_test_inode_uncached" since it now no longer "always get a new
inode".

Actually, maybe this should be merged with v9fs_test_inode_dotl - the
behavior is basically the same when inodeident=path is enabled.  Maybe the
approach could be that v9fs always re-use inodes (as long as qid matches,
and when inodeident=path, the path matches as well), but if in uncached
mode, it will also always refresh metadata?  (Basically inodes has to be
re-used, even in uncached mode, for Landlock and Fanotify using inode
marks to work)

Doing so does mean that if one sets inodeident=none, in a pathological
9pfs where different file/dirs have same qids, the inode will mistakenly
be re-used (like before be2ca38253 (Revert "fs/9p: simplify iget to remove
unnecessary paths")), but given that the user has specifically set
inodeident=none (i.e. not the default as proposed in this patch), I wonder
if this is acceptable behaviour?

 {
+	struct v9fs_inode *v9inode = V9FS_I(inode);
+	struct p9_stat_dotl *st = ((struct iget_data *)data)->st;
+	struct dentry *dentry = ((struct iget_data *)data)->dentry;
+	struct v9fs_session_info *v9ses = v9fs_inode2v9ses(inode);
+
+	/*
+	 * Don't reuse inode of different type, even if we have
+	 * inodeident=path and path matches.
+	 */
+	if (inode_wrong_type(inode, st->st_mode))
+		return 0;
+
+	/*
+	 * We're only getting here if QID2INO stays the same anyway, so
+	 * mirroring the qid checks in v9fs_test_inode_dotl
+	 * (but maybe that check is unnecessary anyway? at least on 64bit)
+	 */
+
+	if (v9inode->qid.type != st->qid.type)
+		return 0;
+
+	if (v9inode->qid.path != st->qid.path)
+		return 0;
+
+	if (v9fs_inode_ident_path(v9ses) && dentry && v9inode->path) {
+		if (ino_path_compare(V9FS_I(inode)->path, dentry)) {
+			p9_debug(P9_DEBUG_VFS,
+				 "Reusing inode %p based on path match", inode);
+			return 1;
+		}
+	}
+
 	return 0;
 }
 
 static int v9fs_set_inode_dotl(struct inode *inode,  void *data)
 {
 	struct v9fs_inode *v9inode = V9FS_I(inode);
-	struct p9_stat_dotl *st = (struct p9_stat_dotl *)data;
+	struct v9fs_session_info *v9ses = v9fs_inode2v9ses(inode);
+	struct iget_data *idata = data;
+	struct p9_stat_dotl *st = idata->st;
+	struct dentry *dentry = idata->dentry;
 
 	memcpy(&v9inode->qid, &st->qid, sizeof(st->qid));
 	inode->i_generation = st->st_gen;
+	if (v9fs_inode_ident_path(v9ses)) {
+		if (dentry) {
+			v9inode->path = make_ino_path(dentry);
+			if (!v9inode->path)
+				return -ENOMEM;
+		} else {
+			v9inode->path = NULL;
+		}
+	}

I realized that this leaves v9inode->path uninitialized if
inodeident=none.  The proper way is

	v9inode->path = NULL;
	if (v9fs_inode_ident_path(v9ses) && dentry) {
		v9inode->path = make_ino_path(dentry);
		if (!v9inode->path)
			return -ENOMEM;
	}

Same change applies for the non-.L version.

 	return 0;
 }
 
[...]