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@xxxxxxxxxx> 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)
