Re: [PATCH 3/3] cifs: cache the directory content for shroot

["Aurélien Aptel" <aaptel@suse.com>]

Hi Ronnie,

The more we isolate code with clear opaque interfaces, the less we have
to understand about their low-level implementation.

I was thinking we could move all dir cache related code to a new dcache.c
file or something like that.

One question: when does the cache gets cleared? We dont want to have the
same stale content every time we query the root. Should it be time based?

Comments on code below:

Ronnie Sahlberg <lsahlber@redhat.com> writes:
>  fs/cifs/cifsglob.h |  21 +++++++
>  fs/cifs/misc.c     |   2 +
>  fs/cifs/readdir.c  | 173 ++++++++++++++++++++++++++++++++++++++++++++++++++---
>  fs/cifs/smb2ops.c  |  14 +++++
>  4 files changed, 203 insertions(+), 7 deletions(-)


>
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index b565d83ba89e..e8f2b4a642d4 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -1073,6 +1073,26 @@ cap_unix(struct cifs_ses *ses)
>  	return ses->server->vals->cap_unix & ses->capabilities;
>  }
>  
> +struct cached_dirent {
> +	struct list_head entry;
> +	char *name;
> +	int namelen;
> +	loff_t pos;
> +	u64 ino;
> +	unsigned type;
> +};
> +
> +struct cached_dirents {
> +	bool is_valid:1;
> +	bool is_failed:1;
> +	struct dir_context *ctx; /* Only used to make sure we only take entries
> +				  * from a single context. Never dereferenced.
> +				  */

This type of comments are not in the kernel coding style. First comment
line with "/*" shouldnt have text.

> +	struct mutex de_mutex;
> +	int pos;		 /* Expected ctx->pos */
> +	struct list_head entries;
> +};
> +
>  struct cached_fid {
>  	bool is_valid:1;	/* Do we have a useable root fid */
>  	bool file_all_info_is_valid:1;
> @@ -1083,6 +1103,7 @@ struct cached_fid {
>  	struct cifs_tcon *tcon;
>  	struct work_struct lease_break;
>  	struct smb2_file_all_info file_all_info;
> +	struct cached_dirents dirents;
>  };

>  	}
> +	INIT_LIST_HEAD(&ret_buf->crfid.dirents.entries);
> +	mutex_init(&ret_buf->crfid.dirents.de_mutex);
>  
>  	atomic_inc(&tconInfoAllocCount);
>  	ret_buf->tidStatus = CifsNew;
> diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c
> index 31a18aae5e64..17861c3d2e08 100644
> --- a/fs/cifs/readdir.c
> +++ b/fs/cifs/readdir.c
> @@ -811,9 +811,119 @@ find_cifs_entry(const unsigned int xid, struct cifs_tcon *tcon, loff_t pos,
>  	return rc;
>  }
>  
> +static void init_cached_dirents(struct cached_dirents *cde,
> +				struct dir_context *ctx)
> +{
> +	if (ctx->pos == 2 && cde->ctx == NULL) {
> +		cde->ctx = ctx;
> +		cde->pos = 2;
> +	}
> +}

2 is for "." and ".."? Can you add more comments to document this?

Can you document which functions are expecting to be called with a lock
held?

> +
> +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);
> +		if (ctx->pos >= dirent->pos)
> +			continue;
> +		ctx->pos = dirent->pos;
> +		rc = dir_emit(ctx, dirent->name, dirent->namelen,
> +			      dirent->ino, dirent->type);
> +		if (!rc)
> +			return rc;
> +	}
> +	return true;
> +}
> +
> +static void update_cached_dirents_count(struct cached_dirents *cde,
> +					struct dir_context *ctx)
> +{
> +	if (cde->ctx != ctx)
> +		return;
> +	if (cde->is_valid || cde->is_failed)
> +		return;
> +
> +	cde->pos++;
> +}
> +
> +static void finished_cached_dirents_count(struct cached_dirents *cde,
> +					struct dir_context *ctx)
> +{
> +	if (cde->ctx != ctx)
> +		return;
> +	if (cde->is_valid || cde->is_failed)
> +		return;
> +	if (ctx->pos != cde->pos)
> +		return;
> +
> +	cde->is_valid = 1;
> +}
> +
> +static void add_cached_dirent(struct cached_dirents *cde,
> +			      struct dir_context *ctx,
> +			      const char *name, int namelen,
> +			      u64 ino, unsigned type)
> +{
> +	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, GFP_ATOMIC);
> +	if (de->name == NULL) {
> +		kfree(de);
> +		cde->is_failed = 1;
> +		return;
> +	}
> +	memcpy(de->name, name, namelen);
> +	de->namelen = namelen;
> +	de->pos = ctx->pos;
> +	de->ino = ino;
> +	de->type = type;
> +
> +	list_add_tail(&de->entry, &cde->entries);
> +}
> +
> +static bool cifs_dir_emit(struct dir_context *ctx,
> +			  const char *name, int namelen,
> +			  u64 ino, unsigned type,
> +			  struct cached_fid *cfid)
> +{
> +	bool rc;
> +
> +	rc = dir_emit(ctx, name, namelen, ino, type);
> +	if (!rc)
> +		return rc;
> +
> +	if (cfid) {
> +		mutex_lock(&cfid->dirents.de_mutex);
> +		add_cached_dirent(&cfid->dirents, ctx, name, namelen, ino,
> +				  type);
> +		mutex_unlock(&cfid->dirents.de_mutex);
> +	}
> +
> +	return rc;
> +}
> +
>  static int cifs_filldir(char *find_entry, struct file *file,
> -		struct dir_context *ctx,
> -		char *scratch_buf, unsigned int max_len)
> +			struct dir_context *ctx,
> +			char *scratch_buf, unsigned int max_len,
> +			struct cached_fid *cfid)
>  {
>  	struct cifsFileInfo *file_info = file->private_data;
>  	struct super_block *sb = file_inode(file)->i_sb;
> @@ -903,10 +1013,10 @@ static int cifs_filldir(char *find_entry, struct file *file,
>  	cifs_prime_dcache(file_dentry(file), &name, &fattr);
>  
>  	ino = cifs_uniqueid_to_ino_t(fattr.cf_uniqueid);
> -	return !dir_emit(ctx, name.name, name.len, ino, fattr.cf_dtype);
> +	return !cifs_dir_emit(ctx, name.name, name.len, ino, fattr.cf_dtype,
> +			      cfid);
>  }
>  
> -
>  int cifs_readdir(struct file *file, struct dir_context *ctx)
>  {
>  	int rc = 0;
> @@ -920,6 +1030,8 @@ int cifs_readdir(struct file *file, struct dir_context *ctx)
>  	char *end_of_smb;
>  	unsigned int max_len;
>  	char *full_path = NULL;
> +	struct cached_fid *cfid = NULL;
> +	struct cifs_sb_info *cifs_sb = CIFS_FILE_SB(file);
>  
>  	xid = get_xid();
>  
> @@ -928,7 +1040,6 @@ int cifs_readdir(struct file *file, struct dir_context *ctx)
>  		rc = -ENOMEM;
>  		goto rddir2_exit;
>  	}
> -
>  	/*
>  	 * Ensure FindFirst doesn't fail before doing filldir() for '.' and
>  	 * '..'. Otherwise we won't be able to notify VFS in case of failure.
> @@ -949,6 +1060,31 @@ int cifs_readdir(struct file *file, struct dir_context *ctx)
>  		if after then keep searching till find it */
>  
>  	cifsFile = file->private_data;
> +	tcon = tlink_tcon(cifsFile->tlink);
> +	if (tcon->ses->server->vals->header_preamble_size == 0 &&

header_preamble_size == 0 is a test for smb1? we have is_smb1_server()

> +		!strcmp(full_path, "")) {
> +		rc = open_shroot(xid, tcon, cifs_sb, &cfid);
> +		if (rc)
> +			goto cache_not_found;
> +
> +		mutex_lock(&cfid->dirents.de_mutex);
> +		/* if this was reading from the start of the directory
> +		 * we might need to initialize scanning and storing the
> +		 * directory content.
> +		 */
> +		init_cached_dirents(&cfid->dirents, ctx);
> +		/* If we already have the entire directory cached then
> +		 * we cna just serve the cache.
> +		 */

comment style

> +		if (cfid->dirents.is_valid) {
> +			emit_cached_dirents(&cfid->dirents, ctx);
> +			mutex_unlock(&cfid->dirents.de_mutex);
> +			goto rddir2_exit;
> +		}
> +		mutex_unlock(&cfid->dirents.de_mutex);
> +	}
> + cache_not_found:
> +
>  	if (cifsFile->srch_inf.endOfSearch) {
>  		if (cifsFile->srch_inf.emptyDir) {
>  			cifs_dbg(FYI, "End of search, empty dir\n");
> @@ -960,15 +1096,30 @@ int cifs_readdir(struct file *file, struct dir_context *ctx)
>  		tcon->ses->server->close(xid, tcon, &cifsFile->fid);
>  	} */
>  
> -	tcon = tlink_tcon(cifsFile->tlink);
> +	/* Drop the cache while calling find_cifs_entry in case there will
> +	 * be reconnects during query_directory.
> +	 */

comment style

> +	if (cfid) {
> +		close_shroot(cfid);
> +		cfid = NULL;
> +	}
>  	rc = find_cifs_entry(xid, tcon, ctx->pos, file, full_path,
>  			     &current_entry, &num_to_fill);
> +	if (tcon->ses->server->vals->header_preamble_size == 0 &&
> +		!strcmp(full_path, "")) {
> +		open_shroot(xid, tcon, cifs_sb, &cfid);
> +	}
>  	if (rc) {
>  		cifs_dbg(FYI, "fce error %d\n", rc);
>  		goto rddir2_exit;
>  	} else if (current_entry != NULL) {
>  		cifs_dbg(FYI, "entry %lld found\n", ctx->pos);
>  	} else {
> +		if (cfid) {
> +			mutex_lock(&cfid->dirents.de_mutex);
> +			finished_cached_dirents_count(&cfid->dirents, ctx);
> +			mutex_unlock(&cfid->dirents.de_mutex);
> +		}
>  		cifs_dbg(FYI, "Could not find entry\n");
>  		goto rddir2_exit;
>  	}
> @@ -998,7 +1149,7 @@ int cifs_readdir(struct file *file, struct dir_context *ctx)
>  		 */
>  		*tmp_buf = 0;
>  		rc = cifs_filldir(current_entry, file, ctx,
> -				  tmp_buf, max_len);
> +				  tmp_buf, max_len, cfid);
>  		if (rc) {
>  			if (rc > 0)
>  				rc = 0;
> @@ -1006,6 +1157,12 @@ int cifs_readdir(struct file *file, struct dir_context *ctx)
>  		}
>  
>  		ctx->pos++;
> +		if (cfid) {
> +			mutex_lock(&cfid->dirents.de_mutex);
> +			update_cached_dirents_count(&cfid->dirents, ctx);
> +			mutex_unlock(&cfid->dirents.de_mutex);
> +		}
> +
>  		if (ctx->pos ==
>  			cifsFile->srch_inf.index_of_last_entry) {
>  			cifs_dbg(FYI, "last entry in buf at pos %lld %s\n",
> @@ -1020,6 +1177,8 @@ int cifs_readdir(struct file *file, struct dir_context *ctx)
>  	kfree(tmp_buf);
>  
>  rddir2_exit:
> +	if (cfid)
> +		close_shroot(cfid);
>  	kfree(full_path);
>  	free_xid(xid);
>  	return rc;
> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> index fd6c136066df..280464e21a5f 100644
> --- a/fs/cifs/smb2ops.c
> +++ b/fs/cifs/smb2ops.c
> @@ -605,6 +605,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");
> @@ -613,6 +615,18 @@ smb2_close_cached_fid(struct kref *ref)
>  		cfid->is_valid = false;
>  		cfid->file_all_info_is_valid = false;
>  		cfid->has_lease = false;
> +		mutex_lock(&cfid->dirents.de_mutex);
> +		list_for_each_safe(pos, q, &cfid->dirents.entries) {
> +			dirent = list_entry(pos, struct cached_dirent, entry);
> +			list_del(pos);
> +			kfree(dirent->name);
> +			kfree(dirent);
> +		}
> +		cfid->dirents.is_valid = 0;
> +		cfid->dirents.is_failed = 0;
> +		cfid->dirents.ctx = NULL;
> +		cfid->dirents.pos = 0;
> +		mutex_unlock(&cfid->dirents.de_mutex);
>  	}
>  }


Cheers,
-- 
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)