On 4/6/25 21:43, Tingmao Wang wrote:
> [...]
>
> +struct iget_data {
> + struct p9_stat_dotl *st;
> + struct dentry *dentry;
> +};
> +
> static int v9fs_test_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 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 match inode of different type */
> if (inode_wrong_type(inode, st->st_mode))
> @@ -74,22 +81,74 @@ static int v9fs_test_inode_dotl(struct inode *inode, void *data)
>
> if (v9inode->qid.path != st->qid.path)
> return 0;
> +
> + if (v9fs_inode_ident_path(v9ses)) {
> + if (!ino_path_compare(v9inode->path, dentry)) {
> + p9_debug(P9_DEBUG_VFS, "Refusing to reuse inode %p based on path mismatch",
> + inode);
> + return 0;
> + }
> + }
> return 1;
> }
>
> /* Always get a new inode */> static int v9fs_test_new_inode_dotl(struct inode *inode, void *data)
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;
> }
>
> [...]
