Re: [PATCH] Fix bad data from non-direct-io read after direct-io write

[Karl Schendel <kschendel@datallegro.com>]

Linus Torvalds wrote:
> 
> On Fri, 26 Oct 2007, Zach Brown wrote:
>> I can throw together a patch if you haven't already committed one by the
>> time you read this ;).
> 
> I'm not touching that code except to send out possible patches for others 
> to test and comment on. I have no test-cases, nor any real interest in it. 
> So yeah, please send me a tested and thought-through patch.
> 
> It sounded like Karl actually had a test-case to trigger this...

Yes, I do - I'd been tripping over it once every couple weeks,
and I finally figured out how to hold my mouth right so that it
fails (almost) every time.

The below 3rd try takes on your suggestion of just invalidating
no matter what the retval from the direct_IO call.  I ran it
thru the test-case several times and it has worked every time.
The post-invalidate is probably still too early for async-directio,
but I don't have a testcase for that;  just sync.  And, this
won't be any worse in the async case.

Karl

---

--- linux-2.6.23.1-base/mm/filemap.c	2007-10-12 12:43:44.000000000 -0400
+++ linux-2.6.23.1/mm/filemap.c	2007-10-26 19:21:20.000000000 -0400
@@ -2194,21 +2194,17 @@ generic_file_direct_IO(int rw, struct ki
 	}

 	retval = mapping->a_ops->direct_IO(rw, iocb, iov, offset, nr_segs);
-	if (retval)
-		goto out;

 	/*
 	 * Finally, try again to invalidate clean pages which might have been
-	 * faulted in by get_user_pages() if the source of the write was an
-	 * mmap()ed region of the file we're writing.  That's a pretty crazy
-	 * thing to do, so we don't support it 100%.  If this invalidation
-	 * fails and we have -EIOCBQUEUED we ignore the failure.
+	 * cached by non-direct readahead, or faulted in by get_user_pages()
+	 * if the source of the write was an mmap'ed region of the file
+	 * we're writing.  Either one is a pretty crazy thing to do,
+	 * so we don't support it 100%.  If this invalidation
+	 * fails, tough, the write still worked...
 	 */
 	if (rw == WRITE && mapping->nrpages) {
-		int err = invalidate_inode_pages2_range(mapping,
-					      offset >> PAGE_CACHE_SHIFT, end);
-		if (err && retval >= 0)
-			retval = err;
+		invalidate_inode_pages2_range(mapping, offset >> PAGE_CACHE_SHIFT, end);
 	}
 out:
 	return retval;