[PATCH] fs/9p: delete unnnecessary condition

[PATCH] fs/9p: delete unnnecessary condition

[Dan Carpenter]

We already know that "retval" is negative, so there is no need to check
again.  Also the statement is not indented far enough.  Delete it.

Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 fs/9p/vfs_dentry.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/9p/vfs_dentry.c b/fs/9p/vfs_dentry.c
index c1acbc98465d..c5bf74d547e8 100644
--- a/fs/9p/vfs_dentry.c
+++ b/fs/9p/vfs_dentry.c
@@ -109,7 +109,6 @@ static int __v9fs_lookup_revalidate(struct dentry *dentry, unsigned int flags)
 			p9_debug(P9_DEBUG_VFS,
 				"refresh inode: dentry = %pd (%p), got error %pe\n",
 				dentry, dentry, ERR_PTR(retval));
-		if (retval < 0)
 			return retval;
 		}
 	}
-- 
2.51.0

Re: [PATCH] fs/9p: delete unnnecessary condition

[Christian Schoenebeck]

On Friday, October 24, 2025 1:26:00 PM CEST Dan Carpenter wrote:
> We already know that "retval" is negative, so there is no need to check
> again.  Also the statement is not indented far enough.  Delete it.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---

Fixes: 43c36a5
Reviewed-by: Christian Schoenebeck <linux_oss@crudebyte.com>

Apparently a manual revert copy paste error. The rest of the revert commit
LGTM.

/Christian

>  fs/9p/vfs_dentry.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/fs/9p/vfs_dentry.c b/fs/9p/vfs_dentry.c
> index c1acbc98465d..c5bf74d547e8 100644
> --- a/fs/9p/vfs_dentry.c
> +++ b/fs/9p/vfs_dentry.c
> @@ -109,7 +109,6 @@ static int __v9fs_lookup_revalidate(struct dentry
> *dentry, unsigned int flags) p9_debug(P9_DEBUG_VFS,
>  				"refresh inode: dentry = %pd (%p), got error %pe\n",
>  				dentry, dentry, ERR_PTR(retval));
> -		if (retval < 0)
>  			return retval;
>  		}
>  	}

Re: [PATCH] fs/9p: delete unnnecessary condition

[Dominique Martinet]

Christian Schoenebeck wrote on Fri, Oct 24, 2025 at 01:59:46PM +0200:
> On Friday, October 24, 2025 1:26:00 PM CEST Dan Carpenter wrote:
> > We already know that "retval" is negative, so there is no need to check
> > again.  Also the statement is not indented far enough.  Delete it.
> > 
> > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> > ---
> 
> Fixes: 43c36a5
> Reviewed-by: Christian Schoenebeck <linux_oss@crudebyte.com>
> 
> Apparently a manual revert copy paste error. The rest of the revert commit
> LGTM.

Ah, I left that part of the reverted hunk while keeping the HEAD part:
```
<<<<<<< HEAD
                }
                if (v9inode->cache_validity & V9FS_INO_INVALID_ATTR) {
                        p9_debug(P9_DEBUG_VFS, "dentry: %pd (%p) invalidated due to type change\n",
                                 dentry, dentry);
                        return 0;
                }
                if (retval < 0) {
                        p9_debug(P9_DEBUG_VFS,
                                "refresh inode: dentry = %pd (%p), got error %pe\n",
                                dentry, dentry, ERR_PTR(retval));
||||||| 290434474c33 (fs/9p: Refresh metadata in d_revalidate for uncached mode too)
                if (!cached && v9inode->cache_validity & V9FS_INO_INVALID_ATTR)
                        return 0;
                if (retval < 0)
=======
                if (retval < 0)
>>>>>>> parent of 290434474c33 (fs/9p: Refresh metadata in d_revalidate for uncached mode too)
                        return retval;
```

For a proper revert I should have removed the first `if
(v9inode->cache_validity & V9FS_INO_INVALID_ATTR)` too :/

OTOH it still makes sense even without the rest of the patch (the only
reason V9FS_INO_INVALID_ATTR would still be set is on type change, and
in that case we do want to return 0 even on refresh inode error)


Anyway, thanksfully this particular double retval < 0 check is harmless
-- I'll pick this up for -next for now but hopefully we'll be able to
"revert the revert" and fix the other problems Tingmao pointed out by
the time we reach the 6.19 merge window...

Thank you both!
-- 
Dominique Martinet | Asmadeus

Re: [PATCH] fs/9p: delete unnnecessary condition

[Tingmao Wang]

On 10/24/25 14:34, Dominique Martinet wrote:
> [...]
> Anyway, thanksfully this particular double retval < 0 check is harmless
> -- I'll pick this up for -next for now but hopefully we'll be able to
> "revert the revert" and fix the other problems Tingmao pointed out by
> the time we reach the 6.19 merge window...

I'm hoping that the solution I proposed [1], which is to pass
V9FS_STAT2INODE_KEEP_ISIZE in v9fs_refresh_inode_dotl if v9ses->cache &
(CACHE_LOOSE | CACHE_WRITEBACK), would be good enough to prevent these
sorts of issues.  But given I've already missed a thing the first time,
I'm less confident in my judgement now 😅

(The other alternative I see is to flush the cache before updating
metadata, like we do in v9fs_vfs_getattr, but then that's a even larger
change, and I'm also not sure if it would be race-free (i.e. if another
write is processed after we flush but before we refresh metadata))

I can try and do some more testing on this, and would welcome pointers on
things to try.  Also let me know if you think the above suggestion
(V9FS_V9FS_STAT2INODE_KEEP_ISIZE) is safe on its own.

(Also, for some context, the main victim of this stale metadata issue,
afaik, is just the workaround for Landlock to work on 9pfs, in which we
get something to deliberately hold the paths used in rules open for the
inode to be reused.  Although I guess some other workloads which do a
similar thing might also suffer from this so still good to get this (stale
metadata issue) fixed.  I hope we eventually come to a proper solution for
the Landlock issue tho)

[1]: https://lore.kernel.org/all/6c74ad63-3afc-4549-9ac6-494b9a63e839@maowtm.org/