[PATCH 1/3] fs: sort out cosmetic differences between stat funcs and add predicts

[PATCH 1/3] fs: sort out cosmetic differences between stat funcs and add predicts

[Mateusz Guzik]

This is a nop, but I did verify asm improves.

Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
---
 fs/stat.c | 35 ++++++++++++++++++++---------------
 1 file changed, 20 insertions(+), 15 deletions(-)

diff --git a/fs/stat.c b/fs/stat.c
index f13308bfdc98..b79ddb83914b 100644
--- a/fs/stat.c
+++ b/fs/stat.c
@@ -241,7 +241,7 @@ int vfs_getattr(const struct path *path, struct kstat *stat,
 	int retval;
 
 	retval = security_inode_getattr(path);
-	if (retval)
+	if (unlikely(retval))
 		return retval;
 	return vfs_getattr_nosec(path, stat, request_mask, query_flags);
 }
@@ -421,7 +421,7 @@ SYSCALL_DEFINE2(stat, const char __user *, filename,
 	int error;
 
 	error = vfs_stat(filename, &stat);
-	if (error)
+	if (unlikely(error))
 		return error;
 
 	return cp_old_stat(&stat, statbuf);
@@ -434,7 +434,7 @@ SYSCALL_DEFINE2(lstat, const char __user *, filename,
 	int error;
 
 	error = vfs_lstat(filename, &stat);
-	if (error)
+	if (unlikely(error))
 		return error;
 
 	return cp_old_stat(&stat, statbuf);
@@ -443,12 +443,13 @@ SYSCALL_DEFINE2(lstat, const char __user *, filename,
 SYSCALL_DEFINE2(fstat, unsigned int, fd, struct __old_kernel_stat __user *, statbuf)
 {
 	struct kstat stat;
-	int error = vfs_fstat(fd, &stat);
+	int error;
 
-	if (!error)
-		error = cp_old_stat(&stat, statbuf);
+	error = vfs_fstat(fd, &stat);
+	if (unlikely(error))
+		return error;
 
-	return error;
+	return cp_old_stat(&stat, statbuf);
 }
 
 #endif /* __ARCH_WANT_OLD_STAT */
@@ -502,10 +503,12 @@ SYSCALL_DEFINE2(newstat, const char __user *, filename,
 		struct stat __user *, statbuf)
 {
 	struct kstat stat;
-	int error = vfs_stat(filename, &stat);
+	int error;
 
-	if (error)
+	error = vfs_stat(filename, &stat);
+	if (unlikely(error))
 		return error;
+
 	return cp_new_stat(&stat, statbuf);
 }
 
@@ -516,7 +519,7 @@ SYSCALL_DEFINE2(newlstat, const char __user *, filename,
 	int error;
 
 	error = vfs_lstat(filename, &stat);
-	if (error)
+	if (unlikely(error))
 		return error;
 
 	return cp_new_stat(&stat, statbuf);
@@ -530,8 +533,9 @@ SYSCALL_DEFINE4(newfstatat, int, dfd, const char __user *, filename,
 	int error;
 
 	error = vfs_fstatat(dfd, filename, &stat, flag);
-	if (error)
+	if (unlikely(error))
 		return error;
+
 	return cp_new_stat(&stat, statbuf);
 }
 #endif
@@ -539,12 +543,13 @@ SYSCALL_DEFINE4(newfstatat, int, dfd, const char __user *, filename,
 SYSCALL_DEFINE2(newfstat, unsigned int, fd, struct stat __user *, statbuf)
 {
 	struct kstat stat;
-	int error = vfs_fstat(fd, &stat);
+	int error;
 
-	if (!error)
-		error = cp_new_stat(&stat, statbuf);
+	error = vfs_fstat(fd, &stat);
+	if (unlikely(error))
+		return error;
 
-	return error;
+	return cp_new_stat(&stat, statbuf);
 }
 #endif
 
-- 
2.43.0

[PATCH 2/3] fs: predict not having to do anything in fdput()

[Mateusz Guzik]

This matches the annotation in fdget().

Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
---
 include/linux/file.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/file.h b/include/linux/file.h
index 302f11355b10..af1768d934a0 100644
--- a/include/linux/file.h
+++ b/include/linux/file.h
@@ -59,7 +59,7 @@ static inline struct fd CLONED_FD(struct file *f)
 
 static inline void fdput(struct fd fd)
 {
-	if (fd.word & FDPUT_FPUT)
+	if (unlikely(fd.word & FDPUT_FPUT))
 		fput(fd_file(fd));
 }
 
-- 
2.43.0

[PATCH 3/3 v3] fs: make generic_fillattr() tail-callable and utilize it in ext2/ext4

[Mateusz Guzik]

Unfortunately the other filesystems I checked make adjustments after
their own call to generic_fillattr() and consequently can't benefit.

This is a nop for unmodified consumers.

Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
---

v3:
- clarify no callers *need* to change to accmodate this patch

v2:
- also patch vfs_getattr_nosec

There are weird slowdowns on fstat, this submission is a byproduct of
trying to straighten out the fast path.

Not benchmarked, but I did confirm the compiler jmps out to the routine
instead of emitting a call which is the right thing to do here.

that said I'm not going to argue, but I like to see this out of the way.

there are nasty things which need to be addressed separately

 fs/ext2/inode.c    |  3 +--
 fs/ext4/inode.c    |  3 +--
 fs/stat.c          | 10 +++++++---
 include/linux/fs.h |  2 +-
 4 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
index 30f8201c155f..cf1f89922207 100644
--- a/fs/ext2/inode.c
+++ b/fs/ext2/inode.c
@@ -1629,8 +1629,7 @@ int ext2_getattr(struct mnt_idmap *idmap, const struct path *path,
 			STATX_ATTR_IMMUTABLE |
 			STATX_ATTR_NODUMP);
 
-	generic_fillattr(&nop_mnt_idmap, request_mask, inode, stat);
-	return 0;
+	return generic_fillattr(&nop_mnt_idmap, request_mask, inode, stat);
 }
 
 int ext2_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 1dc09ed5d403..3edd6e60dd9b 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -5687,8 +5687,7 @@ int ext4_getattr(struct mnt_idmap *idmap, const struct path *path,
 				  STATX_ATTR_NODUMP |
 				  STATX_ATTR_VERITY);
 
-	generic_fillattr(idmap, request_mask, inode, stat);
-	return 0;
+	return generic_fillattr(idmap, request_mask, inode, stat);
 }
 
 int ext4_file_getattr(struct mnt_idmap *idmap,
diff --git a/fs/stat.c b/fs/stat.c
index b79ddb83914b..46e10af29f4b 100644
--- a/fs/stat.c
+++ b/fs/stat.c
@@ -78,8 +78,12 @@ EXPORT_SYMBOL(fill_mg_cmtime);
  * take care to map the inode according to @idmap before filling in the
  * uid and gid filds. On non-idmapped mounts or if permission checking is to be
  * performed on the raw inode simply pass @nop_mnt_idmap.
+ *
+ * The routine always succeeds (i.e., nobody needs to check its return value).
+ * We make it return 0 so that consumers can tail-call it at the end of their
+ * own getattr function.
  */
-void generic_fillattr(struct mnt_idmap *idmap, u32 request_mask,
+int generic_fillattr(struct mnt_idmap *idmap, u32 request_mask,
 		      struct inode *inode, struct kstat *stat)
 {
 	vfsuid_t vfsuid = i_uid_into_vfsuid(idmap, inode);
@@ -110,6 +114,7 @@ void generic_fillattr(struct mnt_idmap *idmap, u32 request_mask,
 		stat->change_cookie = inode_query_iversion(inode);
 	}
 
+	return 0;
 }
 EXPORT_SYMBOL(generic_fillattr);
 
@@ -209,8 +214,7 @@ int vfs_getattr_nosec(const struct path *path, struct kstat *stat,
 					    request_mask,
 					    query_flags);
 
-	generic_fillattr(idmap, request_mask, inode, stat);
-	return 0;
+	return generic_fillattr(idmap, request_mask, inode, stat);
 }
 EXPORT_SYMBOL(vfs_getattr_nosec);
 
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 016b0fe1536e..754893d8d2a8 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3471,7 +3471,7 @@ extern int page_symlink(struct inode *inode, const char *symname, int len);
 extern const struct inode_operations page_symlink_inode_operations;
 extern void kfree_link(void *);
 void fill_mg_cmtime(struct kstat *stat, u32 request_mask, struct inode *inode);
-void generic_fillattr(struct mnt_idmap *, u32, struct inode *, struct kstat *);
+int generic_fillattr(struct mnt_idmap *, u32, struct inode *, struct kstat *);
 void generic_fill_statx_attr(struct inode *inode, struct kstat *stat);
 void generic_fill_statx_atomic_writes(struct kstat *stat,
 				      unsigned int unit_min,
-- 
2.43.0

Re: [PATCH 3/3 v3] fs: make generic_fillattr() tail-callable and utilize it in ext2/ext4

[Christoph Hellwig]

On Mon, Apr 07, 2025 at 01:58:06AM +0200, Mateusz Guzik wrote:
> Unfortunately the other filesystems I checked make adjustments after
> their own call to generic_fillattr() and consequently can't benefit.

Still a bad idea for all the reason state in reply to v2, and still
lacks any justification in the commit log.

Re: (subset) [PATCH 1/3] fs: sort out cosmetic differences between stat funcs and add predicts

[Christian Brauner]

On Mon, 07 Apr 2025 01:58:04 +0200, Mateusz Guzik wrote:
> This is a nop, but I did verify asm improves.
> 
> 

Applied to the vfs-6.16.misc branch of the vfs/vfs.git tree.
Patches in the vfs-6.16.misc branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs-6.16.misc

[1/3] fs: sort out cosmetic differences between stat funcs and add predicts
      https://git.kernel.org/vfs/vfs/c/eaec2cd1670d
[2/3] fs: predict not having to do anything in fdput()
      https://git.kernel.org/vfs/vfs/c/5f3e0b4a1f59

Re: [PATCH 1/3] fs: sort out cosmetic differences between stat funcs and add predicts

[Jan Kara]

On Mon 07-04-25 01:58:04, Mateusz Guzik wrote:
> This is a nop, but I did verify asm improves.
> 
> Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/stat.c | 35 ++++++++++++++++++++---------------
>  1 file changed, 20 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/stat.c b/fs/stat.c
> index f13308bfdc98..b79ddb83914b 100644
> --- a/fs/stat.c
> +++ b/fs/stat.c
> @@ -241,7 +241,7 @@ int vfs_getattr(const struct path *path, struct kstat *stat,
>  	int retval;
>  
>  	retval = security_inode_getattr(path);
> -	if (retval)
> +	if (unlikely(retval))
>  		return retval;
>  	return vfs_getattr_nosec(path, stat, request_mask, query_flags);
>  }
> @@ -421,7 +421,7 @@ SYSCALL_DEFINE2(stat, const char __user *, filename,
>  	int error;
>  
>  	error = vfs_stat(filename, &stat);
> -	if (error)
> +	if (unlikely(error))
>  		return error;
>  
>  	return cp_old_stat(&stat, statbuf);
> @@ -434,7 +434,7 @@ SYSCALL_DEFINE2(lstat, const char __user *, filename,
>  	int error;
>  
>  	error = vfs_lstat(filename, &stat);
> -	if (error)
> +	if (unlikely(error))
>  		return error;
>  
>  	return cp_old_stat(&stat, statbuf);
> @@ -443,12 +443,13 @@ SYSCALL_DEFINE2(lstat, const char __user *, filename,
>  SYSCALL_DEFINE2(fstat, unsigned int, fd, struct __old_kernel_stat __user *, statbuf)
>  {
>  	struct kstat stat;
> -	int error = vfs_fstat(fd, &stat);
> +	int error;
>  
> -	if (!error)
> -		error = cp_old_stat(&stat, statbuf);
> +	error = vfs_fstat(fd, &stat);
> +	if (unlikely(error))
> +		return error;
>  
> -	return error;
> +	return cp_old_stat(&stat, statbuf);
>  }
>  
>  #endif /* __ARCH_WANT_OLD_STAT */
> @@ -502,10 +503,12 @@ SYSCALL_DEFINE2(newstat, const char __user *, filename,
>  		struct stat __user *, statbuf)
>  {
>  	struct kstat stat;
> -	int error = vfs_stat(filename, &stat);
> +	int error;
>  
> -	if (error)
> +	error = vfs_stat(filename, &stat);
> +	if (unlikely(error))
>  		return error;
> +
>  	return cp_new_stat(&stat, statbuf);
>  }
>  
> @@ -516,7 +519,7 @@ SYSCALL_DEFINE2(newlstat, const char __user *, filename,
>  	int error;
>  
>  	error = vfs_lstat(filename, &stat);
> -	if (error)
> +	if (unlikely(error))
>  		return error;
>  
>  	return cp_new_stat(&stat, statbuf);
> @@ -530,8 +533,9 @@ SYSCALL_DEFINE4(newfstatat, int, dfd, const char __user *, filename,
>  	int error;
>  
>  	error = vfs_fstatat(dfd, filename, &stat, flag);
> -	if (error)
> +	if (unlikely(error))
>  		return error;
> +
>  	return cp_new_stat(&stat, statbuf);
>  }
>  #endif
> @@ -539,12 +543,13 @@ SYSCALL_DEFINE4(newfstatat, int, dfd, const char __user *, filename,
>  SYSCALL_DEFINE2(newfstat, unsigned int, fd, struct stat __user *, statbuf)
>  {
>  	struct kstat stat;
> -	int error = vfs_fstat(fd, &stat);
> +	int error;
>  
> -	if (!error)
> -		error = cp_new_stat(&stat, statbuf);
> +	error = vfs_fstat(fd, &stat);
> +	if (unlikely(error))
> +		return error;
>  
> -	return error;
> +	return cp_new_stat(&stat, statbuf);
>  }
>  #endif
>  
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 2/3] fs: predict not having to do anything in fdput()

[Jan Kara]

On Mon 07-04-25 01:58:05, Mateusz Guzik wrote:
> This matches the annotation in fdget().
> 
> Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  include/linux/file.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/file.h b/include/linux/file.h
> index 302f11355b10..af1768d934a0 100644
> --- a/include/linux/file.h
> +++ b/include/linux/file.h
> @@ -59,7 +59,7 @@ static inline struct fd CLONED_FD(struct file *f)
>  
>  static inline void fdput(struct fd fd)
>  {
> -	if (fd.word & FDPUT_FPUT)
> +	if (unlikely(fd.word & FDPUT_FPUT))
>  		fput(fd_file(fd));
>  }
>  
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR