[Ocfs2-devel] [PATCH] ocfs2: make __ocfs2_page_mkwrite handle file end properly.

[Ocfs2-devel] [PATCH] ocfs2: make __ocfs2_page_mkwrite handle file end properly.

[Tao Ma]

__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.

Signed-off-by: Tao Ma <tao.ma@oracle.com>
---
 fs/ocfs2/mmap.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

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) {
+	last_index = (size - 1) >> PAGE_CACHE_SHIFT;
+	if (unlikely(!size || page->index > last_index)) {
 		ret = -EINVAL;
 		goto out;
 	}
@@ -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;
 
 	ret = ocfs2_write_begin_nolock(mapping, pos, len, 0, &locked_page,
 				       &fsdata, di_bh, page);
-- 
1.7.1.GIT

[Ocfs2-devel] [PATCH] ocfs2: make __ocfs2_page_mkwrite handle file end properly.

[Joel Becker]

On Fri, Jul 16, 2010 at 10:21:00PM +0800, Tao Ma wrote:
> __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.

	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.

> 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) {

	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.

> @@ -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;

	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.

Joel

-- 

Life's Little Instruction Book #15

	"Own a great stereo system."

Joel Becker
Consulting Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127

[Ocfs2-devel] [PATCH] ocfs2: make __ocfs2_page_mkwrite handle file end properly.

[Tao Ma]

Joel Becker wrote:
> On Fri, Jul 16, 2010 at 10:21:00PM +0800, Tao Ma wrote:
>    
>> __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.
>>      
>
> 	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.
>
>    
>> 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) {
>>      
>
> 	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.
>    
No, it is broken.
so say i_size = 4096. the last valid page index should be 0 not 1.
and if the page that requirs to mk_write has page->index = 1, we should 
fail in this case.
While the old one let us go.
As I said in the comment, this code is borrowed from 
do_generic_file_read. So you can check
that file for detail.
1034                 isize = i_size_read(inode);
1035                 end_index = (isize - 1) >> PAGE_CACHE_SHIFT;
1036                 if (unlikely(!isize || index > end_index)) {
1037                         page_cache_release(page);
1038                         goto out;
1039                 }

>    
>> @@ -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;
>>      
>
> 	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.
>    
My code is borrowed from do_generic_file_read and I think it looks good. 
See here.
1042                 nr = PAGE_CACHE_SIZE;
1043                 if (index == end_index) {
1044                         nr = ((isize - 1) & ~PAGE_CACHE_MASK) + 1;
1045                         if (nr <= offset) {
1046                                 page_cache_release(page);
1047                                 goto out;
1048                         }
1049                 }

Regards,
Tao

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://oss.oracle.com/pipermail/ocfs2-devel/attachments/20100717/b492db37/attachment.html