* [PATCH 0/3 V1] cifs: cache the directory content for shroot
@ 2020-10-05 2:37 Ronnie Sahlberg
2020-10-05 2:37 ` [PATCH 1/3] cifs: return cached_fid from open_shroot Ronnie Sahlberg
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Ronnie Sahlberg @ 2020-10-05 2:37 UTC (permalink / raw)
To: linux-cifs; +Cc: Steve French
Steve, Aurelien, List
Please find updated version.
With these patches we now server readdir() out of cache for shroot
which eliminates at least three roundtrips:
* the initial Create+QueryDirectory
* the Query_Directory that fails with no more files
* the Close
Since we only cache name, inode and type we will still have the
Create/GetInfo/Close calls for every direcotry member.
To solve that and eliminate those calls I chatted with Steve
and we could cache all information about those child objects for ~1 second.
That would not plug into cifs_readdir() but in parts different parts of
the cifs.ko code so it should into its own patch for
"fetch stat() data for objects from the directory cache, if available".
That would be a different patch series.
V3:
* always take out shroot on the master tcon
* fix bug where we would still do one FindFirst even if we had everything cached
V2: addressing Aureliens comments
* Fix comment style
* Describe what ctx->pos == 2 means
* use is_smb1_server()
See initial implementation of a mechanism to cache the directory entries for
a shared cache handle (shroot).
We cache all the entries during the initial readdir() scan, using the context
from the vfs layer as the key to handle if there are multiple concurrent readir() scans
of the same directory.
Then if/when we have successfully cached the entire direcotry we will server any
subsequent readdir() from out of cache, avoinding making any query direcotry calls to the server.
As with all of shroot, the cache is kept until the direcotry lease is broken.
The first two patches are small and just a preparation for the third patch. They go as separate
patches to make review easier.
The third patch adds the actual meat of the dirent caching .
For now this might not be too exciting because the only cache the root handle.
I hope in the future we will expand the directory caching to handle any/many direcotries.
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/3] cifs: return cached_fid from open_shroot
2020-10-05 2:37 [PATCH 0/3 V1] cifs: cache the directory content for shroot Ronnie Sahlberg
@ 2020-10-05 2:37 ` Ronnie Sahlberg
2020-10-05 2:37 ` [PATCH 2/3] cifs: compute full_path already in cifs_readdir() Ronnie Sahlberg
2020-10-05 2:37 ` [PATCH 3/3] cifs: cache the directory content for shroot Ronnie Sahlberg
2 siblings, 0 replies; 14+ messages in thread
From: Ronnie Sahlberg @ 2020-10-05 2:37 UTC (permalink / raw)
To: linux-cifs; +Cc: Steve French
Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
---
fs/cifs/smb2inode.c | 11 ++++++-----
fs/cifs/smb2ops.c | 22 +++++++++++++++-------
fs/cifs/smb2proto.h | 3 ++-
3 files changed, 23 insertions(+), 13 deletions(-)
diff --git a/fs/cifs/smb2inode.c b/fs/cifs/smb2inode.c
index eba01d0908dd..df6212e55e10 100644
--- a/fs/cifs/smb2inode.c
+++ b/fs/cifs/smb2inode.c
@@ -511,9 +511,9 @@ smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
int rc;
struct smb2_file_all_info *smb2_data;
__u32 create_options = 0;
- struct cifs_fid fid;
bool no_cached_open = tcon->nohandlecache;
struct cifsFileInfo *cfile;
+ struct cached_fid *cfid = NULL;
*adjust_tz = false;
*symlink = false;
@@ -525,7 +525,7 @@ smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
/* If it is a root and its handle is cached then use it */
if (!strlen(full_path) && !no_cached_open) {
- rc = open_shroot(xid, tcon, cifs_sb, &fid);
+ rc = open_shroot(xid, tcon, cifs_sb, &cfid);
if (rc)
goto out;
@@ -533,12 +533,13 @@ smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon,
move_smb2_info_to_cifs(data,
&tcon->crfid.file_all_info);
} else {
- rc = SMB2_query_info(xid, tcon, fid.persistent_fid,
- fid.volatile_fid, smb2_data);
+ rc = SMB2_query_info(xid, tcon,
+ cfid->fid->persistent_fid,
+ cfid->fid->volatile_fid, smb2_data);
if (!rc)
move_smb2_info_to_cifs(data, smb2_data);
}
- close_shroot(&tcon->crfid);
+ close_shroot(cfid);
goto out;
}
diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index 32f90dc82c84..fd6c136066df 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -651,7 +651,8 @@ smb2_cached_lease_break(struct work_struct *work)
* Open the directory at the root of a share
*/
int open_shroot(unsigned int xid, struct cifs_tcon *tcon,
- struct cifs_sb_info *cifs_sb, struct cifs_fid *pfid)
+ struct cifs_sb_info *cifs_sb,
+ struct cached_fid **cfid)
{
struct cifs_ses *ses = tcon->ses;
struct TCP_Server_Info *server = ses->server;
@@ -666,11 +667,12 @@ int open_shroot(unsigned int xid, struct cifs_tcon *tcon,
int rc, flags = 0;
__le16 utf16_path = 0; /* Null - since an open of top of share */
u8 oplock = SMB2_OPLOCK_LEVEL_II;
+ struct cifs_fid *pfid;
mutex_lock(&tcon->crfid.fid_mutex);
if (tcon->crfid.is_valid) {
cifs_dbg(FYI, "found a cached root file handle\n");
- memcpy(pfid, tcon->crfid.fid, sizeof(struct cifs_fid));
+ *cfid = &tcon->crfid;
kref_get(&tcon->crfid.refcount);
mutex_unlock(&tcon->crfid.fid_mutex);
return 0;
@@ -691,6 +693,7 @@ int open_shroot(unsigned int xid, struct cifs_tcon *tcon,
if (!server->ops->new_lease_key)
return -EIO;
+ pfid = tcon->crfid.fid;
server->ops->new_lease_key(pfid);
memset(rqst, 0, sizeof(rqst));
@@ -820,6 +823,8 @@ int open_shroot(unsigned int xid, struct cifs_tcon *tcon,
SMB2_query_info_free(&rqst[1]);
free_rsp_buf(resp_buftype[0], rsp_iov[0].iov_base);
free_rsp_buf(resp_buftype[1], rsp_iov[1].iov_base);
+ if (rc == 0)
+ *cfid = &tcon->crfid;
return rc;
}
@@ -833,6 +838,7 @@ smb3_qfs_tcon(const unsigned int xid, struct cifs_tcon *tcon,
struct cifs_open_parms oparms;
struct cifs_fid fid;
bool no_cached_open = tcon->nohandlecache;
+ struct cached_fid *cfid = NULL;
oparms.tcon = tcon;
oparms.desired_access = FILE_READ_ATTRIBUTES;
@@ -841,12 +847,14 @@ smb3_qfs_tcon(const unsigned int xid, struct cifs_tcon *tcon,
oparms.fid = &fid;
oparms.reconnect = false;
- if (no_cached_open)
+ if (no_cached_open) {
rc = SMB2_open(xid, &oparms, &srch_path, &oplock, NULL, NULL,
NULL, NULL);
- else
- rc = open_shroot(xid, tcon, cifs_sb, &fid);
-
+ } else {
+ rc = open_shroot(xid, tcon, cifs_sb, &cfid);
+ if (rc == 0)
+ memcpy(&fid, cfid->fid, sizeof(struct cifs_fid));
+ }
if (rc)
return;
@@ -863,7 +871,7 @@ smb3_qfs_tcon(const unsigned int xid, struct cifs_tcon *tcon,
if (no_cached_open)
SMB2_close(xid, tcon, fid.persistent_fid, fid.volatile_fid);
else
- close_shroot(&tcon->crfid);
+ close_shroot(cfid);
}
static void
diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h
index 2f8ecbf54214..67c50d78caa1 100644
--- a/fs/cifs/smb2proto.h
+++ b/fs/cifs/smb2proto.h
@@ -70,7 +70,8 @@ extern int smb3_handle_read_data(struct TCP_Server_Info *server,
struct mid_q_entry *mid);
extern int open_shroot(unsigned int xid, struct cifs_tcon *tcon,
- struct cifs_sb_info *cifs_sb, struct cifs_fid *pfid);
+ struct cifs_sb_info *cifs_sb,
+ struct cached_fid **cfid);
extern void close_shroot(struct cached_fid *cfid);
extern void close_shroot_lease(struct cached_fid *cfid);
extern void close_shroot_lease_locked(struct cached_fid *cfid);
--
2.13.6
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/3] cifs: compute full_path already in cifs_readdir()
2020-10-05 2:37 [PATCH 0/3 V1] cifs: cache the directory content for shroot Ronnie Sahlberg
2020-10-05 2:37 ` [PATCH 1/3] cifs: return cached_fid from open_shroot Ronnie Sahlberg
@ 2020-10-05 2:37 ` Ronnie Sahlberg
2020-10-05 2:37 ` [PATCH 3/3] cifs: cache the directory content for shroot Ronnie Sahlberg
2 siblings, 0 replies; 14+ messages in thread
From: Ronnie Sahlberg @ 2020-10-05 2:37 UTC (permalink / raw)
To: linux-cifs; +Cc: Steve French
Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
---
fs/cifs/readdir.c | 30 ++++++++++++++++--------------
1 file changed, 16 insertions(+), 14 deletions(-)
diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c
index 6df0922e7e30..31a18aae5e64 100644
--- a/fs/cifs/readdir.c
+++ b/fs/cifs/readdir.c
@@ -360,11 +360,11 @@ int get_symlink_reparse_path(char *full_path, struct cifs_sb_info *cifs_sb,
*/
static int
-initiate_cifs_search(const unsigned int xid, struct file *file)
+initiate_cifs_search(const unsigned int xid, struct file *file,
+ char *full_path)
{
__u16 search_flags;
int rc = 0;
- char *full_path = NULL;
struct cifsFileInfo *cifsFile;
struct cifs_sb_info *cifs_sb = CIFS_FILE_SB(file);
struct tcon_link *tlink = NULL;
@@ -400,12 +400,6 @@ initiate_cifs_search(const unsigned int xid, struct file *file)
cifsFile->invalidHandle = true;
cifsFile->srch_inf.endOfSearch = false;
- full_path = build_path_from_dentry(file_dentry(file));
- if (full_path == NULL) {
- rc = -ENOMEM;
- goto error_exit;
- }
-
cifs_dbg(FYI, "Full path: %s start at: %lld\n", full_path, file->f_pos);
ffirst_retry:
@@ -444,7 +438,6 @@ initiate_cifs_search(const unsigned int xid, struct file *file)
goto ffirst_retry;
}
error_exit:
- kfree(full_path);
cifs_put_tlink(tlink);
return rc;
}
@@ -688,7 +681,8 @@ static int cifs_save_resume_key(const char *current_entry,
*/
static int
find_cifs_entry(const unsigned int xid, struct cifs_tcon *tcon, loff_t pos,
- struct file *file, char **current_entry, int *num_to_ret)
+ struct file *file, char *full_path,
+ char **current_entry, int *num_to_ret)
{
__u16 search_flags;
int rc = 0;
@@ -741,7 +735,7 @@ find_cifs_entry(const unsigned int xid, struct cifs_tcon *tcon, loff_t pos,
ntwrk_buf_start);
cfile->srch_inf.ntwrk_buf_start = NULL;
}
- rc = initiate_cifs_search(xid, file);
+ rc = initiate_cifs_search(xid, file, full_path);
if (rc) {
cifs_dbg(FYI, "error %d reinitiating a search on rewind\n",
rc);
@@ -925,15 +919,22 @@ int cifs_readdir(struct file *file, struct dir_context *ctx)
char *tmp_buf = NULL;
char *end_of_smb;
unsigned int max_len;
+ char *full_path = NULL;
xid = get_xid();
+ full_path = build_path_from_dentry(file_dentry(file));
+ if (full_path == NULL) {
+ 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.
*/
if (file->private_data == NULL) {
- rc = initiate_cifs_search(xid, file);
+ rc = initiate_cifs_search(xid, file, full_path);
cifs_dbg(FYI, "initiate cifs search rc %d\n", rc);
if (rc)
goto rddir2_exit;
@@ -960,8 +961,8 @@ int cifs_readdir(struct file *file, struct dir_context *ctx)
} */
tcon = tlink_tcon(cifsFile->tlink);
- rc = find_cifs_entry(xid, tcon, ctx->pos, file, ¤t_entry,
- &num_to_fill);
+ rc = find_cifs_entry(xid, tcon, ctx->pos, file, full_path,
+ ¤t_entry, &num_to_fill);
if (rc) {
cifs_dbg(FYI, "fce error %d\n", rc);
goto rddir2_exit;
@@ -1019,6 +1020,7 @@ int cifs_readdir(struct file *file, struct dir_context *ctx)
kfree(tmp_buf);
rddir2_exit:
+ kfree(full_path);
free_xid(xid);
return rc;
}
--
2.13.6
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 3/3] cifs: cache the directory content for shroot
2020-10-05 2:37 [PATCH 0/3 V1] cifs: cache the directory content for shroot Ronnie Sahlberg
2020-10-05 2:37 ` [PATCH 1/3] cifs: return cached_fid from open_shroot Ronnie Sahlberg
2020-10-05 2:37 ` [PATCH 2/3] cifs: compute full_path already in cifs_readdir() Ronnie Sahlberg
@ 2020-10-05 2:37 ` Ronnie Sahlberg
2 siblings, 0 replies; 14+ messages in thread
From: Ronnie Sahlberg @ 2020-10-05 2:37 UTC (permalink / raw)
To: linux-cifs; +Cc: Steve French
Add caching of the directory content for the shroot.
Populate the cache during the initial scan in readdir()
and then try to serve out of the cache for subsequent scans.
Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
---
fs/cifs/cifsglob.h | 22 +++++++
fs/cifs/misc.c | 2 +
fs/cifs/readdir.c | 175 ++++++++++++++++++++++++++++++++++++++++++++++++++---
fs/cifs/smb2ops.c | 14 +++++
4 files changed, 205 insertions(+), 8 deletions(-)
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index b565d83ba89e..487e6de80e47 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -1073,6 +1073,27 @@ 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.
+ */
+ 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 +1104,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;
};
/*
diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
index 1c14cf01dbef..a106bd3cc20b 100644
--- a/fs/cifs/misc.c
+++ b/fs/cifs/misc.c
@@ -124,6 +124,8 @@ tconInfoAlloc(void)
kfree(ret_buf);
return NULL;
}
+ 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..a6432ab3995d 100644
--- a/fs/cifs/readdir.c
+++ b/fs/cifs/readdir.c
@@ -811,9 +811,110 @@ find_cifs_entry(const unsigned int xid, struct cifs_tcon *tcon, loff_t pos,
return rc;
}
+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,16 +1004,16 @@ 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;
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,57 @@ 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);
+ 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))
+ 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.
+ */
+ if (cfid) {
+ close_shroot(cfid);
+ cfid = NULL;
+ }
+
+
/*
* 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 +1092,7 @@ 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 (cifsFile->srch_inf.endOfSearch) {
if (cifsFile->srch_inf.emptyDir) {
cifs_dbg(FYI, "End of search, empty dir\n");
@@ -960,15 +1104,22 @@ int cifs_readdir(struct file *file, struct dir_context *ctx)
tcon->ses->server->close(xid, tcon, &cifsFile->fid);
} */
- tcon = tlink_tcon(cifsFile->tlink);
rc = find_cifs_entry(xid, tcon, ctx->pos, file, full_path,
¤t_entry, &num_to_fill);
+ if (!is_smb1_server(tcon->ses->server) && !strcmp(full_path, "")) {
+ open_shroot(xid, mtcon, 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);
}
}
--
2.13.6
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] cifs: cache the directory content for shroot
2020-10-01 20:50 ` [PATCH 3/3] cifs: cache the " Ronnie Sahlberg
2020-10-01 22:24 ` Steve French
2020-10-02 15:29 ` Aurélien Aptel
@ 2020-10-20 10:28 ` Shyam Prasad N
2 siblings, 0 replies; 14+ messages in thread
From: Shyam Prasad N @ 2020-10-20 10:28 UTC (permalink / raw)
To: Ronnie Sahlberg; +Cc: linux-cifs, Steve French
Please read my comments inline...
On Fri, Oct 2, 2020 at 2:21 AM Ronnie Sahlberg <lsahlber@redhat.com> wrote:
>
> Add caching of the directory content for the shroot.
> Populate the cache during the initial scan in readdir()
> and then try to serve out of the cache for subsequent scans.
>
> Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> ---
> 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.
> + */
Since this is an opaque value, can we save this as a non-pointer or
void * so that any references in the future will emit compiler errors?
> + 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;
> };
>
> /*
> diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
> index 1c14cf01dbef..a106bd3cc20b 100644
> --- a/fs/cifs/misc.c
> +++ b/fs/cifs/misc.c
> @@ -124,6 +124,8 @@ tconInfoAlloc(void)
> kfree(ret_buf);
> return NULL;
> }
> + 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;
Is this needed? pos is already 2.
> + }
> +}
> +
> +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) {
If the directory is large, can this loop take a long time?
Maybe okay for now. But maybe something to think?
> + 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 &&
> + !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 ctx passed is different from the ctx in cfid->dirents, should we
skip serving from cache?
Do we need to invalidate the cached dentries in that case?
Also, do we need to check if cifsFile->invalidHandle?
> + /* If we already have the entire directory cached then
> + * we cna just serve the cache.
> + */
Typo: cna -> can
> + if (cfid->dirents.is_valid) {
Should we also check if is_dir_changed(file)? If the dir has changed,
we should not invalidate the cache.
> + emit_cached_dirents(&cfid->dirents, ctx);
> + mutex_unlock(&cfid->dirents.de_mutex);
> + goto rddir2_exit;
What if we can cache only a portion of the request from cache? Can
there still be entries which we have not served from cache?
I think we'd still be okay, since the user would call readdir again
with pos updated. But can we not go to rddir2_exit and proceed here?
Since ctx->pos would be updated anyway, I think we don't need to end here.
> + }
> + 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.
> + */
> + 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);
> }
> }
>
> --
> 2.13.6
>
Also, does it make sense to keep a mount option to enable/disable
directory caching till this feature is stabilized?
--
-Shyam
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] cifs: cache the directory content for shroot
2020-10-05 21:16 Ronnie Sahlberg
@ 2020-10-07 12:21 ` Aurélien Aptel
0 siblings, 0 replies; 14+ messages in thread
From: Aurélien Aptel @ 2020-10-07 12:21 UTC (permalink / raw)
To: Ronnie Sahlberg, linux-cifs; +Cc: Steve French
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)
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 3/3] cifs: cache the directory content for shroot
@ 2020-10-05 21:16 Ronnie Sahlberg
2020-10-07 12:21 ` Aurélien Aptel
0 siblings, 1 reply; 14+ messages in thread
From: Ronnie Sahlberg @ 2020-10-05 21:16 UTC (permalink / raw)
To: linux-cifs; +Cc: Steve French
Add caching of the directory content for the shroot.
Populate the cache during the initial scan in readdir()
and then try to serve out of the cache for subsequent scans.
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Julia Lawall <julia.lawall@lip6.fr>
Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
---
fs/cifs/cifsglob.h | 22 +++++++
fs/cifs/misc.c | 2 +
fs/cifs/readdir.c | 177 ++++++++++++++++++++++++++++++++++++++++++++++++++---
fs/cifs/smb2ops.c | 14 +++++
4 files changed, 207 insertions(+), 8 deletions(-)
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index b565d83ba89e..487e6de80e47 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -1073,6 +1073,27 @@ 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.
+ */
+ 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 +1104,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;
};
/*
diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
index 1c14cf01dbef..a106bd3cc20b 100644
--- a/fs/cifs/misc.c
+++ b/fs/cifs/misc.c
@@ -124,6 +124,8 @@ tconInfoAlloc(void)
kfree(ret_buf);
return NULL;
}
+ 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..4779ae15307f 100644
--- a/fs/cifs/readdir.c
+++ b/fs/cifs/readdir.c
@@ -811,9 +811,110 @@ find_cifs_entry(const unsigned int xid, struct cifs_tcon *tcon, loff_t pos,
return rc;
}
+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,16 +1004,16 @@ 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;
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);
+ 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.
+ */
+ if (cfid) {
+ close_shroot(cfid);
+ cfid = NULL;
+ }
+
+
/*
* 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 +1094,7 @@ 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 (cifsFile->srch_inf.endOfSearch) {
if (cifsFile->srch_inf.emptyDir) {
cifs_dbg(FYI, "End of search, empty dir\n");
@@ -960,15 +1106,22 @@ int cifs_readdir(struct file *file, struct dir_context *ctx)
tcon->ses->server->close(xid, tcon, &cifsFile->fid);
} */
- tcon = tlink_tcon(cifsFile->tlink);
rc = find_cifs_entry(xid, tcon, ctx->pos, file, full_path,
¤t_entry, &num_to_fill);
+ if (!is_smb1_server(tcon->ses->server) && !strcmp(full_path, "")) {
+ open_shroot(xid, mtcon, 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 +1151,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 +1159,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 +1179,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);
}
}
--
2.13.6
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] cifs: cache the directory content for shroot
@ 2020-10-05 6:47 kernel test robot
0 siblings, 0 replies; 14+ messages in thread
From: kernel test robot @ 2020-10-05 6:47 UTC (permalink / raw)
To: kbuild
[-- Attachment #1: Type: text/plain, Size: 15037 bytes --]
CC: kbuild-all(a)lists.01.org
In-Reply-To: <20201005023754.13604-4-lsahlber@redhat.com>
References: <20201005023754.13604-4-lsahlber@redhat.com>
TO: Ronnie Sahlberg <lsahlber@redhat.com>
TO: "linux-cifs" <linux-cifs@vger.kernel.org>
CC: Steve French <smfrench@gmail.com>
Hi Ronnie,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on cifs/for-next]
[also build test WARNING on v5.9-rc8 next-20201002]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Ronnie-Sahlberg/cifs-cache-the-directory-content-for-shroot/20201005-104037
base: git://git.samba.org/sfrench/cifs-2.6.git for-next
:::::: branch date: 4 hours ago
:::::: commit date: 4 hours ago
config: i386-randconfig-c001-20201005 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Julia Lawall <julia.lawall@lip6.fr>
echo
echo "coccinelle warnings: (new ones prefixed by >>)"
echo
>> fs/cifs/readdir.c:1184:1-7: preceding lock on line 1040
vim +1184 fs/cifs/readdir.c
^1da177e4c3f415 Linus Torvalds 2005-04-16 1010
be4ccdcc2575ae1 Al Viro 2013-05-22 1011 int cifs_readdir(struct file *file, struct dir_context *ctx)
^1da177e4c3f415 Linus Torvalds 2005-04-16 1012 {
^1da177e4c3f415 Linus Torvalds 2005-04-16 1013 int rc = 0;
6d5786a34d98bff Pavel Shilovsky 2012-06-20 1014 unsigned int xid;
6d5786a34d98bff Pavel Shilovsky 2012-06-20 1015 int i;
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1016 struct cifs_tcon *tcon, *mtcon;
^1da177e4c3f415 Linus Torvalds 2005-04-16 1017 struct cifsFileInfo *cifsFile = NULL;
^1da177e4c3f415 Linus Torvalds 2005-04-16 1018 char *current_entry;
^1da177e4c3f415 Linus Torvalds 2005-04-16 1019 int num_to_fill = 0;
^1da177e4c3f415 Linus Torvalds 2005-04-16 1020 char *tmp_buf = NULL;
^1da177e4c3f415 Linus Torvalds 2005-04-16 1021 char *end_of_smb;
18295796a30cada Jeff Layton 2009-04-30 1022 unsigned int max_len;
010d984773e7614 Ronnie Sahlberg 2020-10-05 1023 char *full_path = NULL;
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1024 struct cached_fid *cfid = NULL;
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1025 struct cifs_sb_info *cifs_sb = CIFS_FILE_SB(file);
^1da177e4c3f415 Linus Torvalds 2005-04-16 1026
6d5786a34d98bff Pavel Shilovsky 2012-06-20 1027 xid = get_xid();
010d984773e7614 Ronnie Sahlberg 2020-10-05 1028 full_path = build_path_from_dentry(file_dentry(file));
010d984773e7614 Ronnie Sahlberg 2020-10-05 1029 if (full_path == NULL) {
010d984773e7614 Ronnie Sahlberg 2020-10-05 1030 rc = -ENOMEM;
010d984773e7614 Ronnie Sahlberg 2020-10-05 1031 goto rddir2_exit;
010d984773e7614 Ronnie Sahlberg 2020-10-05 1032 }
010d984773e7614 Ronnie Sahlberg 2020-10-05 1033
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1034 mtcon = cifs_sb_master_tcon(cifs_sb);
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1035 if (!is_smb1_server(mtcon->ses->server) && !strcmp(full_path, "")) {
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1036 rc = open_shroot(xid, mtcon, cifs_sb, &cfid);
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1037 if (rc)
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1038 goto cache_not_found;
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1039
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 @1040 mutex_lock(&cfid->dirents.de_mutex);
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1041 /*
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1042 * If this was reading from the start of the directory
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1043 * we need to initialize scanning and storing the
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1044 * directory content.
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1045 */
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1046 if (ctx->pos == 0 && cfid->dirents.ctx == NULL) {
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1047 cfid->dirents.ctx = ctx;
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1048 cfid->dirents.pos = 2;
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1049 }
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1050 /*
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1051 * If we already have the entire directory cached then
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1052 * we can just serve the cache.
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1053 */
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1054 if (cfid->dirents.is_valid) {
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1055 if (!dir_emit_dots(file, ctx))
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1056 goto rddir2_exit;
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1057 emit_cached_dirents(&cfid->dirents, ctx);
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1058 mutex_unlock(&cfid->dirents.de_mutex);
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1059 goto rddir2_exit;
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1060 }
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1061 mutex_unlock(&cfid->dirents.de_mutex);
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1062 }
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1063 cache_not_found:
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1064
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1065 /* Drop the cache while calling initiate_cifs_search and
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1066 * find_cifs_entry in case there will be reconnects during
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1067 * query_directory.
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1068 */
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1069 if (cfid) {
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1070 close_shroot(cfid);
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1071 cfid = NULL;
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1072 }
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1073
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1074
6221ddd0f5e2ddc Suresh Jayaraman 2010-10-01 1075 /*
6221ddd0f5e2ddc Suresh Jayaraman 2010-10-01 1076 * Ensure FindFirst doesn't fail before doing filldir() for '.' and
6221ddd0f5e2ddc Suresh Jayaraman 2010-10-01 1077 * '..'. Otherwise we won't be able to notify VFS in case of failure.
6221ddd0f5e2ddc Suresh Jayaraman 2010-10-01 1078 */
6221ddd0f5e2ddc Suresh Jayaraman 2010-10-01 1079 if (file->private_data == NULL) {
010d984773e7614 Ronnie Sahlberg 2020-10-05 1080 rc = initiate_cifs_search(xid, file, full_path);
f96637be081141d Joe Perches 2013-05-04 1081 cifs_dbg(FYI, "initiate cifs search rc %d\n", rc);
6221ddd0f5e2ddc Suresh Jayaraman 2010-10-01 1082 if (rc)
6221ddd0f5e2ddc Suresh Jayaraman 2010-10-01 1083 goto rddir2_exit;
6221ddd0f5e2ddc Suresh Jayaraman 2010-10-01 1084 }
6221ddd0f5e2ddc Suresh Jayaraman 2010-10-01 1085
be4ccdcc2575ae1 Al Viro 2013-05-22 1086 if (!dir_emit_dots(file, ctx))
be4ccdcc2575ae1 Al Viro 2013-05-22 1087 goto rddir2_exit;
be4ccdcc2575ae1 Al Viro 2013-05-22 1088
^1da177e4c3f415 Linus Torvalds 2005-04-16 1089 /* 1) If search is active,
^1da177e4c3f415 Linus Torvalds 2005-04-16 1090 is in current search buffer?
^1da177e4c3f415 Linus Torvalds 2005-04-16 1091 if it before then restart search
^1da177e4c3f415 Linus Torvalds 2005-04-16 1092 if after then keep searching till find it */
^1da177e4c3f415 Linus Torvalds 2005-04-16 1093
^1da177e4c3f415 Linus Torvalds 2005-04-16 1094 cifsFile = file->private_data;
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1095 tcon = tlink_tcon(cifsFile->tlink);
^1da177e4c3f415 Linus Torvalds 2005-04-16 1096 if (cifsFile->srch_inf.endOfSearch) {
^1da177e4c3f415 Linus Torvalds 2005-04-16 1097 if (cifsFile->srch_inf.emptyDir) {
f96637be081141d Joe Perches 2013-05-04 1098 cifs_dbg(FYI, "End of search, empty dir\n");
^1da177e4c3f415 Linus Torvalds 2005-04-16 1099 rc = 0;
be4ccdcc2575ae1 Al Viro 2013-05-22 1100 goto rddir2_exit;
^1da177e4c3f415 Linus Torvalds 2005-04-16 1101 }
^1da177e4c3f415 Linus Torvalds 2005-04-16 1102 } /* else {
4b18f2a9c3964f7 Steve French 2008-04-29 1103 cifsFile->invalidHandle = true;
92fc65a74a2be13 Pavel Shilovsky 2012-09-18 1104 tcon->ses->server->close(xid, tcon, &cifsFile->fid);
aaa9bbe039febf1 Steve French 2008-05-23 1105 } */
^1da177e4c3f415 Linus Torvalds 2005-04-16 1106
010d984773e7614 Ronnie Sahlberg 2020-10-05 1107 rc = find_cifs_entry(xid, tcon, ctx->pos, file, full_path,
010d984773e7614 Ronnie Sahlberg 2020-10-05 1108 ¤t_entry, &num_to_fill);
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1109 if (!is_smb1_server(tcon->ses->server) && !strcmp(full_path, "")) {
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1110 open_shroot(xid, mtcon, cifs_sb, &cfid);
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1111 }
^1da177e4c3f415 Linus Torvalds 2005-04-16 1112 if (rc) {
f96637be081141d Joe Perches 2013-05-04 1113 cifs_dbg(FYI, "fce error %d\n", rc);
^1da177e4c3f415 Linus Torvalds 2005-04-16 1114 goto rddir2_exit;
^1da177e4c3f415 Linus Torvalds 2005-04-16 1115 } else if (current_entry != NULL) {
be4ccdcc2575ae1 Al Viro 2013-05-22 1116 cifs_dbg(FYI, "entry %lld found\n", ctx->pos);
^1da177e4c3f415 Linus Torvalds 2005-04-16 1117 } else {
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1118 if (cfid) {
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1119 mutex_lock(&cfid->dirents.de_mutex);
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1120 finished_cached_dirents_count(&cfid->dirents, ctx);
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1121 mutex_unlock(&cfid->dirents.de_mutex);
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1122 }
a0a3036b81f1f66 Joe Perches 2020-04-14 1123 cifs_dbg(FYI, "Could not find entry\n");
^1da177e4c3f415 Linus Torvalds 2005-04-16 1124 goto rddir2_exit;
^1da177e4c3f415 Linus Torvalds 2005-04-16 1125 }
f96637be081141d Joe Perches 2013-05-04 1126 cifs_dbg(FYI, "loop through %d times filling dir for net buf %p\n",
b6b38f704a8193d Joe Perches 2010-04-21 1127 num_to_fill, cifsFile->srch_inf.ntwrk_buf_start);
92fc65a74a2be13 Pavel Shilovsky 2012-09-18 1128 max_len = tcon->ses->server->ops->calc_smb_size(
9ec672bd17131fe Ronnie Sahlberg 2018-04-22 1129 cifsFile->srch_inf.ntwrk_buf_start,
9ec672bd17131fe Ronnie Sahlberg 2018-04-22 1130 tcon->ses->server);
5bafd76593f0605 Steve French 2006-06-07 1131 end_of_smb = cifsFile->srch_inf.ntwrk_buf_start + max_len;
5bafd76593f0605 Steve French 2006-06-07 1132
f58841666bc22e8 Jeff Layton 2009-04-30 1133 tmp_buf = kmalloc(UNICODE_NAME_MAX, GFP_KERNEL);
f55fdcca6bf1c17 Kulikov Vasiliy 2010-07-16 1134 if (tmp_buf == NULL) {
f55fdcca6bf1c17 Kulikov Vasiliy 2010-07-16 1135 rc = -ENOMEM;
be4ccdcc2575ae1 Al Viro 2013-05-22 1136 goto rddir2_exit;
f55fdcca6bf1c17 Kulikov Vasiliy 2010-07-16 1137 }
f55fdcca6bf1c17 Kulikov Vasiliy 2010-07-16 1138
be4ccdcc2575ae1 Al Viro 2013-05-22 1139 for (i = 0; i < num_to_fill; i++) {
^1da177e4c3f415 Linus Torvalds 2005-04-16 1140 if (current_entry == NULL) {
^1da177e4c3f415 Linus Torvalds 2005-04-16 1141 /* evaluate whether this case is an error */
f96637be081141d Joe Perches 2013-05-04 1142 cifs_dbg(VFS, "past SMB end, num to fill %d i %d\n",
b6b38f704a8193d Joe Perches 2010-04-21 1143 num_to_fill, i);
^1da177e4c3f415 Linus Torvalds 2005-04-16 1144 break;
^1da177e4c3f415 Linus Torvalds 2005-04-16 1145 }
92fc65a74a2be13 Pavel Shilovsky 2012-09-18 1146 /*
92fc65a74a2be13 Pavel Shilovsky 2012-09-18 1147 * if buggy server returns . and .. late do we want to
92fc65a74a2be13 Pavel Shilovsky 2012-09-18 1148 * check for that here?
92fc65a74a2be13 Pavel Shilovsky 2012-09-18 1149 */
01b9b0b28626db4 Vasily Averin 2016-01-14 1150 *tmp_buf = 0;
be4ccdcc2575ae1 Al Viro 2013-05-22 1151 rc = cifs_filldir(current_entry, file, ctx,
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1152 tmp_buf, max_len, cfid);
be4ccdcc2575ae1 Al Viro 2013-05-22 1153 if (rc) {
be4ccdcc2575ae1 Al Viro 2013-05-22 1154 if (rc > 0)
7ca85ba752e521f Steve French 2006-10-30 1155 rc = 0;
7ca85ba752e521f Steve French 2006-10-30 1156 break;
7ca85ba752e521f Steve French 2006-10-30 1157 }
7ca85ba752e521f Steve French 2006-10-30 1158
be4ccdcc2575ae1 Al Viro 2013-05-22 1159 ctx->pos++;
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1160 if (cfid) {
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1161 mutex_lock(&cfid->dirents.de_mutex);
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1162 update_cached_dirents_count(&cfid->dirents, ctx);
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1163 mutex_unlock(&cfid->dirents.de_mutex);
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1164 }
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1165
be4ccdcc2575ae1 Al Viro 2013-05-22 1166 if (ctx->pos ==
3979877e5606ecc Steve French 2006-05-31 1167 cifsFile->srch_inf.index_of_last_entry) {
f96637be081141d Joe Perches 2013-05-04 1168 cifs_dbg(FYI, "last entry in buf at pos %lld %s\n",
be4ccdcc2575ae1 Al Viro 2013-05-22 1169 ctx->pos, tmp_buf);
^1da177e4c3f415 Linus Torvalds 2005-04-16 1170 cifs_save_resume_key(current_entry, cifsFile);
^1da177e4c3f415 Linus Torvalds 2005-04-16 1171 break;
^1da177e4c3f415 Linus Torvalds 2005-04-16 1172 } else
5bafd76593f0605 Steve French 2006-06-07 1173 current_entry =
5bafd76593f0605 Steve French 2006-06-07 1174 nxt_dir_entry(current_entry, end_of_smb,
5bafd76593f0605 Steve French 2006-06-07 1175 cifsFile->srch_inf.info_level);
^1da177e4c3f415 Linus Torvalds 2005-04-16 1176 }
^1da177e4c3f415 Linus Torvalds 2005-04-16 1177 kfree(tmp_buf);
^1da177e4c3f415 Linus Torvalds 2005-04-16 1178
^1da177e4c3f415 Linus Torvalds 2005-04-16 1179 rddir2_exit:
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1180 if (cfid)
7975d587dc0ef40 Ronnie Sahlberg 2020-10-05 1181 close_shroot(cfid);
010d984773e7614 Ronnie Sahlberg 2020-10-05 1182 kfree(full_path);
6d5786a34d98bff Pavel Shilovsky 2012-06-20 1183 free_xid(xid);
^1da177e4c3f415 Linus Torvalds 2005-04-16 @1184 return rc;
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 30782 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] cifs: cache the directory content for shroot
[not found] ` <CAH2r5mvijc=-JdmPMUxAUqmJKy0-x3o72NsHx+QcByBnggGXMA@mail.gmail.com>
@ 2020-10-05 0:20 ` ronnie sahlberg
0 siblings, 0 replies; 14+ messages in thread
From: ronnie sahlberg @ 2020-10-05 0:20 UTC (permalink / raw)
To: Steve French; +Cc: Aurélien Aptel, Ronnie Sahlberg, linux-cifs
On Mon, Oct 5, 2020 at 9:47 AM Steve French <smfrench@gmail.com> 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@gmail.com> wrote:
>>
>> Thanks Aurelien,
>>
>> On Sat, Oct 3, 2020 at 1:30 AM Aurélien Aptel <aaptel@suse.com> 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@redhat.com> 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)
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 3/3] cifs: cache the directory content for shroot
2020-10-04 23:37 [PATCH 0/3 V2]: cifs: cache " Ronnie Sahlberg
@ 2020-10-04 23:37 ` Ronnie Sahlberg
0 siblings, 0 replies; 14+ messages in thread
From: Ronnie Sahlberg @ 2020-10-04 23:37 UTC (permalink / raw)
To: linux-cifs; +Cc: Steve French
Add caching of the directory content for the shroot.
Populate the cache during the initial scan in readdir()
and then try to serve out of the cache for subsequent scans.
Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
---
fs/cifs/cifsglob.h | 22 +++++++
fs/cifs/misc.c | 2 +
fs/cifs/readdir.c | 169 ++++++++++++++++++++++++++++++++++++++++++++++++++---
fs/cifs/smb2ops.c | 14 +++++
4 files changed, 200 insertions(+), 7 deletions(-)
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index b565d83ba89e..487e6de80e47 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -1073,6 +1073,27 @@ 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.
+ */
+ 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 +1104,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;
};
/*
diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
index 1c14cf01dbef..a106bd3cc20b 100644
--- a/fs/cifs/misc.c
+++ b/fs/cifs/misc.c
@@ -124,6 +124,8 @@ tconInfoAlloc(void)
kfree(ret_buf);
return NULL;
}
+ 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..10b651e585fc 100644
--- a/fs/cifs/readdir.c
+++ b/fs/cifs/readdir.c
@@ -811,9 +811,110 @@ find_cifs_entry(const unsigned int xid, struct cifs_tcon *tcon, loff_t pos,
return rc;
}
+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 +1004,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 +1021,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 +1031,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 +1051,37 @@ 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 (!is_smb1_server(tcon->ses->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 need to initialize scanning and storing the
+ * directory content.
+ * We check for pos == 2 because we have already emitted
+ * '.' and '..' above in dir_emit_dots()
+ */
+ if (ctx->pos == 2 && cfid->dirents.ctx == NULL) {
+ cfid->dirents.ctx = ctx;
+ cfid->dirents.pos = 2;
+ }
+ /*
+ * If we already have the entire directory cached then
+ * we cna just serve the cache.
+ */
+ 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 +1093,29 @@ 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.
+ */
+ if (cfid) {
+ close_shroot(cfid);
+ cfid = NULL;
+ }
rc = find_cifs_entry(xid, tcon, ctx->pos, file, full_path,
¤t_entry, &num_to_fill);
+ if (!is_smb1_server(tcon->ses->server) && !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 +1145,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 +1153,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 +1173,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);
}
}
--
2.13.6
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] cifs: cache the directory content for shroot
2020-10-02 15:29 ` Aurélien Aptel
@ 2020-10-04 23:19 ` ronnie sahlberg
[not found] ` <CAH2r5mvijc=-JdmPMUxAUqmJKy0-x3o72NsHx+QcByBnggGXMA@mail.gmail.com>
0 siblings, 1 reply; 14+ messages in thread
From: ronnie sahlberg @ 2020-10-04 23:19 UTC (permalink / raw)
To: Aurélien Aptel; +Cc: Ronnie Sahlberg, linux-cifs, Steve French
Thanks Aurelien,
On Sat, Oct 3, 2020 at 1:30 AM Aurélien Aptel <aaptel@suse.com> 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@redhat.com> 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)
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] cifs: cache the directory content for shroot
2020-10-01 20:50 ` [PATCH 3/3] cifs: cache the " Ronnie Sahlberg
2020-10-01 22:24 ` Steve French
@ 2020-10-02 15:29 ` Aurélien Aptel
2020-10-04 23:19 ` ronnie sahlberg
2020-10-20 10:28 ` Shyam Prasad N
2 siblings, 1 reply; 14+ messages in thread
From: Aurélien Aptel @ 2020-10-02 15:29 UTC (permalink / raw)
To: Ronnie Sahlberg, linux-cifs; +Cc: Steve French
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@redhat.com> 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)
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] cifs: cache the directory content for shroot
2020-10-01 20:50 ` [PATCH 3/3] cifs: cache the " Ronnie Sahlberg
@ 2020-10-01 22:24 ` Steve French
2020-10-02 15:29 ` Aurélien Aptel
2020-10-20 10:28 ` Shyam Prasad N
2 siblings, 0 replies; 14+ messages in thread
From: Steve French @ 2020-10-01 22:24 UTC (permalink / raw)
To: Ronnie Sahlberg; +Cc: linux-cifs
minor nit pointed out by checkpatch script
0003-cifs-cache-the-directory-content-for-shroot.patch
------------------------------------------------------
WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
#33: FILE: fs/cifs/cifsglob.h:1082:
+ unsigned type;
WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
#135: FILE: fs/cifs/readdir.c:870:
+ u64 ino, unsigned type)
WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
#169: FILE: fs/cifs/readdir.c:904:
+ u64 ino, unsigned type,
total: 0 errors, 3 warnings, 305 lines checked
On Thu, Oct 1, 2020 at 3:50 PM Ronnie Sahlberg <lsahlber@redhat.com> wrote:
>
> Add caching of the directory content for the shroot.
> Populate the cache during the initial scan in readdir()
> and then try to serve out of the cache for subsequent scans.
>
> Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> ---
> 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.
> + */
> + 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;
> };
>
> /*
> diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
> index 1c14cf01dbef..a106bd3cc20b 100644
> --- a/fs/cifs/misc.c
> +++ b/fs/cifs/misc.c
> @@ -124,6 +124,8 @@ tconInfoAlloc(void)
> kfree(ret_buf);
> return NULL;
> }
> + 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;
> + }
> +}
> +
> +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 &&
> + !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.
> + */
> + 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.
> + */
> + 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);
> }
> }
>
> --
> 2.13.6
>
--
Thanks,
Steve
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 3/3] cifs: cache the directory content for shroot
2020-10-01 20:50 [PATCH 0/3] cifs: cache " Ronnie Sahlberg
@ 2020-10-01 20:50 ` Ronnie Sahlberg
2020-10-01 22:24 ` Steve French
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Ronnie Sahlberg @ 2020-10-01 20:50 UTC (permalink / raw)
To: linux-cifs; +Cc: Steve French
Add caching of the directory content for the shroot.
Populate the cache during the initial scan in readdir()
and then try to serve out of the cache for subsequent scans.
Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
---
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.
+ */
+ 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;
};
/*
diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
index 1c14cf01dbef..a106bd3cc20b 100644
--- a/fs/cifs/misc.c
+++ b/fs/cifs/misc.c
@@ -124,6 +124,8 @@ tconInfoAlloc(void)
kfree(ret_buf);
return NULL;
}
+ 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;
+ }
+}
+
+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 &&
+ !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.
+ */
+ 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.
+ */
+ 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);
}
}
--
2.13.6
^ permalink raw reply related [flat|nested] 14+ messages in thread
end of thread, other threads:[~2020-10-20 10:29 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-05 2:37 [PATCH 0/3 V1] cifs: cache the directory content for shroot Ronnie Sahlberg
2020-10-05 2:37 ` [PATCH 1/3] cifs: return cached_fid from open_shroot Ronnie Sahlberg
2020-10-05 2:37 ` [PATCH 2/3] cifs: compute full_path already in cifs_readdir() Ronnie Sahlberg
2020-10-05 2:37 ` [PATCH 3/3] cifs: cache the directory content for shroot Ronnie Sahlberg
-- strict thread matches above, loose matches on Subject: below --
2020-10-05 21:16 Ronnie Sahlberg
2020-10-07 12:21 ` Aurélien Aptel
2020-10-05 6:47 kernel test robot
2020-10-04 23:37 [PATCH 0/3 V2]: cifs: cache " Ronnie Sahlberg
2020-10-04 23:37 ` [PATCH 3/3] cifs: cache the " Ronnie Sahlberg
2020-10-01 20:50 [PATCH 0/3] cifs: cache " Ronnie Sahlberg
2020-10-01 20:50 ` [PATCH 3/3] cifs: cache the " Ronnie Sahlberg
2020-10-01 22:24 ` Steve French
2020-10-02 15:29 ` Aurélien Aptel
2020-10-04 23:19 ` ronnie sahlberg
[not found] ` <CAH2r5mvijc=-JdmPMUxAUqmJKy0-x3o72NsHx+QcByBnggGXMA@mail.gmail.com>
2020-10-05 0:20 ` ronnie sahlberg
2020-10-20 10:28 ` Shyam Prasad N
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.