On Tue, 20 Sept 2022 at 00:58, Enzo Matsumiya <ematsumiya@xxxxxxx> wrote:
>
> On 09/19, Ronnie Sahlberg wrote:
> >This is the opposite case of kernel bugzilla 216301.
> >If we mmap a file using cache=none and then proceed to update the mmapped
> >area these updates are not reflected in a later pread() of that part of the
> >file.
> >To fix this we must first destage any dirty pages in the range before
> >we allow the pread() to proceed.
> >
> >Signed-off-by: Ronnie Sahlberg <lsahlber@xxxxxxxxxx>
>
> Reviewed-by: Enzo Matsumiya <ematsumiya@xxxxxxx>
>
> I could verify by using the reproducer from the write case.
>
> >---
> > fs/cifs/file.c | 10 ++++++++++
> > 1 file changed, 10 insertions(+)
> >
> >diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> >index 6f38b134a346..b38cebefb0db 100644
> >--- a/fs/cifs/file.c
> >+++ b/fs/cifs/file.c
> >@@ -4271,6 +4271,16 @@ static ssize_t __cifs_readv(
> > len = ctx->len;
> > }
> >
> >+ if (direct && file->f_inode->i_mapping &&
> >+ file->f_inode->i_mapping->nrpages != 0) {
>
> Just a minor nitpick, and actually a real question of mine now: can
> i_mapping ever be NULL in this case (read)? Furthermore, if so, can
> i_mapping->nrpages ever be 0? I looked around briefly, and those
> seem to be validated before hitting cifs code.
>
> I'm wondering because if those can ever be NULL/0, wouldn't it be a case
> for a BUG/WARN()? And, if so, there are a couple of other places we
> should check it as well.
>
> Please someone correct me if I missed something.
I think you are right and will remove these conditionals as they are a no-op.
The original intention was not to have them there for correctness
but as a very cheap way to detect and avoid even calling into
fiemap_write_and_wait
if we already know there is nothing to do.
>
> >+ rc = filemap_write_and_wait_range(file->f_inode->i_mapping,
> >+ offset, offset + len - 1);
> >+ if (rc) {
> >+ kref_put(&ctx->refcount, cifs_aio_ctx_release);
> >+ return rc;
> >+ }
> >+ }
> >+
> > /* grab a lock here due to read response handlers can access ctx */
> > mutex_lock(&ctx->aio_mutex);
> >
> >--
> >2.35.3
>
> Cheers,
>
> Enzo
