Re: [PATCH] cifs: fix skipping to incorrect offset in emit_cached_dirents

[Steve French <smfrench@gmail.com>]

I ran the buildbot on this and it looks like this version of the patch
fixed the tests which failed with the earlier version of the patch

On Tue, Oct 11, 2022 at 11:38 AM Tom Talpey <tom@talpey.com> wrote:
>
> On 10/11/2022 2:46 AM, 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@redhat.com>
> > ---
> >   fs/cifs/readdir.c | 15 ++++++++++-----
> >   1 file changed, 10 insertions(+), 5 deletions(-)
> >
> > diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c
> > index 8e060c00c969..7170614434a1 100644
> > --- a/fs/cifs/readdir.c
> > +++ b/fs/cifs/readdir.c
> > @@ -847,14 +847,19 @@ static bool emit_cached_dirents(struct cached_dirents *cde,
> >       int rc;
> >
> >       list_for_each_entry(dirent, &cde->entries, entry) {
> > -             if (ctx->pos >= dirent->pos)
> > +             if (ctx->pos > dirent->pos)
> >                       continue;
> > +             /*
> > +              * There may be holes in the ->pos sequence
> > +              * so always force ctx->pos to the current position.
> > +              */
>
> The comment is a bit vague by referring to "->pos", because
> it's the same name in both ctx and dirent.
>
> But I have a second question, does squeezing out the holes
> affect a later possible lseek on the dirhandle? I'm having
> trouble tracking down where that might happen.
>
> >               ctx->pos = dirent->pos;
> >               rc = dir_emit(ctx, dirent->name, dirent->namelen,
> >                             dirent->fattr.cf_uniqueid,
> >                             dirent->fattr.cf_dtype);
> >               if (!rc)
> >                       return rc;
>
> It's weird that "rc" is an integer, but dir_emit() returns a bool.
> It's also confusing that this isn't coded as
>
>                 if (!rc)
>                         return false;
>
> Other than those questions, it looks like a clever fix.
>
> Tom.
> > +             ctx->pos++;
> >       }
> >       return true;
> >   }
> > @@ -1202,10 +1207,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);
> >       }
> >       kfree(tmp_buf);
> >



-- 
Thanks,

Steve