linux-kernel - Re: [RFC PATCH] vfs: dodge strlen() in vfs_readlink() when ->i

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAGudoHECQkQQrcHuWkP2badRP6eXequEiBD2=VTcMfd_Tfj+rA@mail.gmail.com>
Date: Mon, 18 Nov 2024 15:51:34 +0100
From: Mateusz Guzik <mjguzik@...il.com>
To: Jan Kara <jack@...e.cz>
Cc: brauner@...nel.org, viro@...iv.linux.org.uk, linux-kernel@...r.kernel.org, 
	linux-fsdevel@...r.kernel.org
Subject: Re: [RFC PATCH] vfs: dodge strlen() in vfs_readlink() when ->i_link
 is populated

On Mon, Nov 18, 2024 at 3:41 PM Jan Kara <jack@...e.cz> wrote:
>
> On Mon 18-11-24 13:20:09, Mateusz Guzik wrote:
> > On Mon, Nov 18, 2024 at 12:53 PM Jan Kara <jack@...e.cz> wrote:
> > >
> > > On Mon 18-11-24 09:53:57, Mateusz Guzik wrote:
> > > > This gives me about 1.5% speed up when issuing readlink on /initrd.img
> > > > on ext4.
> > > >
> > > > Signed-off-by: Mateusz Guzik <mjguzik@...il.com>
> > > > ---
> ...
> > > > I would leave something of that sort in if it was not defeating the
> > > > point of the change.
> > > >
> > > > However, I'm a little worried some crap fs *does not* fill this in
> > > > despite populating i_link.
> > > >
> > > > Perhaps it would make sense to keep the above with the patch hanging out
> > > > in next and remove later?
> > > >
> > > > Anyhow, worst case, should it turn out i_size does not work there are at
> > > > least two 4-byte holes which can be used to store the length (and
> > > > chances are some existing field can be converted into a union instead).
> > >
> > > I'm not sure I completely follow your proposal here...
> > >
> >
> > I am saying if the size has to be explicitly stored specifically for
> > symlinks, 2 options are:
> > - fill up one of the holes
> > - find a field which is never looked at for symlink inodes and convert
> > into a union
> >
> > I'm going to look into it.
>
> I guess there's limited enthusiasm for complexity to achieve 1.5% improvement
> in readlink which is not *that* common. But I haven't seen the patch and
> other guys may have different opinions :) So we'll see.
>

I'm thinking an i_opflag "this inode has a cached symlink with size
stored in i_linklen", so I don't think there is much in way of
complexity here. Then interested filesystems could call a helper like
so:

diff --git a/mm/shmem.c b/mm/shmem.c
index 3d17753afd94..9dedf432ae13 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -3870,6 +3870,7 @@ static int shmem_symlink(struct mnt_idmap
*idmap, struct inode *dir,
        int len;
        struct inode *inode;
        struct folio *folio;
+       const char *link;

        len = strlen(symname) + 1;
        if (len > PAGE_SIZE)
@@ -3891,12 +3892,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(link, len);
        } else {
                inode_nohighmem(inode);
                inode->i_mapping->a_ops = &shmem_aops;


This is only 1.5% because of other weird slowdowns which don't need to
be there, notably putname using atomics. If the other crap was already
fixed it would be closer to 5%.

Here comes a funny note that readlink is used to implement realpath
and vast majority of calls are for directories(!), for which the patch
is a nop.

However, actual readlinks on symlinks do happen every time you run gcc
for example:
readlink("/usr/bin/cc", "/etc/alternatives/cc", 1023) = 20
readlink("/etc/alternatives/cc", "/usr/bin/gcc", 1023) = 12
readlink("/usr/bin/gcc", "gcc-12", 1023) = 6
readlink("/usr/bin/gcc-12", "x86_64-linux-gnu-gcc-12", 1023) = 23
readlink("/usr/bin/cc", "/etc/alternatives/cc", 1023) = 20
readlink("/etc/alternatives/cc", "/usr/bin/gcc", 1023) = 12
readlink("/usr/bin/gcc", "gcc-12", 1023) = 6
readlink("/usr/bin/gcc-12", "x86_64-linux-gnu-gcc-12", 1023) = 23

that said, no, this is not earth shattering by any means but I don't
see any reason to *object* to it
-- 
Mateusz Guzik <mjguzik gmail.com>