Re: generic_permission() optimization

[Mateusz Guzik <mjguzik@gmail.com>]

On Thu, Nov 7, 2024 at 11:49 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Thu, 7 Nov 2024 at 12:22, Mateusz Guzik <mjguzik@gmail.com> wrote:
> >
> > How about filesystems maintaing a flag: IOP_EVERYONECANTRAREVERSE?
>
> It's actually just easier if a filesystem just does
>
>         cache_no_acl(inode);
>
> in its read-inode function if it knows it has no ACL's.
>
> Some filesystems already do that, eg btrfs has
>
>         /*
>          * try to precache a NULL acl entry for files that don't have
>          * any xattrs or acls
>          */
>         ....
>         if (!maybe_acls)
>                 cache_no_acl(inode);
>
> in btrfs_read_locked_inode(). If that 'maybe' is just reliable enough,
> that's all it takes.
>
> I tried to do the same thing for ext4, and failed miserably, but
> that's probably because my logic for "maybe_acls" was broken since I'm
> not familiar enough with ext4 at that level, and I made it do just
>
>         /* Initialize the "no ACL's" state for the simple cases */
>         if (!ext4_test_inode_state(inode, EXT4_STATE_XATTR) && !ei->i_file_acl)
>                 cache_no_acl(inode);
>
> which doesn't seem to be a strong enough text.
>

[ roping in ext4 people ]

I plopped your snippet towards the end of __ext4_iget:

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 4008551bbb2d..34189d85e363 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -5071,7 +5071,12 @@ struct inode *__ext4_iget(struct super_block
*sb, unsigned long ino,
                goto bad_inode;
        }

+       /* Initialize the "no ACL's" state for the simple cases */
+       if (!ext4_test_inode_state(inode, EXT4_STATE_XATTR) && !ei->i_file_acl)
+               cache_no_acl(inode);
+
        brelse(iloc.bh);

bpftrace over a kernel build shows almost everything is sorted out:
 bpftrace -e 'kprobe:security_inode_permission { @[((struct inode
*)arg0)->i_acl] = count(); }'

@[0xffffffffffffffff]: 23810
@[0x0]: 65984202

That's just shy of 66 mln calls where the acls were explicitly set to
empty, compared to less than 24k where it was the default "uncached"
state.

So indeed *something* is missed, but the patch does cover almost everything.

Perhaps the ext4 guys would chime in and see it through? :)

The context is speeding path lookup by avoiding some of the branches
during permission checking.

-- 
Mateusz Guzik <mjguzik gmail.com>