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 what used to be a 4-byte hole. If necessary the field can be made smaller and converted into a union with something not used with symlinks.

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             | 12 ++++++++++--
  security/apparmor/apparmorfs.c |  2 +-
  4 files changed, 31 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 +2 -1

index 972147da71f9..30e332fb399d 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

@@ -686,7 +687,7 @@ struct inode {
 
  	/* Misc */
  	u32			i_state;
-	/* 32-bit hole */
+	int			i_linklen;	/* for symlinks */
  	struct rw_semaphore	i_rwsem;
 
  	unsigned long		dirtied_when;	/* jiffies of first dirtying */

include/linux/fs.h +4 -1

+	inode->i_opflags |= IOP_CACHED_LINK;
+}
+
  /*
   * Get bit address from inode->i_state to use with wait_var_event()
   * infrastructre.

@@ -3351,7 +3359,7 @@ extern const struct file_operations generic_ro_fops;
 
  #define special_file(m) (S_ISCHR(m)||S_ISBLK(m)||S_ISFIFO(m)||S_ISSOCK(m))
 
-extern int readlink_copy(char __user *, int, const char *);
+extern int readlink_copy(char __user *, int, const char *, int);
  extern int page_readlink(struct dentry *, char __user *, int);
  extern const char *page_get_link(struct dentry *, struct inode *,
  				 struct delayed_call *);

security/apparmor/apparmorfs.c +1 -1

index 01b923d97a44..60959cfba672 100644
@@ -2611,7 +2611,7 @@ static int policy_readlink(struct dentry *dentry, char __user *buffer,
  	res = snprintf(name, sizeof(name), "%s:[%lu]", AAFS_NAME,
  		       d_inode(dentry)->i_ino);
  	if (res > 0 && res < sizeof(name))
-		res = readlink_copy(buffer, buflen, name);
+		res = readlink_copy(buffer, buflen, name, strlen(name));
  	else
  		res = -ENOENT;
 

include/linux/fs.h struct inode { · line 756

Quoted 3 lines

include/linux/fs.h +3 -0

+{
+	inode->i_link = link;
+	inode->i_linklen = linklen;

Just curious, is this linklen equal to inode size? if it is, why don't use it?

Quoted 5 lines · wangjianjian (C)

include/linux/fs.h struct inode { · line 756

Quoted 3 lines

include/linux/fs.h +3 -0

+{
+	inode->i_link = link;
+	inode->i_linklen = linklen;

Just curious, is this linklen equal to inode size? if it is, why don't use it?

Maybe you should read v1 of the patch series where Jan explained where that's not true.

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.

Quoted 4 lines · Mateusz Guzik

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.

Do you have a workload in mind which calls readlink() at sufficiently high numbers such that this would be noticeable in non-micro-benchmarks? What motiviated you to do this work?

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

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.

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.).

Quoted 12 lines · Mateusz Guzik

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

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.

Quoted 3 lines

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.

The thing is that you're stealing one of the holes I just put into struct inode a cycle ago or so. The general idea has been to shrink struct inode if we can and I'm not sure that caching the link length is actually worth losing that hole. Otherwise I wouldn't object.

Quoted 3 lines

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.).

They should probably also be made inline functions and likely/unlikely sprinkled in there.

Quoted 18 lines · Christian Brauner

Quoted 15 lines · Mateusz Guzik

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

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.

Quoted 5 lines

The thing is that you're stealing one of the holes I just put into struct inode a cycle ago or so. The general idea has been to shrink struct inode if we can and I'm not sure that caching the link length is actually worth losing that hole. Otherwise I wouldn't object.

Per the patch description this can be a union with something not used for symlinks. I'll find a nice field.

Quoted 7 lines

Quoted 4 lines

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.).

They should probably also be made inline functions and likely/unlikely sprinkled in there.

someone(tm) should at least do a sweep through in-vfs code. for

example LOOKUP_IS_SCOPED is sometimes marked as unlikely and other times has no annotations whatsoever, even though ultimately it all executes in the same setting

Interestingly even __read_seqcount_begin (used *twice* in path_init()) is missing one. I sent a patch to fix it long time ago but the recipient did not respond

Quoted 3 lines · Mateusz Guzik

Interestingly even __read_seqcount_begin (used *twice* in path_init()) is missing one. I sent a patch to fix it long time ago but the recipient did not respond

I snatched it.