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: <20241120-eisbahn-frost-824303fa16d9@brauner>
Date: Wed, 20 Nov 2024 12:12:59 +0100
From: Christian Brauner <brauner@...nel.org>
To: Mateusz Guzik <mjguzik@...il.com>
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:42:33AM +0100, Mateusz Guzik wrote:
> 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.

Ok!

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

Yeah, in this case I was specifically talking about make_vfs{g,u}id().
They should be inlines and they should contain likely/unlikely.

> 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

I snatched it.