Re: [PATCH] cifs: cache dirent names for cached directories

[Paulo Alcantara <pc@cjr.nz>]

Hi Ronnie,

Ronnie Sahlberg <lsahlber@redhat.com> writes:

> diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c
> index 1929e80c09ee..8b3fbe6b3580 100644
> --- a/fs/cifs/readdir.c
> +++ b/fs/cifs/readdir.c
> @@ -840,9 +840,112 @@ find_cifs_entry(const unsigned int xid, struct cifs_tcon *tcon, loff_t pos,
>  	return rc;
>  }
>  
> +static bool emit_cached_dirents(struct cached_dirents *cde,
> +				struct dir_context *ctx)
> +{
> +	struct list_head *tmp;
> +	struct cached_dirent *dirent;
> +	int rc;
> +
> +	list_for_each(tmp, &cde->entries) {
> +		dirent = list_entry(tmp, struct cached_dirent, entry);

What about list_for_each_entry()?

> +static void add_cached_dirent(struct cached_dirents *cde,
> +			      struct dir_context *ctx,
> +			      const char *name, int namelen,
> +			      struct cifs_fattr *fattr)
> +{
> +	struct cached_dirent *de;
> +
> +	if (cde->ctx != ctx)
> +		return;
> +	if (cde->is_valid || cde->is_failed)
> +		return;
> +	if (ctx->pos != cde->pos) {
> +		cde->is_failed = 1;
> +		return;
> +	}
> +	de = kzalloc(sizeof(*de), GFP_ATOMIC);
> +	if (de == NULL) {
> +		cde->is_failed = 1;
> +		return;
> +	}
> +	de->name = kzalloc(namelen + 1, GFP_ATOMIC);
> +	if (de->name == NULL) {
> +		kfree(de);
> +		cde->is_failed = 1;
> +		return;
> +	}
> +	memcpy(de->name, name, namelen);

Replace the above kzalloc() & memcpy() with kstrndup()?

> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> index d6aaeff4a30a..10170266f44b 100644
> --- a/fs/cifs/smb2ops.c
> +++ b/fs/cifs/smb2ops.c
> @@ -699,6 +699,8 @@ smb2_close_cached_fid(struct kref *ref)
>  {
>  	struct cached_fid *cfid = container_of(ref, struct cached_fid,
>  					       refcount);
> +	struct list_head *pos, *q;
> +	struct cached_dirent *dirent;
>  
>  	if (cfid->is_valid) {
>  		cifs_dbg(FYI, "clear cached root file handle\n");
> @@ -718,6 +720,21 @@ smb2_close_cached_fid(struct kref *ref)
>  		dput(cfid->dentry);
>  		cfid->dentry = NULL;
>  	}
> +	/*
> +	 * Delete all cached dirent names
> +	 */
> +	mutex_lock(&cfid->dirents.de_mutex);
> +	list_for_each_safe(pos, q, &cfid->dirents.entries) {
> +		dirent = list_entry(pos, struct cached_dirent, entry);

list_for_each_entry_safe()?

Nice job!  Looks good to me,

Reviewed-by: Paulo Alcantara (SUSE) <pc@cjr.nz>