ocfs2: Don't duplicate page passes i_size during CoW.
PATCH 1/1 ocfs2: Don't duplicate page passes i_size during CoW.
During CoW, actually all the pages after i_size contains garbage data, so don't read and duplicate them.
--- fs/ocfs2/refcounttree.c | 10 +++++++++- 1 files changed, 9 insertions(+), 1 deletions(-)
fs/ocfs2/refcounttree.c +9 -1
@@ -2921,7 +2921,7 @@ static int ocfs2_duplicate_clusters_by_page(handle_t *handle,struct super_block *sb =ocfs2_metadata_cache_get_super (ci);u64 new_block =ocfs2_clusters_to_blocks (sb, new_cluster);struct page *page; -pgoff_t page_index; +pgoff_t page_index, last_page;unsigned int from, to;loff_t offset, end, map_end;struct address_space *mapping = context->inode->i_mapping;
@@ -2932,12 +2932,20 @@ static int ocfs2_duplicate_clusters_by_page(handle_t *handle, offset = ((loff_t )cpos) <<OCFS2_SB (sb)->s_clustersize_bits; end = offset + (new_len <<OCFS2_SB (sb)->s_clustersize_bits); + last_page =i_size_read (context->inode) >>PAGE_CACHE_SHIFT ;while (offset < end) { page_index = offset >>PAGE_CACHE_SHIFT ; map_end = ((loff_t )page_index +1 ) <<PAGE_CACHE_SHIFT ;if (map_end > end) map_end = end; +/* +* If this page is beyond the page contains i_size, +* stop duplicating it. +*/ +if (page_index > last_page) +break ; +/* from, to is the offset within the page. */ from = offset & (PAGE_CACHE_SIZE -1 ); to =PAGE_CACHE_SIZE ;
Signature · 1 line
1.7.1.571.gba4d01
PATCH 1/1 ocfs2: Don't duplicate page passes i_size during CoW.
Quoted 22 lines · Tao Ma
During CoW, actually all the pages after
i_sizecontains garbage data, so don't read and duplicate them.Signed-off-by Tao Ma <[email protected]>--- fs/ocfs2/refcounttree.c | 10 +++++++++- 1 files changed, 9 insertions(+), 1 deletions(-)fs/ocfs2/refcounttree.c +1
-1@@ -2921,7 +2921,7 @@ static int ocfs2_duplicate_clusters_by_page(handle_t *handle,struct super_block *sb =ocfs2_metadata_cache_get_super (ci);u64 new_block =ocfs2_clusters_to_blocks (sb, new_cluster);struct page *page; -pgoff_t page_index; +pgoff_t page_index, last_page;unsigned int from, to;loff_t offset, end, map_end;struct address_space *mapping = context->inode->i_mapping;
fs/ocfs2/refcounttree.c static int ocfs2_duplicate_clusters_by_page(handle_t *handle, · line 2936
Quoted 5 lines
fs/ocfs2/refcounttree.c +1
-0@@ -2932,12 +2932,20 @@ static int ocfs2_duplicate_clusters_by_page(handle_t *handle, offset = ((loff_t )cpos) <<OCFS2_SB (sb)->s_clustersize_bits; end = offset + (new_len <<OCFS2_SB (sb)->s_clustersize_bits); + last_page =i_size_read (context->inode) >>PAGE_CACHE_SHIFT ;while (offset < end) {
Why trigger on both index and byte offset? Why not just adjust
end?
if (end < i_size_read(context->inode)) end = i_size_read(inode); Then you don't need the last_page variable at all, because
you're guaranteed never to go past i_size.
Joel
Signature · 9 lines
"Three o'clock is always too late or too early for anything you
want to do."
- Jean-Paul Sartre
Joel Becker
Consulting Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127PATCH v2 1/1 ocfs2: Don't duplicate page passes i_size during CoW.
Hi Joel,
Quoted 9 lines
Review static int ocfs2_duplicate_clusters_by_page(handle_t *handle, · line 2936
Quoted 6 lines
(quoted diff) +1
-0@@ -2932,12 +2932,20 @@ static int ocfs2_duplicate_clusters_by_page(handle_t *handle, offset = ((loff_t)cpos)<< OCFS2_SB(sb)->s_clustersize_bits; end = offset + (new_len<< OCFS2_SB(sb)->s_clustersize_bits); + last_page = i_size_read(context->inode)>> PAGE_CACHE_SHIFT; while (offset< end) {Why trigger on both index and byte offset? Why not just adjust
end?
OK, here is the udpated one.
Regards,
Tao
From b30377da2dff2b5823c6a4a66153aa035334a632 Mon Sep 17 00:00:00 2001
From: Tao Ma <[email protected]>
Date: Tue, 13 Jul 2010 09:53:28 +0800
Subject: [PATCH v2] ocfs2: Don't duplicate page passes i_size during CoW.
During CoW, actually all the pages after i_size contains garbage data, so don't read and duplicate them.
--- fs/ocfs2/refcounttree.c | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-)
fs/ocfs2/refcounttree.c +7 -0
@@ -2931,6 +2931,13 @@ static int ocfs2_duplicate_clusters_by_page(handle_t *handle, offset = ((loff_t )cpos) <<OCFS2_SB (sb)->s_clustersize_bits; end = offset + (new_len <<OCFS2_SB (sb)->s_clustersize_bits); +/* +* We only duplicate pages until we reach i_size. +* So trim 'end' to the boundary of that page. +*/ +if (end >i_size_read (context->inode)) + end = ((i_size_read (context->inode) +PAGE_CACHE_SIZE -1 ) >> +PAGE_CACHE_SHIFT ) <<PAGE_CACHE_SHIFT ;while (offset < end) { page_index = offset >>PAGE_CACHE_SHIFT ;
Signature · 1 line
1.7.1.571.gba4d01
PATCH v2 1/1 ocfs2: Don't duplicate page passes i_size during CoW.
fs/ocfs2/refcounttree.c static int ocfs2_duplicate_clusters_by_page(handle_t *handle, · line 2940
Quoted 7 lines · Tao Ma
fs/ocfs2/refcounttree.c +7
-0+/* +* We only duplicate pages until we reach i_size. +* So trim 'end' to the boundary of that page. +*/ +if (end >i_size_read (context->inode)) + end = ((i_size_read (context->inode) +PAGE_CACHE_SIZE -1 ) >> +PAGE_CACHE_SHIFT ) <<PAGE_CACHE_SHIFT ;
Just use PAGE_CACHE_ALIGN().
Joel
Signature · 9 lines
"Any man who is under 30, and is not a liberal, has not heart;
and any man who is over 30, and is not a conservative, has no brains."
- Sir Winston Churchill
Joel Becker
Consulting Software Developer
Oracle
E-mail: [email protected]
Phone: (650) 506-8127PATCH v2 1/1 ocfs2: Don't duplicate page passes i_size during CoW.
Quoted 8 lines · Joel Becker
fs/ocfs2/refcounttree.c static int ocfs2_duplicate_clusters_by_page(handle_t *handle, · line 2940
Quoted 7 lines · Tao Ma
fs/ocfs2/refcounttree.c +7
-0+/* +* We only duplicate pages until we reach i_size. +* So trim 'end' to the boundary of that page. +*/ +if (end >i_size_read (context->inode)) + end = ((i_size_read (context->inode) +PAGE_CACHE_SIZE -1 ) >> +PAGE_CACHE_SHIFT ) <<PAGE_CACHE_SHIFT ;
Why even worry about that? We only need up to i_size. If end
is safe and not page aligned, so is i_size.
Joel
Signature · 9 lines
Life's Little Instruction Book #451 "Don't be afraid to say, 'I'm sorry.'" Joel Becker Consulting Software Developer Oracle E-mail: [email protected] Phone: (650) 506-8127
PATCH v2 1/1 ocfs2: Don't duplicate page passes i_size during CoW.
Quoted 4 lines · Joel Becker
Quoted 2 lines · Joel Becker
On Tue, Jul 13, 2010 at 11:22:32AM +0800, Tao Ma wrote:
Quoted 12 lines
Quoted 8 lines
fs/ocfs2/refcounttree.c static int ocfs2_duplicate_clusters_by_page(handle_t *handle, · line 2940
Quoted 8 lines
fs/ocfs2/refcounttree.c +7
-0+/* +* We only duplicate pages until we reach i_size. +* So trim 'end' to the boundary of that page. +*/ +if (end >i_size_read (context->inode)) + end = ((i_size_read (context->inode) +PAGE_CACHE_SIZE -1 ) >> +PAGE_CACHE_SHIFT ) <<PAGE_CACHE_SHIFT ;Why even worry about that? We only need up to
i_size. If end
is safe and not page aligned, so isi_size.
i_size isn't safe in ocfs2_duplicate_clusters_by_page. Check the below function.
if (page_has_buffers(page)) {
ret = walk_page_buffers(handle, page_buffers(page),
from, to, &partial,
ocfs2_clear_cow_buffer);
if (ret) {
mlog_errno(ret);
goto unlock;
}
}So 'to' is limited to 'map_end' and then 'end'. If we set 'end' to i_size, we may not clear all the buffer heads
since walk_page_buffers will only call ocfs2_clear_cow_buffer within from and to.
Maybe we can improve it somehow later, but I would leave it as-is now.
Regards,
Tao
PATCH v2 1/1 ocfs2: Don't duplicate page passes i_size during CoW.
Quoted 15 lines · Tao Ma
Quoted 2 lines · Joel Becker
Why even worry about that? We only need up to
i_size. If end
is safe and not page aligned, so isi_size.
i_sizeisn't safe inocfs2_duplicate_clusters_by_page. Check the below function.if (page_has_buffers(page)) { ret = walk_page_buffers(handle, page_buffers(page), from, to, &partial, ocfs2_clear_cow_buffer); if (ret) { mlog_errno(ret); goto unlock; } }
Quoted 2 lines
So 'to' is limited to '
map_end' and then 'end'. If we set 'end' toi_size, we may not clear all the buffer heads
We don't need to clear all the buffer heads. This is the same
as the logic in block_prepare_write(). We only need to clear the blocks that encompass i_size. Unless we did something wrong, the buffers past i_size should not be mapped.
More importantly, your 'end' has the same layout. If end is not
page aligned, it will only clear some of the blocks in its page. Which is fine. There's no difference between end and i_size - they don't have to fall on page boundaries.
Joel
Signature · 9 lines
"Here's something to think about: How come you never see a headline like ``Psychic Wins Lottery''?" - Jay Leno Joel Becker Consulting Software Developer Oracle E-mail: joel.becker at oracle.com Phone: (650) 506-8127
PATCH v2 1/1 ocfs2: Don't duplicate page passes i_size during CoW.
Quoted 23 lines · Joel Becker
Quoted 17 lines · Tao Ma
Quoted 3 lines · Joel Becker
Why even worry about that? We only need up to
i_size. If end
is safe and not page aligned, so isi_size.
i_sizeisn't safe inocfs2_duplicate_clusters_by_page. Check the below function.if (page_has_buffers(page)) { ret = walk_page_buffers(handle, page_buffers(page), from, to, &partial, ocfs2_clear_cow_buffer); if (ret) { mlog_errno(ret); goto unlock; } }Quoted 3 lines
So 'to' is limited to '
map_end' and then 'end'. If we set 'end' toi_size, we may not clear all the buffer heads
Quoted 9 lines
We don't need to clear all the buffer heads. This is the same
as the logic inblock_prepare_write(). We only need to clear the blocks that encompassi_size. Unless we did something wrong, the buffers pasti_sizeshould not be mapped.
More importantly, your 'end' has the same layout. If end is not
page aligned, it will only clear some of the blocks in its page. Which is fine. There's no difference between end andi_size- they don't have to fall on page boundaries.
oh, I see.block_read_full_page will only map and read the blocks within i_size, cool. Then end = i_size should work fine.
I will test it and then send a final one(hope so ;))
Thanks for pointing it out.
Regards,
Tao
PATCH v5 1/1 ocfs2: Don't duplicate page passes i_size during CoW.
In reply to Joel BeckerDuring CoW, actually all the pages after i_size contains garbage data, so don't read and duplicate them.
--- fs/ocfs2/refcounttree.c | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-)
fs/ocfs2/refcounttree.c +6 -0
@@ -2931,6 +2931,12 @@ static int ocfs2_duplicate_clusters_by_page(handle_t *handle, offset = ((loff_t )cpos) <<OCFS2_SB (sb)->s_clustersize_bits; end = offset + (new_len <<OCFS2_SB (sb)->s_clustersize_bits); +/* +* We only duplicate pages until we reach the page contains i_size - 1. +* So trim 'end' to i_size. +*/ +if (end >i_size_read (context->inode)) + end =i_size_read (context->inode);while (offset < end) { page_index = offset >>PAGE_CACHE_SHIFT ;
Signature · 1 line
1.7.1.571.gba4d01
PATCH v5 1/1 ocfs2: Don't duplicate page passes i_size during CoW.
Quoted 25 lines · Tao Ma
During CoW, actually all the pages after
i_sizecontains garbage data, so don't read and duplicate them.Signed-off-by Tao Ma <[email protected]>--- fs/ocfs2/refcounttree.c | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-)fs/ocfs2/refcounttree.c +6
-0@@ -2931,6 +2931,12 @@ static int ocfs2_duplicate_clusters_by_page(handle_t *handle, offset = ((loff_t )cpos) <<OCFS2_SB (sb)->s_clustersize_bits; end = offset + (new_len <<OCFS2_SB (sb)->s_clustersize_bits); +/* +* We only duplicate pages until we reach the page contains i_size - 1. +* So trim 'end' to i_size. +*/ +if (end >i_size_read (context->inode)) + end =i_size_read (context->inode);while (offset < end) { page_index = offset >>PAGE_CACHE_SHIFT ;
PATCH v5 1/1 ocfs2: Don't duplicate page passes i_size during CoW.
In reply to Tao MaQuoted 4 lines · Tao Ma
During CoW, actually all the pages after
i_sizecontains garbage data, so don't read and duplicate them.Signed-off-by Tao Ma <[email protected]>
This patch is now in the fixes branch of ocfs2.git.
Joel
Signature · 9 lines
"I don't know anything about music. In my line you don't have
to."
- Elvis Presley
Joel Becker
Consulting Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127PATCH v3 1/1 ocfs2: Don't duplicate page passes i_size during CoW.
In reply to Joel BeckerQuoted 10 lines · Joel Becker
Review
Quoted 7 lines · Tao Ma
(quoted diff) +7
-0+ /* + * We only duplicate pages until we reach i_size. + * So trim 'end' to the boundary of that page. + */ + if (end > i_size_read(context->inode)) + end = ((i_size_read(context->inode) + PAGE_CACHE_SIZE - 1) >> + PAGE_CACHE_SHIFT) << PAGE_CACHE_SHIFT;Just use
PAGE_CACHE_ALIGN().
oh, I don't know we have such a good macro. thanks. Here is the updated one.
Regards,
Tao
From c1c2b02b2e5c30021a2265d950db6ed23c8d57d0 Mon Sep 17 00:00:00 2001
From: Tao Ma <[email protected]>
Date: Tue, 13 Jul 2010 22:13:37 +0800
Subject: [PATCH v3] ocfs2: Don't duplicate page passes i_size during CoW.
During CoW, actually all the pages after i_size contains garbage data, so don't read and duplicate them.
--- fs/ocfs2/refcounttree.c | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-)
fs/ocfs2/refcounttree.c +6 -0
@@ -2931,6 +2931,12 @@ static int ocfs2_duplicate_clusters_by_page(handle_t *handle, offset = ((loff_t )cpos) <<OCFS2_SB (sb)->s_clustersize_bits; end = offset + (new_len <<OCFS2_SB (sb)->s_clustersize_bits); +/* +* We only duplicate pages until we reach the page contains i_size - 1. +* So trim 'end' to the boundary of that page. +*/ +if (end >i_size_read (context->inode)) + end =PAGE_CACHE_ALIGN (i_size_read (context->inode));while (offset < end) { page_index = offset >>PAGE_CACHE_SHIFT ;
Signature · 1 line
1.7.1.571.gba4d01
PATCH v4 1/1 ocfs2: Don't duplicate page passes i_size during CoW.
Hi Joel,
I suddenly thought there is a bug in my previous patch.
In ppc where page size may be larger than cluster size, we may increase 'end' to an invalid page end which our current cow can't handle. So here is the updated one.
Regards,
Tao
From fd6d19b09efa9d7305375132e0cfb6a8eec9108a Mon Sep 17 00:00:00 2001
From: Tao Ma <[email protected]>
Date: Tue, 13 Jul 2010 22:51:01 +0800
Subject: [PATCH v4] ocfs2: Don't duplicate page passes i_size during CoW.
During CoW, actually all the pages after i_size contains garbage data, so don't read and duplicate them.
--- fs/ocfs2/refcounttree.c | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-)
fs/ocfs2/refcounttree.c +7 -0
@@ -2931,6 +2931,13 @@ static int ocfs2_duplicate_clusters_by_page(handle_t *handle, offset = ((loff_t )cpos) <<OCFS2_SB (sb)->s_clustersize_bits; end = offset + (new_len <<OCFS2_SB (sb)->s_clustersize_bits); +/* +* We only duplicate pages until we reach the page contains i_size - 1. +* So trim 'end' to the boundary of that page. +*/ +if (end >i_size_read (context->inode)) + end =min (end, + (loff_t )PAGE_CACHE_ALIGN (i_size_read (context->inode)));while (offset < end) { page_index = offset >>PAGE_CACHE_SHIFT ;
Signature · 1 line
1.7.1.571.gba4d01