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