linux-ext4 - Re: [PATCH v2 0/3] symlink length caching

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: <CAGudoHGOC6to4_nJX9vhWV8HnF19U2xmmZY3Nc0ZbZnyTtGyxw@mail.gmail.com>
Date: Wed, 20 Nov 2024 11:42:33 +0100
From: Mateusz Guzik <mjguzik@...il.com>
To: Christian Brauner <brauner@...nel.org>
Cc: viro@...iv.linux.org.uk, jack@...e.cz, linux-kernel@...r.kernel.org, 
	linux-fsdevel@...r.kernel.org, hughd@...gle.com, linux-ext4@...r.kernel.org, 
	tytso@....edu, linux-mm@...ck.org
Subject: Re: [PATCH v2 0/3] symlink length caching

On Wed, Nov 20, 2024 at 11:33 AM Christian Brauner <brauner@...nel.org> wrote:
>
> On Tue, Nov 19, 2024 at 10:45:52AM +0100, Mateusz Guzik wrote:
> > 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.
>
> 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.

> > 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, maybe I should resend with more people in
cc (who though?), see:
https://lore.kernel.org/all/20230727180355.813995-1-mjguzik@gmail.com/

-- 
Mateusz Guzik <mjguzik gmail.com>