symlink length caching

When utilized it dodges strlen() in vfs_readlink(), giving about 1.5% speed up when issuing readlink on /initrd.img on ext4.

Filesystems opt in by calling inode_set_cached_link() when creating an inode.

The size is stored in a new union utilizing the same space as i_devices, thus avoiding growing the struct or taking up any more space.

Churn-wise the current readlink_copy() helper is patched to accept the size instead of calculating it.

Signed-off-by Mateusz Guzik <[email protected]>

---
 fs/namei.c                     | 34 +++++++++++++++++++---------------
 fs/proc/namespaces.c           |  2 +-
 include/linux/fs.h             | 15 +++++++++++++--
 security/apparmor/apparmorfs.c |  2 +-
 4 files changed, 34 insertions(+), 19 deletions(-)

fs/namei.c +19 -15

index 9d30c7aa9aa6..e56c29a22d26 100644
@@ -5272,19 +5272,16 @@ SYSCALL_DEFINE2(rename, const char __user *, oldname, const char __user *, newna
 				getname(newname), 0);
 }
 
-int readlink_copy(char __user *buffer, int buflen, const char *link)
+int readlink_copy(char __user *buffer, int buflen, const char *link, int linklen)
 {
-	int len = PTR_ERR(link);
-	if (IS_ERR(link))
-		goto out;
+	int copylen;
 
-	len = strlen(link);
-	if (len > (unsigned) buflen)
-		len = buflen;
-	if (copy_to_user(buffer, link, len))
-		len = -EFAULT;
-out:
-	return len;
+	copylen = linklen;
+	if (unlikely(copylen > (unsigned) buflen))
+		copylen = buflen;
+	if (copy_to_user(buffer, link, copylen))
+		copylen = -EFAULT;
+	return copylen;
 }
 
 /**

@@ -5304,6 +5301,9 @@ int vfs_readlink(struct dentry *dentry, char __user *buffer, int buflen)
 	const char *link;
 	int res;
 
+	if (inode->i_opflags & IOP_CACHED_LINK)
+		return readlink_copy(buffer, buflen, inode->i_link, inode->i_linklen);
+
 	if (unlikely(!(inode->i_opflags & IOP_DEFAULT_READLINK))) {
 		if (unlikely(inode->i_op->readlink))
 			return inode->i_op->readlink(dentry, buffer, buflen);

@@ -5322,7 +5322,7 @@ int vfs_readlink(struct dentry *dentry, char __user *buffer, int buflen)
 		if (IS_ERR(link))
 			return PTR_ERR(link);
 	}
-	res = readlink_copy(buffer, buflen, link);
+	res = readlink_copy(buffer, buflen, link, strlen(link));
 	do_delayed_call(&done);
 	return res;
 }

@@ -5391,10 +5391,14 @@ EXPORT_SYMBOL(page_put_link);
 
 int page_readlink(struct dentry *dentry, char __user *buffer, int buflen)
 {
+	const char *link;
+	int res;
+
 	DEFINE_DELAYED_CALL(done);
-	int res = readlink_copy(buffer, buflen,
-				page_get_link(dentry, d_inode(dentry),
-					      &done));
+	link = page_get_link(dentry, d_inode(dentry), &done);
+	res = PTR_ERR(link);
+	if (!IS_ERR(link))
+		res = readlink_copy(buffer, buflen, link, strlen(link));
 	do_delayed_call(&done);
 	return res;
 }

fs/proc/namespaces.c +1 -1

index 8e159fc78c0a..c610224faf10 100644
@@ -83,7 +83,7 @@ static int proc_ns_readlink(struct dentry *dentry, char __user *buffer, int bufl
 	if (ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS)) {
 		res = ns_get_name(name, sizeof(name), task, ns_ops);
 		if (res >= 0)
-			res = readlink_copy(buffer, buflen, name);
+			res = readlink_copy(buffer, buflen, name, strlen(name));
 	}
 	put_task_struct(task);
 	return res;

include/linux/fs.h +1 -0

index 7e29433c5ecc..2cc98de5af43 100644
@@ -626,6 +626,7 @@ is_uncached_acl(struct posix_acl *acl)
 #define IOP_XATTR	0x0008
 #define IOP_DEFAULT_READLINK	0x0010
 #define IOP_MGTIME	0x0020
+#define IOP_CACHED_LINK	0x0040
 
 /*
  * Keep mostly read-only and often accessed (especially for

I think that i_devices should be moved into the union as it's really only used with i_cdev but it's not that easily done because list_head needs to be initialized. I roughly envisioned something like:

union {
        struct {
                struct cdev             *i_cdev;
                struct list_head        i_devices;
        };
        struct {
                char                    *i_link;
                unsigned int            i_link_len;
        };
        struct pipe_inode_info          *i_pipe;
        unsigned                        i_dir_seq;
};

But it's not important enough imho.

I think that i_devices should be moved into the union as it's really only used with i_cdev but it's not that easily done because list_head needs to be initialized.

I roughly envisioned something like:

union {
        struct {
                struct cdev             *i_cdev;
                struct list_head        i_devices;
        };
        struct {
                char                    *i_link;
                unsigned int            i_link_len;
        };
        struct pipe_inode_info          *i_pipe;
        unsigned                        i_dir_seq;
};

Signed-off-by Mateusz Guzik <[email protected]>

---
 fs/ext4/inode.c | 3 ++-
 fs/ext4/namei.c | 4 +++-
 2 files changed, 5 insertions(+), 2 deletions(-)

fs/ext4/inode.c +2 -1

index 89aade6f45f6..7c54ae5fcbd4 100644
@@ -5006,10 +5006,11 @@ struct inode *__ext4_iget(struct super_block *sb, unsigned long ino,
 		if (IS_ENCRYPTED(inode)) {
 			inode->i_op = &ext4_encrypted_symlink_inode_operations;
 		} else if (ext4_inode_is_fast_symlink(inode)) {
-			inode->i_link = (char *)ei->i_data;
 			inode->i_op = &ext4_fast_symlink_inode_operations;
 			nd_terminate_link(ei->i_data, inode->i_size,
 				sizeof(ei->i_data) - 1);
+			inode_set_cached_link(inode, (char *)ei->i_data,
+					      inode->i_size);
 		} else {
 			inode->i_op = &ext4_symlink_inode_operations;
 		}

fs/ext4/namei.c +3 -1

index bcf2737078b8..536d56d15072 100644
@@ -3418,7 +3418,6 @@ static int ext4_symlink(struct mnt_idmap *idmap, struct inode *dir,
 			inode->i_op = &ext4_symlink_inode_operations;
 		} else {
 			inode->i_op = &ext4_fast_symlink_inode_operations;
-			inode->i_link = (char *)&EXT4_I(inode)->i_data;
 		}
 	}
 

@@ -3434,6 +3433,9 @@ static int ext4_symlink(struct mnt_idmap *idmap, struct inode *dir,
 		       disk_link.len);
 		inode->i_size = disk_link.len - 1;
 		EXT4_I(inode)->i_disksize = inode->i_size;
+		if (!IS_ENCRYPTED(inode))
+			inode_set_cached_link(inode, (char *)&EXT4_I(inode)->i_data,
+					      inode->i_size);
 	}
 	err = ext4_add_nondir(handle, dentry, &inode);
 	if (handle)

Signature · 1 line

2.43.0

Signed-off-by Mateusz Guzik <[email protected]>

---
 mm/shmem.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

mm/shmem.c +4 -2

index c7881e16f4be..135f38eb2ff1 100644
@@ -3868,6 +3868,7 @@ static int shmem_symlink(struct mnt_idmap *idmap, struct inode *dir,
 	int len;
 	struct inode *inode;
 	struct folio *folio;
+	char *link;
 
 	len = strlen(symname) + 1;
 	if (len > PAGE_SIZE)

@@ -3889,12 +3890,13 @@ static int shmem_symlink(struct mnt_idmap *idmap, struct inode *dir,
 
 	inode->i_size = len-1;
 	if (len <= SHORT_SYMLINK_LEN) {
-		inode->i_link = kmemdup(symname, len, GFP_KERNEL);
-		if (!inode->i_link) {
+		link = kmemdup(symname, len, GFP_KERNEL);
+		if (!link) {
 			error = -ENOMEM;
 			goto out_remove_offset;
 		}
 		inode->i_op = &shmem_short_symlink_operations;
+		inode_set_cached_link(inode, link, len - 1);
 	} else {
 		inode_nohighmem(inode);
 		inode->i_mapping->a_ops = &shmem_aops;

Signature · 1 line

2.43.0

quote:
When utilized it dodges strlen() in vfs_readlink(), giving about 1.5% speed up when issuing readlink on /initrd.img on ext4.

The size is stored in a union with i_devices, which is never looked at unless the inode is for a device.

[...]

[1/3] vfs: support caching symlink lengths in inodes

      https://git.kernel.org/vfs/vfs/c/5cbb3c7e0051
[2/3] ext4: use inode_set_cached_link()
      https://git.kernel.org/vfs/vfs/c/740456f67017
[3/3] tmpfs: use inode_set_cached_link()
      https://git.kernel.org/vfs/vfs/c/30071e02c163

Quoted 6 lines · Christian Brauner

[1/3] vfs: support caching symlink lengths in inodes

      https://git.kernel.org/vfs/vfs/c/5cbb3c7e0051
[2/3] ext4: use inode_set_cached_link()
      https://git.kernel.org/vfs/vfs/c/740456f67017
[3/3] tmpfs: use inode_set_cached_link()
      https://git.kernel.org/vfs/vfs/c/30071e02c163

_Very_ belated note: that needs D/f/porting.rst chunk that would cover readlink_copy() calling conventions change.

quote:
When utilized it dodges strlen() in vfs_readlink(), giving about 1.5% speed up when issuing readlink on /initrd.img on ext4.

The size is stored in a union with i_devices, which is never looked at unless the inode is for a device.

Benchmark code at the bottom.

ext4 and tmpfs are patched, other filesystems can also get there with some more work.

Arguably the current get_link API should be patched to let the fs return the size, but that's not a churn I'm interested into diving in.

On my v1 Jan remarked 1.5% is not a particularly high win questioning whether doing this makes sense. I noted the value is only this small because of other slowdowns.

To elaborate here are highlights while benching on Sapphire Rapids:
1. putname using atomics (over 3.5% on my profile)

sounds like Al has plans to do something here(?), I'm not touching it if it can be helped. the atomic definitely does not have to be there in the common case.

2. kmem_cache_alloc_noprof/kmem_cache_free (over 7% combined)

They are both dog slow due to cmpxchg16b. A patchset was posted which
adds a different allocation/free fast path which should whack majority
of the problem, see: https://lore.kernel.org/linux-mm/[email protected]/

If this lands I'll definitely try to make the pathname allocs use it, should drop about 5-6 percentage points on this sucker.

3. __legitimize_mnt issues a full fence (again over 3%)

As far as avoiding the fence is concerned waiting on rcu grace period on unmount should do the trick. However, I found there is a bunch complexity there to sort out before doing this will be feasible (notably there are multiple mounts freed in one go, this needs to be batched). There may be other issues which I missed and which make this not worth it, but the fence is definitely avoidable in principle and I would be surprised if there was no way to sensibly get there. No ETA, for now I'm just pointing this out.

There is also the idea to speculatively elide lockref, but when I tried that last time I ran into significant LSM-related trouble.

All that aside there is also quite a bit of branching and func calling which does not need to be there (example: make vfsuid/vfsgid, could be combined into one routine etc.).

Ultimately there is single-threaded perf left on the table in various spots.

v3:
- use a union instead of a dedicated field, used up with i_devices

v2:
- add a dedicated field, flag and a helper instead of using i_size
https://lore.kernel.org/linux-fsdevel/[email protected]/

v1 can be found here:
https://lore.kernel.org/linux-fsdevel/[email protected]/

benchmark:
plug into will-it-scale into tests/readlink1.c:

#include <stdlib.h>
#include <unistd.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <assert.h>
#include <string.h>

char *testcase_description = "readlink /initrd.img";

void testcase(unsigned long long *iterations, unsigned long nr)

{
        char *tmplink = "/initrd.img";
        char buf[1024];

        while (1) {
                int error = readlink(tmplink, buf, sizeof(buf));
                assert(error > 0);

                (*iterations)++;
        }
}

Mateusz Guzik (3):
vfs: support caching symlink lengths in inodes
ext4: use inode_set_cached_link()
tmpfs: use inode_set_cached_link()

 fs/ext4/inode.c                |  3 ++-
 fs/ext4/namei.c                |  4 +++-
 fs/namei.c                     | 34 +++++++++++++++++++---------------
 fs/proc/namespaces.c           |  2 +-
 include/linux/fs.h             | 15 +++++++++++++--
 mm/shmem.c                     |  6 ++++--
 security/apparmor/apparmorfs.c |  2 +-
 7 files changed, 43 insertions(+), 23 deletions(-)