fs/ext2/inode.c | 3 +-- fs/ext4/inode.c | 3 +-- fs/stat.c | 6 +++++- include/linux/fs.h | 2 +- 4 files changed, 8 insertions(+), 6 deletions(-)
Unfortunately the other filesystems I checked make adjustments after
their own call to generic_fillattr() and consequently can't benefit.
Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
---
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 | 6 +++++-
include/linux/fs.h | 2 +-
4 files changed, 8 insertions(+), 6 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 f13308bfdc98..581a95376e70 100644
--- a/fs/stat.c
+++ b/fs/stat.c
@@ -78,8 +78,11 @@ 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. We make it return a value so that consumers can
+ * tail-call it.
*/
-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 +113,7 @@ void generic_fillattr(struct mnt_idmap *idmap, u32 request_mask,
stat->change_cookie = inode_query_iversion(inode);
}
+ return 0;
}
EXPORT_SYMBOL(generic_fillattr);
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
fs/ext2/inode.c | 3 +--
fs/ext4/inode.c | 3 +--
fs/stat.c | 9 ++++++---
include/linux/fs.h | 2 +-
4 files changed, 9 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 f13308bfdc98..7d390bcd74ab 100644
--- a/fs/stat.c
+++ b/fs/stat.c
@@ -78,8 +78,11 @@ 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. We make it return a value so that consumers can
+ * tail-call it.
*/
-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 +113,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 +213,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.0On Tue, Apr 01, 2025 at 06:52:52PM +0200, Mateusz Guzik wrote: > Unfortunately the other filesystems I checked make adjustments after > their own call to generic_fillattr() and consequently can't benefit. This is in no way a useful commit message. Why do you even do this change? What's the point of it? And why do you think making a function tail callable for two callers, one of which is basically irrelevant warrants adding a pointless return that now needs to be generated and checked by all other callers (which this patch fails to do)?
On Fri, Apr 4, 2025 at 10:33 AM Christoph Hellwig <hch@infradead.org> wrote: > > On Tue, Apr 01, 2025 at 06:52:52PM +0200, Mateusz Guzik wrote: > > Unfortunately the other filesystems I checked make adjustments after > > their own call to generic_fillattr() and consequently can't benefit. > > This is in no way a useful commit message. > > Why do you even do this change? What's the point of it? And why do you > think making a function tail callable for two callers, one of which is > basically irrelevant warrants adding a pointless return that now needs > to be generated and checked by all other callers (which this patch fails > to do)? > Callers don't need to check it because it is guaranteed to be 0. Also returning 0 vs returning nothing makes virtually no difference to anyone. As for general context, there are several small slowdowns when issuing fstat() and I'm tackling them bit by bit (and yes, tail calling vs returning to the caller and that caller exiting is a small optimization). -- Mateusz Guzik <mjguzik gmail.com>
On Fri, Apr 04, 2025 at 10:14:32PM +0200, Mateusz Guzik wrote: > Callers don't need to check it because it is guaranteed to be 0. Also > returning 0 vs returning nothing makes virtually no difference to > anyone. Nom the return value very clearly states it can return something else with your patch, and people will sooner or later do that. Let's stop these silly things that just create nasty landmines for no reason at all. > As for general context, there are several small slowdowns when issuing > fstat() and I'm tackling them bit by bit (and yes, tail calling vs > returning to the caller and that caller exiting is a small > optimization). Please prove that it makes a difference. And if it does do the changes in a sane way by keeping the helper as is except for marking it as inline candidate and a special _tail helper below that inlines it. But the bar for such micro-optimizations should be high, and so far you have no managed to provide any rationale for it.
On Tue, Apr 01, 2025 at 06:52:52PM +0200, Mateusz Guzik wrote: > Unfortunately the other filesystems I checked make adjustments after > their own call to generic_fillattr() and consequently can't benefit. > > Signed-off-by: Mateusz Guzik <mjguzik@gmail.com> > --- You need to resend this during -rc1 or at least when all fs trees have been merged based on mainline.
© 2016 - 2025 Red Hat, Inc.