From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from outgoing.mit.edu (outgoing-auth-1.mit.edu [18.9.28.11]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 7A37C203716 for ; Sat, 12 Apr 2025 21:53:23 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=18.9.28.11 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1744494805; cv=none; b=rwVeWRWfN3YVlcFx7BxMCzH+atJ6Xox6gInFZvKF0rfq9Dts95px2lxdsTTGraFD72dqNw2Ymrpum2+kb7q0FN0A7TAEDRTRgI0G6zz2JBMXklmSlco5jMbuMor6mMKP2WbemrNIbDDAecwtdUCMBhI/cv+ECl4jalFKcqYwDF0= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1744494805; c=relaxed/simple; bh=Zbs/TcJE/Yr0ARJquYUmR8vlylohAXTmvMb/IC0mplU=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=L6SxIbDDcQrWktp4k37YTnc16ubvZQdp8OtrS0LtI1mmoeXlNZCyHRtx1dyL9x87bKw5JJI0rKcF42KqC3YKnSmKB6PsIFhRDUcm3HWsZHQIj6nrZdeycrynfz/xMt3+p3RYUMT+wUXcqgp0CI3M5asUoSH4gcRFJucIjhDBQIY= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=mit.edu; spf=pass smtp.mailfrom=mit.edu; arc=none smtp.client-ip=18.9.28.11 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=mit.edu Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=mit.edu Received: from trampoline.thunk.org (pool-173-48-82-137.bstnma.fios.verizon.net [173.48.82.137]) (authenticated bits=0) (User authenticated as tytso@ATHENA.MIT.EDU) by outgoing.mit.edu (8.14.7/8.12.4) with ESMTP id 53CLqwkd017084 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Sat, 12 Apr 2025 17:52:58 -0400 Received: by trampoline.thunk.org (Postfix, from userid 15806) id C3C412E00E9; Sat, 12 Apr 2025 17:52:57 -0400 (EDT) Date: Sat, 12 Apr 2025 17:52:57 -0400 From: "Theodore Ts'o" To: Mateusz Guzik Cc: Linus Torvalds , Christian Brauner , Al Viro , linux-fsdevel , Jan Kara , Ext4 Developers List Subject: Re: generic_permission() optimization Message-ID: <20250412215257.GF13132@mit.edu> References: <20241031-klaglos-geldmangel-c0e7775d42a7@brauner> Precedence: bulk X-Mailing-List: linux-fsdevel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: On Sat, Apr 12, 2025 at 06:26:09PM +0200, Mateusz Guzik wrote: > > 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 Linus, what problems did you run into? > > /* 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. Linus, were you running into false positives or false negatives? You said "failed miserably", which seems to imply that you ran into cases where cache_no_acl() got called when the inode actually had an ACL --- e.g., a false positive. That's different from what Mateuz is reporting which is that most inodes w/o ACL were getting cache_no_acl(), but some cases cache_no_acl() was't getting called when it should. That is, a flase negative: > 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. So the test which we're talking about: > 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); > tests if the inode does not have an extended attribute. Posix ACL's are stored as xattr's --- but if there are any extended attributes (or in some cases, inline data), in order to authoratatively determine whether there is an ACL or not will require iterating over all of the extended attributes. This can be rather heavyweight, so doing it unconditionally any time we do an iget() is probably not warranted. Still, if this works 99.99% of the time, given that most peple don't enable inline_data, and most folks aren't setting extended attributes, it should be fine. Of course, given that SELinux's security ID's are stored as extended attriutes, at that point this optimization won't work. But if you are using SELinux, you probably don't care about performance anyway. :-) - Ted