diff mbox series

[v19,69/70] ceph: switch ceph_open() to use new fscrypt helper

Message ID 20230417032654.32352-70-xiubli@redhat.com (mailing list archive)
State New, archived
Headers show
Series ceph+fscrypt: full support | expand

Commit Message

Xiubo Li April 17, 2023, 3:26 a.m. UTC
From: Luís Henriques <lhenriques@suse.de>

Instead of setting the no-key dentry in ceph_lookup(), use the new
fscrypt_prepare_lookup_partial() helper.  We still need to mark the
directory as incomplete if the directory was just unlocked.

Tested-by: Luís Henriques <lhenriques@suse.de>
Tested-by: Venky Shankar <vshankar@redhat.com>
Signed-off-by: Luís Henriques <lhenriques@suse.de>
Signed-off-by: Xiubo Li <xiubli@redhat.com>
---
 fs/ceph/dir.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

Comments

Milind Changire June 6, 2023, 6:25 a.m. UTC | #1
nit:
the commit title refers to ceph_open() but the code changes are
pertaining to ceph_lookup()

Otherwise it looks good to me.

Reviewed-by: Milind Changire <mchangir@redhat.com>

On Mon, Apr 17, 2023 at 9:03 AM <xiubli@redhat.com> wrote:
>
> From: Luís Henriques <lhenriques@suse.de>
>
> Instead of setting the no-key dentry in ceph_lookup(), use the new
> fscrypt_prepare_lookup_partial() helper.  We still need to mark the
> directory as incomplete if the directory was just unlocked.
>
> Tested-by: Luís Henriques <lhenriques@suse.de>
> Tested-by: Venky Shankar <vshankar@redhat.com>
> Signed-off-by: Luís Henriques <lhenriques@suse.de>
> Signed-off-by: Xiubo Li <xiubli@redhat.com>
> ---
>  fs/ceph/dir.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> index fe48a5d26c1d..c28de23e12a1 100644
> --- a/fs/ceph/dir.c
> +++ b/fs/ceph/dir.c
> @@ -784,14 +784,15 @@ static struct dentry *ceph_lookup(struct inode *dir, struct dentry *dentry,
>                 return ERR_PTR(-ENAMETOOLONG);
>
>         if (IS_ENCRYPTED(dir)) {
> -               err = ceph_fscrypt_prepare_readdir(dir);
> +               bool had_key = fscrypt_has_encryption_key(dir);
> +
> +               err = fscrypt_prepare_lookup_partial(dir, dentry);
>                 if (err < 0)
>                         return ERR_PTR(err);
> -               if (!fscrypt_has_encryption_key(dir)) {
> -                       spin_lock(&dentry->d_lock);
> -                       dentry->d_flags |= DCACHE_NOKEY_NAME;
> -                       spin_unlock(&dentry->d_lock);
> -               }
> +
> +               /* mark directory as incomplete if it has been unlocked */
> +               if (!had_key && fscrypt_has_encryption_key(dir))
> +                       ceph_dir_clear_complete(dir);
>         }
>
>         /* can we conclude ENOENT locally? */
> --
> 2.39.1
>
Xiubo Li June 6, 2023, 8:37 a.m. UTC | #2
On 6/6/23 14:25, Milind Changire wrote:
> nit:
> the commit title refers to ceph_open() but the code changes are
> pertaining to ceph_lookup()

Good catch.

I couldn't remember why we didn't fix this before as I remembered I have 
found this.

Thanks

- Xiubo


> Otherwise it looks good to me.
>
> Reviewed-by: Milind Changire <mchangir@redhat.com>
>
> On Mon, Apr 17, 2023 at 9:03 AM <xiubli@redhat.com> wrote:
>> From: Luís Henriques <lhenriques@suse.de>
>>
>> Instead of setting the no-key dentry in ceph_lookup(), use the new
>> fscrypt_prepare_lookup_partial() helper.  We still need to mark the
>> directory as incomplete if the directory was just unlocked.
>>
>> Tested-by: Luís Henriques <lhenriques@suse.de>
>> Tested-by: Venky Shankar <vshankar@redhat.com>
>> Signed-off-by: Luís Henriques <lhenriques@suse.de>
>> Signed-off-by: Xiubo Li <xiubli@redhat.com>
>> ---
>>   fs/ceph/dir.c | 13 +++++++------
>>   1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
>> index fe48a5d26c1d..c28de23e12a1 100644
>> --- a/fs/ceph/dir.c
>> +++ b/fs/ceph/dir.c
>> @@ -784,14 +784,15 @@ static struct dentry *ceph_lookup(struct inode *dir, struct dentry *dentry,
>>                  return ERR_PTR(-ENAMETOOLONG);
>>
>>          if (IS_ENCRYPTED(dir)) {
>> -               err = ceph_fscrypt_prepare_readdir(dir);
>> +               bool had_key = fscrypt_has_encryption_key(dir);
>> +
>> +               err = fscrypt_prepare_lookup_partial(dir, dentry);
>>                  if (err < 0)
>>                          return ERR_PTR(err);
>> -               if (!fscrypt_has_encryption_key(dir)) {
>> -                       spin_lock(&dentry->d_lock);
>> -                       dentry->d_flags |= DCACHE_NOKEY_NAME;
>> -                       spin_unlock(&dentry->d_lock);
>> -               }
>> +
>> +               /* mark directory as incomplete if it has been unlocked */
>> +               if (!had_key && fscrypt_has_encryption_key(dir))
>> +                       ceph_dir_clear_complete(dir);
>>          }
>>
>>          /* can we conclude ENOENT locally? */
>> --
>> 2.39.1
>>
>
Luis Henriques June 6, 2023, 9:05 a.m. UTC | #3
Xiubo Li <xiubli@redhat.com> writes:

> On 6/6/23 14:25, Milind Changire wrote:
>> nit:
>> the commit title refers to ceph_open() but the code changes are
>> pertaining to ceph_lookup()
>
> Good catch.
>
> I couldn't remember why we didn't fix this before as I remembered I have found
> this.

Yeah, it's really odd we didn't catch this before.  I had to go look at
old email: this helper was initially used in ceph_atomic_open() only, but
then in v3 it was made more generic so that it could also be used in
ceph_lookup().

Xiubo, do you want me to send out a new version of this patch, or are you
OK simply updating the subject, using 'ceph_lookup' instead of
'ceph_open'?

Cheers,
Xiubo Li June 6, 2023, 9:09 a.m. UTC | #4
On 6/6/23 17:05, Luís Henriques wrote:
> Xiubo Li <xiubli@redhat.com> writes:
>
>> On 6/6/23 14:25, Milind Changire wrote:
>>> nit:
>>> the commit title refers to ceph_open() but the code changes are
>>> pertaining to ceph_lookup()
>> Good catch.
>>
>> I couldn't remember why we didn't fix this before as I remembered I have found
>> this.
> Yeah, it's really odd we didn't catch this before.  I had to go look at
> old email: this helper was initially used in ceph_atomic_open() only, but
> then in v3 it was made more generic so that it could also be used in
> ceph_lookup().
>
> Xiubo, do you want me to send out a new version of this patch, or are you
> OK simply updating the subject, using 'ceph_lookup' instead of
> 'ceph_open'?

No worry, I have fixed this in the 'testing' branch and I will send out 
the v20 for this whole series later.

Thank

- Xiubo

> Cheers,
diff mbox series

Patch

diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
index fe48a5d26c1d..c28de23e12a1 100644
--- a/fs/ceph/dir.c
+++ b/fs/ceph/dir.c
@@ -784,14 +784,15 @@  static struct dentry *ceph_lookup(struct inode *dir, struct dentry *dentry,
 		return ERR_PTR(-ENAMETOOLONG);
 
 	if (IS_ENCRYPTED(dir)) {
-		err = ceph_fscrypt_prepare_readdir(dir);
+		bool had_key = fscrypt_has_encryption_key(dir);
+
+		err = fscrypt_prepare_lookup_partial(dir, dentry);
 		if (err < 0)
 			return ERR_PTR(err);
-		if (!fscrypt_has_encryption_key(dir)) {
-			spin_lock(&dentry->d_lock);
-			dentry->d_flags |= DCACHE_NOKEY_NAME;
-			spin_unlock(&dentry->d_lock);
-		}
+
+		/* mark directory as incomplete if it has been unlocked */
+		if (!had_key && fscrypt_has_encryption_key(dir))
+			ceph_dir_clear_complete(dir);
 	}
 
 	/* can we conclude ENOENT locally? */