Re: [PATCH] JFS fsync wrong behavior when I/O failure occurs

Re: [PATCH] JFS fsync wrong behavior when I/O failure occurs

[Dave Kleikamp]

On Mon, 2005-04-25 at 22:36 +0800, qufuping wrote:
> JFS contains a bug with sys_fsync(jfs_fsync), also with fs/mpage.c:mpage_end_io_write

ACK.  This patch looks good to me.

Thanks,
Shaggy

> Symptom
> The open-write-fsync-close code will not return error when disk I/O occurs
> between open and fsync.
-- 
David Kleikamp
IBM Linux Technology Center

[PATCH] JFS fsync wrong behavior when I/O failure occurs

[fs]

The patch has sent to JFS maintainer, and ACKed,
but filtered by LKML, so I send it again.

JFS contains a bug with sys_fsync(jfs_fsync), also with
fs/mpage.c:mpage_end_io_write

Symptom
The open-write-fsync-close code will not return error when disk I/O
occurs
between open and fsync.

Details
1. sys_fsync does 3 jobs: 
  submit all pages taged with PAGECACHE_TAG_DIRTY to disk I/O, 
  do file-system related fsync operations
  wait all pages taged with PAGECACHE_TAG_WRITEBACK to complete.
Here, all disk I/O are asynchronous, when finished, mpage_end_io_write
will
remove page from mapping's PAGECACHE_TAG_WRITEBACK tree. If I/O fails,
it will also set PG_error flag, but it FORGET to set the mapping->flags
AS_EIO
flag. So in step 3, sys_fsync won't notice these pages.
2. Besides 1, jfs_fsync should return error also, but it doesn't wait
the page
uptodate, e.g.:
jfs_fsync->jfs_commit_inode->txCommit->diWrite->read_metapage->__get_metapage
->read_cache_page reads a page from disk. Because read is async, when
read_cache_page: err = filler(data, page), filler will not return error,
it just submits I/O request and returns. So, page is not uptodate.
only use if(IS_ERROR(mp->page)) is not enough, we should add 
"|| !PageUptodate(mp->page)" - remember, mp->data = kmap(mp->page) +
page_offst, 
later, the outer function will have mem access with mp->data. If
mp->data is not
uptodate, dangerous.

Patch
diff -urN linux-2.6.11.7/fs/jfs/jfs_metapage.c
linux-2.6.11.7.new/fs/jfs/jfs_metapage.c
--- linux-2.6.11.7/fs/jfs/jfs_metapage.c 2005-04-07 14:57:26.000000000
-0400
+++ linux-2.6.11.7.new/fs/jfs/jfs_metapage.c 2005-04-25
16:33:59.000000000 -0400
@@ -347,7 +347,13 @@
    jfs_info("__get_metapage: Calling read_cache_page");
    mp->page = read_cache_page(mapping, lblock,
         (filler_t *)mapping->a_ops->readpage, NULL);
-   if (IS_ERR(mp->page)) {
+   if (IS_ERR(mp->page) || !PageUptodate(mp->page)) {
+    /* Page hasn't been uptodate yet because of
+     * async I/O, in most cases, I/O error has 
+     * occurred. We must make sure the page is 
+     * uptodate - the outer function will do mem
+     * operations with kmap(mp->page) + page_offset
+     */
     jfs_err("read_cache_page failed!");
     goto freeit;
    } else
diff -urN linux-2.6.11.7/fs/mpage.c linux-2.6.11.7.new/fs/mpage.c
--- linux-2.6.11.7/fs/mpage.c 2005-04-07 14:57:25.000000000 -0400
+++ linux-2.6.11.7.new/fs/mpage.c 2005-04-25 16:38:05.000000000 -0400
@@ -79,8 +79,10 @@
   if (--bvec >= bio->bi_io_vec)
    prefetchw(&bvec->bv_page->flags);
 
-  if (!uptodate)
+  if (!uptodate){
    SetPageError(page);
+   set_bit(AS_EIO, &page->mapping->flags);
+  }
   end_page_writeback(page);
  } while (bvec >= bio->bi_io_vec);
  bio_put(bio);

Ref
http://developer.osdl.jp/projects/doubt/fs-consistency-and-coherency/index.html

Signed-off-by: Qu Fuping<qufuping@ercist.iscas.ac.cn>