* Updated patch for the corruption with cache=none and mmap
@ 2022-09-19 21:37 Ronnie Sahlberg
2022-09-19 21:37 ` [PATCH] cifs: destage dirty pages before re-reading them for cache=none Ronnie Sahlberg
2022-09-20 23:46 ` Updated patch for the corruption with cache=none and mmap ronnie sahlberg
0 siblings, 2 replies; 6+ messages in thread
From: Ronnie Sahlberg @ 2022-09-19 21:37 UTC (permalink / raw)
To: linux-cifs; +Cc: Steve French
Steve, Enzo,
Updated patch that removes the redundant conditionals that Enzo pointed out
on the list.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] cifs: destage dirty pages before re-reading them for cache=none
2022-09-19 21:37 Updated patch for the corruption with cache=none and mmap Ronnie Sahlberg
@ 2022-09-19 21:37 ` Ronnie Sahlberg
2022-09-20 1:43 ` Tom Talpey
2022-09-20 23:46 ` Updated patch for the corruption with cache=none and mmap ronnie sahlberg
1 sibling, 1 reply; 6+ messages in thread
From: Ronnie Sahlberg @ 2022-09-19 21:37 UTC (permalink / raw)
To: linux-cifs; +Cc: Steve French, Ronnie Sahlberg, Paulo Alcantara, Enzo Matsumiya
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] cifs: destage dirty pages before re-reading them for cache=none
2022-09-19 21:37 ` [PATCH] cifs: destage dirty pages before re-reading them for cache=none Ronnie Sahlberg
@ 2022-09-20 1:43 ` Tom Talpey
2022-09-20 4:08 ` Leif Sahlberg
0 siblings, 1 reply; 6+ messages in thread
From: Tom Talpey @ 2022-09-20 1:43 UTC (permalink / raw)
To: Ronnie Sahlberg, linux-cifs; +Cc: Steve French, Paulo Alcantara, Enzo Matsumiya
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);
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] cifs: destage dirty pages before re-reading them for cache=none
2022-09-20 1:43 ` Tom Talpey
@ 2022-09-20 4:08 ` Leif Sahlberg
2022-09-20 4:10 ` Steve French
0 siblings, 1 reply; 6+ messages in thread
From: Leif Sahlberg @ 2022-09-20 4:08 UTC (permalink / raw)
To: Tom Talpey; +Cc: linux-cifs, Steve French, Paulo Alcantara, Enzo Matsumiya
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);
> >
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] cifs: destage dirty pages before re-reading them for cache=none
2022-09-20 4:08 ` Leif Sahlberg
@ 2022-09-20 4:10 ` Steve French
0 siblings, 0 replies; 6+ messages in thread
From: Steve French @ 2022-09-20 4:10 UTC (permalink / raw)
To: Leif Sahlberg; +Cc: Tom Talpey, linux-cifs, Paulo Alcantara, Enzo Matsumiya
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Updated patch for the corruption with cache=none and mmap
2022-09-19 21:37 Updated patch for the corruption with cache=none and mmap Ronnie Sahlberg
2022-09-19 21:37 ` [PATCH] cifs: destage dirty pages before re-reading them for cache=none Ronnie Sahlberg
@ 2022-09-20 23:46 ` ronnie sahlberg
1 sibling, 0 replies; 6+ messages in thread
From: ronnie sahlberg @ 2022-09-20 23:46 UTC (permalink / raw)
To: Ronnie Sahlberg; +Cc: linux-cifs, Steve French
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.
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-09-20 23:46 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-19 21:37 Updated patch for the corruption with cache=none and mmap Ronnie Sahlberg
2022-09-19 21:37 ` [PATCH] cifs: destage dirty pages before re-reading them for cache=none Ronnie Sahlberg
2022-09-20 1:43 ` Tom Talpey
2022-09-20 4:08 ` Leif Sahlberg
2022-09-20 4:10 ` Steve French
2022-09-20 23:46 ` Updated patch for the corruption with cache=none and mmap ronnie sahlberg
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).