| Message ID | 20220919053901.465232-2-lsahlber@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cifs: destage dirty pages before re-reading them for cache=none | expand |
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>
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
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
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);
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(+)