[Ocfs2-devel] [PATCH] Zero from EOF instead of next block

[Goldwyn Rodrigues]

On Wed, Feb 16, 2011 at 12:26 AM, Goldwyn Rodrigues <rgoldwyn at gmail.com> wrote:
> On Mon, Feb 14, 2011 at 4:25 PM, Joel Becker <jlbec at evilplan.org> wrote:
>> On Mon, Feb 14, 2011 at 04:16:37PM -0600, Goldwyn Rodrigues wrote:
>>> Oh, I thought the test case was self-explanatory. The test case first writes
>>> a lot of dirty data on disk "0xbaadfeed", syncs the data and removes
>>> the file so that the filesystem has a lot of "0xbaadfeed" data on it.
>>>
>>> The second executable writes records of 32 bytes and seeks another 32
>>> bytes. While reading the file back, 0xbaadfeed is read when zeros are
>>> expected. The program shows where the unexpected data is found.
>>>
>>> So, for holes not spanning extents or even blocks, the data which was
>>> on disk previously shows up.
>>
>>        That shouldn't happen, of course.  But the solution will not be
>> writing past the last block of the file.  We should be catching it
>> instead when extending the i_size into a previously uninitialized block.
>>
>
> The problem occurs when the last few bytes of the previous page is a
> hole, and the write starts from the beginning of next page. This
> causes the page to be read in ocfs2_map_page_blocks(), and since this
> is a new block, it picks up garbage from the disk. So, the best way
> seems to be enforce it to be a new page when writing from block
> boundary by testing against pos.
>
> I tested this patch. However, could you check if this would affect
> anything else?
>

Of course that is incorrect. It has to be dependent on the file size
as well. The corrected patch is:

diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c
index 1fbb0e2..10199e7 100644
--- a/fs/ocfs2/aops.c
+++ b/fs/ocfs2/aops.c
@@ -1026,6 +1026,9 @@ static int ocfs2_prepare_page_for_write(struct
inode *inode, u64 *p_blkno,
        ocfs2_figure_cluster_boundaries(OCFS2_SB(inode->i_sb), cpos,
                                        &cluster_start, &cluster_end);

+       new = new | ((page_offset(page) == user_pos)
+                       && (page_offset(page) > i_size_read(inode)));
+
        if (page == wc->w_target_page) {
                map_from = user_pos & (PAGE_CACHE_SIZE - 1);
                map_to = map_from + user_len;



-- 
Goldwyn