[PATCH] dio: falling through to buffered I/O when invalidation of a page fails

[PATCH] dio: falling through to buffered I/O when invalidation of a page fails

[Hisashi Hifumi]

Hi.

Current dio has some problems:
1, In ext3 ordered, dio write can return with EIO because of the race 
between invalidation of
a page and jbd. jbd pins the bhs while committing journal so 
try_to_release_page fails when jbd
is committing the transaction.

Past discussion about this issue is as follows.
http://marc.info/?t=119343431200004&r=1&w=2
http://marc.info/?t=112656762800002&r=1&w=2

2, invalidate_inode_pages2_range() sets ret=-EIO when invalidate_complete_page2()
fails, but this ret is cleared if do_launder_page() succeed on a page of 
next index.
In this case, dio is carried out even if invalidate_complete_page2() fails 
on some pages.
This can cause inconsistency between memory and blocks on HDD because the page
cache still exists.

I solved problems above by introducing invalidate_inode_pages3_range() and 
falling
through to buffered I/O when invalidation of a page failed.
We can distinguish between failure of page invalidation and other errors
with the return value of invalidate_inode_pages3_range(). When invalidation 
of a page
fails, this function exits immediately so that inconsistency between memory and
HDD blocks can be avoided and the number of the cached page that should be
acquired again is smaller.

Thanks.

Signed-off-by :Hisashi Hifumi <hifumi.hisashi@oss.ntt.co.jp>

diff -Nrup linux-2.6.24-rc4.org/include/linux/fs.h 
linux-2.6.24-rc4/include/linux/fs.h
--- linux-2.6.24-rc4.org/include/linux/fs.h	2007-12-06 12:05:30.000000000 +0900
+++ linux-2.6.24-rc4/include/linux/fs.h	2007-12-06 12:06:38.000000000 +0900
@@ -1670,6 +1670,8 @@ static inline void invalidate_remote_ino
  extern int invalidate_inode_pages2(struct address_space *mapping);
  extern int invalidate_inode_pages2_range(struct address_space *mapping,
  					 pgoff_t start, pgoff_t end);
+extern int invalidate_inode_pages3_range(struct address_space *mapping,
+					 pgoff_t start, pgoff_t end);
  extern int write_inode_now(struct inode *, int);
  extern int filemap_fdatawrite(struct address_space *);
  extern int filemap_flush(struct address_space *);
diff -Nrup linux-2.6.24-rc4.org/mm/filemap.c linux-2.6.24-rc4/mm/filemap.c
--- linux-2.6.24-rc4.org/mm/filemap.c	2007-12-06 12:05:31.000000000 +0900
+++ linux-2.6.24-rc4/mm/filemap.c	2007-12-06 12:06:38.000000000 +0900
@@ -2491,14 +2491,18 @@ generic_file_direct_IO(int rw, struct ki
  	/*
  	 * After a write we want buffered reads to be sure to go to disk to get
  	 * the new data.  We invalidate clean cached page from the region we're
-	 * about to write.  We do this *before* the write so that we can return
-	 * -EIO without clobbering -EIOCBQUEUED from ->direct_IO().
+	 * about to write.
+	 * If invalidation of any pages fails, we return 0 so that we can fall
+	 * through to buffered I/O.
  	 */
  	if (rw == WRITE && mapping->nrpages) {
-		retval = invalidate_inode_pages2_range(mapping,
+		retval = invalidate_inode_pages3_range(mapping,
  					offset >> PAGE_CACHE_SHIFT, end);
-		if (retval)
+		if (retval) {
+			if (retval > 0)
+				retval = 0;
  			goto out;
+		}
  	}

  	retval = mapping->a_ops->direct_IO(rw, iocb, iov, offset, nr_segs);
diff -Nrup linux-2.6.24-rc4.org/mm/truncate.c linux-2.6.24-rc4/mm/truncate.c
--- linux-2.6.24-rc4.org/mm/truncate.c	2007-12-06 12:05:31.000000000 +0900
+++ linux-2.6.24-rc4/mm/truncate.c	2007-12-07 12:14:13.000000000 +0900
@@ -452,6 +452,86 @@ int invalidate_inode_pages2_range(struct
  EXPORT_SYMBOL_GPL(invalidate_inode_pages2_range);

  /**
+ * invalidate_inode_pages3_range - remove range of pages from an address_space
+ * @mapping: the address_space
+ * @start: the page offset 'from' which to invalidate
+ * @end: the page offset 'to' which to invalidate (inclusive)
+ *
+ * Any pages which are found to be mapped into pagetables are unmapped prior to
+ * invalidation.
+ *
+ * When a page can not be invalidated, this function exits immediately.
+ *
+ * Returns 1 if any pages could not be invalidated.
+ */
+int invalidate_inode_pages3_range(struct address_space *mapping,
+				  pgoff_t start, pgoff_t end)
+{
+	struct pagevec pvec;
+	pgoff_t next;
+	int i;
+	int ret = 0;
+	int did_range_unmap = 0;
+	int wrapped = 0;
+
+	pagevec_init(&pvec, 0);
+	next = start;
+	while (next <= end && !ret && !wrapped &&
+		pagevec_lookup(&pvec, mapping, next,
+			min(end - next, (pgoff_t)PAGEVEC_SIZE - 1) + 1)) {
+		for (i = 0; !ret && i < pagevec_count(&pvec); i++) {
+			struct page *page = pvec.pages[i];
+			pgoff_t page_index;
+
+			lock_page(page);
+			if (page->mapping != mapping) {
+				unlock_page(page);
+				continue;
+			}
+			page_index = page->index;
+			next = page_index + 1;
+			if (next == 0)
+				wrapped = 1;
+			if (page_index > end) {
+				unlock_page(page);
+				break;
+			}
+			wait_on_page_writeback(page);
+			if (page_mapped(page)) {
+				if (!did_range_unmap) {
+					/*
+					 * Zap the rest of the file in one hit.
+					 */
+					unmap_mapping_range(mapping,
+					   (loff_t)page_index<<PAGE_CACHE_SHIFT,
+					   (loff_t)(end - page_index + 1)
+							<< PAGE_CACHE_SHIFT,
+					    0);
+					did_range_unmap = 1;
+				} else {
+					/*
+					 * Just zap this page
+					 */
+					unmap_mapping_range(mapping,
+					  (loff_t)page_index<<PAGE_CACHE_SHIFT,
+					  PAGE_CACHE_SIZE, 0);
+				}
+			}
+			BUG_ON(page_mapped(page));
+			ret = do_launder_page(mapping, page);
+			if (ret == 0 && !invalidate_complete_page2(mapping, page))
+				ret = 1;
+			unlock_page(page);
+		}
+		if (!ret)
+			pagevec_release(&pvec);
+		cond_resched();
+	}
+	return ret;
+}
+EXPORT_SYMBOL_GPL(invalidate_inode_pages3_range);
+
+/**
   * invalidate_inode_pages2 - remove all pages from an address_space
   * @mapping: the address_space
   * 

Re: [PATCH] dio: falling through to buffered I/O when invalidation of a page fails

[Zach Brown]

Hisashi Hifumi wrote:
> Hi.
> 
> Current dio has some problems:
> 1, In ext3 ordered, dio write can return with EIO because of the race
> between invalidation of
> a page and jbd. jbd pins the bhs while committing journal so
> try_to_release_page fails when jbd
> is committing the transaction.

Yeah.  It sure would be fantastic if some ext3 expert could stop this
from happening somehow.  But that hasn't happened in.. uh.. Badari, for
how many years has this been on the radar? :)

> 
> Past discussion about this issue is as follows.
> http://marc.info/?t=119343431200004&r=1&w=2
> http://marc.info/?t=112656762800002&r=1&w=2
> 
> 2, invalidate_inode_pages2_range() sets ret=-EIO when
> invalidate_complete_page2()
> fails, but this ret is cleared if do_launder_page() succeed on a page of
> next index.

Oops.  That's too bad.  So maybe we should fix it by not stomping on
that return code?

	ret2 = do_launder()
	if (ret2 == 0)
		ret2 = invalidate()
	if (ret == 0)
		ret = ret2

I'd be surprised if we ever wanted to mask an -EIO when later pages
laundered successfully.

> In this case, dio is carried out even if invalidate_complete_page2()
> fails on some pages.
> This can cause inconsistency between memory and blocks on HDD because
> the page
> cache still exists.

Yeah.

> I solved problems above by introducing invalidate_inode_pages3_range()
> and falling
> through to buffered I/O when invalidation of a page failed.

Well, I like the idea of more intelligently dealing with the known
problem between dio and ext3.  I'm not sure that falling back to
buffered is right.

If dio can tell that it only failed because jbd held the buffer, should
we have waited for the transaction to complete before trying to
invalidate again?

If we could do that, we'd avoid performing the IO twice.

> We can distinguish between failure of page invalidation and other errors
> with the return value of invalidate_inode_pages3_range().

I'm not sure duplicating the invalidation loop into a new function is
the right thing.  Maybe we'd just tweak inode_pages2 to indicate to the
caller the specific failing circumstances somehow.  Maybe.

- z

Re: [PATCH] dio: falling through to buffered I/O when invalidationof a page fails

[Hisashi Hifumi]

 >
 >>
 >> Past discussion about this issue is as follows.
 >> http://marc.info/?t=119343431200004&r=1&w=2
 >> http://marc.info/?t=112656762800002&r=1&w=2
 >>
 >> 2, invalidate_inode_pages2_range() sets ret=-EIO when
 >> invalidate_complete_page2()
 >> fails, but this ret is cleared if do_launder_page() succeed on a page of
 >> next index.
 >
 >Oops.  That's too bad.  So maybe we should fix it by not stomping on
 >that return code?
 >
 >	ret2 = do_launder()
 >	if (ret2 == 0)
 >		ret2 = invalidate()
 >	if (ret == 0)
 >		ret = ret2
 >
 >I'd be surprised if we ever wanted to mask an -EIO when later pages
 >laundered successfully.

This can preserve ret of -EIO. But it cannot be distinguished between
the error of invalidation and other error only by this fix.


 >> I solved problems above by introducing invalidate_inode_pages3_range()
 >> and falling
 >> through to buffered I/O when invalidation of a page failed.
 >
 >Well, I like the idea of more intelligently dealing with the known
 >problem between dio and ext3.  I'm not sure that falling back to
 >buffered is right.

In the past discussion above, it was said that dio was a sort of best
effort function. So dio falls back to buffered IO when writing to hall, I think.
My idea is that this notion applies to this issue.

 >
 >> We can distinguish between failure of page invalidation and other errors
 >> with the return value of invalidate_inode_pages3_range().
 >
 >I'm not sure duplicating the invalidation loop into a new function is
 >the right thing.  Maybe we'd just tweak inode_pages2 to indicate to the
 >caller the specific failing circumstances somehow.  Maybe.

It will be better that adding one parameter to invalidate_inode_pages2_range so
that caller can deal with a number of cases.

Re: [PATCH] dio: falling through to buffered I/O when invalidation of a page fails

[Badari Pulavarty]

On Tue, 2007-12-11 at 17:00 -0800, Zach Brown wrote:
> Hisashi Hifumi wrote:
> > Hi.
> > 
> > Current dio has some problems:
> > 1, In ext3 ordered, dio write can return with EIO because of the race
> > between invalidation of
> > a page and jbd. jbd pins the bhs while committing journal so
> > try_to_release_page fails when jbd
> > is committing the transaction.
> 
> Yeah.  It sure would be fantastic if some ext3 expert could stop this
> from happening somehow.  But that hasn't happened in.. uh.. Badari, for
> how many years has this been on the radar? :)

I used to have a test case that would reproduce the problem some what
consistently. But with invalidate_range() introduction and Jan Kara's
re-write of journal commit handling, I can't reproduce the problem
anymore. So I gave up fixing the problem which I can't reproduce.

If anyone has a testcase - I can take a look at the problem again.

Thanks,
Badari

Re: [PATCH] dio: falling through to buffered I/O when invalidation of a page fails

[Zach Brown]

> If anyone has a testcase - I can take a look at the problem again.

I can try and throw something together..

- z

Re: [PATCH] dio: falling through to buffered I/O when invalidationof a page fails

[Hisashi Hifumi]

At 04:15 07/12/15, Zach Brown wrote:
 >
 >> If anyone has a testcase - I can take a look at the problem again.
 >
 >I can try and throw something together..
 >
 >- z

I did a test by using fsstress.

I modified the dio write() of fsstress to check return value, and input
following command;

# fsstress -d /root/testdir/ -l 0 -n 1000 -p 1000 -z -f creat=1 -f write=1 
-f dwrite=1

I got EIO from dio write.


I encountered some PostgreSQL mailing list archive that mentioned about 
this issue.
http://groups.google.com/group/pgsql.hackers/browse_thread/thread/c7515ccd5ac1269c/dc2ddeac3fe0d8c1?lnk=raot&fwc=1

dio can be used in PostgreSQL to write transaction log(WAL). It is said 
that when dio is used,
EIO is returned under some workload.