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@xxxxxxxxxx> 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,
> ¤t_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)
