[V7] ceph: do not dencrypt the dentry name twice for readdir

From: Xiubo Li <xiubli@redhat.com>

For the readdir request the dentries will be pasred and dencrypted
in ceph_readdir_prepopulate(). And in ceph_readdir() we could just
get the dentry name from the dentry cache instead of parsing and
dencrypting them again. This could improve performance.

Signed-off-by: Xiubo Li <xiubli@redhat.com>
---

V7:
- Fix the xfstest generic/006 crash bug about the rde->dentry == NULL.

V6:
- Remove CEPH_ENCRYPTED_LONG_SNAP_NAME_MAX macro and use the NAME_MAX
  instead, since we are limiting the max length of snapshot name to
  240, which is NAME_MAX - 2 * sizeof('_') - sizeof(<inode#>).

V5:
- fix typo of CEPH_ENCRYPTED_LONG_SNAP_NAME_MAX macro
- release the rde->dentry in destroy_reply_info



 fs/ceph/dir.c        | 56 ++++++++++++++++++++------------------------
 fs/ceph/inode.c      |  7 ++++++
 fs/ceph/mds_client.c |  1 +
 fs/ceph/mds_client.h |  1 +
 4 files changed, 34 insertions(+), 31 deletions(-)

On 3/9/22 9:59 PM, xiubli@redhat.com wrote:
> From: Xiubo Li <xiubli@redhat.com>
>
> For the readdir request the dentries will be pasred and dencrypted
> in ceph_readdir_prepopulate(). And in ceph_readdir() we could just
> get the dentry name from the dentry cache instead of parsing and
> dencrypting them again. This could improve performance.
>
> Signed-off-by: Xiubo Li <xiubli@redhat.com>
> ---
>
> V7:
> - Fix the xfstest generic/006 crash bug about the rde->dentry == NULL.
>
> V6:
> - Remove CEPH_ENCRYPTED_LONG_SNAP_NAME_MAX macro and use the NAME_MAX
>    instead, since we are limiting the max length of snapshot name to
>    240, which is NAME_MAX - 2 * sizeof('_') - sizeof(<inode#>).
>
> V5:
> - fix typo of CEPH_ENCRYPTED_LONG_SNAP_NAME_MAX macro
> - release the rde->dentry in destroy_reply_info
>
>
>
>   fs/ceph/dir.c        | 56 ++++++++++++++++++++------------------------
>   fs/ceph/inode.c      |  7 ++++++
>   fs/ceph/mds_client.c |  1 +
>   fs/ceph/mds_client.h |  1 +
>   4 files changed, 34 insertions(+), 31 deletions(-)
>
> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> index 6df2a91af236..2397c34e9173 100644
> --- a/fs/ceph/dir.c
> +++ b/fs/ceph/dir.c
> @@ -316,8 +316,7 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>   	int err;
>   	unsigned frag = -1;
>   	struct ceph_mds_reply_info_parsed *rinfo;
> -	struct fscrypt_str tname = FSTR_INIT(NULL, 0);
> -	struct fscrypt_str oname = FSTR_INIT(NULL, 0);
> +	char *dentry_name = NULL;
>   
>   	dout("readdir %p file %p pos %llx\n", inode, file, ctx->pos);
>   	if (dfi->file_info.flags & CEPH_F_ATEND)
> @@ -369,14 +368,6 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>   		spin_unlock(&ci->i_ceph_lock);
>   	}
>   
> -	err = ceph_fname_alloc_buffer(inode, &tname);
> -	if (err < 0)
> -		goto out;
> -
> -	err = ceph_fname_alloc_buffer(inode, &oname);
> -	if (err < 0)
> -		goto out;
> -
>   	/* proceed with a normal readdir */
>   more:
>   	/* do we have the correct frag content buffered? */
> @@ -528,31 +519,36 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>   			}
>   		}
>   	}
> +
> +	dentry_name = kmalloc(NAME_MAX, GFP_KERNEL);
> +	if (!dentry_name) {
> +		err = -ENOMEM;
> +		ceph_mdsc_put_request(dfi->last_readdir);
> +		dfi->last_readdir = NULL;
> +		goto out;
> +	}
> +
>   	for (; i < rinfo->dir_nr; i++) {
>   		struct ceph_mds_reply_dir_entry *rde = rinfo->dir_entries + i;
> -		struct ceph_fname fname = { .dir	= inode,
> -					    .name	= rde->name,
> -					    .name_len	= rde->name_len,
> -					    .ctext	= rde->altname,
> -					    .ctext_len	= rde->altname_len };
> -		u32 olen = oname.len;
> -
> -		err = ceph_fname_to_usr(&fname, &tname, &oname, NULL);
> -		if (err) {
> -			pr_err("%s unable to decode %.*s, got %d\n", __func__,
> -			       rde->name_len, rde->name, err);
> -			goto out;
> -		}
> +		struct dentry *dn = rde->dentry;
> +		int name_len;
>   
>   		BUG_ON(rde->offset < ctx->pos);
>   		BUG_ON(!rde->inode.in);
> +		BUG_ON(!rde->dentry);
>   
>   		ctx->pos = rde->offset;
> -		dout("readdir (%d/%d) -> %llx '%.*s' %p\n",
> -		     i, rinfo->dir_nr, ctx->pos,
> -		     rde->name_len, rde->name, &rde->inode.in);
>   
> -		if (!dir_emit(ctx, oname.name, oname.len,
> +		spin_lock(&dn->d_lock);
> +		memcpy(dentry_name, dn->d_name.name, dn->d_name.len);
> +		name_len = dn->d_name.len;
> +		spin_unlock(&dn->d_lock);
> +

Hi Jeff,

BTW, does the dn->d_lock is must here ? From the code comments, the 
d_lock intends to protect the 'd_flag' and 'd_lockref.count'.

In ceph code I found some places are using the d_lock when accessing the 
d_name, some are not. And in none ceph code, they will almost never use 
the d_lock to protect the d_name.

- Xiubo


> +		dentry_name[name_len] = '\0';
> +		dout("readdir (%d/%d) -> %llx '%s' %p\n",
> +		     i, rinfo->dir_nr, ctx->pos, dentry_name, &rde->inode.in);
> +
> +		if (!dir_emit(ctx, dentry_name, name_len,
>   			      ceph_present_ino(inode->i_sb, le64_to_cpu(rde->inode.in->ino)),
>   			      le32_to_cpu(rde->inode.in->mode) >> 12)) {
>   			/*
> @@ -566,8 +562,6 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>   			goto out;
>   		}
>   
> -		/* Reset the lengths to their original allocated vals */
> -		oname.len = olen;
>   		ctx->pos++;
>   	}
>   
> @@ -625,8 +619,8 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>   	err = 0;
>   	dout("readdir %p file %p done.\n", inode, file);
>   out:
> -	ceph_fname_free_buffer(inode, &tname);
> -	ceph_fname_free_buffer(inode, &oname);
> +	if (dentry_name)
> +		kfree(dentry_name);
>   	return err;
>   }
>   
> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> index b573a0f33450..19e5275eae1c 100644
> --- a/fs/ceph/inode.c
> +++ b/fs/ceph/inode.c
> @@ -1909,6 +1909,7 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
>   			goto out;
>   		}
>   
> +		rde->dentry = NULL;
>   		dname.name = oname.name;
>   		dname.len = oname.len;
>   		dname.hash = full_name_hash(parent, dname.name, dname.len);
> @@ -1969,6 +1970,12 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
>   			goto retry_lookup;
>   		}
>   
> +		/*
> +		 * ceph_readdir will use the dentry to get the name
> +		 * to avoid doing the dencrypt again there.
> +		 */
> +		rde->dentry = dget(dn);
> +
>   		/* inode */
>   		if (d_really_is_positive(dn)) {
>   			in = d_inode(dn);
> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> index 8d704ddd7291..9e0a51ef1dfa 100644
> --- a/fs/ceph/mds_client.c
> +++ b/fs/ceph/mds_client.c
> @@ -733,6 +733,7 @@ static void destroy_reply_info(struct ceph_mds_reply_info_parsed *info)
>   
>   		kfree(rde->inode.fscrypt_auth);
>   		kfree(rde->inode.fscrypt_file);
> +		dput(rde->dentry);
>   	}
>   	free_pages((unsigned long)info->dir_entries, get_order(info->dir_buf_size));
>   }
> diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
> index 0dfe24f94567..663d7754d57d 100644
> --- a/fs/ceph/mds_client.h
> +++ b/fs/ceph/mds_client.h
> @@ -96,6 +96,7 @@ struct ceph_mds_reply_info_in {
>   };
>   
>   struct ceph_mds_reply_dir_entry {
> +	struct dentry		      *dentry;
>   	char                          *name;
>   	u8			      *altname;
>   	u32                           name_len;

On 3/9/22 9:59 PM, xiubli@redhat.com wrote:
> From: Xiubo Li <xiubli@redhat.com>
>
> For the readdir request the dentries will be pasred and dencrypted
> in ceph_readdir_prepopulate(). And in ceph_readdir() we could just
> get the dentry name from the dentry cache instead of parsing and
> dencrypting them again. This could improve performance.
>
> Signed-off-by: Xiubo Li <xiubli@redhat.com>
> ---
>
> V7:
> - Fix the xfstest generic/006 crash bug about the rde->dentry == NULL.
>
> V6:
> - Remove CEPH_ENCRYPTED_LONG_SNAP_NAME_MAX macro and use the NAME_MAX
>    instead, since we are limiting the max length of snapshot name to
>    240, which is NAME_MAX - 2 * sizeof('_') - sizeof(<inode#>).
>
> V5:
> - fix typo of CEPH_ENCRYPTED_LONG_SNAP_NAME_MAX macro
> - release the rde->dentry in destroy_reply_info
>
>
>
>   fs/ceph/dir.c        | 56 ++++++++++++++++++++------------------------
>   fs/ceph/inode.c      |  7 ++++++
>   fs/ceph/mds_client.c |  1 +
>   fs/ceph/mds_client.h |  1 +
>   4 files changed, 34 insertions(+), 31 deletions(-)
>
> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> index 6df2a91af236..2397c34e9173 100644
> --- a/fs/ceph/dir.c
> +++ b/fs/ceph/dir.c
> @@ -316,8 +316,7 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>   	int err;
>   	unsigned frag = -1;
>   	struct ceph_mds_reply_info_parsed *rinfo;
> -	struct fscrypt_str tname = FSTR_INIT(NULL, 0);
> -	struct fscrypt_str oname = FSTR_INIT(NULL, 0);
> +	char *dentry_name = NULL;
>   
>   	dout("readdir %p file %p pos %llx\n", inode, file, ctx->pos);
>   	if (dfi->file_info.flags & CEPH_F_ATEND)
> @@ -369,14 +368,6 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>   		spin_unlock(&ci->i_ceph_lock);
>   	}
>   
> -	err = ceph_fname_alloc_buffer(inode, &tname);
> -	if (err < 0)
> -		goto out;
> -
> -	err = ceph_fname_alloc_buffer(inode, &oname);
> -	if (err < 0)
> -		goto out;
> -
>   	/* proceed with a normal readdir */
>   more:
>   	/* do we have the correct frag content buffered? */
> @@ -528,31 +519,36 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>   			}
>   		}
>   	}
> +
> +	dentry_name = kmalloc(NAME_MAX, GFP_KERNEL);
> +	if (!dentry_name) {
> +		err = -ENOMEM;
> +		ceph_mdsc_put_request(dfi->last_readdir);
> +		dfi->last_readdir = NULL;
> +		goto out;
> +	}
> +

This should move up before the 'more' tag.


>   	for (; i < rinfo->dir_nr; i++) {
>   		struct ceph_mds_reply_dir_entry *rde = rinfo->dir_entries + i;
> -		struct ceph_fname fname = { .dir	= inode,
> -					    .name	= rde->name,
> -					    .name_len	= rde->name_len,
> -					    .ctext	= rde->altname,
> -					    .ctext_len	= rde->altname_len };
> -		u32 olen = oname.len;
> -
> -		err = ceph_fname_to_usr(&fname, &tname, &oname, NULL);
> -		if (err) {
> -			pr_err("%s unable to decode %.*s, got %d\n", __func__,
> -			       rde->name_len, rde->name, err);
> -			goto out;
> -		}
> +		struct dentry *dn = rde->dentry;
> +		int name_len;
>   
>   		BUG_ON(rde->offset < ctx->pos);
>   		BUG_ON(!rde->inode.in);
> +		BUG_ON(!rde->dentry);
>   
>   		ctx->pos = rde->offset;
> -		dout("readdir (%d/%d) -> %llx '%.*s' %p\n",
> -		     i, rinfo->dir_nr, ctx->pos,
> -		     rde->name_len, rde->name, &rde->inode.in);
>   
> -		if (!dir_emit(ctx, oname.name, oname.len,
> +		spin_lock(&dn->d_lock);
> +		memcpy(dentry_name, dn->d_name.name, dn->d_name.len);
> +		name_len = dn->d_name.len;
> +		spin_unlock(&dn->d_lock);
> +
> +		dentry_name[name_len] = '\0';

Possibly caused by this. Since it useless here and I will remove it.


> +		dout("readdir (%d/%d) -> %llx '%s' %p\n",
> +		     i, rinfo->dir_nr, ctx->pos, dentry_name, &rde->inode.in);
> +
> +		if (!dir_emit(ctx, dentry_name, name_len,
>   			      ceph_present_ino(inode->i_sb, le64_to_cpu(rde->inode.in->ino)),
>   			      le32_to_cpu(rde->inode.in->mode) >> 12)) {
>   			/*
> @@ -566,8 +562,6 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>   			goto out;
>   		}
>   
> -		/* Reset the lengths to their original allocated vals */
> -		oname.len = olen;
>   		ctx->pos++;
>   	}
>   
> @@ -625,8 +619,8 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>   	err = 0;
>   	dout("readdir %p file %p done.\n", inode, file);
>   out:
> -	ceph_fname_free_buffer(inode, &tname);
> -	ceph_fname_free_buffer(inode, &oname);
> +	if (dentry_name)
> +		kfree(dentry_name);
>   	return err;
>   }
>   
> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> index b573a0f33450..19e5275eae1c 100644
> --- a/fs/ceph/inode.c
> +++ b/fs/ceph/inode.c
> @@ -1909,6 +1909,7 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
>   			goto out;
>   		}
>   
> +		rde->dentry = NULL;
>   		dname.name = oname.name;
>   		dname.len = oname.len;
>   		dname.hash = full_name_hash(parent, dname.name, dname.len);
> @@ -1969,6 +1970,12 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
>   			goto retry_lookup;
>   		}
>   
> +		/*
> +		 * ceph_readdir will use the dentry to get the name
> +		 * to avoid doing the dencrypt again there.
> +		 */
> +		rde->dentry = dget(dn);
> +
>   		/* inode */
>   		if (d_really_is_positive(dn)) {
>   			in = d_inode(dn);
> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> index 8d704ddd7291..9e0a51ef1dfa 100644
> --- a/fs/ceph/mds_client.c
> +++ b/fs/ceph/mds_client.c
> @@ -733,6 +733,7 @@ static void destroy_reply_info(struct ceph_mds_reply_info_parsed *info)
>   
>   		kfree(rde->inode.fscrypt_auth);
>   		kfree(rde->inode.fscrypt_file);
> +		dput(rde->dentry);
>   	}
>   	free_pages((unsigned long)info->dir_entries, get_order(info->dir_buf_size));
>   }
> diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
> index 0dfe24f94567..663d7754d57d 100644
> --- a/fs/ceph/mds_client.h
> +++ b/fs/ceph/mds_client.h
> @@ -96,6 +96,7 @@ struct ceph_mds_reply_info_in {
>   };
>   
>   struct ceph_mds_reply_dir_entry {
> +	struct dentry		      *dentry;
>   	char                          *name;
>   	u8			      *altname;
>   	u32                           name_len;

On Thu, 2022-03-10 at 14:08 +0800, Xiubo Li wrote:
> On 3/9/22 9:59 PM, xiubli@redhat.com wrote:
> > From: Xiubo Li <xiubli@redhat.com>
> > 
> > For the readdir request the dentries will be pasred and dencrypted
> > in ceph_readdir_prepopulate(). And in ceph_readdir() we could just
> > get the dentry name from the dentry cache instead of parsing and
> > dencrypting them again. This could improve performance.
> > 
> > Signed-off-by: Xiubo Li <xiubli@redhat.com>
> > ---
> > 
> > V7:
> > - Fix the xfstest generic/006 crash bug about the rde->dentry == NULL.
> > 
> > V6:
> > - Remove CEPH_ENCRYPTED_LONG_SNAP_NAME_MAX macro and use the NAME_MAX
> >    instead, since we are limiting the max length of snapshot name to
> >    240, which is NAME_MAX - 2 * sizeof('_') - sizeof(<inode#>).
> > 
> > V5:
> > - fix typo of CEPH_ENCRYPTED_LONG_SNAP_NAME_MAX macro
> > - release the rde->dentry in destroy_reply_info
> > 
> > 
> > 
> >   fs/ceph/dir.c        | 56 ++++++++++++++++++++------------------------
> >   fs/ceph/inode.c      |  7 ++++++
> >   fs/ceph/mds_client.c |  1 +
> >   fs/ceph/mds_client.h |  1 +
> >   4 files changed, 34 insertions(+), 31 deletions(-)
> > 
> > diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> > index 6df2a91af236..2397c34e9173 100644
> > --- a/fs/ceph/dir.c
> > +++ b/fs/ceph/dir.c
> > @@ -316,8 +316,7 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
> >   	int err;
> >   	unsigned frag = -1;
> >   	struct ceph_mds_reply_info_parsed *rinfo;
> > -	struct fscrypt_str tname = FSTR_INIT(NULL, 0);
> > -	struct fscrypt_str oname = FSTR_INIT(NULL, 0);
> > +	char *dentry_name = NULL;
> >   
> >   	dout("readdir %p file %p pos %llx\n", inode, file, ctx->pos);
> >   	if (dfi->file_info.flags & CEPH_F_ATEND)
> > @@ -369,14 +368,6 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
> >   		spin_unlock(&ci->i_ceph_lock);
> >   	}
> >   
> > -	err = ceph_fname_alloc_buffer(inode, &tname);
> > -	if (err < 0)
> > -		goto out;
> > -
> > -	err = ceph_fname_alloc_buffer(inode, &oname);
> > -	if (err < 0)
> > -		goto out;
> > -
> >   	/* proceed with a normal readdir */
> >   more:
> >   	/* do we have the correct frag content buffered? */
> > @@ -528,31 +519,36 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
> >   			}
> >   		}
> >   	}
> > +
> > +	dentry_name = kmalloc(NAME_MAX, GFP_KERNEL);
> > +	if (!dentry_name) {
> > +		err = -ENOMEM;
> > +		ceph_mdsc_put_request(dfi->last_readdir);
> > +		dfi->last_readdir = NULL;
> > +		goto out;
> > +	}
> > +
> >   	for (; i < rinfo->dir_nr; i++) {
> >   		struct ceph_mds_reply_dir_entry *rde = rinfo->dir_entries + i;
> > -		struct ceph_fname fname = { .dir	= inode,
> > -					    .name	= rde->name,
> > -					    .name_len	= rde->name_len,
> > -					    .ctext	= rde->altname,
> > -					    .ctext_len	= rde->altname_len };
> > -		u32 olen = oname.len;
> > -
> > -		err = ceph_fname_to_usr(&fname, &tname, &oname, NULL);
> > -		if (err) {
> > -			pr_err("%s unable to decode %.*s, got %d\n", __func__,
> > -			       rde->name_len, rde->name, err);
> > -			goto out;
> > -		}
> > +		struct dentry *dn = rde->dentry;
> > +		int name_len;
> >   
> >   		BUG_ON(rde->offset < ctx->pos);
> >   		BUG_ON(!rde->inode.in);
> > +		BUG_ON(!rde->dentry);
> >   
> >   		ctx->pos = rde->offset;
> > -		dout("readdir (%d/%d) -> %llx '%.*s' %p\n",
> > -		     i, rinfo->dir_nr, ctx->pos,
> > -		     rde->name_len, rde->name, &rde->inode.in);
> >   
> > -		if (!dir_emit(ctx, oname.name, oname.len,
> > +		spin_lock(&dn->d_lock);
> > +		memcpy(dentry_name, dn->d_name.name, dn->d_name.len);
> > +		name_len = dn->d_name.len;
> > +		spin_unlock(&dn->d_lock);
> > +
> 
> Hi Jeff,
> 
> BTW, does the dn->d_lock is must here ? From the code comments, the 
> d_lock intends to protect the 'd_flag' and 'd_lockref.count'.
> 
> In ceph code I found some places are using the d_lock when accessing the 
> d_name, some are not. And in none ceph code, they will almost never use 
> the d_lock to protect the d_name.
> 

It's probably not needed. The d_name can only change if there are no
other outstanding references to the dentry.

> 
> 
> > +		dentry_name[name_len] = '\0';
> > +		dout("readdir (%d/%d) -> %llx '%s' %p\n",
> > +		     i, rinfo->dir_nr, ctx->pos, dentry_name, &rde->inode.in);
> > +
> > +		if (!dir_emit(ctx, dentry_name, name_len,
> >   			      ceph_present_ino(inode->i_sb, le64_to_cpu(rde->inode.in->ino)),
> >   			      le32_to_cpu(rde->inode.in->mode) >> 12)) {
> >   			/*
> > @@ -566,8 +562,6 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
> >   			goto out;
> >   		}
> >   
> > -		/* Reset the lengths to their original allocated vals */
> > -		oname.len = olen;
> >   		ctx->pos++;
> >   	}
> >   
> > @@ -625,8 +619,8 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
> >   	err = 0;
> >   	dout("readdir %p file %p done.\n", inode, file);
> >   out:
> > -	ceph_fname_free_buffer(inode, &tname);
> > -	ceph_fname_free_buffer(inode, &oname);
> > +	if (dentry_name)
> > +		kfree(dentry_name);
> >   	return err;
> >   }
> >   
> > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> > index b573a0f33450..19e5275eae1c 100644
> > --- a/fs/ceph/inode.c
> > +++ b/fs/ceph/inode.c
> > @@ -1909,6 +1909,7 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
> >   			goto out;
> >   		}
> >   
> > +		rde->dentry = NULL;
> >   		dname.name = oname.name;
> >   		dname.len = oname.len;
> >   		dname.hash = full_name_hash(parent, dname.name, dname.len);
> > @@ -1969,6 +1970,12 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
> >   			goto retry_lookup;
> >   		}
> >   
> > +		/*
> > +		 * ceph_readdir will use the dentry to get the name
> > +		 * to avoid doing the dencrypt again there.
> > +		 */
> > +		rde->dentry = dget(dn);
> > +
> >   		/* inode */
> >   		if (d_really_is_positive(dn)) {
> >   			in = d_inode(dn);
> > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> > index 8d704ddd7291..9e0a51ef1dfa 100644
> > --- a/fs/ceph/mds_client.c
> > +++ b/fs/ceph/mds_client.c
> > @@ -733,6 +733,7 @@ static void destroy_reply_info(struct ceph_mds_reply_info_parsed *info)
> >   
> >   		kfree(rde->inode.fscrypt_auth);
> >   		kfree(rde->inode.fscrypt_file);
> > +		dput(rde->dentry);
> >   	}
> >   	free_pages((unsigned long)info->dir_entries, get_order(info->dir_buf_size));
> >   }
> > diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
> > index 0dfe24f94567..663d7754d57d 100644
> > --- a/fs/ceph/mds_client.h
> > +++ b/fs/ceph/mds_client.h
> > @@ -96,6 +96,7 @@ struct ceph_mds_reply_info_in {
> >   };
> >   
> >   struct ceph_mds_reply_dir_entry {
> > +	struct dentry		      *dentry;
> >   	char                          *name;
> >   	u8			      *altname;
> >   	u32                           name_len;
>

On Thu, 2022-03-10 at 16:22 +0800, Xiubo Li wrote:
> On 3/9/22 9:59 PM, xiubli@redhat.com wrote:
> > From: Xiubo Li <xiubli@redhat.com>
> > 
> > For the readdir request the dentries will be pasred and dencrypted
> > in ceph_readdir_prepopulate(). And in ceph_readdir() we could just
> > get the dentry name from the dentry cache instead of parsing and
> > dencrypting them again. This could improve performance.
> > 
> > Signed-off-by: Xiubo Li <xiubli@redhat.com>
> > ---
> > 
> > V7:
> > - Fix the xfstest generic/006 crash bug about the rde->dentry == NULL.
> > 
> > V6:
> > - Remove CEPH_ENCRYPTED_LONG_SNAP_NAME_MAX macro and use the NAME_MAX
> >    instead, since we are limiting the max length of snapshot name to
> >    240, which is NAME_MAX - 2 * sizeof('_') - sizeof(<inode#>).
> > 
> > V5:
> > - fix typo of CEPH_ENCRYPTED_LONG_SNAP_NAME_MAX macro
> > - release the rde->dentry in destroy_reply_info
> > 
> > 
> > 
> >   fs/ceph/dir.c        | 56 ++++++++++++++++++++------------------------
> >   fs/ceph/inode.c      |  7 ++++++
> >   fs/ceph/mds_client.c |  1 +
> >   fs/ceph/mds_client.h |  1 +
> >   4 files changed, 34 insertions(+), 31 deletions(-)
> > 
> > diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> > index 6df2a91af236..2397c34e9173 100644
> > --- a/fs/ceph/dir.c
> > +++ b/fs/ceph/dir.c
> > @@ -316,8 +316,7 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
> >   	int err;
> >   	unsigned frag = -1;
> >   	struct ceph_mds_reply_info_parsed *rinfo;
> > -	struct fscrypt_str tname = FSTR_INIT(NULL, 0);
> > -	struct fscrypt_str oname = FSTR_INIT(NULL, 0);
> > +	char *dentry_name = NULL;
> >   
> >   	dout("readdir %p file %p pos %llx\n", inode, file, ctx->pos);
> >   	if (dfi->file_info.flags & CEPH_F_ATEND)
> > @@ -369,14 +368,6 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
> >   		spin_unlock(&ci->i_ceph_lock);
> >   	}
> >   
> > -	err = ceph_fname_alloc_buffer(inode, &tname);
> > -	if (err < 0)
> > -		goto out;
> > -
> > -	err = ceph_fname_alloc_buffer(inode, &oname);
> > -	if (err < 0)
> > -		goto out;
> > -
> >   	/* proceed with a normal readdir */
> >   more:
> >   	/* do we have the correct frag content buffered? */
> > @@ -528,31 +519,36 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
> >   			}
> >   		}
> >   	}
> > +
> > +	dentry_name = kmalloc(NAME_MAX, GFP_KERNEL);
> > +	if (!dentry_name) {
> > +		err = -ENOMEM;
> > +		ceph_mdsc_put_request(dfi->last_readdir);
> > +		dfi->last_readdir = NULL;
> > +		goto out;
> > +	}
> > +
> 
> This should move up before the 'more' tag.
> 

Yep, good catch.

> 
> >   	for (; i < rinfo->dir_nr; i++) {
> >   		struct ceph_mds_reply_dir_entry *rde = rinfo->dir_entries + i;
> > -		struct ceph_fname fname = { .dir	= inode,
> > -					    .name	= rde->name,
> > -					    .name_len	= rde->name_len,
> > -					    .ctext	= rde->altname,
> > -					    .ctext_len	= rde->altname_len };
> > -		u32 olen = oname.len;
> > -
> > -		err = ceph_fname_to_usr(&fname, &tname, &oname, NULL);
> > -		if (err) {
> > -			pr_err("%s unable to decode %.*s, got %d\n", __func__,
> > -			       rde->name_len, rde->name, err);
> > -			goto out;
> > -		}
> > +		struct dentry *dn = rde->dentry;
> > +		int name_len;
> >   
> >   		BUG_ON(rde->offset < ctx->pos);
> >   		BUG_ON(!rde->inode.in);
> > +		BUG_ON(!rde->dentry);
> >   
> >   		ctx->pos = rde->offset;
> > -		dout("readdir (%d/%d) -> %llx '%.*s' %p\n",
> > -		     i, rinfo->dir_nr, ctx->pos,
> > -		     rde->name_len, rde->name, &rde->inode.in);
> >   
> > -		if (!dir_emit(ctx, oname.name, oname.len,
> > +		spin_lock(&dn->d_lock);
> > +		memcpy(dentry_name, dn->d_name.name, dn->d_name.len);
> > +		name_len = dn->d_name.len;
> > +		spin_unlock(&dn->d_lock);
> > +
> > +		dentry_name[name_len] = '\0';
> 
> Possibly caused by this. Since it useless here and I will remove it.
> 
> 

Seems plausible. If you send a v8 patch, I can give it another test and
see if it fixes it.

> > +		dout("readdir (%d/%d) -> %llx '%s' %p\n",
> > +		     i, rinfo->dir_nr, ctx->pos, dentry_name, &rde->inode.in);
> > +
> > +		if (!dir_emit(ctx, dentry_name, name_len,
> >   			      ceph_present_ino(inode->i_sb, le64_to_cpu(rde->inode.in->ino)),
> >   			      le32_to_cpu(rde->inode.in->mode) >> 12)) {
> >   			/*
> > @@ -566,8 +562,6 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
> >   			goto out;
> >   		}
> >   
> > -		/* Reset the lengths to their original allocated vals */
> > -		oname.len = olen;
> >   		ctx->pos++;
> >   	}
> >   
> > @@ -625,8 +619,8 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
> >   	err = 0;
> >   	dout("readdir %p file %p done.\n", inode, file);
> >   out:
> > -	ceph_fname_free_buffer(inode, &tname);
> > -	ceph_fname_free_buffer(inode, &oname);
> > +	if (dentry_name)
> > +		kfree(dentry_name);
> >   	return err;
> >   }
> >   
> > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> > index b573a0f33450..19e5275eae1c 100644
> > --- a/fs/ceph/inode.c
> > +++ b/fs/ceph/inode.c
> > @@ -1909,6 +1909,7 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
> >   			goto out;
> >   		}
> >   
> > +		rde->dentry = NULL;
> >   		dname.name = oname.name;
> >   		dname.len = oname.len;
> >   		dname.hash = full_name_hash(parent, dname.name, dname.len);
> > @@ -1969,6 +1970,12 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
> >   			goto retry_lookup;
> >   		}
> >   
> > +		/*
> > +		 * ceph_readdir will use the dentry to get the name
> > +		 * to avoid doing the dencrypt again there.
> > +		 */
> > +		rde->dentry = dget(dn);
> > +
> >   		/* inode */
> >   		if (d_really_is_positive(dn)) {
> >   			in = d_inode(dn);
> > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> > index 8d704ddd7291..9e0a51ef1dfa 100644
> > --- a/fs/ceph/mds_client.c
> > +++ b/fs/ceph/mds_client.c
> > @@ -733,6 +733,7 @@ static void destroy_reply_info(struct ceph_mds_reply_info_parsed *info)
> >   
> >   		kfree(rde->inode.fscrypt_auth);
> >   		kfree(rde->inode.fscrypt_file);
> > +		dput(rde->dentry);
> >   	}
> >   	free_pages((unsigned long)info->dir_entries, get_order(info->dir_buf_size));
> >   }
> > diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
> > index 0dfe24f94567..663d7754d57d 100644
> > --- a/fs/ceph/mds_client.h
> > +++ b/fs/ceph/mds_client.h
> > @@ -96,6 +96,7 @@ struct ceph_mds_reply_info_in {
> >   };
> >   
> >   struct ceph_mds_reply_dir_entry {
> > +	struct dentry		      *dentry;
> >   	char                          *name;
> >   	u8			      *altname;
> >   	u32                           name_len;
>

On 3/10/22 6:38 PM, Jeff Layton wrote:
> On Thu, 2022-03-10 at 16:22 +0800, Xiubo Li wrote:
>> On 3/9/22 9:59 PM, xiubli@redhat.com wrote:
>>> From: Xiubo Li <xiubli@redhat.com>
>>>
>>> For the readdir request the dentries will be pasred and dencrypted
>>> in ceph_readdir_prepopulate(). And in ceph_readdir() we could just
>>> get the dentry name from the dentry cache instead of parsing and
>>> dencrypting them again. This could improve performance.
>>>
>>> Signed-off-by: Xiubo Li <xiubli@redhat.com>
>>> ---
>>>
>>> V7:
>>> - Fix the xfstest generic/006 crash bug about the rde->dentry == NULL.
>>>
>>> V6:
>>> - Remove CEPH_ENCRYPTED_LONG_SNAP_NAME_MAX macro and use the NAME_MAX
>>>     instead, since we are limiting the max length of snapshot name to
>>>     240, which is NAME_MAX - 2 * sizeof('_') - sizeof(<inode#>).
>>>
>>> V5:
>>> - fix typo of CEPH_ENCRYPTED_LONG_SNAP_NAME_MAX macro
>>> - release the rde->dentry in destroy_reply_info
>>>
>>>
>>>
>>>    fs/ceph/dir.c        | 56 ++++++++++++++++++++------------------------
>>>    fs/ceph/inode.c      |  7 ++++++
>>>    fs/ceph/mds_client.c |  1 +
>>>    fs/ceph/mds_client.h |  1 +
>>>    4 files changed, 34 insertions(+), 31 deletions(-)
>>>
>>> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
>>> index 6df2a91af236..2397c34e9173 100644
>>> --- a/fs/ceph/dir.c
>>> +++ b/fs/ceph/dir.c
>>> @@ -316,8 +316,7 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>>>    	int err;
>>>    	unsigned frag = -1;
>>>    	struct ceph_mds_reply_info_parsed *rinfo;
>>> -	struct fscrypt_str tname = FSTR_INIT(NULL, 0);
>>> -	struct fscrypt_str oname = FSTR_INIT(NULL, 0);
>>> +	char *dentry_name = NULL;
>>>    
>>>    	dout("readdir %p file %p pos %llx\n", inode, file, ctx->pos);
>>>    	if (dfi->file_info.flags & CEPH_F_ATEND)
>>> @@ -369,14 +368,6 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>>>    		spin_unlock(&ci->i_ceph_lock);
>>>    	}
>>>    
>>> -	err = ceph_fname_alloc_buffer(inode, &tname);
>>> -	if (err < 0)
>>> -		goto out;
>>> -
>>> -	err = ceph_fname_alloc_buffer(inode, &oname);
>>> -	if (err < 0)
>>> -		goto out;
>>> -
>>>    	/* proceed with a normal readdir */
>>>    more:
>>>    	/* do we have the correct frag content buffered? */
>>> @@ -528,31 +519,36 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>>>    			}
>>>    		}
>>>    	}
>>> +
>>> +	dentry_name = kmalloc(NAME_MAX, GFP_KERNEL);
>>> +	if (!dentry_name) {
>>> +		err = -ENOMEM;
>>> +		ceph_mdsc_put_request(dfi->last_readdir);
>>> +		dfi->last_readdir = NULL;
>>> +		goto out;
>>> +	}
>>> +
>> This should move up before the 'more' tag.
>>
> Yep, good catch.
>
>>>    	for (; i < rinfo->dir_nr; i++) {
>>>    		struct ceph_mds_reply_dir_entry *rde = rinfo->dir_entries + i;
>>> -		struct ceph_fname fname = { .dir	= inode,
>>> -					    .name	= rde->name,
>>> -					    .name_len	= rde->name_len,
>>> -					    .ctext	= rde->altname,
>>> -					    .ctext_len	= rde->altname_len };
>>> -		u32 olen = oname.len;
>>> -
>>> -		err = ceph_fname_to_usr(&fname, &tname, &oname, NULL);
>>> -		if (err) {
>>> -			pr_err("%s unable to decode %.*s, got %d\n", __func__,
>>> -			       rde->name_len, rde->name, err);
>>> -			goto out;
>>> -		}
>>> +		struct dentry *dn = rde->dentry;
>>> +		int name_len;
>>>    
>>>    		BUG_ON(rde->offset < ctx->pos);
>>>    		BUG_ON(!rde->inode.in);
>>> +		BUG_ON(!rde->dentry);
>>>    
>>>    		ctx->pos = rde->offset;
>>> -		dout("readdir (%d/%d) -> %llx '%.*s' %p\n",
>>> -		     i, rinfo->dir_nr, ctx->pos,
>>> -		     rde->name_len, rde->name, &rde->inode.in);
>>>    
>>> -		if (!dir_emit(ctx, oname.name, oname.len,
>>> +		spin_lock(&dn->d_lock);
>>> +		memcpy(dentry_name, dn->d_name.name, dn->d_name.len);
>>> +		name_len = dn->d_name.len;
>>> +		spin_unlock(&dn->d_lock);
>>> +
>>> +		dentry_name[name_len] = '\0';
>> Possibly caused by this. Since it useless here and I will remove it.
>>
>>
> Seems plausible. If you send a v8 patch, I can give it another test and
> see if it fixes it.

Sure, will send it soon.

And locally I have test several hours and haven't see any issue yet with V8.

- Xiubo

>>> +		dout("readdir (%d/%d) -> %llx '%s' %p\n",
>>> +		     i, rinfo->dir_nr, ctx->pos, dentry_name, &rde->inode.in);
>>> +
>>> +		if (!dir_emit(ctx, dentry_name, name_len,
>>>    			      ceph_present_ino(inode->i_sb, le64_to_cpu(rde->inode.in->ino)),
>>>    			      le32_to_cpu(rde->inode.in->mode) >> 12)) {
>>>    			/*
>>> @@ -566,8 +562,6 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>>>    			goto out;
>>>    		}
>>>    
>>> -		/* Reset the lengths to their original allocated vals */
>>> -		oname.len = olen;
>>>    		ctx->pos++;
>>>    	}
>>>    
>>> @@ -625,8 +619,8 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>>>    	err = 0;
>>>    	dout("readdir %p file %p done.\n", inode, file);
>>>    out:
>>> -	ceph_fname_free_buffer(inode, &tname);
>>> -	ceph_fname_free_buffer(inode, &oname);
>>> +	if (dentry_name)
>>> +		kfree(dentry_name);
>>>    	return err;
>>>    }
>>>    
>>> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
>>> index b573a0f33450..19e5275eae1c 100644
>>> --- a/fs/ceph/inode.c
>>> +++ b/fs/ceph/inode.c
>>> @@ -1909,6 +1909,7 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
>>>    			goto out;
>>>    		}
>>>    
>>> +		rde->dentry = NULL;
>>>    		dname.name = oname.name;
>>>    		dname.len = oname.len;
>>>    		dname.hash = full_name_hash(parent, dname.name, dname.len);
>>> @@ -1969,6 +1970,12 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
>>>    			goto retry_lookup;
>>>    		}
>>>    
>>> +		/*
>>> +		 * ceph_readdir will use the dentry to get the name
>>> +		 * to avoid doing the dencrypt again there.
>>> +		 */
>>> +		rde->dentry = dget(dn);
>>> +
>>>    		/* inode */
>>>    		if (d_really_is_positive(dn)) {
>>>    			in = d_inode(dn);
>>> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
>>> index 8d704ddd7291..9e0a51ef1dfa 100644
>>> --- a/fs/ceph/mds_client.c
>>> +++ b/fs/ceph/mds_client.c
>>> @@ -733,6 +733,7 @@ static void destroy_reply_info(struct ceph_mds_reply_info_parsed *info)
>>>    
>>>    		kfree(rde->inode.fscrypt_auth);
>>>    		kfree(rde->inode.fscrypt_file);
>>> +		dput(rde->dentry);
>>>    	}
>>>    	free_pages((unsigned long)info->dir_entries, get_order(info->dir_buf_size));
>>>    }
>>> diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
>>> index 0dfe24f94567..663d7754d57d 100644
>>> --- a/fs/ceph/mds_client.h
>>> +++ b/fs/ceph/mds_client.h
>>> @@ -96,6 +96,7 @@ struct ceph_mds_reply_info_in {
>>>    };
>>>    
>>>    struct ceph_mds_reply_dir_entry {
>>> +	struct dentry		      *dentry;
>>>    	char                          *name;
>>>    	u8			      *altname;
>>>    	u32                           name_len;

On 3/10/22 6:36 PM, Jeff Layton wrote:
> On Thu, 2022-03-10 at 14:08 +0800, Xiubo Li wrote:
>> On 3/9/22 9:59 PM, xiubli@redhat.com wrote:
>>> From: Xiubo Li <xiubli@redhat.com>
>>>
>>> For the readdir request the dentries will be pasred and dencrypted
>>> in ceph_readdir_prepopulate(). And in ceph_readdir() we could just
>>> get the dentry name from the dentry cache instead of parsing and
>>> dencrypting them again. This could improve performance.
>>>
>>> Signed-off-by: Xiubo Li <xiubli@redhat.com>
>>> ---
>>>
>>> V7:
>>> - Fix the xfstest generic/006 crash bug about the rde->dentry == NULL.
>>>
>>> V6:
>>> - Remove CEPH_ENCRYPTED_LONG_SNAP_NAME_MAX macro and use the NAME_MAX
>>>     instead, since we are limiting the max length of snapshot name to
>>>     240, which is NAME_MAX - 2 * sizeof('_') - sizeof(<inode#>).
>>>
>>> V5:
>>> - fix typo of CEPH_ENCRYPTED_LONG_SNAP_NAME_MAX macro
>>> - release the rde->dentry in destroy_reply_info
>>>
>>>
>>>
>>>    fs/ceph/dir.c        | 56 ++++++++++++++++++++------------------------
>>>    fs/ceph/inode.c      |  7 ++++++
>>>    fs/ceph/mds_client.c |  1 +
>>>    fs/ceph/mds_client.h |  1 +
>>>    4 files changed, 34 insertions(+), 31 deletions(-)
>>>
>>> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
>>> index 6df2a91af236..2397c34e9173 100644
>>> --- a/fs/ceph/dir.c
>>> +++ b/fs/ceph/dir.c
>>> @@ -316,8 +316,7 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>>>    	int err;
>>>    	unsigned frag = -1;
>>>    	struct ceph_mds_reply_info_parsed *rinfo;
>>> -	struct fscrypt_str tname = FSTR_INIT(NULL, 0);
>>> -	struct fscrypt_str oname = FSTR_INIT(NULL, 0);
>>> +	char *dentry_name = NULL;
>>>    
>>>    	dout("readdir %p file %p pos %llx\n", inode, file, ctx->pos);
>>>    	if (dfi->file_info.flags & CEPH_F_ATEND)
>>> @@ -369,14 +368,6 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>>>    		spin_unlock(&ci->i_ceph_lock);
>>>    	}
>>>    
>>> -	err = ceph_fname_alloc_buffer(inode, &tname);
>>> -	if (err < 0)
>>> -		goto out;
>>> -
>>> -	err = ceph_fname_alloc_buffer(inode, &oname);
>>> -	if (err < 0)
>>> -		goto out;
>>> -
>>>    	/* proceed with a normal readdir */
>>>    more:
>>>    	/* do we have the correct frag content buffered? */
>>> @@ -528,31 +519,36 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>>>    			}
>>>    		}
>>>    	}
>>> +
>>> +	dentry_name = kmalloc(NAME_MAX, GFP_KERNEL);
>>> +	if (!dentry_name) {
>>> +		err = -ENOMEM;
>>> +		ceph_mdsc_put_request(dfi->last_readdir);
>>> +		dfi->last_readdir = NULL;
>>> +		goto out;
>>> +	}
>>> +
>>>    	for (; i < rinfo->dir_nr; i++) {
>>>    		struct ceph_mds_reply_dir_entry *rde = rinfo->dir_entries + i;
>>> -		struct ceph_fname fname = { .dir	= inode,
>>> -					    .name	= rde->name,
>>> -					    .name_len	= rde->name_len,
>>> -					    .ctext	= rde->altname,
>>> -					    .ctext_len	= rde->altname_len };
>>> -		u32 olen = oname.len;
>>> -
>>> -		err = ceph_fname_to_usr(&fname, &tname, &oname, NULL);
>>> -		if (err) {
>>> -			pr_err("%s unable to decode %.*s, got %d\n", __func__,
>>> -			       rde->name_len, rde->name, err);
>>> -			goto out;
>>> -		}
>>> +		struct dentry *dn = rde->dentry;
>>> +		int name_len;
>>>    
>>>    		BUG_ON(rde->offset < ctx->pos);
>>>    		BUG_ON(!rde->inode.in);
>>> +		BUG_ON(!rde->dentry);
>>>    
>>>    		ctx->pos = rde->offset;
>>> -		dout("readdir (%d/%d) -> %llx '%.*s' %p\n",
>>> -		     i, rinfo->dir_nr, ctx->pos,
>>> -		     rde->name_len, rde->name, &rde->inode.in);
>>>    
>>> -		if (!dir_emit(ctx, oname.name, oname.len,
>>> +		spin_lock(&dn->d_lock);
>>> +		memcpy(dentry_name, dn->d_name.name, dn->d_name.len);
>>> +		name_len = dn->d_name.len;
>>> +		spin_unlock(&dn->d_lock);
>>> +
>> Hi Jeff,
>>
>> BTW, does the dn->d_lock is must here ? From the code comments, the
>> d_lock intends to protect the 'd_flag' and 'd_lockref.count'.
>>
>> In ceph code I found some places are using the d_lock when accessing the
>> d_name, some are not. And in none ceph code, they will almost never use
>> the d_lock to protect the d_name.
>>
> It's probably not needed. The d_name can only change if there are no
> other outstanding references to the dentry.

If so, the dentry_name = kmalloc() is not needed any more.

- Xiubo


>>
>>> +		dentry_name[name_len] = '\0';
>>> +		dout("readdir (%d/%d) -> %llx '%s' %p\n",
>>> +		     i, rinfo->dir_nr, ctx->pos, dentry_name, &rde->inode.in);
>>> +
>>> +		if (!dir_emit(ctx, dentry_name, name_len,
>>>    			      ceph_present_ino(inode->i_sb, le64_to_cpu(rde->inode.in->ino)),
>>>    			      le32_to_cpu(rde->inode.in->mode) >> 12)) {
>>>    			/*
>>> @@ -566,8 +562,6 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>>>    			goto out;
>>>    		}
>>>    
>>> -		/* Reset the lengths to their original allocated vals */
>>> -		oname.len = olen;
>>>    		ctx->pos++;
>>>    	}
>>>    
>>> @@ -625,8 +619,8 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>>>    	err = 0;
>>>    	dout("readdir %p file %p done.\n", inode, file);
>>>    out:
>>> -	ceph_fname_free_buffer(inode, &tname);
>>> -	ceph_fname_free_buffer(inode, &oname);
>>> +	if (dentry_name)
>>> +		kfree(dentry_name);
>>>    	return err;
>>>    }
>>>    
>>> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
>>> index b573a0f33450..19e5275eae1c 100644
>>> --- a/fs/ceph/inode.c
>>> +++ b/fs/ceph/inode.c
>>> @@ -1909,6 +1909,7 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
>>>    			goto out;
>>>    		}
>>>    
>>> +		rde->dentry = NULL;
>>>    		dname.name = oname.name;
>>>    		dname.len = oname.len;
>>>    		dname.hash = full_name_hash(parent, dname.name, dname.len);
>>> @@ -1969,6 +1970,12 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
>>>    			goto retry_lookup;
>>>    		}
>>>    
>>> +		/*
>>> +		 * ceph_readdir will use the dentry to get the name
>>> +		 * to avoid doing the dencrypt again there.
>>> +		 */
>>> +		rde->dentry = dget(dn);
>>> +
>>>    		/* inode */
>>>    		if (d_really_is_positive(dn)) {
>>>    			in = d_inode(dn);
>>> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
>>> index 8d704ddd7291..9e0a51ef1dfa 100644
>>> --- a/fs/ceph/mds_client.c
>>> +++ b/fs/ceph/mds_client.c
>>> @@ -733,6 +733,7 @@ static void destroy_reply_info(struct ceph_mds_reply_info_parsed *info)
>>>    
>>>    		kfree(rde->inode.fscrypt_auth);
>>>    		kfree(rde->inode.fscrypt_file);
>>> +		dput(rde->dentry);
>>>    	}
>>>    	free_pages((unsigned long)info->dir_entries, get_order(info->dir_buf_size));
>>>    }
>>> diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
>>> index 0dfe24f94567..663d7754d57d 100644
>>> --- a/fs/ceph/mds_client.h
>>> +++ b/fs/ceph/mds_client.h
>>> @@ -96,6 +96,7 @@ struct ceph_mds_reply_info_in {
>>>    };
>>>    
>>>    struct ceph_mds_reply_dir_entry {
>>> +	struct dentry		      *dentry;
>>>    	char                          *name;
>>>    	u8			      *altname;
>>>    	u32                           name_len;

On Thu, 2022-03-10 at 18:54 +0800, Xiubo Li wrote:
> On 3/10/22 6:36 PM, Jeff Layton wrote:
> > On Thu, 2022-03-10 at 14:08 +0800, Xiubo Li wrote:
> > > On 3/9/22 9:59 PM, xiubli@redhat.com wrote:
> > > > From: Xiubo Li <xiubli@redhat.com>
> > > > 
> > > > For the readdir request the dentries will be pasred and dencrypted
> > > > in ceph_readdir_prepopulate(). And in ceph_readdir() we could just
> > > > get the dentry name from the dentry cache instead of parsing and
> > > > dencrypting them again. This could improve performance.
> > > > 
> > > > Signed-off-by: Xiubo Li <xiubli@redhat.com>
> > > > ---
> > > > 
> > > > V7:
> > > > - Fix the xfstest generic/006 crash bug about the rde->dentry == NULL.
> > > > 
> > > > V6:
> > > > - Remove CEPH_ENCRYPTED_LONG_SNAP_NAME_MAX macro and use the NAME_MAX
> > > >     instead, since we are limiting the max length of snapshot name to
> > > >     240, which is NAME_MAX - 2 * sizeof('_') - sizeof(<inode#>).
> > > > 
> > > > V5:
> > > > - fix typo of CEPH_ENCRYPTED_LONG_SNAP_NAME_MAX macro
> > > > - release the rde->dentry in destroy_reply_info
> > > > 
> > > > 
> > > > 
> > > >    fs/ceph/dir.c        | 56 ++++++++++++++++++++------------------------
> > > >    fs/ceph/inode.c      |  7 ++++++
> > > >    fs/ceph/mds_client.c |  1 +
> > > >    fs/ceph/mds_client.h |  1 +
> > > >    4 files changed, 34 insertions(+), 31 deletions(-)
> > > > 
> > > > diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> > > > index 6df2a91af236..2397c34e9173 100644
> > > > --- a/fs/ceph/dir.c
> > > > +++ b/fs/ceph/dir.c
> > > > @@ -316,8 +316,7 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
> > > >    	int err;
> > > >    	unsigned frag = -1;
> > > >    	struct ceph_mds_reply_info_parsed *rinfo;
> > > > -	struct fscrypt_str tname = FSTR_INIT(NULL, 0);
> > > > -	struct fscrypt_str oname = FSTR_INIT(NULL, 0);
> > > > +	char *dentry_name = NULL;
> > > >    
> > > >    	dout("readdir %p file %p pos %llx\n", inode, file, ctx->pos);
> > > >    	if (dfi->file_info.flags & CEPH_F_ATEND)
> > > > @@ -369,14 +368,6 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
> > > >    		spin_unlock(&ci->i_ceph_lock);
> > > >    	}
> > > >    
> > > > -	err = ceph_fname_alloc_buffer(inode, &tname);
> > > > -	if (err < 0)
> > > > -		goto out;
> > > > -
> > > > -	err = ceph_fname_alloc_buffer(inode, &oname);
> > > > -	if (err < 0)
> > > > -		goto out;
> > > > -
> > > >    	/* proceed with a normal readdir */
> > > >    more:
> > > >    	/* do we have the correct frag content buffered? */
> > > > @@ -528,31 +519,36 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
> > > >    			}
> > > >    		}
> > > >    	}
> > > > +
> > > > +	dentry_name = kmalloc(NAME_MAX, GFP_KERNEL);
> > > > +	if (!dentry_name) {
> > > > +		err = -ENOMEM;
> > > > +		ceph_mdsc_put_request(dfi->last_readdir);
> > > > +		dfi->last_readdir = NULL;
> > > > +		goto out;
> > > > +	}
> > > > +
> > > >    	for (; i < rinfo->dir_nr; i++) {
> > > >    		struct ceph_mds_reply_dir_entry *rde = rinfo->dir_entries + i;
> > > > -		struct ceph_fname fname = { .dir	= inode,
> > > > -					    .name	= rde->name,
> > > > -					    .name_len	= rde->name_len,
> > > > -					    .ctext	= rde->altname,
> > > > -					    .ctext_len	= rde->altname_len };
> > > > -		u32 olen = oname.len;
> > > > -
> > > > -		err = ceph_fname_to_usr(&fname, &tname, &oname, NULL);
> > > > -		if (err) {
> > > > -			pr_err("%s unable to decode %.*s, got %d\n", __func__,
> > > > -			       rde->name_len, rde->name, err);
> > > > -			goto out;
> > > > -		}
> > > > +		struct dentry *dn = rde->dentry;
> > > > +		int name_len;
> > > >    
> > > >    		BUG_ON(rde->offset < ctx->pos);
> > > >    		BUG_ON(!rde->inode.in);
> > > > +		BUG_ON(!rde->dentry);
> > > >    
> > > >    		ctx->pos = rde->offset;
> > > > -		dout("readdir (%d/%d) -> %llx '%.*s' %p\n",
> > > > -		     i, rinfo->dir_nr, ctx->pos,
> > > > -		     rde->name_len, rde->name, &rde->inode.in);
> > > >    
> > > > -		if (!dir_emit(ctx, oname.name, oname.len,
> > > > +		spin_lock(&dn->d_lock);
> > > > +		memcpy(dentry_name, dn->d_name.name, dn->d_name.len);
> > > > +		name_len = dn->d_name.len;
> > > > +		spin_unlock(&dn->d_lock);
> > > > +
> > > Hi Jeff,
> > > 
> > > BTW, does the dn->d_lock is must here ? From the code comments, the
> > > d_lock intends to protect the 'd_flag' and 'd_lockref.count'.
> > > 
> > > In ceph code I found some places are using the d_lock when accessing the
> > > d_name, some are not. And in none ceph code, they will almost never use
> > > the d_lock to protect the d_name.
> > > 
> > It's probably not needed. The d_name can only change if there are no
> > other outstanding references to the dentry.
> 
> If so, the dentry_name = kmalloc() is not needed any more.
> 
> 

I take it back.

I think the d_lock is the only thing that protects this against a
concurrent rename. Have a look at __d_move, and (in particular) the call
to copy_name. The d_lock is what guards against that happening while
you're working with it here.

You might be able to get away without using it, but you'll need to
follow similar rules as RCU walk.

FWIW, the best source for this info is Neil Brown's writeup in
Documentation/filesystems/path_lookup.rst.

> 
> > > 
> > > > +		dentry_name[name_len] = '\0';
> > > > +		dout("readdir (%d/%d) -> %llx '%s' %p\n",
> > > > +		     i, rinfo->dir_nr, ctx->pos, dentry_name, &rde->inode.in);
> > > > +
> > > > +		if (!dir_emit(ctx, dentry_name, name_len,
> > > >    			      ceph_present_ino(inode->i_sb, le64_to_cpu(rde->inode.in->ino)),
> > > >    			      le32_to_cpu(rde->inode.in->mode) >> 12)) {
> > > >    			/*
> > > > @@ -566,8 +562,6 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
> > > >    			goto out;
> > > >    		}
> > > >    
> > > > -		/* Reset the lengths to their original allocated vals */
> > > > -		oname.len = olen;
> > > >    		ctx->pos++;
> > > >    	}
> > > >    
> > > > @@ -625,8 +619,8 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
> > > >    	err = 0;
> > > >    	dout("readdir %p file %p done.\n", inode, file);
> > > >    out:
> > > > -	ceph_fname_free_buffer(inode, &tname);
> > > > -	ceph_fname_free_buffer(inode, &oname);
> > > > +	if (dentry_name)
> > > > +		kfree(dentry_name);
> > > >    	return err;
> > > >    }
> > > >    
> > > > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> > > > index b573a0f33450..19e5275eae1c 100644
> > > > --- a/fs/ceph/inode.c
> > > > +++ b/fs/ceph/inode.c
> > > > @@ -1909,6 +1909,7 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
> > > >    			goto out;
> > > >    		}
> > > >    
> > > > +		rde->dentry = NULL;
> > > >    		dname.name = oname.name;
> > > >    		dname.len = oname.len;
> > > >    		dname.hash = full_name_hash(parent, dname.name, dname.len);
> > > > @@ -1969,6 +1970,12 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
> > > >    			goto retry_lookup;
> > > >    		}
> > > >    
> > > > +		/*
> > > > +		 * ceph_readdir will use the dentry to get the name
> > > > +		 * to avoid doing the dencrypt again there.
> > > > +		 */
> > > > +		rde->dentry = dget(dn);
> > > > +
> > > >    		/* inode */
> > > >    		if (d_really_is_positive(dn)) {
> > > >    			in = d_inode(dn);
> > > > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> > > > index 8d704ddd7291..9e0a51ef1dfa 100644
> > > > --- a/fs/ceph/mds_client.c
> > > > +++ b/fs/ceph/mds_client.c
> > > > @@ -733,6 +733,7 @@ static void destroy_reply_info(struct ceph_mds_reply_info_parsed *info)
> > > >    
> > > >    		kfree(rde->inode.fscrypt_auth);
> > > >    		kfree(rde->inode.fscrypt_file);
> > > > +		dput(rde->dentry);
> > > >    	}
> > > >    	free_pages((unsigned long)info->dir_entries, get_order(info->dir_buf_size));
> > > >    }
> > > > diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
> > > > index 0dfe24f94567..663d7754d57d 100644
> > > > --- a/fs/ceph/mds_client.h
> > > > +++ b/fs/ceph/mds_client.h
> > > > @@ -96,6 +96,7 @@ struct ceph_mds_reply_info_in {
> > > >    };
> > > >    
> > > >    struct ceph_mds_reply_dir_entry {
> > > > +	struct dentry		      *dentry;
> > > >    	char                          *name;
> > > >    	u8			      *altname;
> > > >    	u32                           name_len;
>

On 3/10/22 7:21 PM, Jeff Layton wrote:
> On Thu, 2022-03-10 at 18:54 +0800, Xiubo Li wrote:
>> On 3/10/22 6:36 PM, Jeff Layton wrote:
>>> On Thu, 2022-03-10 at 14:08 +0800, Xiubo Li wrote:
>>>> On 3/9/22 9:59 PM, xiubli@redhat.com wrote:
>>>>> From: Xiubo Li <xiubli@redhat.com>
>>>>>
>>>>> For the readdir request the dentries will be pasred and dencrypted
>>>>> in ceph_readdir_prepopulate(). And in ceph_readdir() we could just
>>>>> get the dentry name from the dentry cache instead of parsing and
>>>>> dencrypting them again. This could improve performance.
>>>>>
>>>>> Signed-off-by: Xiubo Li <xiubli@redhat.com>
>>>>> ---
>>>>>
>>>>> V7:
>>>>> - Fix the xfstest generic/006 crash bug about the rde->dentry == NULL.
>>>>>
>>>>> V6:
>>>>> - Remove CEPH_ENCRYPTED_LONG_SNAP_NAME_MAX macro and use the NAME_MAX
>>>>>      instead, since we are limiting the max length of snapshot name to
>>>>>      240, which is NAME_MAX - 2 * sizeof('_') - sizeof(<inode#>).
>>>>>
>>>>> V5:
>>>>> - fix typo of CEPH_ENCRYPTED_LONG_SNAP_NAME_MAX macro
>>>>> - release the rde->dentry in destroy_reply_info
>>>>>
>>>>>
>>>>>
>>>>>     fs/ceph/dir.c        | 56 ++++++++++++++++++++------------------------
>>>>>     fs/ceph/inode.c      |  7 ++++++
>>>>>     fs/ceph/mds_client.c |  1 +
>>>>>     fs/ceph/mds_client.h |  1 +
>>>>>     4 files changed, 34 insertions(+), 31 deletions(-)
>>>>>
>>>>> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
>>>>> index 6df2a91af236..2397c34e9173 100644
>>>>> --- a/fs/ceph/dir.c
>>>>> +++ b/fs/ceph/dir.c
>>>>> @@ -316,8 +316,7 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>>>>>     	int err;
>>>>>     	unsigned frag = -1;
>>>>>     	struct ceph_mds_reply_info_parsed *rinfo;
>>>>> -	struct fscrypt_str tname = FSTR_INIT(NULL, 0);
>>>>> -	struct fscrypt_str oname = FSTR_INIT(NULL, 0);
>>>>> +	char *dentry_name = NULL;
>>>>>     
>>>>>     	dout("readdir %p file %p pos %llx\n", inode, file, ctx->pos);
>>>>>     	if (dfi->file_info.flags & CEPH_F_ATEND)
>>>>> @@ -369,14 +368,6 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>>>>>     		spin_unlock(&ci->i_ceph_lock);
>>>>>     	}
>>>>>     
>>>>> -	err = ceph_fname_alloc_buffer(inode, &tname);
>>>>> -	if (err < 0)
>>>>> -		goto out;
>>>>> -
>>>>> -	err = ceph_fname_alloc_buffer(inode, &oname);
>>>>> -	if (err < 0)
>>>>> -		goto out;
>>>>> -
>>>>>     	/* proceed with a normal readdir */
>>>>>     more:
>>>>>     	/* do we have the correct frag content buffered? */
>>>>> @@ -528,31 +519,36 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>>>>>     			}
>>>>>     		}
>>>>>     	}
>>>>> +
>>>>> +	dentry_name = kmalloc(NAME_MAX, GFP_KERNEL);
>>>>> +	if (!dentry_name) {
>>>>> +		err = -ENOMEM;
>>>>> +		ceph_mdsc_put_request(dfi->last_readdir);
>>>>> +		dfi->last_readdir = NULL;
>>>>> +		goto out;
>>>>> +	}
>>>>> +
>>>>>     	for (; i < rinfo->dir_nr; i++) {
>>>>>     		struct ceph_mds_reply_dir_entry *rde = rinfo->dir_entries + i;
>>>>> -		struct ceph_fname fname = { .dir	= inode,
>>>>> -					    .name	= rde->name,
>>>>> -					    .name_len	= rde->name_len,
>>>>> -					    .ctext	= rde->altname,
>>>>> -					    .ctext_len	= rde->altname_len };
>>>>> -		u32 olen = oname.len;
>>>>> -
>>>>> -		err = ceph_fname_to_usr(&fname, &tname, &oname, NULL);
>>>>> -		if (err) {
>>>>> -			pr_err("%s unable to decode %.*s, got %d\n", __func__,
>>>>> -			       rde->name_len, rde->name, err);
>>>>> -			goto out;
>>>>> -		}
>>>>> +		struct dentry *dn = rde->dentry;
>>>>> +		int name_len;
>>>>>     
>>>>>     		BUG_ON(rde->offset < ctx->pos);
>>>>>     		BUG_ON(!rde->inode.in);
>>>>> +		BUG_ON(!rde->dentry);
>>>>>     
>>>>>     		ctx->pos = rde->offset;
>>>>> -		dout("readdir (%d/%d) -> %llx '%.*s' %p\n",
>>>>> -		     i, rinfo->dir_nr, ctx->pos,
>>>>> -		     rde->name_len, rde->name, &rde->inode.in);
>>>>>     
>>>>> -		if (!dir_emit(ctx, oname.name, oname.len,
>>>>> +		spin_lock(&dn->d_lock);
>>>>> +		memcpy(dentry_name, dn->d_name.name, dn->d_name.len);
>>>>> +		name_len = dn->d_name.len;
>>>>> +		spin_unlock(&dn->d_lock);
>>>>> +
>>>> Hi Jeff,
>>>>
>>>> BTW, does the dn->d_lock is must here ? From the code comments, the
>>>> d_lock intends to protect the 'd_flag' and 'd_lockref.count'.
>>>>
>>>> In ceph code I found some places are using the d_lock when accessing the
>>>> d_name, some are not. And in none ceph code, they will almost never use
>>>> the d_lock to protect the d_name.
>>>>
>>> It's probably not needed. The d_name can only change if there are no
>>> other outstanding references to the dentry.
>> If so, the dentry_name = kmalloc() is not needed any more.
>>
>>
> I take it back.
>
> I think the d_lock is the only thing that protects this against a
> concurrent rename. Have a look at __d_move, and (in particular) the call
> to copy_name. The d_lock is what guards against that happening while
> you're working with it here.
>
> You might be able to get away without using it, but you'll need to
> follow similar rules as RCU walk.

Yeah, for this I was also checking how the function 'dentry_name()' in 
lib/vsprintf.c is working without the d_lock.

For now to be simple let's keep what it is now.

- Xiubo


> FWIW, the best source for this info is Neil Brown's writeup in
> Documentation/filesystems/path_lookup.rst.
>
>>>>> +		dentry_name[name_len] = '\0';
>>>>> +		dout("readdir (%d/%d) -> %llx '%s' %p\n",
>>>>> +		     i, rinfo->dir_nr, ctx->pos, dentry_name, &rde->inode.in);
>>>>> +
>>>>> +		if (!dir_emit(ctx, dentry_name, name_len,
>>>>>     			      ceph_present_ino(inode->i_sb, le64_to_cpu(rde->inode.in->ino)),
>>>>>     			      le32_to_cpu(rde->inode.in->mode) >> 12)) {
>>>>>     			/*
>>>>> @@ -566,8 +562,6 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>>>>>     			goto out;
>>>>>     		}
>>>>>     
>>>>> -		/* Reset the lengths to their original allocated vals */
>>>>> -		oname.len = olen;
>>>>>     		ctx->pos++;
>>>>>     	}
>>>>>     
>>>>> @@ -625,8 +619,8 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>>>>>     	err = 0;
>>>>>     	dout("readdir %p file %p done.\n", inode, file);
>>>>>     out:
>>>>> -	ceph_fname_free_buffer(inode, &tname);
>>>>> -	ceph_fname_free_buffer(inode, &oname);
>>>>> +	if (dentry_name)
>>>>> +		kfree(dentry_name);
>>>>>     	return err;
>>>>>     }
>>>>>     
>>>>> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
>>>>> index b573a0f33450..19e5275eae1c 100644
>>>>> --- a/fs/ceph/inode.c
>>>>> +++ b/fs/ceph/inode.c
>>>>> @@ -1909,6 +1909,7 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
>>>>>     			goto out;
>>>>>     		}
>>>>>     
>>>>> +		rde->dentry = NULL;
>>>>>     		dname.name = oname.name;
>>>>>     		dname.len = oname.len;
>>>>>     		dname.hash = full_name_hash(parent, dname.name, dname.len);
>>>>> @@ -1969,6 +1970,12 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
>>>>>     			goto retry_lookup;
>>>>>     		}
>>>>>     
>>>>> +		/*
>>>>> +		 * ceph_readdir will use the dentry to get the name
>>>>> +		 * to avoid doing the dencrypt again there.
>>>>> +		 */
>>>>> +		rde->dentry = dget(dn);
>>>>> +
>>>>>     		/* inode */
>>>>>     		if (d_really_is_positive(dn)) {
>>>>>     			in = d_inode(dn);
>>>>> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
>>>>> index 8d704ddd7291..9e0a51ef1dfa 100644
>>>>> --- a/fs/ceph/mds_client.c
>>>>> +++ b/fs/ceph/mds_client.c
>>>>> @@ -733,6 +733,7 @@ static void destroy_reply_info(struct ceph_mds_reply_info_parsed *info)
>>>>>     
>>>>>     		kfree(rde->inode.fscrypt_auth);
>>>>>     		kfree(rde->inode.fscrypt_file);
>>>>> +		dput(rde->dentry);
>>>>>     	}
>>>>>     	free_pages((unsigned long)info->dir_entries, get_order(info->dir_buf_size));
>>>>>     }
>>>>> diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
>>>>> index 0dfe24f94567..663d7754d57d 100644
>>>>> --- a/fs/ceph/mds_client.h
>>>>> +++ b/fs/ceph/mds_client.h
>>>>> @@ -96,6 +96,7 @@ struct ceph_mds_reply_info_in {
>>>>>     };
>>>>>     
>>>>>     struct ceph_mds_reply_dir_entry {
>>>>> +	struct dentry		      *dentry;
>>>>>     	char                          *name;
>>>>>     	u8			      *altname;
>>>>>     	u32                           name_len;

On 3/10/22 7:21 PM, Jeff Layton wrote:
> On Thu, 2022-03-10 at 18:54 +0800, Xiubo Li wrote:
>> On 3/10/22 6:36 PM, Jeff Layton wrote:
>>> On Thu, 2022-03-10 at 14:08 +0800, Xiubo Li wrote:
>>>> On 3/9/22 9:59 PM, xiubli@redhat.com wrote:
>>>>> From: Xiubo Li <xiubli@redhat.com>
>>>>>
>>>>> For the readdir request the dentries will be pasred and dencrypted
>>>>> in ceph_readdir_prepopulate(). And in ceph_readdir() we could just
>>>>> get the dentry name from the dentry cache instead of parsing and
>>>>> dencrypting them again. This could improve performance.
>>>>>
>>>>> Signed-off-by: Xiubo Li <xiubli@redhat.com>
>>>>> ---
>>>>>
>>>>> V7:
>>>>> - Fix the xfstest generic/006 crash bug about the rde->dentry == NULL.
>>>>>
>>>>> V6:
>>>>> - Remove CEPH_ENCRYPTED_LONG_SNAP_NAME_MAX macro and use the NAME_MAX
>>>>>      instead, since we are limiting the max length of snapshot name to
>>>>>      240, which is NAME_MAX - 2 * sizeof('_') - sizeof(<inode#>).
>>>>>
>>>>> V5:
>>>>> - fix typo of CEPH_ENCRYPTED_LONG_SNAP_NAME_MAX macro
>>>>> - release the rde->dentry in destroy_reply_info
>>>>>
>>>>>
>>>>>
>>>>>     fs/ceph/dir.c        | 56 ++++++++++++++++++++------------------------
>>>>>     fs/ceph/inode.c      |  7 ++++++
>>>>>     fs/ceph/mds_client.c |  1 +
>>>>>     fs/ceph/mds_client.h |  1 +
>>>>>     4 files changed, 34 insertions(+), 31 deletions(-)
>>>>>
>>>>> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
>>>>> index 6df2a91af236..2397c34e9173 100644
>>>>> --- a/fs/ceph/dir.c
>>>>> +++ b/fs/ceph/dir.c
>>>>> @@ -316,8 +316,7 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>>>>>     	int err;
>>>>>     	unsigned frag = -1;
>>>>>     	struct ceph_mds_reply_info_parsed *rinfo;
>>>>> -	struct fscrypt_str tname = FSTR_INIT(NULL, 0);
>>>>> -	struct fscrypt_str oname = FSTR_INIT(NULL, 0);
>>>>> +	char *dentry_name = NULL;
>>>>>     
>>>>>     	dout("readdir %p file %p pos %llx\n", inode, file, ctx->pos);
>>>>>     	if (dfi->file_info.flags & CEPH_F_ATEND)
>>>>> @@ -369,14 +368,6 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>>>>>     		spin_unlock(&ci->i_ceph_lock);
>>>>>     	}
>>>>>     
>>>>> -	err = ceph_fname_alloc_buffer(inode, &tname);
>>>>> -	if (err < 0)
>>>>> -		goto out;
>>>>> -
>>>>> -	err = ceph_fname_alloc_buffer(inode, &oname);
>>>>> -	if (err < 0)
>>>>> -		goto out;
>>>>> -
>>>>>     	/* proceed with a normal readdir */
>>>>>     more:
>>>>>     	/* do we have the correct frag content buffered? */
>>>>> @@ -528,31 +519,36 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>>>>>     			}
>>>>>     		}
>>>>>     	}
>>>>> +
>>>>> +	dentry_name = kmalloc(NAME_MAX, GFP_KERNEL);
>>>>> +	if (!dentry_name) {
>>>>> +		err = -ENOMEM;
>>>>> +		ceph_mdsc_put_request(dfi->last_readdir);
>>>>> +		dfi->last_readdir = NULL;
>>>>> +		goto out;
>>>>> +	}
>>>>> +
>>>>>     	for (; i < rinfo->dir_nr; i++) {
>>>>>     		struct ceph_mds_reply_dir_entry *rde = rinfo->dir_entries + i;
>>>>> -		struct ceph_fname fname = { .dir	= inode,
>>>>> -					    .name	= rde->name,
>>>>> -					    .name_len	= rde->name_len,
>>>>> -					    .ctext	= rde->altname,
>>>>> -					    .ctext_len	= rde->altname_len };
>>>>> -		u32 olen = oname.len;
>>>>> -
>>>>> -		err = ceph_fname_to_usr(&fname, &tname, &oname, NULL);
>>>>> -		if (err) {
>>>>> -			pr_err("%s unable to decode %.*s, got %d\n", __func__,
>>>>> -			       rde->name_len, rde->name, err);
>>>>> -			goto out;
>>>>> -		}
>>>>> +		struct dentry *dn = rde->dentry;
>>>>> +		int name_len;
>>>>>     
>>>>>     		BUG_ON(rde->offset < ctx->pos);
>>>>>     		BUG_ON(!rde->inode.in);
>>>>> +		BUG_ON(!rde->dentry);
>>>>>     
>>>>>     		ctx->pos = rde->offset;
>>>>> -		dout("readdir (%d/%d) -> %llx '%.*s' %p\n",
>>>>> -		     i, rinfo->dir_nr, ctx->pos,
>>>>> -		     rde->name_len, rde->name, &rde->inode.in);
>>>>>     
>>>>> -		if (!dir_emit(ctx, oname.name, oname.len,
>>>>> +		spin_lock(&dn->d_lock);
>>>>> +		memcpy(dentry_name, dn->d_name.name, dn->d_name.len);
>>>>> +		name_len = dn->d_name.len;
>>>>> +		spin_unlock(&dn->d_lock);
>>>>> +
>>>> Hi Jeff,
>>>>
>>>> BTW, does the dn->d_lock is must here ? From the code comments, the
>>>> d_lock intends to protect the 'd_flag' and 'd_lockref.count'.
>>>>
>>>> In ceph code I found some places are using the d_lock when accessing the
>>>> d_name, some are not. And in none ceph code, they will almost never use
>>>> the d_lock to protect the d_name.
>>>>
>>> It's probably not needed. The d_name can only change if there are no
>>> other outstanding references to the dentry.
>> If so, the dentry_name = kmalloc() is not needed any more.
>>
>>
> I take it back.
>
> I think the d_lock is the only thing that protects this against a
> concurrent rename. Have a look at __d_move, and (in particular) the call
> to copy_name. The d_lock is what guards against that happening while
> you're working with it here.
>
> You might be able to get away without using it, but you'll need to
> follow similar rules as RCU walk.
>
> FWIW, the best source for this info is Neil Brown's writeup in
> Documentation/filesystems/path_lookup.rst.

IMO we can get rid of the 'd_lock' by:

char *dentry_name = READ_ONCE(dentry->d_name.name);

if (!dir_emit(ctx, dentry_name, strlen(dentry_name),....

Because both the swap_names() and copy_name() won't release the memory 
of dentry->d_name.name.

But possible we will see a corrupted dentry name if it's doing the 
copy_name() ?

>>>>> +		dentry_name[name_len] = '\0';
>>>>> +		dout("readdir (%d/%d) -> %llx '%s' %p\n",
>>>>> +		     i, rinfo->dir_nr, ctx->pos, dentry_name, &rde->inode.in);
>>>>> +
>>>>> +		if (!dir_emit(ctx, dentry_name, name_len,
>>>>>     			      ceph_present_ino(inode->i_sb, le64_to_cpu(rde->inode.in->ino)),
>>>>>     			      le32_to_cpu(rde->inode.in->mode) >> 12)) {
>>>>>     			/*
>>>>> @@ -566,8 +562,6 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>>>>>     			goto out;
>>>>>     		}
>>>>>     
>>>>> -		/* Reset the lengths to their original allocated vals */
>>>>> -		oname.len = olen;
>>>>>     		ctx->pos++;
>>>>>     	}
>>>>>     
>>>>> @@ -625,8 +619,8 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>>>>>     	err = 0;
>>>>>     	dout("readdir %p file %p done.\n", inode, file);
>>>>>     out:
>>>>> -	ceph_fname_free_buffer(inode, &tname);
>>>>> -	ceph_fname_free_buffer(inode, &oname);
>>>>> +	if (dentry_name)
>>>>> +		kfree(dentry_name);
>>>>>     	return err;
>>>>>     }
>>>>>     
>>>>> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
>>>>> index b573a0f33450..19e5275eae1c 100644
>>>>> --- a/fs/ceph/inode.c
>>>>> +++ b/fs/ceph/inode.c
>>>>> @@ -1909,6 +1909,7 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
>>>>>     			goto out;
>>>>>     		}
>>>>>     
>>>>> +		rde->dentry = NULL;
>>>>>     		dname.name = oname.name;
>>>>>     		dname.len = oname.len;
>>>>>     		dname.hash = full_name_hash(parent, dname.name, dname.len);
>>>>> @@ -1969,6 +1970,12 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
>>>>>     			goto retry_lookup;
>>>>>     		}
>>>>>     
>>>>> +		/*
>>>>> +		 * ceph_readdir will use the dentry to get the name
>>>>> +		 * to avoid doing the dencrypt again there.
>>>>> +		 */
>>>>> +		rde->dentry = dget(dn);
>>>>> +
>>>>>     		/* inode */
>>>>>     		if (d_really_is_positive(dn)) {
>>>>>     			in = d_inode(dn);
>>>>> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
>>>>> index 8d704ddd7291..9e0a51ef1dfa 100644
>>>>> --- a/fs/ceph/mds_client.c
>>>>> +++ b/fs/ceph/mds_client.c
>>>>> @@ -733,6 +733,7 @@ static void destroy_reply_info(struct ceph_mds_reply_info_parsed *info)
>>>>>     
>>>>>     		kfree(rde->inode.fscrypt_auth);
>>>>>     		kfree(rde->inode.fscrypt_file);
>>>>> +		dput(rde->dentry);
>>>>>     	}
>>>>>     	free_pages((unsigned long)info->dir_entries, get_order(info->dir_buf_size));
>>>>>     }
>>>>> diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
>>>>> index 0dfe24f94567..663d7754d57d 100644
>>>>> --- a/fs/ceph/mds_client.h
>>>>> +++ b/fs/ceph/mds_client.h
>>>>> @@ -96,6 +96,7 @@ struct ceph_mds_reply_info_in {
>>>>>     };
>>>>>     
>>>>>     struct ceph_mds_reply_dir_entry {
>>>>> +	struct dentry		      *dentry;
>>>>>     	char                          *name;
>>>>>     	u8			      *altname;
>>>>>     	u32                           name_len;

On Thu, 2022-03-10 at 20:21 +0800, Xiubo Li wrote:
> On 3/10/22 7:21 PM, Jeff Layton wrote:
> > On Thu, 2022-03-10 at 18:54 +0800, Xiubo Li wrote:
> > > On 3/10/22 6:36 PM, Jeff Layton wrote:
> > > > On Thu, 2022-03-10 at 14:08 +0800, Xiubo Li wrote:
> > > > > On 3/9/22 9:59 PM, xiubli@redhat.com wrote:
> > > > > > From: Xiubo Li <xiubli@redhat.com>
> > > > > > 
> > > > > > For the readdir request the dentries will be pasred and dencrypted
> > > > > > in ceph_readdir_prepopulate(). And in ceph_readdir() we could just
> > > > > > get the dentry name from the dentry cache instead of parsing and
> > > > > > dencrypting them again. This could improve performance.
> > > > > > 
> > > > > > Signed-off-by: Xiubo Li <xiubli@redhat.com>
> > > > > > ---
> > > > > > 
> > > > > > V7:
> > > > > > - Fix the xfstest generic/006 crash bug about the rde->dentry == NULL.
> > > > > > 
> > > > > > V6:
> > > > > > - Remove CEPH_ENCRYPTED_LONG_SNAP_NAME_MAX macro and use the NAME_MAX
> > > > > >      instead, since we are limiting the max length of snapshot name to
> > > > > >      240, which is NAME_MAX - 2 * sizeof('_') - sizeof(<inode#>).
> > > > > > 
> > > > > > V5:
> > > > > > - fix typo of CEPH_ENCRYPTED_LONG_SNAP_NAME_MAX macro
> > > > > > - release the rde->dentry in destroy_reply_info
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > >     fs/ceph/dir.c        | 56 ++++++++++++++++++++------------------------
> > > > > >     fs/ceph/inode.c      |  7 ++++++
> > > > > >     fs/ceph/mds_client.c |  1 +
> > > > > >     fs/ceph/mds_client.h |  1 +
> > > > > >     4 files changed, 34 insertions(+), 31 deletions(-)
> > > > > > 
> > > > > > diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> > > > > > index 6df2a91af236..2397c34e9173 100644
> > > > > > --- a/fs/ceph/dir.c
> > > > > > +++ b/fs/ceph/dir.c
> > > > > > @@ -316,8 +316,7 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
> > > > > >     	int err;
> > > > > >     	unsigned frag = -1;
> > > > > >     	struct ceph_mds_reply_info_parsed *rinfo;
> > > > > > -	struct fscrypt_str tname = FSTR_INIT(NULL, 0);
> > > > > > -	struct fscrypt_str oname = FSTR_INIT(NULL, 0);
> > > > > > +	char *dentry_name = NULL;
> > > > > >     
> > > > > >     	dout("readdir %p file %p pos %llx\n", inode, file, ctx->pos);
> > > > > >     	if (dfi->file_info.flags & CEPH_F_ATEND)
> > > > > > @@ -369,14 +368,6 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
> > > > > >     		spin_unlock(&ci->i_ceph_lock);
> > > > > >     	}
> > > > > >     
> > > > > > -	err = ceph_fname_alloc_buffer(inode, &tname);
> > > > > > -	if (err < 0)
> > > > > > -		goto out;
> > > > > > -
> > > > > > -	err = ceph_fname_alloc_buffer(inode, &oname);
> > > > > > -	if (err < 0)
> > > > > > -		goto out;
> > > > > > -
> > > > > >     	/* proceed with a normal readdir */
> > > > > >     more:
> > > > > >     	/* do we have the correct frag content buffered? */
> > > > > > @@ -528,31 +519,36 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
> > > > > >     			}
> > > > > >     		}
> > > > > >     	}
> > > > > > +
> > > > > > +	dentry_name = kmalloc(NAME_MAX, GFP_KERNEL);
> > > > > > +	if (!dentry_name) {
> > > > > > +		err = -ENOMEM;
> > > > > > +		ceph_mdsc_put_request(dfi->last_readdir);
> > > > > > +		dfi->last_readdir = NULL;
> > > > > > +		goto out;
> > > > > > +	}
> > > > > > +
> > > > > >     	for (; i < rinfo->dir_nr; i++) {
> > > > > >     		struct ceph_mds_reply_dir_entry *rde = rinfo->dir_entries + i;
> > > > > > -		struct ceph_fname fname = { .dir	= inode,
> > > > > > -					    .name	= rde->name,
> > > > > > -					    .name_len	= rde->name_len,
> > > > > > -					    .ctext	= rde->altname,
> > > > > > -					    .ctext_len	= rde->altname_len };
> > > > > > -		u32 olen = oname.len;
> > > > > > -
> > > > > > -		err = ceph_fname_to_usr(&fname, &tname, &oname, NULL);
> > > > > > -		if (err) {
> > > > > > -			pr_err("%s unable to decode %.*s, got %d\n", __func__,
> > > > > > -			       rde->name_len, rde->name, err);
> > > > > > -			goto out;
> > > > > > -		}
> > > > > > +		struct dentry *dn = rde->dentry;
> > > > > > +		int name_len;
> > > > > >     
> > > > > >     		BUG_ON(rde->offset < ctx->pos);
> > > > > >     		BUG_ON(!rde->inode.in);
> > > > > > +		BUG_ON(!rde->dentry);
> > > > > >     
> > > > > >     		ctx->pos = rde->offset;
> > > > > > -		dout("readdir (%d/%d) -> %llx '%.*s' %p\n",
> > > > > > -		     i, rinfo->dir_nr, ctx->pos,
> > > > > > -		     rde->name_len, rde->name, &rde->inode.in);
> > > > > >     
> > > > > > -		if (!dir_emit(ctx, oname.name, oname.len,
> > > > > > +		spin_lock(&dn->d_lock);
> > > > > > +		memcpy(dentry_name, dn->d_name.name, dn->d_name.len);
> > > > > > +		name_len = dn->d_name.len;
> > > > > > +		spin_unlock(&dn->d_lock);
> > > > > > +
> > > > > Hi Jeff,
> > > > > 
> > > > > BTW, does the dn->d_lock is must here ? From the code comments, the
> > > > > d_lock intends to protect the 'd_flag' and 'd_lockref.count'.
> > > > > 
> > > > > In ceph code I found some places are using the d_lock when accessing the
> > > > > d_name, some are not. And in none ceph code, they will almost never use
> > > > > the d_lock to protect the d_name.
> > > > > 
> > > > It's probably not needed. The d_name can only change if there are no
> > > > other outstanding references to the dentry.
> > > If so, the dentry_name = kmalloc() is not needed any more.
> > > 
> > > 
> > I take it back.
> > 
> > I think the d_lock is the only thing that protects this against a
> > concurrent rename. Have a look at __d_move, and (in particular) the call
> > to copy_name. The d_lock is what guards against that happening while
> > you're working with it here.
> > 
> > You might be able to get away without using it, but you'll need to
> > follow similar rules as RCU walk.
> > 
> > FWIW, the best source for this info is Neil Brown's writeup in
> > Documentation/filesystems/path_lookup.rst.
> 

That should have been Documentation/filesystems/path-lookup.rst


> IMO we can get rid of the 'd_lock' by:
> 
> char *dentry_name = READ_ONCE(dentry->d_name.name);
> 
> if (!dir_emit(ctx, dentry_name, strlen(dentry_name),....
> 
> Because both the swap_names() and copy_name() won't release the memory 
> of dentry->d_name.name.
> 
> But possible we will see a corrupted dentry name if it's doing the 
> copy_name() ?
> 

ceph already satisfies readdir out of the dcache in some cases. See
__dcache_readdir, which basically walks a list of dentry pointers
stashed in the pagecache and then calls dir_emit on each (without the
dentry->d_lock held!). That's a bit different though since we have some
assurance that things won't change in that case (it may still be racy
too -- not sure).

I don't think you have the same guarantees here. What might be best is
to either allocate (or declare on-stack) a NAME_MAX buffer, and copy
into that from the d_name while you hold the lock. Then drop the lock
and dir_emit.

I wonder if we're going about this the wrong way...

Once we've decrypted the names, we don't need the crypttext anymore at
all. Maybe it would be better to just decrypt the names in-place, or
copy back over the crypttext after decrypting? Then we might not even
need some of these changes in readdir at all.


> > > > > > +		dentry_name[name_len] = '\0';
> > > > > > +		dout("readdir (%d/%d) -> %llx '%s' %p\n",
> > > > > > +		     i, rinfo->dir_nr, ctx->pos, dentry_name, &rde->inode.in);
> > > > > > +
> > > > > > +		if (!dir_emit(ctx, dentry_name, name_len,
> > > > > >     			      ceph_present_ino(inode->i_sb, le64_to_cpu(rde->inode.in->ino)),
> > > > > >     			      le32_to_cpu(rde->inode.in->mode) >> 12)) {
> > > > > >     			/*
> > > > > > @@ -566,8 +562,6 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
> > > > > >     			goto out;
> > > > > >     		}
> > > > > >     
> > > > > > -		/* Reset the lengths to their original allocated vals */
> > > > > > -		oname.len = olen;
> > > > > >     		ctx->pos++;
> > > > > >     	}
> > > > > >     
> > > > > > @@ -625,8 +619,8 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
> > > > > >     	err = 0;
> > > > > >     	dout("readdir %p file %p done.\n", inode, file);
> > > > > >     out:
> > > > > > -	ceph_fname_free_buffer(inode, &tname);
> > > > > > -	ceph_fname_free_buffer(inode, &oname);
> > > > > > +	if (dentry_name)
> > > > > > +		kfree(dentry_name);
> > > > > >     	return err;
> > > > > >     }
> > > > > >     
> > > > > > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> > > > > > index b573a0f33450..19e5275eae1c 100644
> > > > > > --- a/fs/ceph/inode.c
> > > > > > +++ b/fs/ceph/inode.c
> > > > > > @@ -1909,6 +1909,7 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
> > > > > >     			goto out;
> > > > > >     		}
> > > > > >     
> > > > > > +		rde->dentry = NULL;
> > > > > >     		dname.name = oname.name;
> > > > > >     		dname.len = oname.len;
> > > > > >     		dname.hash = full_name_hash(parent, dname.name, dname.len);
> > > > > > @@ -1969,6 +1970,12 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
> > > > > >     			goto retry_lookup;
> > > > > >     		}
> > > > > >     
> > > > > > +		/*
> > > > > > +		 * ceph_readdir will use the dentry to get the name
> > > > > > +		 * to avoid doing the dencrypt again there.
> > > > > > +		 */
> > > > > > +		rde->dentry = dget(dn);
> > > > > > +
> > > > > >     		/* inode */
> > > > > >     		if (d_really_is_positive(dn)) {
> > > > > >     			in = d_inode(dn);
> > > > > > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> > > > > > index 8d704ddd7291..9e0a51ef1dfa 100644
> > > > > > --- a/fs/ceph/mds_client.c
> > > > > > +++ b/fs/ceph/mds_client.c
> > > > > > @@ -733,6 +733,7 @@ static void destroy_reply_info(struct ceph_mds_reply_info_parsed *info)
> > > > > >     
> > > > > >     		kfree(rde->inode.fscrypt_auth);
> > > > > >     		kfree(rde->inode.fscrypt_file);
> > > > > > +		dput(rde->dentry);
> > > > > >     	}
> > > > > >     	free_pages((unsigned long)info->dir_entries, get_order(info->dir_buf_size));
> > > > > >     }
> > > > > > diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
> > > > > > index 0dfe24f94567..663d7754d57d 100644
> > > > > > --- a/fs/ceph/mds_client.h
> > > > > > +++ b/fs/ceph/mds_client.h
> > > > > > @@ -96,6 +96,7 @@ struct ceph_mds_reply_info_in {
> > > > > >     };
> > > > > >     
> > > > > >     struct ceph_mds_reply_dir_entry {
> > > > > > +	struct dentry		      *dentry;
> > > > > >     	char                          *name;
> > > > > >     	u8			      *altname;
> > > > > >     	u32                           name_len;
>

On 3/10/22 9:12 PM, Jeff Layton wrote:
> On Thu, 2022-03-10 at 20:21 +0800, Xiubo Li wrote:
>> On 3/10/22 7:21 PM, Jeff Layton wrote:
>>> On Thu, 2022-03-10 at 18:54 +0800, Xiubo Li wrote:
>>>> On 3/10/22 6:36 PM, Jeff Layton wrote:
>>>>> On Thu, 2022-03-10 at 14:08 +0800, Xiubo Li wrote:
>>>>>> On 3/9/22 9:59 PM, xiubli@redhat.com wrote:
>>>>>>> From: Xiubo Li <xiubli@redhat.com>
>>>>>>>
>>>>>>> For the readdir request the dentries will be pasred and dencrypted
>>>>>>> in ceph_readdir_prepopulate(). And in ceph_readdir() we could just
>>>>>>> get the dentry name from the dentry cache instead of parsing and
>>>>>>> dencrypting them again. This could improve performance.
>>>>>>>
>>>>>>> Signed-off-by: Xiubo Li <xiubli@redhat.com>
>>>>>>> ---
>>>>>>>
>>>>>>> V7:
>>>>>>> - Fix the xfstest generic/006 crash bug about the rde->dentry == NULL.
>>>>>>>
>>>>>>> V6:
>>>>>>> - Remove CEPH_ENCRYPTED_LONG_SNAP_NAME_MAX macro and use the NAME_MAX
>>>>>>>       instead, since we are limiting the max length of snapshot name to
>>>>>>>       240, which is NAME_MAX - 2 * sizeof('_') - sizeof(<inode#>).
>>>>>>>
>>>>>>> V5:
>>>>>>> - fix typo of CEPH_ENCRYPTED_LONG_SNAP_NAME_MAX macro
>>>>>>> - release the rde->dentry in destroy_reply_info
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>      fs/ceph/dir.c        | 56 ++++++++++++++++++++------------------------
>>>>>>>      fs/ceph/inode.c      |  7 ++++++
>>>>>>>      fs/ceph/mds_client.c |  1 +
>>>>>>>      fs/ceph/mds_client.h |  1 +
>>>>>>>      4 files changed, 34 insertions(+), 31 deletions(-)
>>>>>>>
>>>>>>> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
>>>>>>> index 6df2a91af236..2397c34e9173 100644
>>>>>>> --- a/fs/ceph/dir.c
>>>>>>> +++ b/fs/ceph/dir.c
>>>>>>> @@ -316,8 +316,7 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>>>>>>>      	int err;
>>>>>>>      	unsigned frag = -1;
>>>>>>>      	struct ceph_mds_reply_info_parsed *rinfo;
>>>>>>> -	struct fscrypt_str tname = FSTR_INIT(NULL, 0);
>>>>>>> -	struct fscrypt_str oname = FSTR_INIT(NULL, 0);
>>>>>>> +	char *dentry_name = NULL;
>>>>>>>      
>>>>>>>      	dout("readdir %p file %p pos %llx\n", inode, file, ctx->pos);
>>>>>>>      	if (dfi->file_info.flags & CEPH_F_ATEND)
>>>>>>> @@ -369,14 +368,6 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>>>>>>>      		spin_unlock(&ci->i_ceph_lock);
>>>>>>>      	}
>>>>>>>      
>>>>>>> -	err = ceph_fname_alloc_buffer(inode, &tname);
>>>>>>> -	if (err < 0)
>>>>>>> -		goto out;
>>>>>>> -
>>>>>>> -	err = ceph_fname_alloc_buffer(inode, &oname);
>>>>>>> -	if (err < 0)
>>>>>>> -		goto out;
>>>>>>> -
>>>>>>>      	/* proceed with a normal readdir */
>>>>>>>      more:
>>>>>>>      	/* do we have the correct frag content buffered? */
>>>>>>> @@ -528,31 +519,36 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>>>>>>>      			}
>>>>>>>      		}
>>>>>>>      	}
>>>>>>> +
>>>>>>> +	dentry_name = kmalloc(NAME_MAX, GFP_KERNEL);
>>>>>>> +	if (!dentry_name) {
>>>>>>> +		err = -ENOMEM;
>>>>>>> +		ceph_mdsc_put_request(dfi->last_readdir);
>>>>>>> +		dfi->last_readdir = NULL;
>>>>>>> +		goto out;
>>>>>>> +	}
>>>>>>> +
>>>>>>>      	for (; i < rinfo->dir_nr; i++) {
>>>>>>>      		struct ceph_mds_reply_dir_entry *rde = rinfo->dir_entries + i;
>>>>>>> -		struct ceph_fname fname = { .dir	= inode,
>>>>>>> -					    .name	= rde->name,
>>>>>>> -					    .name_len	= rde->name_len,
>>>>>>> -					    .ctext	= rde->altname,
>>>>>>> -					    .ctext_len	= rde->altname_len };
>>>>>>> -		u32 olen = oname.len;
>>>>>>> -
>>>>>>> -		err = ceph_fname_to_usr(&fname, &tname, &oname, NULL);
>>>>>>> -		if (err) {
>>>>>>> -			pr_err("%s unable to decode %.*s, got %d\n", __func__,
>>>>>>> -			       rde->name_len, rde->name, err);
>>>>>>> -			goto out;
>>>>>>> -		}
>>>>>>> +		struct dentry *dn = rde->dentry;
>>>>>>> +		int name_len;
>>>>>>>      
>>>>>>>      		BUG_ON(rde->offset < ctx->pos);
>>>>>>>      		BUG_ON(!rde->inode.in);
>>>>>>> +		BUG_ON(!rde->dentry);
>>>>>>>      
>>>>>>>      		ctx->pos = rde->offset;
>>>>>>> -		dout("readdir (%d/%d) -> %llx '%.*s' %p\n",
>>>>>>> -		     i, rinfo->dir_nr, ctx->pos,
>>>>>>> -		     rde->name_len, rde->name, &rde->inode.in);
>>>>>>>      
>>>>>>> -		if (!dir_emit(ctx, oname.name, oname.len,
>>>>>>> +		spin_lock(&dn->d_lock);
>>>>>>> +		memcpy(dentry_name, dn->d_name.name, dn->d_name.len);
>>>>>>> +		name_len = dn->d_name.len;
>>>>>>> +		spin_unlock(&dn->d_lock);
>>>>>>> +
>>>>>> Hi Jeff,
>>>>>>
>>>>>> BTW, does the dn->d_lock is must here ? From the code comments, the
>>>>>> d_lock intends to protect the 'd_flag' and 'd_lockref.count'.
>>>>>>
>>>>>> In ceph code I found some places are using the d_lock when accessing the
>>>>>> d_name, some are not. And in none ceph code, they will almost never use
>>>>>> the d_lock to protect the d_name.
>>>>>>
>>>>> It's probably not needed. The d_name can only change if there are no
>>>>> other outstanding references to the dentry.
>>>> If so, the dentry_name = kmalloc() is not needed any more.
>>>>
>>>>
>>> I take it back.
>>>
>>> I think the d_lock is the only thing that protects this against a
>>> concurrent rename. Have a look at __d_move, and (in particular) the call
>>> to copy_name. The d_lock is what guards against that happening while
>>> you're working with it here.
>>>
>>> You might be able to get away without using it, but you'll need to
>>> follow similar rules as RCU walk.
>>>
>>> FWIW, the best source for this info is Neil Brown's writeup in
>>> Documentation/filesystems/path_lookup.rst.
> That should have been Documentation/filesystems/path-lookup.rst
>
>
>> IMO we can get rid of the 'd_lock' by:
>>
>> char *dentry_name = READ_ONCE(dentry->d_name.name);
>>
>> if (!dir_emit(ctx, dentry_name, strlen(dentry_name),....
>>
>> Because both the swap_names() and copy_name() won't release the memory
>> of dentry->d_name.name.
>>
>> But possible we will see a corrupted dentry name if it's doing the
>> copy_name() ?
>>
> ceph already satisfies readdir out of the dcache in some cases. See
> __dcache_readdir, which basically walks a list of dentry pointers
> stashed in the pagecache and then calls dir_emit on each (without the
> dentry->d_lock held!). That's a bit different though since we have some
> assurance that things won't change in that case (it may still be racy
> too -- not sure).
>
> I don't think you have the same guarantees here. What might be best is
> to either allocate (or declare on-stack) a NAME_MAX buffer, and copy
> into that from the d_name while you hold the lock. Then drop the lock
> and dir_emit.
>
> I wonder if we're going about this the wrong way...
>
> Once we've decrypted the names, we don't need the crypttext anymore at
> all. Maybe it would be better to just decrypt the names in-place, or
> copy back over the crypttext after decrypting? Then we might not even
> need some of these changes in readdir at all.
>
For this, I think you mean that in ceph_readdir_prepopulate() when the 
first time decrypting the crypttext we will save it in rde->name or 
rde->altername, then in ceph_readdir() to emit it directly or 
base64_encode it again without key.

Right ?


>>>>>>> +		dentry_name[name_len] = '\0';
>>>>>>> +		dout("readdir (%d/%d) -> %llx '%s' %p\n",
>>>>>>> +		     i, rinfo->dir_nr, ctx->pos, dentry_name, &rde->inode.in);
>>>>>>> +
>>>>>>> +		if (!dir_emit(ctx, dentry_name, name_len,
>>>>>>>      			      ceph_present_ino(inode->i_sb, le64_to_cpu(rde->inode.in->ino)),
>>>>>>>      			      le32_to_cpu(rde->inode.in->mode) >> 12)) {
>>>>>>>      			/*
>>>>>>> @@ -566,8 +562,6 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>>>>>>>      			goto out;
>>>>>>>      		}
>>>>>>>      
>>>>>>> -		/* Reset the lengths to their original allocated vals */
>>>>>>> -		oname.len = olen;
>>>>>>>      		ctx->pos++;
>>>>>>>      	}
>>>>>>>      
>>>>>>> @@ -625,8 +619,8 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>>>>>>>      	err = 0;
>>>>>>>      	dout("readdir %p file %p done.\n", inode, file);
>>>>>>>      out:
>>>>>>> -	ceph_fname_free_buffer(inode, &tname);
>>>>>>> -	ceph_fname_free_buffer(inode, &oname);
>>>>>>> +	if (dentry_name)
>>>>>>> +		kfree(dentry_name);
>>>>>>>      	return err;
>>>>>>>      }
>>>>>>>      
>>>>>>> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
>>>>>>> index b573a0f33450..19e5275eae1c 100644
>>>>>>> --- a/fs/ceph/inode.c
>>>>>>> +++ b/fs/ceph/inode.c
>>>>>>> @@ -1909,6 +1909,7 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
>>>>>>>      			goto out;
>>>>>>>      		}
>>>>>>>      
>>>>>>> +		rde->dentry = NULL;
>>>>>>>      		dname.name = oname.name;
>>>>>>>      		dname.len = oname.len;
>>>>>>>      		dname.hash = full_name_hash(parent, dname.name, dname.len);
>>>>>>> @@ -1969,6 +1970,12 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
>>>>>>>      			goto retry_lookup;
>>>>>>>      		}
>>>>>>>      
>>>>>>> +		/*
>>>>>>> +		 * ceph_readdir will use the dentry to get the name
>>>>>>> +		 * to avoid doing the dencrypt again there.
>>>>>>> +		 */
>>>>>>> +		rde->dentry = dget(dn);
>>>>>>> +
>>>>>>>      		/* inode */
>>>>>>>      		if (d_really_is_positive(dn)) {
>>>>>>>      			in = d_inode(dn);
>>>>>>> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
>>>>>>> index 8d704ddd7291..9e0a51ef1dfa 100644
>>>>>>> --- a/fs/ceph/mds_client.c
>>>>>>> +++ b/fs/ceph/mds_client.c
>>>>>>> @@ -733,6 +733,7 @@ static void destroy_reply_info(struct ceph_mds_reply_info_parsed *info)
>>>>>>>      
>>>>>>>      		kfree(rde->inode.fscrypt_auth);
>>>>>>>      		kfree(rde->inode.fscrypt_file);
>>>>>>> +		dput(rde->dentry);
>>>>>>>      	}
>>>>>>>      	free_pages((unsigned long)info->dir_entries, get_order(info->dir_buf_size));
>>>>>>>      }
>>>>>>> diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
>>>>>>> index 0dfe24f94567..663d7754d57d 100644
>>>>>>> --- a/fs/ceph/mds_client.h
>>>>>>> +++ b/fs/ceph/mds_client.h
>>>>>>> @@ -96,6 +96,7 @@ struct ceph_mds_reply_info_in {
>>>>>>>      };
>>>>>>>      
>>>>>>>      struct ceph_mds_reply_dir_entry {
>>>>>>> +	struct dentry		      *dentry;
>>>>>>>      	char                          *name;
>>>>>>>      	u8			      *altname;
>>>>>>>      	u32                           name_len;

On Thu, 2022-03-10 at 22:14 +0800, Xiubo Li wrote:
> On 3/10/22 9:12 PM, Jeff Layton wrote:
> > On Thu, 2022-03-10 at 20:21 +0800, Xiubo Li wrote:
> > > On 3/10/22 7:21 PM, Jeff Layton wrote:
> > > > On Thu, 2022-03-10 at 18:54 +0800, Xiubo Li wrote:
> > > > > On 3/10/22 6:36 PM, Jeff Layton wrote:
> > > > > > On Thu, 2022-03-10 at 14:08 +0800, Xiubo Li wrote:
> > > > > > > On 3/9/22 9:59 PM, xiubli@redhat.com wrote:
> > > > > > > > From: Xiubo Li <xiubli@redhat.com>
> > > > > > > > 
> > > > > > > > For the readdir request the dentries will be pasred and dencrypted
> > > > > > > > in ceph_readdir_prepopulate(). And in ceph_readdir() we could just
> > > > > > > > get the dentry name from the dentry cache instead of parsing and
> > > > > > > > dencrypting them again. This could improve performance.
> > > > > > > > 
> > > > > > > > Signed-off-by: Xiubo Li <xiubli@redhat.com>
> > > > > > > > ---
> > > > > > > > 
> > > > > > > > V7:
> > > > > > > > - Fix the xfstest generic/006 crash bug about the rde->dentry == NULL.
> > > > > > > > 
> > > > > > > > V6:
> > > > > > > > - Remove CEPH_ENCRYPTED_LONG_SNAP_NAME_MAX macro and use the NAME_MAX
> > > > > > > >       instead, since we are limiting the max length of snapshot name to
> > > > > > > >       240, which is NAME_MAX - 2 * sizeof('_') - sizeof(<inode#>).
> > > > > > > > 
> > > > > > > > V5:
> > > > > > > > - fix typo of CEPH_ENCRYPTED_LONG_SNAP_NAME_MAX macro
> > > > > > > > - release the rde->dentry in destroy_reply_info
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > >      fs/ceph/dir.c        | 56 ++++++++++++++++++++------------------------
> > > > > > > >      fs/ceph/inode.c      |  7 ++++++
> > > > > > > >      fs/ceph/mds_client.c |  1 +
> > > > > > > >      fs/ceph/mds_client.h |  1 +
> > > > > > > >      4 files changed, 34 insertions(+), 31 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> > > > > > > > index 6df2a91af236..2397c34e9173 100644
> > > > > > > > --- a/fs/ceph/dir.c
> > > > > > > > +++ b/fs/ceph/dir.c
> > > > > > > > @@ -316,8 +316,7 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
> > > > > > > >      	int err;
> > > > > > > >      	unsigned frag = -1;
> > > > > > > >      	struct ceph_mds_reply_info_parsed *rinfo;
> > > > > > > > -	struct fscrypt_str tname = FSTR_INIT(NULL, 0);
> > > > > > > > -	struct fscrypt_str oname = FSTR_INIT(NULL, 0);
> > > > > > > > +	char *dentry_name = NULL;
> > > > > > > >      
> > > > > > > >      	dout("readdir %p file %p pos %llx\n", inode, file, ctx->pos);
> > > > > > > >      	if (dfi->file_info.flags & CEPH_F_ATEND)
> > > > > > > > @@ -369,14 +368,6 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
> > > > > > > >      		spin_unlock(&ci->i_ceph_lock);
> > > > > > > >      	}
> > > > > > > >      
> > > > > > > > -	err = ceph_fname_alloc_buffer(inode, &tname);
> > > > > > > > -	if (err < 0)
> > > > > > > > -		goto out;
> > > > > > > > -
> > > > > > > > -	err = ceph_fname_alloc_buffer(inode, &oname);
> > > > > > > > -	if (err < 0)
> > > > > > > > -		goto out;
> > > > > > > > -
> > > > > > > >      	/* proceed with a normal readdir */
> > > > > > > >      more:
> > > > > > > >      	/* do we have the correct frag content buffered? */
> > > > > > > > @@ -528,31 +519,36 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
> > > > > > > >      			}
> > > > > > > >      		}
> > > > > > > >      	}
> > > > > > > > +
> > > > > > > > +	dentry_name = kmalloc(NAME_MAX, GFP_KERNEL);
> > > > > > > > +	if (!dentry_name) {
> > > > > > > > +		err = -ENOMEM;
> > > > > > > > +		ceph_mdsc_put_request(dfi->last_readdir);
> > > > > > > > +		dfi->last_readdir = NULL;
> > > > > > > > +		goto out;
> > > > > > > > +	}
> > > > > > > > +
> > > > > > > >      	for (; i < rinfo->dir_nr; i++) {
> > > > > > > >      		struct ceph_mds_reply_dir_entry *rde = rinfo->dir_entries + i;
> > > > > > > > -		struct ceph_fname fname = { .dir	= inode,
> > > > > > > > -					    .name	= rde->name,
> > > > > > > > -					    .name_len	= rde->name_len,
> > > > > > > > -					    .ctext	= rde->altname,
> > > > > > > > -					    .ctext_len	= rde->altname_len };
> > > > > > > > -		u32 olen = oname.len;
> > > > > > > > -
> > > > > > > > -		err = ceph_fname_to_usr(&fname, &tname, &oname, NULL);
> > > > > > > > -		if (err) {
> > > > > > > > -			pr_err("%s unable to decode %.*s, got %d\n", __func__,
> > > > > > > > -			       rde->name_len, rde->name, err);
> > > > > > > > -			goto out;
> > > > > > > > -		}
> > > > > > > > +		struct dentry *dn = rde->dentry;
> > > > > > > > +		int name_len;
> > > > > > > >      
> > > > > > > >      		BUG_ON(rde->offset < ctx->pos);
> > > > > > > >      		BUG_ON(!rde->inode.in);
> > > > > > > > +		BUG_ON(!rde->dentry);
> > > > > > > >      
> > > > > > > >      		ctx->pos = rde->offset;
> > > > > > > > -		dout("readdir (%d/%d) -> %llx '%.*s' %p\n",
> > > > > > > > -		     i, rinfo->dir_nr, ctx->pos,
> > > > > > > > -		     rde->name_len, rde->name, &rde->inode.in);
> > > > > > > >      
> > > > > > > > -		if (!dir_emit(ctx, oname.name, oname.len,
> > > > > > > > +		spin_lock(&dn->d_lock);
> > > > > > > > +		memcpy(dentry_name, dn->d_name.name, dn->d_name.len);
> > > > > > > > +		name_len = dn->d_name.len;
> > > > > > > > +		spin_unlock(&dn->d_lock);
> > > > > > > > +
> > > > > > > Hi Jeff,
> > > > > > > 
> > > > > > > BTW, does the dn->d_lock is must here ? From the code comments, the
> > > > > > > d_lock intends to protect the 'd_flag' and 'd_lockref.count'.
> > > > > > > 
> > > > > > > In ceph code I found some places are using the d_lock when accessing the
> > > > > > > d_name, some are not. And in none ceph code, they will almost never use
> > > > > > > the d_lock to protect the d_name.
> > > > > > > 
> > > > > > It's probably not needed. The d_name can only change if there are no
> > > > > > other outstanding references to the dentry.
> > > > > If so, the dentry_name = kmalloc() is not needed any more.
> > > > > 
> > > > > 
> > > > I take it back.
> > > > 
> > > > I think the d_lock is the only thing that protects this against a
> > > > concurrent rename. Have a look at __d_move, and (in particular) the call
> > > > to copy_name. The d_lock is what guards against that happening while
> > > > you're working with it here.
> > > > 
> > > > You might be able to get away without using it, but you'll need to
> > > > follow similar rules as RCU walk.
> > > > 
> > > > FWIW, the best source for this info is Neil Brown's writeup in
> > > > Documentation/filesystems/path_lookup.rst.
> > That should have been Documentation/filesystems/path-lookup.rst
> > 
> > 
> > > IMO we can get rid of the 'd_lock' by:
> > > 
> > > char *dentry_name = READ_ONCE(dentry->d_name.name);
> > > 
> > > if (!dir_emit(ctx, dentry_name, strlen(dentry_name),....
> > > 
> > > Because both the swap_names() and copy_name() won't release the memory
> > > of dentry->d_name.name.
> > > 
> > > But possible we will see a corrupted dentry name if it's doing the
> > > copy_name() ?
> > > 
> > ceph already satisfies readdir out of the dcache in some cases. See
> > __dcache_readdir, which basically walks a list of dentry pointers
> > stashed in the pagecache and then calls dir_emit on each (without the
> > dentry->d_lock held!). That's a bit different though since we have some
> > assurance that things won't change in that case (it may still be racy
> > too -- not sure).
> > 
> > I don't think you have the same guarantees here. What might be best is
> > to either allocate (or declare on-stack) a NAME_MAX buffer, and copy
> > into that from the d_name while you hold the lock. Then drop the lock
> > and dir_emit.
> > 
> > I wonder if we're going about this the wrong way...
> > 
> > Once we've decrypted the names, we don't need the crypttext anymore at
> > all. Maybe it would be better to just decrypt the names in-place, or
> > copy back over the crypttext after decrypting? Then we might not even
> > need some of these changes in readdir at all.
> > 
> For this, I think you mean that in ceph_readdir_prepopulate() when the 
> first time decrypting the crypttext we will save it in rde->name or 
> rde->altername, then in ceph_readdir() to emit it directly or 
> base64_encode it again without key.
> 
> Right ?
> 

Right.

Or, possibly even do that in an extra step before
ceph_readdir_prepopulate. Then we might not need as many changes to that
function.

> 
> > > > > > > > +		dentry_name[name_len] = '\0';
> > > > > > > > +		dout("readdir (%d/%d) -> %llx '%s' %p\n",
> > > > > > > > +		     i, rinfo->dir_nr, ctx->pos, dentry_name, &rde->inode.in);
> > > > > > > > +
> > > > > > > > +		if (!dir_emit(ctx, dentry_name, name_len,
> > > > > > > >      			      ceph_present_ino(inode->i_sb, le64_to_cpu(rde->inode.in->ino)),
> > > > > > > >      			      le32_to_cpu(rde->inode.in->mode) >> 12)) {
> > > > > > > >      			/*
> > > > > > > > @@ -566,8 +562,6 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
> > > > > > > >      			goto out;
> > > > > > > >      		}
> > > > > > > >      
> > > > > > > > -		/* Reset the lengths to their original allocated vals */
> > > > > > > > -		oname.len = olen;
> > > > > > > >      		ctx->pos++;
> > > > > > > >      	}
> > > > > > > >      
> > > > > > > > @@ -625,8 +619,8 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
> > > > > > > >      	err = 0;
> > > > > > > >      	dout("readdir %p file %p done.\n", inode, file);
> > > > > > > >      out:
> > > > > > > > -	ceph_fname_free_buffer(inode, &tname);
> > > > > > > > -	ceph_fname_free_buffer(inode, &oname);
> > > > > > > > +	if (dentry_name)
> > > > > > > > +		kfree(dentry_name);
> > > > > > > >      	return err;
> > > > > > > >      }
> > > > > > > >      
> > > > > > > > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> > > > > > > > index b573a0f33450..19e5275eae1c 100644
> > > > > > > > --- a/fs/ceph/inode.c
> > > > > > > > +++ b/fs/ceph/inode.c
> > > > > > > > @@ -1909,6 +1909,7 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
> > > > > > > >      			goto out;
> > > > > > > >      		}
> > > > > > > >      
> > > > > > > > +		rde->dentry = NULL;
> > > > > > > >      		dname.name = oname.name;
> > > > > > > >      		dname.len = oname.len;
> > > > > > > >      		dname.hash = full_name_hash(parent, dname.name, dname.len);
> > > > > > > > @@ -1969,6 +1970,12 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
> > > > > > > >      			goto retry_lookup;
> > > > > > > >      		}
> > > > > > > >      
> > > > > > > > +		/*
> > > > > > > > +		 * ceph_readdir will use the dentry to get the name
> > > > > > > > +		 * to avoid doing the dencrypt again there.
> > > > > > > > +		 */
> > > > > > > > +		rde->dentry = dget(dn);
> > > > > > > > +
> > > > > > > >      		/* inode */
> > > > > > > >      		if (d_really_is_positive(dn)) {
> > > > > > > >      			in = d_inode(dn);
> > > > > > > > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> > > > > > > > index 8d704ddd7291..9e0a51ef1dfa 100644
> > > > > > > > --- a/fs/ceph/mds_client.c
> > > > > > > > +++ b/fs/ceph/mds_client.c
> > > > > > > > @@ -733,6 +733,7 @@ static void destroy_reply_info(struct ceph_mds_reply_info_parsed *info)
> > > > > > > >      
> > > > > > > >      		kfree(rde->inode.fscrypt_auth);
> > > > > > > >      		kfree(rde->inode.fscrypt_file);
> > > > > > > > +		dput(rde->dentry);
> > > > > > > >      	}
> > > > > > > >      	free_pages((unsigned long)info->dir_entries, get_order(info->dir_buf_size));
> > > > > > > >      }
> > > > > > > > diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
> > > > > > > > index 0dfe24f94567..663d7754d57d 100644
> > > > > > > > --- a/fs/ceph/mds_client.h
> > > > > > > > +++ b/fs/ceph/mds_client.h
> > > > > > > > @@ -96,6 +96,7 @@ struct ceph_mds_reply_info_in {
> > > > > > > >      };
> > > > > > > >      
> > > > > > > >      struct ceph_mds_reply_dir_entry {
> > > > > > > > +	struct dentry		      *dentry;
> > > > > > > >      	char                          *name;
> > > > > > > >      	u8			      *altname;
> > > > > > > >      	u32                           name_len;
>

On 3/10/22 10:20 PM, Jeff Layton wrote:
> On Thu, 2022-03-10 at 22:14 +0800, Xiubo Li wrote:
>> On 3/10/22 9:12 PM, Jeff Layton wrote:
>>> On Thu, 2022-03-10 at 20:21 +0800, Xiubo Li wrote:
>>>> On 3/10/22 7:21 PM, Jeff Layton wrote:
>>>>> On Thu, 2022-03-10 at 18:54 +0800, Xiubo Li wrote:
>>>>>> On 3/10/22 6:36 PM, Jeff Layton wrote:
>>>>>>> On Thu, 2022-03-10 at 14:08 +0800, Xiubo Li wrote:
>>>>>>>> On 3/9/22 9:59 PM, xiubli@redhat.com wrote:
>>>>>>>>> From: Xiubo Li <xiubli@redhat.com>
>>>>>>>>>
>>>>>>>>> For the readdir request the dentries will be pasred and dencrypted
>>>>>>>>> in ceph_readdir_prepopulate(). And in ceph_readdir() we could just
>>>>>>>>> get the dentry name from the dentry cache instead of parsing and
>>>>>>>>> dencrypting them again. This could improve performance.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Xiubo Li <xiubli@redhat.com>
>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>> V7:
>>>>>>>>> - Fix the xfstest generic/006 crash bug about the rde->dentry == NULL.
>>>>>>>>>
>>>>>>>>> V6:
>>>>>>>>> - Remove CEPH_ENCRYPTED_LONG_SNAP_NAME_MAX macro and use the NAME_MAX
>>>>>>>>>        instead, since we are limiting the max length of snapshot name to
>>>>>>>>>        240, which is NAME_MAX - 2 * sizeof('_') - sizeof(<inode#>).
>>>>>>>>>
>>>>>>>>> V5:
>>>>>>>>> - fix typo of CEPH_ENCRYPTED_LONG_SNAP_NAME_MAX macro
>>>>>>>>> - release the rde->dentry in destroy_reply_info
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>       fs/ceph/dir.c        | 56 ++++++++++++++++++++------------------------
>>>>>>>>>       fs/ceph/inode.c      |  7 ++++++
>>>>>>>>>       fs/ceph/mds_client.c |  1 +
>>>>>>>>>       fs/ceph/mds_client.h |  1 +
>>>>>>>>>       4 files changed, 34 insertions(+), 31 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
>>>>>>>>> index 6df2a91af236..2397c34e9173 100644
>>>>>>>>> --- a/fs/ceph/dir.c
>>>>>>>>> +++ b/fs/ceph/dir.c
>>>>>>>>> @@ -316,8 +316,7 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>>>>>>>>>       	int err;
>>>>>>>>>       	unsigned frag = -1;
>>>>>>>>>       	struct ceph_mds_reply_info_parsed *rinfo;
>>>>>>>>> -	struct fscrypt_str tname = FSTR_INIT(NULL, 0);
>>>>>>>>> -	struct fscrypt_str oname = FSTR_INIT(NULL, 0);
>>>>>>>>> +	char *dentry_name = NULL;
>>>>>>>>>       
>>>>>>>>>       	dout("readdir %p file %p pos %llx\n", inode, file, ctx->pos);
>>>>>>>>>       	if (dfi->file_info.flags & CEPH_F_ATEND)
>>>>>>>>> @@ -369,14 +368,6 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>>>>>>>>>       		spin_unlock(&ci->i_ceph_lock);
>>>>>>>>>       	}
>>>>>>>>>       
>>>>>>>>> -	err = ceph_fname_alloc_buffer(inode, &tname);
>>>>>>>>> -	if (err < 0)
>>>>>>>>> -		goto out;
>>>>>>>>> -
>>>>>>>>> -	err = ceph_fname_alloc_buffer(inode, &oname);
>>>>>>>>> -	if (err < 0)
>>>>>>>>> -		goto out;
>>>>>>>>> -
>>>>>>>>>       	/* proceed with a normal readdir */
>>>>>>>>>       more:
>>>>>>>>>       	/* do we have the correct frag content buffered? */
>>>>>>>>> @@ -528,31 +519,36 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>>>>>>>>>       			}
>>>>>>>>>       		}
>>>>>>>>>       	}
>>>>>>>>> +
>>>>>>>>> +	dentry_name = kmalloc(NAME_MAX, GFP_KERNEL);
>>>>>>>>> +	if (!dentry_name) {
>>>>>>>>> +		err = -ENOMEM;
>>>>>>>>> +		ceph_mdsc_put_request(dfi->last_readdir);
>>>>>>>>> +		dfi->last_readdir = NULL;
>>>>>>>>> +		goto out;
>>>>>>>>> +	}
>>>>>>>>> +
>>>>>>>>>       	for (; i < rinfo->dir_nr; i++) {
>>>>>>>>>       		struct ceph_mds_reply_dir_entry *rde = rinfo->dir_entries + i;
>>>>>>>>> -		struct ceph_fname fname = { .dir	= inode,
>>>>>>>>> -					    .name	= rde->name,
>>>>>>>>> -					    .name_len	= rde->name_len,
>>>>>>>>> -					    .ctext	= rde->altname,
>>>>>>>>> -					    .ctext_len	= rde->altname_len };
>>>>>>>>> -		u32 olen = oname.len;
>>>>>>>>> -
>>>>>>>>> -		err = ceph_fname_to_usr(&fname, &tname, &oname, NULL);
>>>>>>>>> -		if (err) {
>>>>>>>>> -			pr_err("%s unable to decode %.*s, got %d\n", __func__,
>>>>>>>>> -			       rde->name_len, rde->name, err);
>>>>>>>>> -			goto out;
>>>>>>>>> -		}
>>>>>>>>> +		struct dentry *dn = rde->dentry;
>>>>>>>>> +		int name_len;
>>>>>>>>>       
>>>>>>>>>       		BUG_ON(rde->offset < ctx->pos);
>>>>>>>>>       		BUG_ON(!rde->inode.in);
>>>>>>>>> +		BUG_ON(!rde->dentry);
>>>>>>>>>       
>>>>>>>>>       		ctx->pos = rde->offset;
>>>>>>>>> -		dout("readdir (%d/%d) -> %llx '%.*s' %p\n",
>>>>>>>>> -		     i, rinfo->dir_nr, ctx->pos,
>>>>>>>>> -		     rde->name_len, rde->name, &rde->inode.in);
>>>>>>>>>       
>>>>>>>>> -		if (!dir_emit(ctx, oname.name, oname.len,
>>>>>>>>> +		spin_lock(&dn->d_lock);
>>>>>>>>> +		memcpy(dentry_name, dn->d_name.name, dn->d_name.len);
>>>>>>>>> +		name_len = dn->d_name.len;
>>>>>>>>> +		spin_unlock(&dn->d_lock);
>>>>>>>>> +
>>>>>>>> Hi Jeff,
>>>>>>>>
>>>>>>>> BTW, does the dn->d_lock is must here ? From the code comments, the
>>>>>>>> d_lock intends to protect the 'd_flag' and 'd_lockref.count'.
>>>>>>>>
>>>>>>>> In ceph code I found some places are using the d_lock when accessing the
>>>>>>>> d_name, some are not. And in none ceph code, they will almost never use
>>>>>>>> the d_lock to protect the d_name.
>>>>>>>>
>>>>>>> It's probably not needed. The d_name can only change if there are no
>>>>>>> other outstanding references to the dentry.
>>>>>> If so, the dentry_name = kmalloc() is not needed any more.
>>>>>>
>>>>>>
>>>>> I take it back.
>>>>>
>>>>> I think the d_lock is the only thing that protects this against a
>>>>> concurrent rename. Have a look at __d_move, and (in particular) the call
>>>>> to copy_name. The d_lock is what guards against that happening while
>>>>> you're working with it here.
>>>>>
>>>>> You might be able to get away without using it, but you'll need to
>>>>> follow similar rules as RCU walk.
>>>>>
>>>>> FWIW, the best source for this info is Neil Brown's writeup in
>>>>> Documentation/filesystems/path_lookup.rst.
>>> That should have been Documentation/filesystems/path-lookup.rst
>>>
>>>
>>>> IMO we can get rid of the 'd_lock' by:
>>>>
>>>> char *dentry_name = READ_ONCE(dentry->d_name.name);
>>>>
>>>> if (!dir_emit(ctx, dentry_name, strlen(dentry_name),....
>>>>
>>>> Because both the swap_names() and copy_name() won't release the memory
>>>> of dentry->d_name.name.
>>>>
>>>> But possible we will see a corrupted dentry name if it's doing the
>>>> copy_name() ?
>>>>
>>> ceph already satisfies readdir out of the dcache in some cases. See
>>> __dcache_readdir, which basically walks a list of dentry pointers
>>> stashed in the pagecache and then calls dir_emit on each (without the
>>> dentry->d_lock held!). That's a bit different though since we have some
>>> assurance that things won't change in that case (it may still be racy
>>> too -- not sure).
>>>
>>> I don't think you have the same guarantees here. What might be best is
>>> to either allocate (or declare on-stack) a NAME_MAX buffer, and copy
>>> into that from the d_name while you hold the lock. Then drop the lock
>>> and dir_emit.
>>>
>>> I wonder if we're going about this the wrong way...
>>>
>>> Once we've decrypted the names, we don't need the crypttext anymore at
>>> all. Maybe it would be better to just decrypt the names in-place, or
>>> copy back over the crypttext after decrypting? Then we might not even
>>> need some of these changes in readdir at all.
>>>
>> For this, I think you mean that in ceph_readdir_prepopulate() when the
>> first time decrypting the crypttext we will save it in rde->name or
>> rde->altername, then in ceph_readdir() to emit it directly or
>> base64_encode it again without key.
>>
>> Right ?
>>
> Right.
>
> Or, possibly even do that in an extra step before
> ceph_readdir_prepopulate. Then we might not need as many changes to that
> function.

Yeah, this approach sounds good too.

Let me try it.

- Xiubo


>>>>>>>>> +		dentry_name[name_len] = '\0';
>>>>>>>>> +		dout("readdir (%d/%d) -> %llx '%s' %p\n",
>>>>>>>>> +		     i, rinfo->dir_nr, ctx->pos, dentry_name, &rde->inode.in);
>>>>>>>>> +
>>>>>>>>> +		if (!dir_emit(ctx, dentry_name, name_len,
>>>>>>>>>       			      ceph_present_ino(inode->i_sb, le64_to_cpu(rde->inode.in->ino)),
>>>>>>>>>       			      le32_to_cpu(rde->inode.in->mode) >> 12)) {
>>>>>>>>>       			/*
>>>>>>>>> @@ -566,8 +562,6 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>>>>>>>>>       			goto out;
>>>>>>>>>       		}
>>>>>>>>>       
>>>>>>>>> -		/* Reset the lengths to their original allocated vals */
>>>>>>>>> -		oname.len = olen;
>>>>>>>>>       		ctx->pos++;
>>>>>>>>>       	}
>>>>>>>>>       
>>>>>>>>> @@ -625,8 +619,8 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>>>>>>>>>       	err = 0;
>>>>>>>>>       	dout("readdir %p file %p done.\n", inode, file);
>>>>>>>>>       out:
>>>>>>>>> -	ceph_fname_free_buffer(inode, &tname);
>>>>>>>>> -	ceph_fname_free_buffer(inode, &oname);
>>>>>>>>> +	if (dentry_name)
>>>>>>>>> +		kfree(dentry_name);
>>>>>>>>>       	return err;
>>>>>>>>>       }
>>>>>>>>>       
>>>>>>>>> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
>>>>>>>>> index b573a0f33450..19e5275eae1c 100644
>>>>>>>>> --- a/fs/ceph/inode.c
>>>>>>>>> +++ b/fs/ceph/inode.c
>>>>>>>>> @@ -1909,6 +1909,7 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
>>>>>>>>>       			goto out;
>>>>>>>>>       		}
>>>>>>>>>       
>>>>>>>>> +		rde->dentry = NULL;
>>>>>>>>>       		dname.name = oname.name;
>>>>>>>>>       		dname.len = oname.len;
>>>>>>>>>       		dname.hash = full_name_hash(parent, dname.name, dname.len);
>>>>>>>>> @@ -1969,6 +1970,12 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req,
>>>>>>>>>       			goto retry_lookup;
>>>>>>>>>       		}
>>>>>>>>>       
>>>>>>>>> +		/*
>>>>>>>>> +		 * ceph_readdir will use the dentry to get the name
>>>>>>>>> +		 * to avoid doing the dencrypt again there.
>>>>>>>>> +		 */
>>>>>>>>> +		rde->dentry = dget(dn);
>>>>>>>>> +
>>>>>>>>>       		/* inode */
>>>>>>>>>       		if (d_really_is_positive(dn)) {
>>>>>>>>>       			in = d_inode(dn);
>>>>>>>>> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
>>>>>>>>> index 8d704ddd7291..9e0a51ef1dfa 100644
>>>>>>>>> --- a/fs/ceph/mds_client.c
>>>>>>>>> +++ b/fs/ceph/mds_client.c
>>>>>>>>> @@ -733,6 +733,7 @@ static void destroy_reply_info(struct ceph_mds_reply_info_parsed *info)
>>>>>>>>>       
>>>>>>>>>       		kfree(rde->inode.fscrypt_auth);
>>>>>>>>>       		kfree(rde->inode.fscrypt_file);
>>>>>>>>> +		dput(rde->dentry);
>>>>>>>>>       	}
>>>>>>>>>       	free_pages((unsigned long)info->dir_entries, get_order(info->dir_buf_size));
>>>>>>>>>       }
>>>>>>>>> diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
>>>>>>>>> index 0dfe24f94567..663d7754d57d 100644
>>>>>>>>> --- a/fs/ceph/mds_client.h
>>>>>>>>> +++ b/fs/ceph/mds_client.h
>>>>>>>>> @@ -96,6 +96,7 @@ struct ceph_mds_reply_info_in {
>>>>>>>>>       };
>>>>>>>>>       
>>>>>>>>>       struct ceph_mds_reply_dir_entry {
>>>>>>>>> +	struct dentry		      *dentry;
>>>>>>>>>       	char                          *name;
>>>>>>>>>       	u8			      *altname;
>>>>>>>>>       	u32                           name_len;