[Ocfs2-devel] [PATCH] ocfs2: Don't duplicate page passes i_size during CoW.

[Ocfs2-devel] [PATCH] ocfs2: Don't duplicate page passes i_size during CoW.

[Tao Ma]

During CoW, actually all the pages after i_size contains
garbage data, so don't read and duplicate them.

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

diff --git a/fs/ocfs2/refcounttree.c b/fs/ocfs2/refcounttree.c
index 1cf9cda..e082623 100644
--- a/fs/ocfs2/refcounttree.c
+++ b/fs/ocfs2/refcounttree.c
@@ -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;
-- 
1.7.1.571.gba4d01

[Ocfs2-devel] [PATCH] ocfs2: Don't duplicate page passes i_size during CoW.

[Joel Becker]

On Mon, Jul 12, 2010 at 03:19:48PM +0800, Tao Ma wrote:
> During CoW, actually all the pages after i_size contains
> garbage data, so don't read and duplicate them.
> 
> Signed-off-by: Tao Ma <tao.ma@oracle.com>
> ---
>  fs/ocfs2/refcounttree.c |   10 +++++++++-
>  1 files changed, 9 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/ocfs2/refcounttree.c b/fs/ocfs2/refcounttree.c
> index 1cf9cda..e082623 100644
> --- a/fs/ocfs2/refcounttree.c
> +++ b/fs/ocfs2/refcounttree.c
> @@ -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) {

	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

-- 

"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-8127

[Ocfs2-devel] [PATCH v2] ocfs2: Don't duplicate page passes i_size during CoW.

[Tao Ma]

Hi Joel,
>> @@ -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 <tao.ma@oracle.com>
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.

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

diff --git a/fs/ocfs2/refcounttree.c b/fs/ocfs2/refcounttree.c
index 1cf9cda..e4e85e6 100644
--- a/fs/ocfs2/refcounttree.c
+++ b/fs/ocfs2/refcounttree.c
@@ -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;
-- 
1.7.1.571.gba4d01

[Ocfs2-devel] [PATCH v2] ocfs2: Don't duplicate page passes i_size during CoW.

[Joel Becker]

On Tue, Jul 13, 2010 at 11:22:32AM +0800, Tao Ma wrote:
> +	/*
> +	 * 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

-- 

"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: joel.becker@oracle.com
Phone: (650) 506-8127

[Ocfs2-devel] [PATCH v2] ocfs2: Don't duplicate page passes i_size during CoW.

[Joel Becker]

On Tue, Jul 13, 2010 at 01:13:06AM -0700, Joel Becker wrote:
> On Tue, Jul 13, 2010 at 11:22:32AM +0800, Tao Ma wrote:
> > +	/*
> > +	 * 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

-- 

Life's Little Instruction Book #451

	"Don't be afraid to say, 'I'm sorry.'"

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

[Ocfs2-devel] [PATCH v2] ocfs2: Don't duplicate page passes i_size during CoW.

[Tao Ma]

Joel Becker wrote:
> On Tue, Jul 13, 2010 at 01:13:06AM -0700, Joel Becker wrote:
>   
>> On Tue, Jul 13, 2010 at 11:22:32AM +0800, Tao Ma wrote:
>>     
>>> +	/*
>>> +	 * 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.
>   
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

[Ocfs2-devel] [PATCH v3] ocfs2: Don't duplicate page passes i_size during CoW.

[Tao Ma]

Joel Becker wrote:
> On Tue, Jul 13, 2010 at 11:22:32AM +0800, Tao Ma wrote:
>> +	/*
>> +	 * 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 <tao.ma@oracle.com>
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.

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

diff --git a/fs/ocfs2/refcounttree.c b/fs/ocfs2/refcounttree.c
index 1cf9cda..efde427 100644
--- a/fs/ocfs2/refcounttree.c
+++ b/fs/ocfs2/refcounttree.c
@@ -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;
-- 
1.7.1.571.gba4d01

[Ocfs2-devel] [PATCH v4] ocfs2: Don't duplicate page passes i_size during CoW.

[Tao Ma]

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 <tao.ma@oracle.com>
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.

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

diff --git a/fs/ocfs2/refcounttree.c b/fs/ocfs2/refcounttree.c
index 1cf9cda..4f07bb1 100644
--- a/fs/ocfs2/refcounttree.c
+++ b/fs/ocfs2/refcounttree.c
@@ -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;
-- 
1.7.1.571.gba4d01

[Ocfs2-devel] [PATCH v2] ocfs2: Don't duplicate page passes i_size during CoW.

[Joel Becker]

On Tue, Jul 13, 2010 at 10:25:53PM +0800, Tao Ma wrote:
> Joel Becker wrote:
> >	Why even worry about that?  We only need up to i_size.  If end
> >is safe and not page aligned, so is i_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

	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

-- 

"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

[Ocfs2-devel] [PATCH v2] ocfs2: Don't duplicate page passes i_size during CoW.

[Tao Ma]

Joel Becker wrote:
> On Tue, Jul 13, 2010 at 10:25:53PM +0800, Tao Ma wrote:
>   
>> Joel Becker wrote:
>>     
>>> 	Why even worry about that?  We only need up to i_size.  If end
>>> is safe and not page aligned, so is i_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
>>     
>
> 	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.
>   
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

[Ocfs2-devel] [PATCH v5] ocfs2: Don't duplicate page passes i_size during CoW.

[Tao Ma]

During CoW, actually all the pages after i_size contains
garbage data, so don't read and duplicate them.

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

diff --git a/fs/ocfs2/refcounttree.c b/fs/ocfs2/refcounttree.c
index 1cf9cda..5e03db2 100644
--- a/fs/ocfs2/refcounttree.c
+++ b/fs/ocfs2/refcounttree.c
@@ -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;
-- 
1.7.1.571.gba4d01

[Ocfs2-devel] [PATCH v5] ocfs2: Don't duplicate page passes i_size during CoW.

[tristan]

Tested-by: Tristan Ye <tristan.ye@oracle.com>

Tao Ma wrote:
> During CoW, actually all the pages after i_size contains
> garbage data, so don't read and duplicate them.
>
> Signed-off-by: Tao Ma <tao.ma@oracle.com>
> ---
>  fs/ocfs2/refcounttree.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
>
> diff --git a/fs/ocfs2/refcounttree.c b/fs/ocfs2/refcounttree.c
> index 1cf9cda..5e03db2 100644
> --- a/fs/ocfs2/refcounttree.c
> +++ b/fs/ocfs2/refcounttree.c
> @@ -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;

[Ocfs2-devel] [PATCH v5] ocfs2: Don't duplicate page passes i_size during CoW.

[Joel Becker]

On Wed, Jul 14, 2010 at 11:19:32AM +0800, Tao Ma wrote:
> During CoW, actually all the pages after i_size contains
> garbage data, so don't read and duplicate them.
> 
> Signed-off-by: Tao Ma <tao.ma@oracle.com>

	This patch is now in the fixes branch of ocfs2.git.

Joel

-- 

"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-8127