On Mon, Nov 18, 2024 at 3:41 PM Jan Kara <jack@xxxxxxx> wrote:
>
> On Mon 18-11-24 13:20:09, Mateusz Guzik wrote:
> > On Mon, Nov 18, 2024 at 12:53 PM Jan Kara <jack@xxxxxxx> 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@xxxxxxxxx>
> > > > ---
> ...
> > > > 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>
