From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (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 BF16F7483; Mon, 14 Apr 2025 10:21:33 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1744626093; cv=none; b=hVlDOjqYLxL/El1CeZG+p/7yJRexREJ16jXUFWH88uJ+aLkciukzcZe724s5rLRVyiivFfr4aqyUdrmhjTelj3E9cXmHOQrcDbLhf7FUCe9lL80SdlzEz3iZOxTiiu6vB+tqUNyGrEb9amCnnX5HxDNaizu26wesHORbTL6ay1c= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1744626093; c=relaxed/simple; bh=gDtoiQ4YmKw5TbLSgIYdRdUHgd1ZOp3Djt6LT8KREdc=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=FPLuCtaoxjTbu3HOymVhg1qykQafJtyTqAPuODl7WBtzxlxwvThLqsVql+vIu9iREU0oFFkilM4HLJH4KCwWpMh5EMiYJaocx1Ak4sIOHHlnaGXgQcsFLcoWCIzkohXbzlSn/57SEiyuju/krpdTeeNTAESPkFfAHhuimcWxjbU= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=D4cqcuge; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="D4cqcuge" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 9C34AC4CEE2; Mon, 14 Apr 2025 10:21:31 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1744626093; bh=gDtoiQ4YmKw5TbLSgIYdRdUHgd1ZOp3Djt6LT8KREdc=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=D4cqcugeP36P1G9DxMdOicsUIS6zOFT/QWuzAG994ryT1y1+xhPaLA24Pm2htPjJl ZRettR0D34pQibbNqCGrfmpkF//yXwe3E+2XQX7MbRpJnyUBE0UW190zMhThWrDUjd t6qeeLdwzeYRBPd26fhXPVO4Ip6x17xsn1cNUZDIJLVmsnVENKQ6LYnnz3zWh/HaOl NZCpQkDCaklXgjBuEjV+Le81wExupsYPGBBRg8aQQadyVERHmV6jLiQ5AkXtIl7FaS 5hOBOKaHdmOK/88z0ORngEoITGK4kII4LTMaUHZshpQpyNzJzrTYi3+P82dyeSVCES 7lO7HNUs6yB+A== Date: Mon, 14 Apr 2025 12:21:28 +0200 From: Christian Brauner To: Linus Torvalds Cc: Mateusz Guzik , Al Viro , linux-fsdevel , Jan Kara , Ext4 Developers List Subject: Re: generic_permission() optimization Message-ID: <20250414-anomalie-abpfiff-9f293dce366b@brauner> References: <20241031-klaglos-geldmangel-c0e7775d42a7@brauner> Precedence: bulk X-Mailing-List: linux-ext4@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: On Sat, Apr 12, 2025 at 01:22:38PM -0700, Linus Torvalds wrote: > On Sat, 12 Apr 2025 at 09:26, Mateusz Guzik wrote: > > > > I plopped your snippet towards the end of __ext4_iget: > > That's literally where I did the same thing, except I put it right after the > > brelse(iloc.bh); > > line, rather than before as you did. > > And it made no difference for me, but I didn't try to figure out why. > Maybe some environment differences? Or maybe I just screwed up my > testing... > > As mentioned earlier in the thread, I had this bi-modal distribution > of results, because if I had a load where the *non*-owner of the inode > looked up the pathnames, then the ACL information would get filled in > when the VFS layer would do the lookup, and then once the ACLs were > cached, everything worked beautifully. > > But if the only lookups of a path were done by the owner of the inodes > (which is typical for at least my normal kernel build tree - nothing > but my build will look at the files, and they are obviously always > owned by me) then the ACL caches will never be filled because there > will never be any real ACL lookups. > > And then rather than doing the nice efficient "no ACLs anywhere, no > need to even look", it ends up having to actually do the vfsuid > comparison for the UID equality check. > > Which then does the extra accesses to look up the idmap etc, and is > visible in the profiles due to that whole dance: > > /* Are we the owner? If so, ACL's don't matter */ > vfsuid = i_uid_into_vfsuid(idmap, inode); > if (likely(vfsuid_eq_kuid(vfsuid, current_fsuid()))) { > > even when idmap is 'nop_mnt_idmap' and it is reasonably cheap. Just > because it ends up calling out to different functions and does extra > D$ accesses to the inode and the suberblock (ie i_user_ns() is this > > return inode->i_sb->s_user_ns; I think we can improve this. Right now multiple mounts from different superblocks can share the same struct mnt_idmap. But I can change the code so that struct mnt_idmap can only be shared between mounts from the same superblock. With that we could do: diff --git a/fs/mnt_idmapping.c b/fs/mnt_idmapping.c index a37991fdb194..a5ec15c8c754 100644 --- a/fs/mnt_idmapping.c +++ b/fs/mnt_idmapping.c @@ -20,6 +20,7 @@ struct mnt_idmap { struct uid_gid_map uid_map; struct uid_gid_map gid_map; + struct user_namespace *s_user_ns; refcount_t count; }; And then stuff like: static inline vfsuid_t i_uid_into_vfsuid(struct mnt_idmap *idmap, const struct inode *inode) { return make_vfsuid(idmap, i_user_ns(inode), inode->i_uid); } just becomes: static inline vfsuid_t i_uid_into_vfsuid(struct mnt_idmap *idmap, const struct inode *inode) { return make_vfsuid(idmap, inode->i_uid); } which means: vfsuid_t make_vfsuid(struct mnt_idmap *idmap, kuid_t kuid) { uid_t uid; if (idmap == &nop_mnt_idmap) return VFSUIDT_INIT(kuid); } will only have to verify nop_mnt_idmap and we never have to access the inode->i_sb->s_user_ns at all. I'll wip up a patch for this. > > so just to *see* that it's nop_mnt_idmap takes effort. > > One improvement might be to cache that 'nop_mnt_idmap' thing in the > inode as a flag. > > But it would be even better if the filesystem just initializes the > inode at inode read time to say "I have no ACL's for this inode" and > none of this code will even trigger. Yes, let's please do this.