Thanks Aurelien,
On Sat, Oct 3, 2020 at 1:30 AM Aurélien Aptel <aaptel@xxxxxxxx> wrote:
>
> 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.
Lets do that as a follow up.
I would also like to expand the caching from not just "" but, say, the
n latest directories we have used.
>
> 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?
Right now it still depends on the lease break.
We can add a timeout value to it as well.
Let's do that as a separate patch. I will do that once this is done.
>
> 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.
Will fix and resubmit.
>
> > + 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?
Yes, I will clarify that pos==2 is due to dir_emit_dots() and it means
we have just started a
fresh scan.
>
> > +
> > +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)
