On 10/11/2022 6:07 PM, Ronnie Sahlberg wrote:
When application has done lseek() to a different offset on a directory fd
we skipped one entry too many before we start emitting directory entries
from the cache.
We need to also make sure that when we are starting to emit directory
entries from the cache, the ->pos sequence might have holes and skip
some indices.
Signed-off-by: Ronnie Sahlberg <lsahlber@xxxxxxxxxx>
---
fs/cifs/readdir.c | 29 +++++++++++++++++++++++------
1 file changed, 23 insertions(+), 6 deletions(-)
diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c
index 8e060c00c969..1bb4624e768b 100644
--- a/fs/cifs/readdir.c
+++ b/fs/cifs/readdir.c
@@ -844,17 +844,34 @@ static bool emit_cached_dirents(struct cached_dirents *cde,
struct dir_context *ctx)
{
struct cached_dirent *dirent;
- int rc;
+ bool rc;
list_for_each_entry(dirent, &cde->entries, entry) {
- if (ctx->pos >= dirent->pos)
+ /*
+ * Skip all early entries prior to the current lseek()
+ * position.
+ */
+ if (ctx->pos > dirent->pos)
continue;
+ /*
+ * We recorded the current ->pos value for the dirent
+ * when we stored it in the cache.
+ * However, this sequence of ->pos values may have holes
+ * in it, for example dot-dirs returned from the server
+ * are suppressed.
+ * Handle this bu forcing ctx->pos to be the same as the
+ * ->pos of the current dirent we emit from the cache.
+ * This means that when we emit these entries from the cache
+ * we now emit them with the same ->pos value as in the
+ * initial scan.
+ */
It's a little wordy, but it's better, so ok.
ctx->pos = dirent->pos;
rc = dir_emit(ctx, dirent->name, dirent->namelen,
dirent->fattr.cf_uniqueid,
dirent->fattr.cf_dtype);
if (!rc)
return rc;
Well, I still think it would be clearer as "return false". And the
name "rc" really seems odd now for a bool. But again, ok I guess.
Either way, I learned a thing or two chasing down dir_emit() and the
overall approach seems good.
+ ctx->pos++;
}
return true;
}
@@ -1202,10 +1219,10 @@ int cifs_readdir(struct file *file, struct dir_context *ctx)
ctx->pos, tmp_buf);
cifs_save_resume_key(current_entry, cifsFile);
break;
- } else
- current_entry =
- nxt_dir_entry(current_entry, end_of_smb,
- cifsFile->srch_inf.info_level);
+ }
+ current_entry =
+ nxt_dir_entry(current_entry, end_of_smb,
+ cifsFile->srch_inf.info_level);
This change isn't actually changing anything. Would it be
better to leave out on principle?
}
kfree(tmp_buf);
Overall,
Reviewed-by: Tom Talpey <tom@xxxxxxxxxx>
