Ocfs2 devel - Feb 2011 - [PATCH] Treat writes as new when holes span across page boundaries

When a hole spans across page boundaries, the next write forces
a read of the block. This could end up reading existing garbage
data from the disk in ocfs2_map_page_blocks. This leads to
non-zero holes. In order to avoid this, mark the writes as new
when the holes span across page boundaries.

Signed-off-by: Goldwyn Rodrigues <rgoldwyn at suse.de>

---
diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c
index 1fbb0e2..4be220d 100644
--- a/fs/ocfs2/aops.c
+++ b/fs/ocfs2/aops.c
@@ -1026,6 +1026,12 @@ 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);

+	/* treat the write as new if the a hole/lseek spanned across
+	 * the page boundary.
+	 */
+	new = new | ((i_size_read(inode) <= page_offset(page)) &&
+			(page_offset(page) <= user_pos));
+
 	if (page == wc->w_target_page) {
 		map_from = user_pos & (PAGE_CACHE_SIZE - 1);
 		map_to = map_from + user_len;

-- 
Goldwyn

On Thu, Feb 17, 2011 at 09:44:40AM -0600, Goldwyn Rodrigues
wrote:
> When a hole spans across page boundaries, the next write forces
> a read of the block. This could end up reading existing garbage
> data from the disk in ocfs2_map_page_blocks. This leads to
> non-zero holes. In order to avoid this, mark the writes as new
> when the holes span across page boundaries.
	Is this a new approach to your earlier patch, or an additional
change?
> ---
> diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c
> index 1fbb0e2..4be220d 100644
> --- a/fs/ocfs2/aops.c
> +++ b/fs/ocfs2/aops.c
> @@ -1026,6 +1026,12 @@ 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);
> 
> +	/* treat the write as new if the a hole/lseek spanned across
> +	 * the page boundary.
> +	 */
> +	new = new | ((i_size_read(inode) <= page_offset(page)) &&
> +			(page_offset(page) <= user_pos));
	There are two problems here.  First, It's not safe to claim
existing data is 'new'.  Imagine you have a 4K page and a 512B
blocksize.  The first 2 blocks of the page have data in them, but your
code change will cause them to be set_uptodate() even if we haven't read
them in yet.
	Secondly, ocfs2_should_read_blk() already checks for blocks past
i_size and skips reading them.  So if you are trying to avoid reading
them, it is already handled.

Joel

-- 

Life's Little Instruction Book #511

	"Call your mother."

			http://www.jlbec.org/
			jlbec at evilplan.org

On Thu, Feb 17, 2011 at 09:44:40AM -0600, Goldwyn Rodrigues
wrote:
> When a hole spans across page boundaries, the next write forces
> a read of the block. This could end up reading existing garbage
> data from the disk in ocfs2_map_page_blocks. This leads to
> non-zero holes. In order to avoid this, mark the writes as new
> when the holes span across page boundaries.
> 
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn at suse.de>
This patch is now part of the merge-window branch of ocfs2.git.

I'm still not sure this is the final fix.  I worry that it papers over
our expectations of our new block/page/cluster code.  But we have more
than report of the problem, and this seems to alleviate those reports.
I can't see any way it breaks existing operation.  So I think it should
go in while we later revisit whether it is the end of the discussion.

Joel

-- 

"Behind every successful man there's a lot of unsuccessful years."
        - Bob Brown

			http://www.jlbec.org/
			jlbec at evilplan.org