Messages in this thread | | | Date | Sun, 04 Jul 2010 23:13:01 +0800 | From | Tao Ma <> | Subject | Re: [PATCH 2/2] ocfs2: No need to zero pages past i_size. i_size v2 |
| |
Hi Joel,
On 07/04/2010 05:33 AM, Joel Becker wrote: > Here's the second patch, the one that keeps us from zeroing > pages past i_size. This should keep ocfs2 and Dave's writeback patch > happy. > > Joel > > ------------------------------------------------------- > > When ocfs2 fills a hole, it does so by allocating clusters. When a > cluster is larger than the write, ocfs2 must zero the portions of the > cluster outside of the write. If the clustersize is smaller than a > pagecache page, this is handled by the normal pagecache mechanisms, but > when the clustersize is larger than a page, ocfs2's write code will zero > the pages adjacent to the write. This makes sure the entire cluster is > zeroed correctly. > > Currently ocfs2 behaves exactly the same when writing past i_size. > However, this means ocfs2 is writing zeroed pages for portions of a new > cluster that are beyond i_size. The page writeback code isn't expecting > this. It treats all pages past the one containing i_size as left behind > due to a previous truncate operation. > > Thankfully, ocfs2 calculates the number of pages it will be working on > up front. The rest of the write code merely honors the original > calculation. We can simply trim the number of pages to only cover the > actual file data. > > Signed-off-by: Joel Becker<joel.becker@oracle.com> > --- > fs/ocfs2/aops.c | 15 +++++++++++---- > 1 files changed, 11 insertions(+), 4 deletions(-) > > diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c > index 96e6aeb..e90ad74 100644 > --- a/fs/ocfs2/aops.c > +++ b/fs/ocfs2/aops.c <snip> > @@ -1142,11 +1143,17 @@ static int ocfs2_grab_pages_for_write(struct address_space *mapping, > /* > * Figure out how many pages we'll be manipulating here. For > * non allocating write, we just change the one > - * page. Otherwise, we'll need a whole clusters worth. > + * page. Otherwise, we'll need a whole clusters worth. If we're > + * writing past i_size, we only need enough pages to cover the > + * last page of the write. The comments for the whole function before the function name also needs this change accordingly? > */ > if (new) { > wc->w_num_pages = ocfs2_pages_per_cluster(inode->i_sb); > start = ocfs2_align_clusters_to_page_index(inode->i_sb, cpos); > + /* This is the index *past* the write */ > + end_index = ((user_pos + user_len)>> PAGE_CACHE_SHIFT) + 1; should it be end_index = ((user_pos + user_len - 1) >> PAGE_CACHE_SHIFT) + 1?
> + if ((start + wc->w_num_pages)> end_index) > + wc->w_num_pages = end_index - start; I just noticed that the below loop in ocfs2_grab_pages_for_write is for (i = 0; i < wc->w_num_pages; i++)
I guess w_num_pages should be set to end_index - start_page_of_the_cluster so that we can make sure we grab all the pages in this cluster until i_size?
Regards, Tao
| |