Hisashi Hifumi wrote:
> Hi.
>
> DIO invalidates page cache through invalidate_inode_pages2_range().
> 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.
>
> Following patch fixes this issue.
I like the idea of fixing this in a separate patch, yes, thanks! But...
>
> Thanks.
>
> Signed-off-by :Hisashi Hifumi <[email protected]>
>
> diff -Nrup linux-2.6.24-rc5.org/mm/truncate.c
> linux-2.6.24-rc5/mm/truncate.c
> --- linux-2.6.24-rc5.org/mm/truncate.c 2007-12-12 16:32:45.000000000
> +0900
> +++ linux-2.6.24-rc5/mm/truncate.c 2007-12-13 11:45:29.000000000 +0900
> @@ -392,6 +392,7 @@ int invalidate_inode_pages2_range(struct
> pgoff_t next;
> int i;
> int ret = 0;
> + int ret2 = 0;
> int did_range_unmap = 0;
> int wrapped = 0;
>
> @@ -441,13 +442,13 @@ int invalidate_inode_pages2_range(struct
> BUG_ON(page_mapped(page));
> ret = do_launder_page(mapping, page);
> if (ret == 0 && !invalidate_complete_page2(mapping, page))
> - ret = -EIO;
> + ret2 = -EIO;
> unlock_page(page);
> }
> pagevec_release(&pvec);
> cond_resched();
> }
> - return ret;
> + return !ret ? ret2 : ret;
> }
> EXPORT_SYMBOL_GPL(invalidate_inode_pages2_range);
... this doesn't work. Notice that it only propagates the -EIO into ret2? It can lose errors from do_launder_page() itself because they aren't stored into ret2, which is what's returned if the last do_launder_page() succeeds.
This isn't I meant when I mentioned that 'ret2' pattern. The idea is to store the errors from inner calls into some private var and then only promote those errors as the function's return code (ret) if there was an error and ret wasn't already set. We do this in a few places in dio, you'll notice. This patch gets the meaning of 'ret' and 'ret2' opposite from those uses, which is kind of confusing.
So, uh, how about the following. Totally untested, but compiled.
-------
invalidate_inode_pages2_range(): consistently return first error
Hisashi Hifumi noticed that we were losing errors in
invalidate_inode_pages2_range(). Later do_launder_page() calls could overwrite
errors generated by earlier calls. Fix this by storing do_launder_page in a
temporary variable which is only promoted to the function's return code if it
hadn't already generated an error.
Signed-off-by: Zach Brown <[email protected]>
Cc: Hisashi Hifumi <[email protected]>
diff --git a/mm/truncate.c b/mm/truncate.c
index cadc156..24578b3 100644
--- a/mm/truncate.c
+++ b/mm/truncate.c
@@ -392,6 +392,7 @@ int invalidate_inode_pages2_range(struct address_space *mapping,
pgoff_t next;
int i;
int ret = 0;
+ int rc;
int did_range_unmap = 0;
int wrapped = 0;
@@ -439,9 +440,11 @@ int invalidate_inode_pages2_range(struct address_space *mapping,
}
}
BUG_ON(page_mapped(page));
- ret = do_launder_page(mapping, page);
- if (ret == 0 && !invalidate_complete_page2(mapping, page))
- ret = -EIO;
+ rc = do_launder_page(mapping, page);
+ if (rc == 0 && !invalidate_complete_page2(mapping, page))
+ rc = -EIO;
+ if (rc && ret == 0)
+ ret = rc;
unlock_page(page);
}
pagevec_release(&pvec);
>
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
