| Message ID | 20221006043609.1193398-2-lsahlber@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | cifs: fix skipping to incorrect offset in emit_cached_dirents | expand |
here is the updated version of patch 4 which I was planning to use (minor rebase, and trivial checkpatch cleanup). Let me know if ok On Wed, Oct 5, 2022 at 11:36 PM Ronnie Sahlberg <lsahlber@redhat.com> 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. > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> > --- > fs/cifs/readdir.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c > index 8e060c00c969..da0d1e188432 100644 > --- a/fs/cifs/readdir.c > +++ b/fs/cifs/readdir.c > @@ -847,9 +847,13 @@ static bool emit_cached_dirents(struct cached_dirents *cde, > int rc; > > list_for_each_entry(dirent, &cde->entries, entry) { > - if (ctx->pos >= dirent->pos) > + /* > + * Skip ahead until we get to the current position of the > + * directory. > + */ > + if (ctx->pos > dirent->pos) > continue; > - ctx->pos = dirent->pos; > + > rc = dir_emit(ctx, dirent->name, dirent->namelen, > dirent->fattr.cf_uniqueid, > dirent->fattr.cf_dtype); > -- > 2.35.3 >
tentatively added to cifs-2.6.git for-next pending testing On Wed, Oct 5, 2022 at 11:36 PM Ronnie Sahlberg <lsahlber@redhat.com> 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. > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> > --- > fs/cifs/readdir.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c > index 8e060c00c969..da0d1e188432 100644 > --- a/fs/cifs/readdir.c > +++ b/fs/cifs/readdir.c > @@ -847,9 +847,13 @@ static bool emit_cached_dirents(struct cached_dirents *cde, > int rc; > > list_for_each_entry(dirent, &cde->entries, entry) { > - if (ctx->pos >= dirent->pos) > + /* > + * Skip ahead until we get to the current position of the > + * directory. > + */ > + if (ctx->pos > dirent->pos) > continue; > - ctx->pos = dirent->pos; > + > rc = dir_emit(ctx, dirent->name, dirent->namelen, > dirent->fattr.cf_uniqueid, > dirent->fattr.cf_dtype); > -- > 2.35.3 >
Adding this one patch onto for-next, it went from passing all (except 452 which is known issue with refcount and umount/mount) of buildbot cifs-testing group to hanging on generic/010 and generic/024 to Windows (interestingly 024 did not hang when run to Samba) On Wed, Oct 5, 2022 at 11:36 PM Ronnie Sahlberg <lsahlber@redhat.com> 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. > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> > --- > fs/cifs/readdir.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c > index 8e060c00c969..da0d1e188432 100644 > --- a/fs/cifs/readdir.c > +++ b/fs/cifs/readdir.c > @@ -847,9 +847,13 @@ static bool emit_cached_dirents(struct cached_dirents *cde, > int rc; > > list_for_each_entry(dirent, &cde->entries, entry) { > - if (ctx->pos >= dirent->pos) > + /* > + * Skip ahead until we get to the current position of the > + * directory. > + */ > + if (ctx->pos > dirent->pos) > continue; > - ctx->pos = dirent->pos; > + > rc = dir_emit(ctx, dirent->name, dirent->namelen, > dirent->fattr.cf_uniqueid, > dirent->fattr.cf_dtype); > -- > 2.35.3 >
Hi, Make sure you're not re-introducing the bug where the first few files are missing when mounting the root of a Windows drive. See fix https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/fs/cifs/smb2ops.c?id=0595751f267994c3c7027377058e4185b3a28e75 And bug https://bugzilla.samba.org/show_bug.cgi?id=13107 Cheers,
On Mon, 10 Oct 2022 at 04:04, Aurélien Aptel <aurelien.aptel@gmail.com> wrote: > > Hi, > > Make sure you're not re-introducing the bug where the first few files > are missing when mounting the root of a Windows drive. > > See fix https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/fs/cifs/smb2ops.c?id=0595751f267994c3c7027377058e4185b3a28e75 > > And bug https://bugzilla.samba.org/show_bug.cgi?id=13107 Yeah. What confuses things is that we do not create a contignous sequence of index positions when we emit a directory. For the dir_context, ctx->pos starts at 0 for the first entry and then increments by one for each other entry. We do not currently create a contignous space but one with one or two holes in it. We start by explicitely emitting '.' and '..' and these are at position 0 and 2 respectively. But then, for a server that for example DOES return entries for '.' and '..' we skip emitting these entries but still increment pos, thus we end up with a sequence for index positions of the entries of 0,1,4,5,6,7,... I.e. there is a hole in the sequence where 2 and 3 are missing becasue these were the dot directories that the server responded and that we did not emit, but we incremented pos. This should ideally be fixed because otherwise, what would lseek(3), mean ? > > Cheers,
diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c index 8e060c00c969..da0d1e188432 100644 --- a/fs/cifs/readdir.c +++ b/fs/cifs/readdir.c @@ -847,9 +847,13 @@ static bool emit_cached_dirents(struct cached_dirents *cde, int rc; list_for_each_entry(dirent, &cde->entries, entry) { - if (ctx->pos >= dirent->pos) + /* + * Skip ahead until we get to the current position of the + * directory. + */ + if (ctx->pos > dirent->pos) continue; - ctx->pos = dirent->pos; + rc = dir_emit(ctx, dirent->name, dirent->namelen, dirent->fattr.cf_uniqueid, dirent->fattr.cf_dtype);
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. Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> --- fs/cifs/readdir.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)