* 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).