On Thu, Nov 7, 2024 at 11:49 PM Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> On Thu, 7 Nov 2024 at 12:22, Mateusz Guzik <mjguzik@xxxxxxxxx> 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>
