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

[Leif Sahlberg <lsahlber@redhat.com>]

On Wed, Oct 12, 2022 at 10:49 AM Tom Talpey <tom@talpey.com> wrote:
>
> 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@redhat.com>
> > ---
> >   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?

I wanted this change since previously it was a little confusing. A
quick glance might raise questions "why only do this conditionally?"
and then when looking more carefully "oh, we actually do this
unconditionally before every new iteration".

It is as you say a no-op change.
It should arguably be done in a separate patch but it is so small I
just took the lazy path and put it in this patch instead of a separate
patch.

Thanks for the review.
>
>
> >       }
> >       kfree(tmp_buf);
> >
> Overall,
>
> Reviewed-by: Tom Talpey <tom@talpey.com>
>