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

[Enzo Matsumiya <ematsumiya@suse.de>]

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