Received-SPF: pass (google.com: domain of linux-ext4+bounces-1048-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Wed, 31 Jan 2024 19:24:33 -0800
From: Eric Biggers <ebiggers@kernel.org>
To: Gabriel Krisman Bertazi <krisman@suse.de>
Cc: viro@zeniv.linux.org.uk, jaegeuk@kernel.org, tytso@mit.edu,
	amir73il@gmail.com, linux-ext4@vger.kernel.org,
	linux-f2fs-devel@lists.sourceforge.net,
	linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH v5 04/12] fscrypt: Drop d_revalidate for valid dentries
 during lookup
Message-ID: <20240201032433.GB1526@sol.localdomain>
References: <20240129204330.32346-1-krisman@suse.de>
 <20240129204330.32346-5-krisman@suse.de>
 <20240131004724.GC2020@sol.localdomain>
 <871q9x2vwj.fsf@mailhost.krisman.be>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <871q9x2vwj.fsf@mailhost.krisman.be>

On Wed, Jan 31, 2024 at 03:35:40PM -0300, Gabriel Krisman Bertazi wrote:
> Eric Biggers <ebiggers@kernel.org> writes:
> 
> > On Mon, Jan 29, 2024 at 05:43:22PM -0300, Gabriel Krisman Bertazi wrote:
> >> Unencrypted and encrypted-dentries where the key is available don't need
> >> to be revalidated with regards to fscrypt, since they don't go stale
> >> from under VFS and the key cannot be removed for the encrypted case
> >> without evicting the dentry.  Mark them with d_set_always_valid, to
> >
> > "d_set_always_valid" doesn't appear in the diff itself.
> >
> >> diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
> >> index 4aaf847955c0..a22997b9f35c 100644
> >> --- a/include/linux/fscrypt.h
> >> +++ b/include/linux/fscrypt.h
> >> @@ -942,11 +942,22 @@ static inline int fscrypt_prepare_rename(struct inode *old_dir,
> >>  static inline void fscrypt_prepare_lookup_dentry(struct dentry *dentry,
> >>  						 bool is_nokey_name)
> >>  {
> >> -	if (is_nokey_name) {
> >> -		spin_lock(&dentry->d_lock);
> >> +	spin_lock(&dentry->d_lock);
> >> +
> >> +	if (is_nokey_name)
> >>  		dentry->d_flags |= DCACHE_NOKEY_NAME;
> >> -		spin_unlock(&dentry->d_lock);
> >> +	else if (dentry->d_flags & DCACHE_OP_REVALIDATE &&
> >> +		 dentry->d_op->d_revalidate == fscrypt_d_revalidate) {
> >> +		/*
> >> +		 * Unencrypted dentries and encrypted dentries where the
> >> +		 * key is available are always valid from fscrypt
> >> +		 * perspective. Avoid the cost of calling
> >> +		 * fscrypt_d_revalidate unnecessarily.
> >> +		 */
> >> +		dentry->d_flags &= ~DCACHE_OP_REVALIDATE;
> >>  	}
> >> +
> >> +	spin_unlock(&dentry->d_lock);
> >
> > This makes lookups in unencrypted directories start doing the
> > spin_lock/spin_unlock pair.  Is that really necessary?
> >
> > These changes also make the inline function fscrypt_prepare_lookup() very long
> > (when including the fscrypt_prepare_lookup_dentry() that's inlined into it).
> > The rule that I'm trying to follow is that to the extent that the fscrypt helper
> > functions are inlined, the inline part should be a fast path for unencrypted
> > directories.  Encrypted directories should be handled out-of-line.
> >
> > So looking at the original fscrypt_prepare_lookup():
> >
> > 	static inline int fscrypt_prepare_lookup(struct inode *dir,
> > 						 struct dentry *dentry,
> > 						 struct fscrypt_name *fname)
> > 	{
> > 		if (IS_ENCRYPTED(dir))
> > 			return __fscrypt_prepare_lookup(dir, dentry, fname);
> >
> > 		memset(fname, 0, sizeof(*fname));
> > 		fname->usr_fname = &dentry->d_name;
> > 		fname->disk_name.name = (unsigned char *)dentry->d_name.name;
> > 		fname->disk_name.len = dentry->d_name.len;
> > 		return 0;
> > 	}
> >
> > If you could just add the DCACHE_OP_REVALIDATE clearing for dentries in
> > unencrypted directories just before the "return 0;", hopefully without the
> > spinlock, that would be good.  Yes, that does mean that
> > __fscrypt_prepare_lookup() will have to handle it too, for the case of dentries
> > in encrypted directories, but that seems okay.
> 
> ok, will do.  IIUC, we might be able to do without the d_lock
> provided there is no store tearing.
> 
> But what was the reason you need the d_lock to set DCACHE_NOKEY_NAME
> during lookup?  Is there a race with parallel lookup setting d_flag that
> I couldn't find? Or is it another reason?

d_flags is documented to be protected by d_lock.  So for setting
DCACHE_NOKEY_NAME, fs/crypto/ just does the safe thing of taking d_lock.  I
never really looked into whether the lock can be skipped there (i.e., whether
anything else can change d_flags while ->lookup is running), since this code
only ran for no-key names, for which performance isn't really important.

This patch would extend that locking to a new context in which it would be
executed several orders of magnitude more often.  So, making sure it's properly
optimized becomes more important.  It looks like it *might* be the case that
->lookup has exclusive access to d_flags, by virtue of having allocated the
dentry, so I'm just wondering if we can take advantage of that (or whether in
classic VFS fashion there's some edge case where that assumption is wrong).

- Eric