| Message ID | 20230417032654.32352-70-xiubli@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ceph+fscrypt: full support | expand |
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 >
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 >> >
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,
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 --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? */