<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
<title></title>
</head>
<body text="#000000" bgcolor="#ffffff">
<br>
<br>
Joel Becker wrote:
<blockquote cite="mid:20100716195137.GA30859@mail.oracle.com"
type="cite">
<pre wrap="">On Fri, Jul 16, 2010 at 10:21:00PM +0800, Tao Ma wrote:
</pre>
<blockquote type="cite">
<pre wrap="">__ocfs2_page_mkwrite now is broken in handling file end.
1. the last page should be the page contains i_size - 1.
2. the len in the last page is also calculated wrong.
So change them accordingly.
</pre>
</blockquote>
<pre wrap=""><!---->
How did you determine these broken? They look correct to me,
and they passed the mmap testing I ran against the tail zeroing fixes.
Comments inline.
</pre>
<blockquote type="cite">
<pre wrap="">diff --git a/fs/ocfs2/mmap.c b/fs/ocfs2/mmap.c
index af2b8fe..4c18f4a 100644
--- a/fs/ocfs2/mmap.c
+++ b/fs/ocfs2/mmap.c
@@ -74,9 +74,11 @@ static int __ocfs2_page_mkwrite(struct inode *inode, struct buffer_head *di_bh,
/*
* Another node might have truncated while we were waiting on
* cluster locks.
+ * We don't check size == 0 before the shift. This is borrowed
+ * from do_generic_file_read.
*/
- last_index = size >> PAGE_CACHE_SHIFT;
- if (page->index > last_index) {
</pre>
</blockquote>
<pre wrap=""><!---->
This original check seems correct. If size is on a page
boundary, sure the last_index is past the last page of the file.
That's OK, we're checking that page->index isn't greater than that.
</pre>
</blockquote>
No, it is broken.<br>
so say i_size = 4096. the last valid page index should be 0 not 1.<br>
and if the page that requirs to mk_write has page->index = 1, we
should fail in this case.<br>
While the old one let us go.<br>
As I said in the comment, this code is borrowed from
do_generic_file_read. So you can check<br>
that file for detail.<br>
1034 isize = i_size_read(inode);<br>
1035 end_index = (isize - 1) >> PAGE_CACHE_SHIFT;<br>
1036 if (unlikely(!isize || index > end_index)) {<br>
1037 page_cache_release(page);<br>
1038 goto out;<br>
1039 }<br>
<br>
<blockquote cite="mid:20100716195137.GA30859@mail.oracle.com"
type="cite">
<pre wrap=""> </pre>
<blockquote type="cite">
<pre wrap="">@@ -107,7 +109,7 @@ static int __ocfs2_page_mkwrite(struct inode *inode, struct buffer_head *di_bh,
* because the "write" would invalidate their data.
*/
if (page->index == last_index)
- len = size & ~PAGE_CACHE_MASK;
+ len = ((size - 1) & ~PAGE_CACHE_MASK) + 1;
</pre>
</blockquote>
<pre wrap=""><!---->
And the len calculation is only broken because you changed what
last_index meant. Originally, if the page equals the last_index, size
cannot be page aligned, and you are guaranteed a proper len.
You know, if you don't like the way last_index reads, we can
always steal the ext4 comparison:
5982 if (page->mapping != mapping || size <= page_offset(page)
5983 || !PageUptodate(page)) {
5984 /* page got truncated from under us? */
5985 goto out_unlock;
5986 }
<snip>
5990
5991 if (page->index == size >> PAGE_CACHE_SHIFT)
5992 len = size & ~PAGE_CACHE_MASK;
5993 else
5994 len = PAGE_CACHE_SIZE;
This is a direct compare on the page_offset, which doesn't
confuse anyone about index vs i_size. Later, they directly check "is
this page the last in the file" before computing len.
</pre>
</blockquote>
My code is borrowed from do_generic_file_read and I think it looks
good. See here.<br>
1042 nr = PAGE_CACHE_SIZE;<br>
1043 if (index == end_index) {<br>
1044 nr = ((isize - 1) & ~PAGE_CACHE_MASK)
+ 1;<br>
1045 if (nr <= offset) {<br>
1046 page_cache_release(page);<br>
1047 goto out;<br>
1048 }<br>
1049 }<br>
<br>
Regards,<br>
Tao<br>
<br>
</body>
</html>