On Mon, Oct 5, 2020 at 9:47 AM Steve French <smfrench@xxxxxxxxx> wrote:
>
> How do we get linkcount? That seems to be the only thing missing from query dir vs query file info
Do you mean the link-count for subdirectories/files that we cache the
dirent for?
I don't think we can cache that.
If we have a lease on "/" and we have an already existing subdirectory
"/foo" in our dirent cache.
If/when the link count for /foo changes, this does not trigger a lease
break for "/"
For this I think we can probably only cache
* name of object
* type of object
* inode number of object
Changes to any of these are probably the only thing that is guaranteed
to give us a lease break and thus tell us we need to refresh.
>
> On Sun, Oct 4, 2020, 17:20 ronnie sahlberg <ronniesahlberg@xxxxxxxxx> wrote:
>>
>> 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)
