Improved fix for reading stale data when cache=none

Improved fix for reading stale data when cache=none

[Ronnie Sahlberg]

Steve, List

Please find a better patch to fix the data corruption issue I posted earlier
for when doing direct reads to an mmaped file after the mmaped area has changed.

The main difference is that I now destage and invalidate only the range that
is affected by the pread() and not the whole file so impact on performance should be reduced.
(that said, with cache=none there is probably no expectation of high performance in the first place )

[PATCH] cifs: destage dirty pages before re-reading them for cache=none

[Ronnie Sahlberg]

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@redhat.com>
---
 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) {
+		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

Re: [PATCH] cifs: destage dirty pages before re-reading them for cache=none

[Paulo Alcantara]

Ronnie Sahlberg <lsahlber@redhat.com> writes:

> 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@redhat.com>
> ---
>  fs/cifs/file.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)

Reviewed-by: Paulo Alcantara (SUSE) <pc@cjr.nz>

Re: [PATCH] cifs: destage dirty pages before re-reading them for cache=none

[Enzo Matsumiya]

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@redhat.com>

Reviewed-by: Enzo Matsumiya <ematsumiya@suse.de>

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.

>+		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

Re: [PATCH] cifs: destage dirty pages before re-reading them for cache=none

[ronnie sahlberg]

On Tue, 20 Sept 2022 at 00:58, Enzo Matsumiya <ematsumiya@suse.de> 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@redhat.com>
>
> Reviewed-by: Enzo Matsumiya <ematsumiya@suse.de>
>
> 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