diff mbox series

[f2fs-dev,v5,04/12] fscrypt: Drop d_revalidate for valid dentries during lookup

Message ID 20240129204330.32346-5-krisman@suse.de (mailing list archive)
State Superseded
Headers show
Series Set casefold/fscrypt dentry operations through sb->s_d_op | expand

Commit Message

Gabriel Krisman Bertazi Jan. 29, 2024, 8:43 p.m. UTC
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
avoid unnecessary revalidation, in preparation to always configuring
d_op through sb->s_d_op.

Since the filesystem might have other features that require
revalidation, only apply this optimization if the d_revalidate handler
is fscrypt_d_revalidate itself.

Signed-off-by: Gabriel Krisman Bertazi <krisman@suse.de>
---
 include/linux/fscrypt.h | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

Comments

Eric Biggers Jan. 31, 2024, 12:47 a.m. UTC | #1
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.

- Eric
Gabriel Krisman Bertazi Jan. 31, 2024, 6:35 p.m. UTC | #2
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?
Eric Biggers Feb. 1, 2024, 3:24 a.m. UTC | #3
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
Gabriel Krisman Bertazi Feb. 2, 2024, 2:50 p.m. UTC | #4
Eric Biggers <ebiggers@kernel.org> writes:

> 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.

Yes, I was looking for the actual race that could happen here, and
couldn't find one. As far as I understand it, the only thing that could
see the dentry during a lookup would be a parallel lookup, but those
will be held waiting for completion in d_alloc_parallel, and won't touch
d_flags.  Currently, right after this code, we call d_set_d_op() in
generic_set_encrypted_ci_d_ops(), which will happily write d_flags without
the d_lock. If this is a problem here, we have a problem there.

What I really don't want to do is keep the lock for DCACHE_NOKEY_NAME,
but drop it for unsetting DCACHE_OP_REVALIDATE right in the same field,
without a good reason.  I get the argument that unencrypted
dentries are a much hotter path and we care more.  But the locking rules
of ->d_lookup don't change for both cases.

So, I'd rather drop the d_lock entirely in this path, not only for the
hunk I'm proposing.  It would be good to get an actual confirmation from
Al or Christian, though.

CC'ing Christian.
Christian Brauner Feb. 9, 2024, 2:03 p.m. UTC | #5
On Fri, Feb 02, 2024 at 11:50:07AM -0300, Gabriel Krisman Bertazi wrote:
> Eric Biggers <ebiggers@kernel.org> writes:
> 
> > 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.
> 
> Yes, I was looking for the actual race that could happen here, and
> couldn't find one. As far as I understand it, the only thing that could
> see the dentry during a lookup would be a parallel lookup, but those
> will be held waiting for completion in d_alloc_parallel, and won't touch
> d_flags.  Currently, right after this code, we call d_set_d_op() in
> generic_set_encrypted_ci_d_ops(), which will happily write d_flags without
> the d_lock. If this is a problem here, we have a problem there.
> 
> What I really don't want to do is keep the lock for DCACHE_NOKEY_NAME,
> but drop it for unsetting DCACHE_OP_REVALIDATE right in the same field,
> without a good reason.  I get the argument that unencrypted
> dentries are a much hotter path and we care more.  But the locking rules
> of ->d_lookup don't change for both cases.

Even if it were to work in this case I don't think it is generally safe
to do. But also, for DCACHE_OP_REVALIDATE afaict this is an
optimization. Why don't you simply accept the raciness, just like fuse
does in fuse_dentry_settime(), check for DCACHE_OP_REVALIDATE locklessly
and only take the lock if that thing is set?
Gabriel Krisman Bertazi Feb. 9, 2024, 2:46 p.m. UTC | #6
Christian Brauner <brauner@kernel.org> writes:

> On Fri, Feb 02, 2024 at 11:50:07AM -0300, Gabriel Krisman Bertazi wrote:
>> Eric Biggers <ebiggers@kernel.org> writes:
>> 
>> > 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.
>> 
>> Yes, I was looking for the actual race that could happen here, and
>> couldn't find one. As far as I understand it, the only thing that could
>> see the dentry during a lookup would be a parallel lookup, but those
>> will be held waiting for completion in d_alloc_parallel, and won't touch
>> d_flags.  Currently, right after this code, we call d_set_d_op() in
>> generic_set_encrypted_ci_d_ops(), which will happily write d_flags without
>> the d_lock. If this is a problem here, we have a problem there.
>> 
>> What I really don't want to do is keep the lock for DCACHE_NOKEY_NAME,
>> but drop it for unsetting DCACHE_OP_REVALIDATE right in the same field,
>> without a good reason.  I get the argument that unencrypted
>> dentries are a much hotter path and we care more.  But the locking rules
>> of ->d_lookup don't change for both cases.
>
> Even if it were to work in this case I don't think it is generally safe
> to do. But also, for DCACHE_OP_REVALIDATE afaict this is an
> optimization. Why don't you simply accept the raciness, just like fuse
> does in fuse_dentry_settime(), check for DCACHE_OP_REVALIDATE locklessly
> and only take the lock if that thing is set?

That sounds extremely reasonable.  I will follow that approach!

Thanks,
diff mbox series

Patch

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);
 }
 
 /**