* Improved fix for reading stale data when cache=none @ 2022-09-19 5:39 Ronnie Sahlberg 2022-09-19 5:39 ` [PATCH] cifs: destage dirty pages before re-reading them for cache=none Ronnie Sahlberg 0 siblings, 1 reply; 5+ messages in thread From: Ronnie Sahlberg @ 2022-09-19 5:39 UTC (permalink / raw) To: linux-cifs; +Cc: Steve French 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 ) ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] cifs: destage dirty pages before re-reading them for cache=none 2022-09-19 5:39 Improved fix for reading stale data when cache=none Ronnie Sahlberg @ 2022-09-19 5:39 ` Ronnie Sahlberg 2022-09-19 14:46 ` Paulo Alcantara 2022-09-19 14:54 ` Enzo Matsumiya 0 siblings, 2 replies; 5+ messages in thread From: Ronnie Sahlberg @ 2022-09-19 5:39 UTC (permalink / raw) To: linux-cifs; +Cc: Steve French, 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] cifs: destage dirty pages before re-reading them for cache=none 2022-09-19 5:39 ` [PATCH] cifs: destage dirty pages before re-reading them for cache=none Ronnie Sahlberg @ 2022-09-19 14:46 ` Paulo Alcantara 2022-09-19 14:54 ` Enzo Matsumiya 1 sibling, 0 replies; 5+ messages in thread From: Paulo Alcantara @ 2022-09-19 14:46 UTC (permalink / raw) To: Ronnie Sahlberg, linux-cifs; +Cc: Steve French, Ronnie Sahlberg 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> ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] cifs: destage dirty pages before re-reading them for cache=none 2022-09-19 5:39 ` [PATCH] cifs: destage dirty pages before re-reading them for cache=none Ronnie Sahlberg 2022-09-19 14:46 ` Paulo Alcantara @ 2022-09-19 14:54 ` Enzo Matsumiya 2022-09-19 21:21 ` ronnie sahlberg 1 sibling, 1 reply; 5+ messages in thread From: Enzo Matsumiya @ 2022-09-19 14:54 UTC (permalink / raw) To: Ronnie Sahlberg; +Cc: linux-cifs, Steve French 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] cifs: destage dirty pages before re-reading them for cache=none 2022-09-19 14:54 ` Enzo Matsumiya @ 2022-09-19 21:21 ` ronnie sahlberg 0 siblings, 0 replies; 5+ messages in thread From: ronnie sahlberg @ 2022-09-19 21:21 UTC (permalink / raw) To: Enzo Matsumiya; +Cc: Ronnie Sahlberg, linux-cifs, Steve French 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-09-19 21:21 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-09-19 5:39 Improved fix for reading stale data when cache=none Ronnie Sahlberg 2022-09-19 5:39 ` [PATCH] cifs: destage dirty pages before re-reading them for cache=none Ronnie Sahlberg 2022-09-19 14:46 ` Paulo Alcantara 2022-09-19 14:54 ` Enzo Matsumiya 2022-09-19 21:21 ` 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).