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

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

Hi Ronnie,

I'm assuming this is the latest V4:

Can you document which functions require a lock to be held when calling?

Would it work properly if in any of the patched functions we have this scenario:

TASK A                              DEMULTIPLEX THREAD
------                              ------------------
open_shroot()                           
...                                    oplock break on shroot
                                       ...close/reopen shroot, fid and ptr gets updated...
...                                       
do stuff with dirents (with old fid/ptr?)
...
close_shroot()

More comments below:

Ronnie Sahlberg <lsahlber@redhat.com> writes:
> +
> +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;
> +}

Should return cfid->dirents.is_failed?

> -
>  int cifs_readdir(struct file *file, struct dir_context *ctx)
>  {
>  	int rc = 0;
>  	unsigned int xid;
>  	int i;
> -	struct cifs_tcon *tcon;
> +	struct cifs_tcon *tcon, *mtcon;
>  	struct cifsFileInfo *cifsFile = NULL;
>  	char *current_entry;
>  	int num_to_fill = 0;
> @@ -920,15 +1021,59 @@ 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();
> -
>  	full_path = build_path_from_dentry(file_dentry(file));
>  	if (full_path == NULL) {
>  		rc = -ENOMEM;
>  		goto rddir2_exit;
>  	}
>  
> +	mtcon = cifs_sb_master_tcon(cifs_sb);

Why using the master tcon? The rest of the code is using the user
tcon.


> +	if (!is_smb1_server(mtcon->ses->server) && !strcmp(full_path, "")) {
> +		rc = open_shroot(xid, mtcon, 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 need to initialize scanning and storing the
> +		 * directory content.
> +		 */
> +		if (ctx->pos == 0 && cfid->dirents.ctx == NULL) {
> +			cfid->dirents.ctx = ctx;
> +			cfid->dirents.pos = 2;
> +		}
> +		/*
> +		 * If we already have the entire directory cached then
> +		 * we can just serve the cache.
> +		 */
> +		if (cfid->dirents.is_valid) {
> +			if (!dir_emit_dots(file, ctx)){
> +				mutex_unlock(&cfid->dirents.de_mutex);
> +				goto rddir2_exit;
> +			}
> +			emit_cached_dirents(&cfid->dirents, ctx);
> +			mutex_unlock(&cfid->dirents.de_mutex);
> +			goto rddir2_exit;
> +		}
> +		mutex_unlock(&cfid->dirents.de_mutex);
> +	}
> + cache_not_found:
> +
> +	/* Drop the cache while calling initiate_cifs_search and
> +	 * find_cifs_entry in case there will be reconnects during
> +	 * query_directory.
> +	 */

comment style

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)