Re: generic_permission() optimization

["Theodore Ts'o" <tytso@mit.edu>]

On Sat, Apr 12, 2025 at 03:36:00PM -0700, Linus Torvalds wrote:
> Indeed. I sent a query to the ext4 list (and I think you) about
> whether my test was even the right one.

Sorry, I must have not seen that message; at least, I don't have any
memory of it.

> Also, while I did a "getfattr -dR" to see if there are any *existing*
> attributes (and couldn't find any), I also assume that if a file has
> ever *had* any attributes, the filesystem may have the attribute block
> allocated even if it's now empty.

Well, getfattr will only show user xattrs.  It won't show security.*
xattr's that might have been set by SELinux, or a
system.posix_acl_access xattr.

> I assume there's some trivial e2fstools thing to show things like
> that, but it needs more ext4 specific knowledge than I have.

Yes, we can test for this using the debugfs command.  For exaple:

root@kvm-xfstests:~# debugfs /dev/vdc
debugfs 1.47.2-rc1 (28-Nov-2024)
debugfs:  stat <13>
Inode: 13   Type: regular    Mode:  0644   Flags: 0x80000
Generation: 1672288850    Version: 0x00000000:00000003
User:     0   Group:     0   Project:     0   Size: 286
File ACL: 0
Links: 1   Blockcount: 8
Fragment:  Address: 0    Number: 0    Size: 0
 ctime: 0x67faf5d0:30d0b2e4 -- Sat Apr 12 19:22:56 2025
 atime: 0x67faf571:7064bd50 -- Sat Apr 12 19:21:21 2025
 mtime: 0x67faf571:71236aa8 -- Sat Apr 12 19:21:21 2025
crtime: 0x67faf571:7064bd50 -- Sat Apr 12 19:21:21 2025
Size of extra inode fields: 32
Extended attributes:
  system.posix_acl_access (28) = 01 00 00 00 01 00 06 00 02 00 04 00 b7 7a 00 00 04 00 04 00 10 00 04 00 20 00 04 00 
Inode checksum: 0xc8f7f1a7
EXTENTS:
(0):33792

(If you know the pathname instead of the inode number, you can also
give that to debugfs's stat command, e.g., "stat /lost+found")

I tested it with a simple variant of your patch, and seems to do the right
thing.  Mateusz, if you want, try the following patch, and then mount
your test file system with "mount -o debug".  (The test_opt is to
avoid a huge amount of noise on your root file system; you can skip it
if it's more trouble than it's worth.)  The patch has a reversed
seense of the test, so it will print a message for every one where
cache_no_acl *wouldn't* be called.  You casn then use debugfs's "stat
<ino#>" to verify whether it has some kind of extended attribute.

	   	  	     	      - Ted

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index f386de8c12f6..3e0ba7c4723a 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -5109,6 +5109,11 @@ struct inode *__ext4_iget(struct super_block *sb, unsigned long ino,
 		goto bad_inode;
 	brelse(iloc.bh);
 
+	if (test_opt(sb, DEBUG) &&
+	    (ext4_test_inode_state(inode, EXT4_STATE_XATTR) ||
+	     ei->i_file_acl))
+		ext4_msg(sb, KERN_DEBUG, "has xattr ino %lu", inode->i_ino);
+
 	unlock_new_inode(inode);
 	return inode;