[Devel,rh7] fs/cleancache: fix data invalidation in the cleancache during direct_io

Submitted by Andrey Ryabinin on April 11, 2017, 3:43 p.m.

Details

Message ID 20170411154333.5060-1-aryabinin@virtuozzo.com
State New
Series "fs/cleancache: fix data invalidation in the cleancache during direct_io"
Headers show

Commit Message

Andrey Ryabinin April 11, 2017, 3:43 p.m.
Currently some direct_io fs hooks call invalidate_inode_pages2_range()
conditionally iff mapping->nrpages is not zero. So if nrpages is zero,
data in cleancache wouldn't be invalidated. So the next buffered read
may get stale data from the cleancache.

Fix this by calling invalidate_inode_pages2_range() regardless of nrpages
value. And if nrpages is zero, bail out from invalidate_inode_pages2_range()
only after cleancache_invalidate_inode(), so that we invalidate cleancache
but still avoid pointless page cache lookups.

https://jira.sw.ru/browse/PSBM-63908
Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
---
 fs/9p/vfs_file.c  |  4 ++--
 fs/nfs/direct.c   | 16 ++++++----------
 fs/nfs/inode.c    |  7 ++++---
 fs/xfs/xfs_file.c | 16 ++++++++--------
 mm/filemap.c      | 28 ++++++++++++----------------
 mm/truncate.c     |  4 ++++
 6 files changed, 36 insertions(+), 39 deletions(-)

Patch hide | download patch | download mbox

diff --git a/fs/9p/vfs_file.c b/fs/9p/vfs_file.c
index 7da03f8..afe0036 100644
--- a/fs/9p/vfs_file.c
+++ b/fs/9p/vfs_file.c
@@ -482,7 +482,7 @@  v9fs_file_write_internal(struct inode *inode, struct p9_fid *fid,
 	if (invalidate && (total > 0)) {
 		pg_start = origin >> PAGE_CACHE_SHIFT;
 		pg_end = (origin + total - 1) >> PAGE_CACHE_SHIFT;
-		if (inode->i_mapping && inode->i_mapping->nrpages)
+		if (inode->i_mapping)
 			invalidate_inode_pages2_range(inode->i_mapping,
 						      pg_start, pg_end);
 		*offset += total;
@@ -688,7 +688,7 @@  v9fs_direct_write(struct file *filp, const char __user * data,
 	 * about to write.  We do this *before* the write so that if we fail
 	 * here we fall back to buffered write
 	 */
-	if (mapping->nrpages) {
+	{
 		pgoff_t pg_start = offset >> PAGE_CACHE_SHIFT;
 		pgoff_t pg_end   = (offset + count - 1) >> PAGE_CACHE_SHIFT;
 
diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c
index ab96f01..9bbbb63 100644
--- a/fs/nfs/direct.c
+++ b/fs/nfs/direct.c
@@ -1132,12 +1132,10 @@  ssize_t nfs_file_direct_write(struct kiocb *iocb, const struct iovec *iov,
 	if (result)
 		goto out_unlock;
 
-	if (mapping->nrpages) {
-		result = invalidate_inode_pages2_range(mapping,
-					pos >> PAGE_CACHE_SHIFT, end);
-		if (result)
-			goto out_unlock;
-	}
+	result = invalidate_inode_pages2_range(mapping,
+				pos >> PAGE_CACHE_SHIFT, end);
+	if (result)
+		goto out_unlock;
 
 	task_io_account_write(count);
 
@@ -1161,10 +1159,8 @@  ssize_t nfs_file_direct_write(struct kiocb *iocb, const struct iovec *iov,
 
 	result = nfs_direct_write_schedule_iovec(dreq, iov, nr_segs, pos, uio);
 
-	if (mapping->nrpages) {
-		invalidate_inode_pages2_range(mapping,
-					      pos >> PAGE_CACHE_SHIFT, end);
-	}
+	invalidate_inode_pages2_range(mapping,
+				pos >> PAGE_CACHE_SHIFT, end);
 
 	mutex_unlock(&inode->i_mutex);
 
diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 8c06aed..779b05c 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -1065,10 +1065,11 @@  static int nfs_invalidate_mapping(struct inode *inode, struct address_space *map
 			if (ret < 0)
 				return ret;
 		}
-		ret = invalidate_inode_pages2(mapping);
-		if (ret < 0)
-			return ret;
 	}
+	ret = invalidate_inode_pages2(mapping);
+	if (ret < 0)
+		return ret;
+
 	if (S_ISDIR(inode->i_mode)) {
 		spin_lock(&inode->i_lock);
 		memset(nfsi->cookieverf, 0, sizeof(nfsi->cookieverf));
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 9a2193b..d758443 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -368,15 +368,15 @@  xfs_file_aio_read(
 				return ret;
 			}
 
-			/*
-			 * Invalidate whole pages. This can return an error if
-			 * we fail to invalidate a page, but this should never
-			 * happen on XFS. Warn if it does fail.
-			 */
-			ret = invalidate_inode_pages2(VFS_I(ip)->i_mapping);
-			WARN_ON_ONCE(ret);
-			ret = 0;
 		}
+		/*
+		 * Invalidate whole pages. This can return an error if
+		 * we fail to invalidate a page, but this should never
+		 * happen on XFS. Warn if it does fail.
+		 */
+		ret = invalidate_inode_pages2(VFS_I(ip)->i_mapping);
+		WARN_ON_ONCE(ret);
+		ret = 0;
 		xfs_rw_ilock_demote(ip, XFS_IOLOCK_EXCL);
 	}
 
diff --git a/mm/filemap.c b/mm/filemap.c
index 16517c6..8e608fc 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2598,18 +2598,16 @@  generic_file_direct_write_iter(struct kiocb *iocb, struct iov_iter *iter,
 	 * about to write.  We do this *before* the write so that we can return
 	 * without clobbering -EIOCBQUEUED from ->direct_IO().
 	 */
-	if (mapping->nrpages) {
-		written = invalidate_inode_pages2_range(mapping,
-					pos >> PAGE_CACHE_SHIFT, end);
-		/*
-		 * If a page can not be invalidated, return 0 to fall back
-		 * to buffered write.
-		 */
-		if (written) {
-			if (written == -EBUSY)
-				return 0;
-			goto out;
-		}
+	written = invalidate_inode_pages2_range(mapping,
+						pos >> PAGE_CACHE_SHIFT, end);
+	/*
+	 * If a page can not be invalidated, return 0 to fall back
+	 * to buffered write.
+	 */
+	if (written) {
+		if (written == -EBUSY)
+			return 0;
+		goto out;
 	}
 
 	written = mapping_direct_IO(mapping, WRITE, iocb, iter, pos);
@@ -2622,10 +2620,8 @@  generic_file_direct_write_iter(struct kiocb *iocb, struct iov_iter *iter,
 	 * so we don't support it 100%.  If this invalidation
 	 * fails, tough, the write still worked...
 	 */
-	if (mapping->nrpages) {
-		invalidate_inode_pages2_range(mapping,
-					      pos >> PAGE_CACHE_SHIFT, end);
-	}
+	invalidate_inode_pages2_range(mapping,
+				pos >> PAGE_CACHE_SHIFT, end);
 
 	if (written > 0) {
 		pos += written;
diff --git a/mm/truncate.c b/mm/truncate.c
index f460e67..f015a86 100644
--- a/mm/truncate.c
+++ b/mm/truncate.c
@@ -628,6 +628,10 @@  int invalidate_inode_pages2_range(struct address_space *mapping,
 	int did_range_unmap = 0;
 
 	cleancache_invalidate_inode(mapping);
+
+	if (mapping->nrpages == 0)
+		return 0;
+
 	pagevec_init(&pvec, 0);
 	index = start;
 	while (index <= end && __pagevec_lookup(&pvec, mapping, index,

Comments

Alexey Kuznetsov April 11, 2017, 5:08 p.m.
Hello!

Good job!

Before submitting this to mainstream look
at truncate_inode_pages.

It has condition:

if (mapping->nrpages == 0 && mapping->nrexceptional == 0)
                return;

I have no idea what are those exceptions are, but it definitely
looks illegal to check only for nrpages in invalidate_inode_pages2_range,
it clears exceptional entries as well.

Also I see no point in invalidation of cleancache on entry
to these routines. It is waste of time, cleancache will be
repopulated by invalidation (which it stupid, of course).
It is enough to do this once at exit.
Andrey Ryabinin April 12, 2017, 2:45 p.m.
On 04/11/2017 08:08 PM, Alexey Kuznetsov wrote:
> Hello!
> 
> Good job!
> 
> Before submitting this to mainstream look
> at truncate_inode_pages.
> 
> It has condition:
> 
> if (mapping->nrpages == 0 && mapping->nrexceptional == 0)
>                 return;
> 
> I have no idea what are those exceptions are, but it definitely
> looks illegal to check only for nrpages in invalidate_inode_pages2_range,
> it clears exceptional entries as well.
> 

AFAIU exceptional entries are either dax entires, they store sector number and
entry size (PMD, PTE,...), or they used by workingset code to store some information
about page eviction. 

Given that, invalidate_inode_pages2_range() supposed to invalidate stale data in page
cache/cleancache (per my understanding at least) I would say that invalidate_inode_pages2_range()
shouldn't remove exceptional entries. But not sure that my understanding is correct,
so I'm going to add ->nrexceptional check in v2, but will ask about this ambiguousity
upstream folks.

> Also I see no point in invalidation of cleancache on entry
> to these routines. It is waste of time, cleancache will be
> repopulated by invalidation (which it stupid, of course).
> It is enough to do this once at exit.
> 

Agreed.