Updated patch for the corruption with cache=none and mmap

Updated patch for the corruption with cache=none and mmap

[Ronnie Sahlberg]

Steve, Enzo,

Updated patch that removes the redundant conditionals that Enzo pointed out
on the list.

[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.

Reviewed-by: Paulo Alcantara (SUSE) <pc@cjr.nz>
Reviewed-by: Enzo Matsumiya <ematsumiya@suse.de>
Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
---
 fs/cifs/file.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 6f38b134a346..1f3eb97ef4ab 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -4271,6 +4271,15 @@ static ssize_t __cifs_readv(
 		len = ctx->len;
 	}
 
+	if (direct) {
+		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

[Tom Talpey]

On 9/19/2022 5:37 PM, 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.
> 
> Reviewed-by: Paulo Alcantara (SUSE) <pc@cjr.nz>
> Reviewed-by: Enzo Matsumiya <ematsumiya@suse.de>
> Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> ---
>   fs/cifs/file.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index 6f38b134a346..1f3eb97ef4ab 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -4271,6 +4271,15 @@ static ssize_t __cifs_readv(
>   		len = ctx->len;
>   	}
>   
> +	if (direct) {
> +		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;

Are the possible return values from filemap_write_and_wait_range() also
valid for returning from __cifs_readv()? It seems a bit odd to return
write errors here.

If not, then perhaps a more generic failure rc would be safer/more POSIX
compliant?

Tom.

> +		}
> +	}
> +
>   	/* grab a lock here due to read response handlers can access ctx */
>   	mutex_lock(&ctx->aio_mutex);
>   

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

[Leif Sahlberg]

On Tue, Sep 20, 2022 at 11:43 AM Tom Talpey <tom@talpey.com> wrote:
>
> On 9/19/2022 5:37 PM, 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.
> >
> > Reviewed-by: Paulo Alcantara (SUSE) <pc@cjr.nz>
> > Reviewed-by: Enzo Matsumiya <ematsumiya@suse.de>
> > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > ---
> >   fs/cifs/file.c | 9 +++++++++
> >   1 file changed, 9 insertions(+)
> >
> > diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> > index 6f38b134a346..1f3eb97ef4ab 100644
> > --- a/fs/cifs/file.c
> > +++ b/fs/cifs/file.c
> > @@ -4271,6 +4271,15 @@ static ssize_t __cifs_readv(
> >               len = ctx->len;
> >       }
> >
> > +     if (direct) {
> > +             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;
>
> Are the possible return values from filemap_write_and_wait_range() also
> valid for returning from __cifs_readv()? It seems a bit odd to return
> write errors here.
>
> If not, then perhaps a more generic failure rc would be safer/more POSIX
> compliant?

Good point. Since an error here means we have dirty pages and
destaging them failed I guess -EAGAIN
would be the best to return. We want/need userspace to retry this,
possibly forever, in that scenario.

Steve, can you change the patch to -EAGAIN or do you want me to re-send it?

regards
ronnie s


>
> Tom.
>
> > +             }
> > +     }
> > +
> >       /* grab a lock here due to read response handlers can access ctx */
> >       mutex_lock(&ctx->aio_mutex);
> >
>

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

[Steve French]

could you resend - also add cc:stable if you think appropriate.

On Mon, Sep 19, 2022 at 11:08 PM Leif Sahlberg <lsahlber@redhat.com> wrote:
>
> On Tue, Sep 20, 2022 at 11:43 AM Tom Talpey <tom@talpey.com> wrote:
> >
> > On 9/19/2022 5:37 PM, 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.
> > >
> > > Reviewed-by: Paulo Alcantara (SUSE) <pc@cjr.nz>
> > > Reviewed-by: Enzo Matsumiya <ematsumiya@suse.de>
> > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > > ---
> > >   fs/cifs/file.c | 9 +++++++++
> > >   1 file changed, 9 insertions(+)
> > >
> > > diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> > > index 6f38b134a346..1f3eb97ef4ab 100644
> > > --- a/fs/cifs/file.c
> > > +++ b/fs/cifs/file.c
> > > @@ -4271,6 +4271,15 @@ static ssize_t __cifs_readv(
> > >               len = ctx->len;
> > >       }
> > >
> > > +     if (direct) {
> > > +             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;
> >
> > Are the possible return values from filemap_write_and_wait_range() also
> > valid for returning from __cifs_readv()? It seems a bit odd to return
> > write errors here.
> >
> > If not, then perhaps a more generic failure rc would be safer/more POSIX
> > compliant?
>
> Good point. Since an error here means we have dirty pages and
> destaging them failed I guess -EAGAIN
> would be the best to return. We want/need userspace to retry this,
> possibly forever, in that scenario.
>
> Steve, can you change the patch to -EAGAIN or do you want me to re-send it?
>
> regards
> ronnie s
>
>
> >
> > Tom.
> >
> > > +             }
> > > +     }
> > > +
> > >       /* grab a lock here due to read response handlers can access ctx */
> > >       mutex_lock(&ctx->aio_mutex);
> > >
> >
>


-- 
Thanks,

Steve

Re: Updated patch for the corruption with cache=none and mmap

[ronnie sahlberg]

I have also added a test for this to cifs/109 in the buildbot.  (After
fixing a previous issue in the test, the tests pass now with the
current patches applied.)

On Tue, 20 Sept 2022 at 07:40, Ronnie Sahlberg <lsahlber@redhat.com> wrote:
>
> Steve, Enzo,
>
> Updated patch that removes the redundant conditionals that Enzo pointed out
> on the list.
>
>

[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.

Cc: stable@vger.kernel.org
Reviewed-by: Paulo Alcantara (SUSE) <pc@cjr.nz>
Reviewed-by: Enzo Matsumiya <ematsumiya@suse.de>
Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
---
 fs/cifs/file.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 6f38b134a346..7d756721e1a6 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -4271,6 +4271,15 @@ static ssize_t __cifs_readv(
 		len = ctx->len;
 	}
 
+	if (direct) {
+		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 -EAGAIN;
+		}
+	}
+
 	/* 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

[Steve French]

tentatively merged into cifs-2.6.git for-next pending testing

Will be curious if this impacts performance of any of the buildbot
tests and also curious if it addresses any of the buildbot tests which
fail and are currently skipped

On Mon, Sep 19, 2022 at 11:32 PM Ronnie Sahlberg <lsahlber@redhat.com> 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.
>
> Cc: stable@vger.kernel.org
> Reviewed-by: Paulo Alcantara (SUSE) <pc@cjr.nz>
> Reviewed-by: Enzo Matsumiya <ematsumiya@suse.de>
> Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> ---
>  fs/cifs/file.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index 6f38b134a346..7d756721e1a6 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -4271,6 +4271,15 @@ static ssize_t __cifs_readv(
>                 len = ctx->len;
>         }
>
> +       if (direct) {
> +               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 -EAGAIN;
> +               }
> +       }
> +
>         /* grab a lock here due to read response handlers can access ctx */
>         mutex_lock(&ctx->aio_mutex);
>
> --
> 2.35.3
>


-- 
Thanks,

Steve

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

[Tom Talpey]

On 9/20/2022 12:32 AM, 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.
> 
> Cc: stable@vger.kernel.org
> Reviewed-by: Paulo Alcantara (SUSE) <pc@cjr.nz>
> Reviewed-by: Enzo Matsumiya <ematsumiya@suse.de>
> Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> ---
>   fs/cifs/file.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index 6f38b134a346..7d756721e1a6 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -4271,6 +4271,15 @@ static ssize_t __cifs_readv(
>   		len = ctx->len;
>   	}
>   
> +	if (direct) {
> +		rc = filemap_write_and_wait_range(file->f_inode->i_mapping,
> +						  offset, offset + len - 1);

It's only a nit, but with the new EAGAIN return code below, it's no
longer necessary to capture "rc" here.

> +		if (rc) {
> +			kref_put(&ctx->refcount, cifs_aio_ctx_release);
> +			return -EAGAIN;
> +		}
> +	}
> +
>   	/* grab a lock here due to read response handlers can access ctx */
>   	mutex_lock(&ctx->aio_mutex);
>   

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