[PATCH 1/3] f2fs: call flush_dcache_page when the page was updated

[PATCH 1/3] f2fs: call flush_dcache_page when the page was updated

[Jaegeuk Kim]

Whenever f2fs updates mapped pages, it needs to call flush_dcache_page.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/dir.c    | 7 ++++++-
 fs/f2fs/inline.c | 4 +++-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
index 5a49995..312fbfc 100644
--- a/fs/f2fs/dir.c
+++ b/fs/f2fs/dir.c
@@ -287,8 +287,10 @@ void f2fs_set_link(struct inode *dir, struct f2fs_dir_entry *de,
 	f2fs_wait_on_page_writeback(page, type);
 	de->ino = cpu_to_le32(inode->i_ino);
 	set_de_type(de, inode);
-	if (!f2fs_has_inline_dentry(dir))
+	if (!f2fs_has_inline_dentry(dir)) {
 		kunmap(page);
+		flush_dcache_page(page);
+	}
 	set_page_dirty(page);
 	dir->i_mtime = dir->i_ctime = CURRENT_TIME;
 	mark_inode_dirty(dir);
@@ -366,6 +368,7 @@ static int make_empty_dir(struct inode *inode,
 	do_make_empty_dir(inode, parent, &d);
 
 	kunmap_atomic(dentry_blk);
+	flush_dcache_page(dentry_page);
 
 	set_page_dirty(dentry_page);
 	f2fs_put_page(dentry_page, 1);
@@ -579,6 +582,7 @@ fail:
 		clear_inode_flag(F2FS_I(dir), FI_UPDATE_DIR);
 	}
 	kunmap(dentry_page);
+	flush_dcache_page(dentry_page);
 	f2fs_put_page(dentry_page, 1);
 	return err;
 }
@@ -661,6 +665,7 @@ void f2fs_delete_entry(struct f2fs_dir_entry *dentry, struct page *page,
 			NR_DENTRY_IN_BLOCK,
 			0);
 	kunmap(page); /* kunmap - pair of f2fs_find_entry */
+	flush_dcache_page(page);
 	set_page_dirty(page);
 
 	dir->i_ctime = dir->i_mtime = CURRENT_TIME;
diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
index f26fb87..8b7cc51 100644
--- a/fs/f2fs/inline.c
+++ b/fs/f2fs/inline.c
@@ -45,8 +45,8 @@ void read_inline_data(struct page *page, struct page *ipage)
 	src_addr = inline_data_addr(ipage);
 	dst_addr = kmap_atomic(page);
 	memcpy(dst_addr, src_addr, MAX_INLINE_DATA);
-	flush_dcache_page(page);
 	kunmap_atomic(dst_addr);
+	flush_dcache_page(page);
 	SetPageUptodate(page);
 }
 
@@ -107,6 +107,7 @@ int f2fs_convert_inline_page(struct dnode_of_data *dn, struct page *page)
 	dst_addr = kmap_atomic(page);
 	memcpy(dst_addr, src_addr, MAX_INLINE_DATA);
 	kunmap_atomic(dst_addr);
+	flush_dcache_page(page);
 	SetPageUptodate(page);
 no_update:
 	/* write data page to try to make data consistent */
@@ -358,6 +359,7 @@ static int f2fs_convert_inline_dir(struct inode *dir, struct page *ipage,
 					NR_INLINE_DENTRY * F2FS_SLOT_LEN);
 
 	kunmap_atomic(dentry_blk);
+	flush_dcache_page(page);
 	SetPageUptodate(page);
 	set_page_dirty(page);
 
-- 
2.1.1


------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk

[PATCH 2/3] f2fs: submit bio for node blocks in the reclaim path

[Jaegeuk Kim]

If a node page is request to be written during the reclaiming path, we should
submit the bio to avoid pending to recliam it.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/node.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index 478ce1e..dbf49cc 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -1326,6 +1326,10 @@ static int f2fs_write_node_page(struct page *page,
 	dec_page_count(sbi, F2FS_DIRTY_NODES);
 	up_read(&sbi->node_write);
 	unlock_page(page);
+
+	if (wbc->for_reclaim)
+		f2fs_submit_merged_bio(sbi, NODE, WRITE);
+
 	return 0;
 
 redirty_out:
-- 
2.1.1


------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk

[PATCH 3/3] f2fs: write SSA pages under memory pressure

[Jaegeuk Kim]

Under memory pressure, we don't need to skip SSA page writes.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/checkpoint.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index 838e8ed..20a917b 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -178,7 +178,7 @@ static int f2fs_write_meta_page(struct page *page,
 
 	if (unlikely(sbi->por_doing))
 		goto redirty_out;
-	if (wbc->for_reclaim)
+	if (wbc->for_reclaim && page->index < GET_SUM_BLOCK(sbi, 0))
 		goto redirty_out;
 	if (unlikely(f2fs_cp_error(sbi)))
 		goto redirty_out;
@@ -187,6 +187,9 @@ static int f2fs_write_meta_page(struct page *page,
 	write_meta_page(sbi, page);
 	dec_page_count(sbi, F2FS_DIRTY_META);
 	unlock_page(page);
+
+	if (wbc->for_reclaim)
+		f2fs_submit_merged_bio(sbi, META, WRITE);
 	return 0;
 
 redirty_out:
-- 
2.1.1


------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk

Re: [PATCH 1/3] f2fs: call flush_dcache_page when the page was updated

[Changman Lee]

Hi Jaegeuk,

We should call flush_dcache_page before kunmap because the purpose of the cache flush is to address aliasing problem related to virtual address.

On Wed, Nov 19, 2014 at 02:35:08PM -0800, Jaegeuk Kim wrote:
> Whenever f2fs updates mapped pages, it needs to call flush_dcache_page.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
>  fs/f2fs/dir.c    | 7 ++++++-
>  fs/f2fs/inline.c | 4 +++-
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
> index 5a49995..312fbfc 100644
> --- a/fs/f2fs/dir.c
> +++ b/fs/f2fs/dir.c
> @@ -287,8 +287,10 @@ void f2fs_set_link(struct inode *dir, struct f2fs_dir_entry *de,
>  	f2fs_wait_on_page_writeback(page, type);
>  	de->ino = cpu_to_le32(inode->i_ino);
>  	set_de_type(de, inode);
> -	if (!f2fs_has_inline_dentry(dir))
> +	if (!f2fs_has_inline_dentry(dir)) {
>  		kunmap(page);
> +		flush_dcache_page(page);
> +	}
>  	set_page_dirty(page);
>  	dir->i_mtime = dir->i_ctime = CURRENT_TIME;
>  	mark_inode_dirty(dir);
> @@ -366,6 +368,7 @@ static int make_empty_dir(struct inode *inode,
>  	do_make_empty_dir(inode, parent, &d);
>  
>  	kunmap_atomic(dentry_blk);
> +	flush_dcache_page(dentry_page);
>  
>  	set_page_dirty(dentry_page);
>  	f2fs_put_page(dentry_page, 1);
> @@ -579,6 +582,7 @@ fail:
>  		clear_inode_flag(F2FS_I(dir), FI_UPDATE_DIR);
>  	}
>  	kunmap(dentry_page);
> +	flush_dcache_page(dentry_page);
>  	f2fs_put_page(dentry_page, 1);
>  	return err;
>  }
> @@ -661,6 +665,7 @@ void f2fs_delete_entry(struct f2fs_dir_entry *dentry, struct page *page,
>  			NR_DENTRY_IN_BLOCK,
>  			0);
>  	kunmap(page); /* kunmap - pair of f2fs_find_entry */
> +	flush_dcache_page(page);
>  	set_page_dirty(page);
>  
>  	dir->i_ctime = dir->i_mtime = CURRENT_TIME;
> diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
> index f26fb87..8b7cc51 100644
> --- a/fs/f2fs/inline.c
> +++ b/fs/f2fs/inline.c
> @@ -45,8 +45,8 @@ void read_inline_data(struct page *page, struct page *ipage)
>  	src_addr = inline_data_addr(ipage);
>  	dst_addr = kmap_atomic(page);
>  	memcpy(dst_addr, src_addr, MAX_INLINE_DATA);
> -	flush_dcache_page(page);
>  	kunmap_atomic(dst_addr);
> +	flush_dcache_page(page);
>  	SetPageUptodate(page);
>  }
>  
> @@ -107,6 +107,7 @@ int f2fs_convert_inline_page(struct dnode_of_data *dn, struct page *page)
>  	dst_addr = kmap_atomic(page);
>  	memcpy(dst_addr, src_addr, MAX_INLINE_DATA);
>  	kunmap_atomic(dst_addr);
> +	flush_dcache_page(page);
>  	SetPageUptodate(page);
>  no_update:
>  	/* write data page to try to make data consistent */
> @@ -358,6 +359,7 @@ static int f2fs_convert_inline_dir(struct inode *dir, struct page *ipage,
>  					NR_INLINE_DENTRY * F2FS_SLOT_LEN);
>  
>  	kunmap_atomic(dentry_blk);
> +	flush_dcache_page(page);
>  	SetPageUptodate(page);
>  	set_page_dirty(page);
>  
> -- 
> 2.1.1
> 
> 
> ------------------------------------------------------------------------------
> Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
> from Actuate! Instantly Supercharge Your Business Reports and Dashboards
> with Interactivity, Sharing, Native Excel Exports, App Integration & more
> Get technology previously reserved for billion-dollar corporations, FREE
> http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk

Re: [f2fs-dev] [PATCH 1/3] f2fs: call flush_dcache_page when the page was updated

[Jaegeuk Kim]

On Thu, Nov 20, 2014 at 03:04:10PM +0900, Changman Lee wrote:
> Hi Jaegeuk,
> 
> We should call flush_dcache_page before kunmap because the purpose of the cache flush is to address aliasing problem related to virtual address.

Oh, I just followed zero_user_segments below.

static inline void zero_user_segments(struct page *page,
	unsigned start1, unsigned end1,
	unsigned start2, unsigned end2)
{
	void *kaddr = kmap_atomic(page);

	BUG_ON(end1 > PAGE_SIZE || end2 > PAGE_SIZE);

	if (end1 > start1)
		memset(kaddr + start1, 0, end1 - start1);

	if (end2 > start2)
		memset(kaddr + start2, 0, end2 - start2);

	kunmap_atomic(kaddr);
	flush_dcache_page(page);
}

Is this a wrong reference? Or, a bug?

Anyway I modified as below.

Thanks,

>From 7cb7b27c8cd2efc8a31d79239bef5b41c6e79216 Mon Sep 17 00:00:00 2001
From: Jaegeuk Kim <jaegeuk@kernel.org>
Date: Tue, 18 Nov 2014 10:50:21 -0800
Subject: [PATCH] f2fs: call flush_dcache_page when the page was updated

Whenever f2fs updates mapped pages, it needs to call flush_dcache_page.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/dir.c    | 7 ++++++-
 fs/f2fs/inline.c | 2 ++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
index 5a49995..fabf4ee 100644
--- a/fs/f2fs/dir.c
+++ b/fs/f2fs/dir.c
@@ -287,8 +287,10 @@ void f2fs_set_link(struct inode *dir, struct f2fs_dir_entry *de,
 	f2fs_wait_on_page_writeback(page, type);
 	de->ino = cpu_to_le32(inode->i_ino);
 	set_de_type(de, inode);
-	if (!f2fs_has_inline_dentry(dir))
+	if (!f2fs_has_inline_dentry(dir)) {
+		flush_dcache_page(page);
 		kunmap(page);
+	}
 	set_page_dirty(page);
 	dir->i_mtime = dir->i_ctime = CURRENT_TIME;
 	mark_inode_dirty(dir);
@@ -365,6 +367,7 @@ static int make_empty_dir(struct inode *inode,
 	make_dentry_ptr(&d, (void *)dentry_blk, 1);
 	do_make_empty_dir(inode, parent, &d);
 
+	flush_dcache_page(dentry_page);
 	kunmap_atomic(dentry_blk);
 
 	set_page_dirty(dentry_page);
@@ -578,6 +581,7 @@ fail:
 		update_inode_page(dir);
 		clear_inode_flag(F2FS_I(dir), FI_UPDATE_DIR);
 	}
+	flush_dcache_page(dentry_page);
 	kunmap(dentry_page);
 	f2fs_put_page(dentry_page, 1);
 	return err;
@@ -660,6 +664,7 @@ void f2fs_delete_entry(struct f2fs_dir_entry *dentry, struct page *page,
 	bit_pos = find_next_bit_le(&dentry_blk->dentry_bitmap,
 			NR_DENTRY_IN_BLOCK,
 			0);
+	flush_dcache_page(page);
 	kunmap(page); /* kunmap - pair of f2fs_find_entry */
 	set_page_dirty(page);
 
diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
index f26fb87..4291c1f 100644
--- a/fs/f2fs/inline.c
+++ b/fs/f2fs/inline.c
@@ -106,6 +106,7 @@ int f2fs_convert_inline_page(struct dnode_of_data *dn, struct page *page)
 	src_addr = inline_data_addr(dn->inode_page);
 	dst_addr = kmap_atomic(page);
 	memcpy(dst_addr, src_addr, MAX_INLINE_DATA);
+	flush_dcache_page(page);
 	kunmap_atomic(dst_addr);
 	SetPageUptodate(page);
 no_update:
@@ -357,6 +358,7 @@ static int f2fs_convert_inline_dir(struct inode *dir, struct page *ipage,
 	memcpy(dentry_blk->filename, inline_dentry->filename,
 					NR_INLINE_DENTRY * F2FS_SLOT_LEN);
 
+	flush_dcache_page(page);
 	kunmap_atomic(dentry_blk);
 	SetPageUptodate(page);
 	set_page_dirty(page);
-- 
2.1.1

Re: [f2fs-dev] [PATCH 1/3] f2fs: call flush_dcache_page when the page was updated

[Changman Lee]

On Wed, Nov 19, 2014 at 10:45:33PM -0800, Jaegeuk Kim wrote:
> On Thu, Nov 20, 2014 at 03:04:10PM +0900, Changman Lee wrote:
> > Hi Jaegeuk,
> > 
> > We should call flush_dcache_page before kunmap because the purpose of the cache flush is to address aliasing problem related to virtual address.
> 
> Oh, I just followed zero_user_segments below.
> 
> static inline void zero_user_segments(struct page *page,
> 	unsigned start1, unsigned end1,
> 	unsigned start2, unsigned end2)
> {
> 	void *kaddr = kmap_atomic(page);
> 
> 	BUG_ON(end1 > PAGE_SIZE || end2 > PAGE_SIZE);
> 
> 	if (end1 > start1)
> 		memset(kaddr + start1, 0, end1 - start1);
> 
> 	if (end2 > start2)
> 		memset(kaddr + start2, 0, end2 - start2);
> 
> 	kunmap_atomic(kaddr);
> 	flush_dcache_page(page);
> }
> 
> Is this a wrong reference? Or, a bug?
> 

Well.. Data in cache only have to be flushed until before other users read the data.
If so, it's not a bug.

> Anyway I modified as below.
> 
> Thanks,
> 
> >From 7cb7b27c8cd2efc8a31d79239bef5b41c6e79216 Mon Sep 17 00:00:00 2001
> From: Jaegeuk Kim <jaegeuk@kernel.org>
> Date: Tue, 18 Nov 2014 10:50:21 -0800
> Subject: [PATCH] f2fs: call flush_dcache_page when the page was updated
> 
> Whenever f2fs updates mapped pages, it needs to call flush_dcache_page.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
>  fs/f2fs/dir.c    | 7 ++++++-
>  fs/f2fs/inline.c | 2 ++
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
> index 5a49995..fabf4ee 100644
> --- a/fs/f2fs/dir.c
> +++ b/fs/f2fs/dir.c
> @@ -287,8 +287,10 @@ void f2fs_set_link(struct inode *dir, struct f2fs_dir_entry *de,
>  	f2fs_wait_on_page_writeback(page, type);
>  	de->ino = cpu_to_le32(inode->i_ino);
>  	set_de_type(de, inode);
> -	if (!f2fs_has_inline_dentry(dir))
> +	if (!f2fs_has_inline_dentry(dir)) {
> +		flush_dcache_page(page);
>  		kunmap(page);
> +	}
>  	set_page_dirty(page);
>  	dir->i_mtime = dir->i_ctime = CURRENT_TIME;
>  	mark_inode_dirty(dir);
> @@ -365,6 +367,7 @@ static int make_empty_dir(struct inode *inode,
>  	make_dentry_ptr(&d, (void *)dentry_blk, 1);
>  	do_make_empty_dir(inode, parent, &d);
>  
> +	flush_dcache_page(dentry_page);
>  	kunmap_atomic(dentry_blk);
>  
>  	set_page_dirty(dentry_page);
> @@ -578,6 +581,7 @@ fail:
>  		update_inode_page(dir);
>  		clear_inode_flag(F2FS_I(dir), FI_UPDATE_DIR);
>  	}
> +	flush_dcache_page(dentry_page);
>  	kunmap(dentry_page);
>  	f2fs_put_page(dentry_page, 1);
>  	return err;
> @@ -660,6 +664,7 @@ void f2fs_delete_entry(struct f2fs_dir_entry *dentry, struct page *page,
>  	bit_pos = find_next_bit_le(&dentry_blk->dentry_bitmap,
>  			NR_DENTRY_IN_BLOCK,
>  			0);
> +	flush_dcache_page(page);
>  	kunmap(page); /* kunmap - pair of f2fs_find_entry */
>  	set_page_dirty(page);
>  
> diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
> index f26fb87..4291c1f 100644
> --- a/fs/f2fs/inline.c
> +++ b/fs/f2fs/inline.c
> @@ -106,6 +106,7 @@ int f2fs_convert_inline_page(struct dnode_of_data *dn, struct page *page)
>  	src_addr = inline_data_addr(dn->inode_page);
>  	dst_addr = kmap_atomic(page);
>  	memcpy(dst_addr, src_addr, MAX_INLINE_DATA);
> +	flush_dcache_page(page);
>  	kunmap_atomic(dst_addr);
>  	SetPageUptodate(page);
>  no_update:
> @@ -357,6 +358,7 @@ static int f2fs_convert_inline_dir(struct inode *dir, struct page *ipage,
>  	memcpy(dentry_blk->filename, inline_dentry->filename,
>  					NR_INLINE_DENTRY * F2FS_SLOT_LEN);
>  
> +	flush_dcache_page(page);
>  	kunmap_atomic(dentry_blk);
>  	SetPageUptodate(page);
>  	set_page_dirty(page);
> -- 
> 2.1.1
> 

Re: [f2fs-dev] [PATCH 1/3] f2fs: call flush_dcache_page when the page was updated

[Simon Baatz]

Hi Changman, Jaegeuk,

On Thu, Nov 20, 2014 at 05:47:29PM +0900, Changman Lee wrote:
> On Wed, Nov 19, 2014 at 10:45:33PM -0800, Jaegeuk Kim wrote:
> > On Thu, Nov 20, 2014 at 03:04:10PM +0900, Changman Lee wrote:
> > > Hi Jaegeuk,
> > > 
> > > We should call flush_dcache_page before kunmap because the purpose of the cache flush is to address aliasing problem related to virtual address.
> > 
> > Oh, I just followed zero_user_segments below.
> > 
> > static inline void zero_user_segments(struct page *page,
> > 	unsigned start1, unsigned end1,
> > 	unsigned start2, unsigned end2)
> > {
> > 	void *kaddr = kmap_atomic(page);
> > 
> > 	BUG_ON(end1 > PAGE_SIZE || end2 > PAGE_SIZE);
> > 
> > 	if (end1 > start1)
> > 		memset(kaddr + start1, 0, end1 - start1);
> > 
> > 	if (end2 > start2)
> > 		memset(kaddr + start2, 0, end2 - start2);
> > 
> > 	kunmap_atomic(kaddr);
> > 	flush_dcache_page(page);
> > }
> > 
> > Is this a wrong reference? Or, a bug?
> > 
> 
> Well.. Data in cache only have to be flushed until before other users read the data.
> If so, it's not a bug.
> 

Yes, it is not a bug, since flush_dcache_page() needs to be able to
deal with non-kmapped pages. However, this may create overhead in
some situations.

According to documentation (see Documentation/cachetlb.txt), this is
a use for flush_kernel_dcache_page(), since the page has been
modified by the kernel only.  In contrast to flush_dcache_page(),
this function must be called before kunmap().

flush_kernel_dcache_page() does not need to flush the user space
aliases.  Additionally, at least on ARM, it does not flush at all
when called within kmap_atomic()/kunmap_atomic(), when
kunmap_atomic() is going to flush the page anyway.  (I know that
almost no one uses flush_kernel_dcache_page() (probably because
almost no one knows when to use which of the two functions), but it
may save a few cache flushes on architectures which are affected by
aliasing)


> > Anyway I modified as below.
> > 
> > Thanks,
> > 
> > >From 7cb7b27c8cd2efc8a31d79239bef5b41c6e79216 Mon Sep 17 00:00:00 2001
> > From: Jaegeuk Kim <jaegeuk@kernel.org>
> > Date: Tue, 18 Nov 2014 10:50:21 -0800
> > Subject: [PATCH] f2fs: call flush_dcache_page when the page was updated
> > 
> > Whenever f2fs updates mapped pages, it needs to call flush_dcache_page.
> > 
> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > ---
> >  fs/f2fs/dir.c    | 7 ++++++-
> >  fs/f2fs/inline.c | 2 ++
> >  2 files changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
> > index 5a49995..fabf4ee 100644
> > --- a/fs/f2fs/dir.c
> > +++ b/fs/f2fs/dir.c
> > @@ -287,8 +287,10 @@ void f2fs_set_link(struct inode *dir, struct f2fs_dir_entry *de,
> >  	f2fs_wait_on_page_writeback(page, type);
> >  	de->ino = cpu_to_le32(inode->i_ino);
> >  	set_de_type(de, inode);
> > -	if (!f2fs_has_inline_dentry(dir))
> > +	if (!f2fs_has_inline_dentry(dir)) {
> > +		flush_dcache_page(page);
> >  		kunmap(page);
> > +	}

Is this a page that may be mapped into user space? (I may be
completely wrong here, since I have no idea how this code works.  But
it looks like as if the answer is "no" ;-) ).

It is not necessary to flush pages that cannot be seen by user space
(see also the NOTE in the documentation of flush_dcache_page() in
cachetlb.txt). Thus, if you know that a page will not be mapped into
user space, please don't create the overhead of flushing it.


- Simon

Re: [PATCH 1/3] f2fs: call flush_dcache_page when the page was updated

[Changman Lee]

Hi Simon,
Thanks for your explanation kindly.

On Sun, Nov 23, 2014 at 11:08:54AM +0100, Simon Baatz wrote:
> Hi Changman, Jaegeuk,
> 
> On Thu, Nov 20, 2014 at 05:47:29PM +0900, Changman Lee wrote:
> > On Wed, Nov 19, 2014 at 10:45:33PM -0800, Jaegeuk Kim wrote:
> > > On Thu, Nov 20, 2014 at 03:04:10PM +0900, Changman Lee wrote:
> > > > Hi Jaegeuk,
> > > > 
> > > > We should call flush_dcache_page before kunmap because the purpose of the cache flush is to address aliasing problem related to virtual address.
> > > 
> > > Oh, I just followed zero_user_segments below.
> > > 
> > > static inline void zero_user_segments(struct page *page,
> > > 	unsigned start1, unsigned end1,
> > > 	unsigned start2, unsigned end2)
> > > {
> > > 	void *kaddr = kmap_atomic(page);
> > > 
> > > 	BUG_ON(end1 > PAGE_SIZE || end2 > PAGE_SIZE);
> > > 
> > > 	if (end1 > start1)
> > > 		memset(kaddr + start1, 0, end1 - start1);
> > > 
> > > 	if (end2 > start2)
> > > 		memset(kaddr + start2, 0, end2 - start2);
> > > 
> > > 	kunmap_atomic(kaddr);
> > > 	flush_dcache_page(page);
> > > }
> > > 
> > > Is this a wrong reference? Or, a bug?
> > > 
> > 
> > Well.. Data in cache only have to be flushed until before other users read the data.
> > If so, it's not a bug.
> > 
> 
> Yes, it is not a bug, since flush_dcache_page() needs to be able to
> deal with non-kmapped pages. However, this may create overhead in
> some situations.
> 

Previously, I was vague but I thought that it should be different
according to vaddr exists or not. So I told jaegeuk that it should
be better to change an order between flush_dache_page and kunmap.
But actually, it doesn't matter the order between them except
the situation you said.
Could you explain the situation that makes overhead by flushing after kummap.
I can't imagine it by just seeing flush_dcache_page code.

> According to documentation (see Documentation/cachetlb.txt), this is
> a use for flush_kernel_dcache_page(), since the page has been
> modified by the kernel only.  In contrast to flush_dcache_page(),
> this function must be called before kunmap().
> 
> flush_kernel_dcache_page() does not need to flush the user space
> aliases.  Additionally, at least on ARM, it does not flush at all
> when called within kmap_atomic()/kunmap_atomic(), when
> kunmap_atomic() is going to flush the page anyway.  (I know that
> almost no one uses flush_kernel_dcache_page() (probably because
> almost no one knows when to use which of the two functions), but it
> may save a few cache flushes on architectures which are affected by
> aliasing)
> 
> 
> > > Anyway I modified as below.
> > > 
> > > Thanks,
> > > 
> > > >From 7cb7b27c8cd2efc8a31d79239bef5b41c6e79216 Mon Sep 17 00:00:00 2001
> > > From: Jaegeuk Kim <jaegeuk@kernel.org>
> > > Date: Tue, 18 Nov 2014 10:50:21 -0800
> > > Subject: [PATCH] f2fs: call flush_dcache_page when the page was updated
> > > 
> > > Whenever f2fs updates mapped pages, it needs to call flush_dcache_page.
> > > 
> > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > > ---
> > >  fs/f2fs/dir.c    | 7 ++++++-
> > >  fs/f2fs/inline.c | 2 ++
> > >  2 files changed, 8 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
> > > index 5a49995..fabf4ee 100644
> > > --- a/fs/f2fs/dir.c
> > > +++ b/fs/f2fs/dir.c
> > > @@ -287,8 +287,10 @@ void f2fs_set_link(struct inode *dir, struct f2fs_dir_entry *de,
> > >  	f2fs_wait_on_page_writeback(page, type);
> > >  	de->ino = cpu_to_le32(inode->i_ino);
> > >  	set_de_type(de, inode);
> > > -	if (!f2fs_has_inline_dentry(dir))
> > > +	if (!f2fs_has_inline_dentry(dir)) {
> > > +		flush_dcache_page(page);
> > >  		kunmap(page);
> > > +	}
> 
> Is this a page that may be mapped into user space? (I may be
> completely wrong here, since I have no idea how this code works.  But
> it looks like as if the answer is "no" ;-) ).
> 
> It is not necessary to flush pages that cannot be seen by user space
> (see also the NOTE in the documentation of flush_dcache_page() in
> cachetlb.txt). Thus, if you know that a page will not be mapped into
> user space, please don't create the overhead of flushing it.
> 

In the case of dentry unlike inline data, this is not mapped to user space, so dcache flush
makes overhead. Do you mean that?

Best regard,
Changman

> 
> - Simon

------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk

Re: [f2fs-dev] [PATCH 1/3] f2fs: call flush_dcache_page when the page was updated

[Jaegeuk Kim]

On Mon, Nov 24, 2014 at 11:46:46AM +0900, Changman Lee wrote:
> Hi Simon,
> Thanks for your explanation kindly.
> 
> On Sun, Nov 23, 2014 at 11:08:54AM +0100, Simon Baatz wrote:
> > Hi Changman, Jaegeuk,
> > 
> > On Thu, Nov 20, 2014 at 05:47:29PM +0900, Changman Lee wrote:
> > > On Wed, Nov 19, 2014 at 10:45:33PM -0800, Jaegeuk Kim wrote:
> > > > On Thu, Nov 20, 2014 at 03:04:10PM +0900, Changman Lee wrote:
> > > > > Hi Jaegeuk,
> > > > > 
> > > > > We should call flush_dcache_page before kunmap because the purpose of the cache flush is to address aliasing problem related to virtual address.
> > > > 
> > > > Oh, I just followed zero_user_segments below.
> > > > 
> > > > static inline void zero_user_segments(struct page *page,
> > > > 	unsigned start1, unsigned end1,
> > > > 	unsigned start2, unsigned end2)
> > > > {
> > > > 	void *kaddr = kmap_atomic(page);
> > > > 
> > > > 	BUG_ON(end1 > PAGE_SIZE || end2 > PAGE_SIZE);
> > > > 
> > > > 	if (end1 > start1)
> > > > 		memset(kaddr + start1, 0, end1 - start1);
> > > > 
> > > > 	if (end2 > start2)
> > > > 		memset(kaddr + start2, 0, end2 - start2);
> > > > 
> > > > 	kunmap_atomic(kaddr);
> > > > 	flush_dcache_page(page);
> > > > }
> > > > 
> > > > Is this a wrong reference? Or, a bug?
> > > > 
> > > 
> > > Well.. Data in cache only have to be flushed until before other users read the data.
> > > If so, it's not a bug.
> > > 
> > 
> > Yes, it is not a bug, since flush_dcache_page() needs to be able to
> > deal with non-kmapped pages. However, this may create overhead in
> > some situations.
> > 
> 
> Previously, I was vague but I thought that it should be different
> according to vaddr exists or not. So I told jaegeuk that it should
> be better to change an order between flush_dache_page and kunmap.
> But actually, it doesn't matter the order between them except
> the situation you said.
> Could you explain the situation that makes overhead by flushing after kummap.
> I can't imagine it by just seeing flush_dcache_page code.
> 
> > According to documentation (see Documentation/cachetlb.txt), this is
> > a use for flush_kernel_dcache_page(), since the page has been
> > modified by the kernel only.  In contrast to flush_dcache_page(),
> > this function must be called before kunmap().
> > 
> > flush_kernel_dcache_page() does not need to flush the user space
> > aliases.  Additionally, at least on ARM, it does not flush at all
> > when called within kmap_atomic()/kunmap_atomic(), when
> > kunmap_atomic() is going to flush the page anyway.  (I know that
> > almost no one uses flush_kernel_dcache_page() (probably because
> > almost no one knows when to use which of the two functions), but it
> > may save a few cache flushes on architectures which are affected by
> > aliasing)
> > 
> > 
> > > > Anyway I modified as below.
> > > > 
> > > > Thanks,
> > > > 
> > > > >From 7cb7b27c8cd2efc8a31d79239bef5b41c6e79216 Mon Sep 17 00:00:00 2001
> > > > From: Jaegeuk Kim <jaegeuk@kernel.org>
> > > > Date: Tue, 18 Nov 2014 10:50:21 -0800
> > > > Subject: [PATCH] f2fs: call flush_dcache_page when the page was updated
> > > > 
> > > > Whenever f2fs updates mapped pages, it needs to call flush_dcache_page.
> > > > 
> > > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > > > ---
> > > >  fs/f2fs/dir.c    | 7 ++++++-
> > > >  fs/f2fs/inline.c | 2 ++
> > > >  2 files changed, 8 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
> > > > index 5a49995..fabf4ee 100644
> > > > --- a/fs/f2fs/dir.c
> > > > +++ b/fs/f2fs/dir.c
> > > > @@ -287,8 +287,10 @@ void f2fs_set_link(struct inode *dir, struct f2fs_dir_entry *de,
> > > >  	f2fs_wait_on_page_writeback(page, type);
> > > >  	de->ino = cpu_to_le32(inode->i_ino);
> > > >  	set_de_type(de, inode);
> > > > -	if (!f2fs_has_inline_dentry(dir))
> > > > +	if (!f2fs_has_inline_dentry(dir)) {
> > > > +		flush_dcache_page(page);
> > > >  		kunmap(page);
> > > > +	}
> > 
> > Is this a page that may be mapped into user space? (I may be
> > completely wrong here, since I have no idea how this code works.  But
> > it looks like as if the answer is "no" ;-) ).
> > 
> > It is not necessary to flush pages that cannot be seen by user space
> > (see also the NOTE in the documentation of flush_dcache_page() in
> > cachetlb.txt). Thus, if you know that a page will not be mapped into
> > user space, please don't create the overhead of flushing it.
> > 
> 
> In the case of dentry unlike inline data, this is not mapped to user space, so dcache flush
> makes overhead. Do you mean that?
> 
> Best regard,
> Changman

Hi Changman,

This would be ok though.

>From 5c1c46d31452a59a37c96f6dcae600776a8002e3 Mon Sep 17 00:00:00 2001
From: Jaegeuk Kim <jaegeuk@kernel.org>
Date: Tue, 18 Nov 2014 10:50:21 -0800
Subject: [PATCH v2] f2fs: call flush_dcache_page when the page was updated

Change log from v1:
 o remove flush_dcache_page calls for dentry pages.

Whenever f2fs updates mapped pages, it needs to call flush_dcache_page.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/inline.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
index f26fb87..914b6d3 100644
--- a/fs/f2fs/inline.c
+++ b/fs/f2fs/inline.c
@@ -106,6 +106,7 @@ int f2fs_convert_inline_page(struct dnode_of_data *dn, struct page *page)
 	src_addr = inline_data_addr(dn->inode_page);
 	dst_addr = kmap_atomic(page);
 	memcpy(dst_addr, src_addr, MAX_INLINE_DATA);
+	flush_dcache_page(page);
 	kunmap_atomic(dst_addr);
 	SetPageUptodate(page);
 no_update:
-- 
2.1.1

Re: [PATCH 1/3] f2fs: call flush_dcache_page when the page was updated

[Simon Baatz]

Hi Changman,

On Mon, Nov 24, 2014 at 11:46:46AM +0900, Changman Lee wrote:
> Hi Simon,
> Thanks for your explanation kindly.
> 
> On Sun, Nov 23, 2014 at 11:08:54AM +0100, Simon Baatz wrote:
> > Hi Changman, Jaegeuk,
> > 
> > On Thu, Nov 20, 2014 at 05:47:29PM +0900, Changman Lee wrote:
> > > On Wed, Nov 19, 2014 at 10:45:33PM -0800, Jaegeuk Kim wrote:
> > > > On Thu, Nov 20, 2014 at 03:04:10PM +0900, Changman Lee wrote:
> > > > > Hi Jaegeuk,
> > > > > 
> > > > > We should call flush_dcache_page before kunmap because the purpose of the cache flush is to address aliasing problem related to virtual address.
> > > > 
> > > > Oh, I just followed zero_user_segments below.
> > > > 
> > > > static inline void zero_user_segments(struct page *page,
> > > > 	unsigned start1, unsigned end1,
> > > > 	unsigned start2, unsigned end2)
> > > > {
> > > > 	void *kaddr = kmap_atomic(page);
> > > > 
> > > > 	BUG_ON(end1 > PAGE_SIZE || end2 > PAGE_SIZE);
> > > > 
> > > > 	if (end1 > start1)
> > > > 		memset(kaddr + start1, 0, end1 - start1);
> > > > 
> > > > 	if (end2 > start2)
> > > > 		memset(kaddr + start2, 0, end2 - start2);
> > > > 
> > > > 	kunmap_atomic(kaddr);
> > > > 	flush_dcache_page(page);
> > > > }
> > > > 
> > > > Is this a wrong reference? Or, a bug?
> > > > 
> > > 
> > > Well.. Data in cache only have to be flushed until before other users read the data.
> > > If so, it's not a bug.
> > > 
> > 
> > Yes, it is not a bug, since flush_dcache_page() needs to be able to
> > deal with non-kmapped pages. However, this may create overhead in
> > some situations.
> > 
> 
> Previously, I was vague but I thought that it should be different
> according to vaddr exists or not. So I told jaegeuk that it should
> be better to change an order between flush_dache_page and kunmap.
> But actually, it doesn't matter the order between them except
> the situation you said.
> Could you explain the situation that makes overhead by flushing after kummap.
> I can't imagine it by just seeing flush_dcache_page code.
> 

I was a not very precise here. Yes, flush_dcache_page() on ARM does
the same in both situations since it has no idea whether it is called
before or after kunmap.  However, flush_kernel_dcache_page() can
assume that it is called before kunmap and thus, for example, does not
need to pin a highmem page by kmap_high_get() (apart from not having
to care about flushing user space mappings)

> > According to documentation (see Documentation/cachetlb.txt), this is
> > a use for flush_kernel_dcache_page(), since the page has been
> > modified by the kernel only.  In contrast to flush_dcache_page(),
> > this function must be called before kunmap().
> > 
> > flush_kernel_dcache_page() does not need to flush the user space
> > aliases.  Additionally, at least on ARM, it does not flush at all
> > when called within kmap_atomic()/kunmap_atomic(), when
> > kunmap_atomic() is going to flush the page anyway.  (I know that
> > almost no one uses flush_kernel_dcache_page() (probably because
> > almost no one knows when to use which of the two functions), but it
> > may save a few cache flushes on architectures which are affected by
> > aliasing)
> > 
> > 
> > > > Anyway I modified as below.
> > > > 
> > > > Thanks,
> > > > 
> > > > >From 7cb7b27c8cd2efc8a31d79239bef5b41c6e79216 Mon Sep 17 00:00:00 2001
> > > > From: Jaegeuk Kim <jaegeuk@kernel.org>
> > > > Date: Tue, 18 Nov 2014 10:50:21 -0800
> > > > Subject: [PATCH] f2fs: call flush_dcache_page when the page was updated
> > > > 
> > > > Whenever f2fs updates mapped pages, it needs to call flush_dcache_page.
> > > > 
> > > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > > > ---
> > > >  fs/f2fs/dir.c    | 7 ++++++-
> > > >  fs/f2fs/inline.c | 2 ++
> > > >  2 files changed, 8 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
> > > > index 5a49995..fabf4ee 100644
> > > > --- a/fs/f2fs/dir.c
> > > > +++ b/fs/f2fs/dir.c
> > > > @@ -287,8 +287,10 @@ void f2fs_set_link(struct inode *dir, struct f2fs_dir_entry *de,
> > > >  	f2fs_wait_on_page_writeback(page, type);
> > > >  	de->ino = cpu_to_le32(inode->i_ino);
> > > >  	set_de_type(de, inode);
> > > > -	if (!f2fs_has_inline_dentry(dir))
> > > > +	if (!f2fs_has_inline_dentry(dir)) {
> > > > +		flush_dcache_page(page);
> > > >  		kunmap(page);
> > > > +	}
> > 
> > Is this a page that may be mapped into user space? (I may be
> > completely wrong here, since I have no idea how this code works.  But
> > it looks like as if the answer is "no" ;-) ).
> > 
> > It is not necessary to flush pages that cannot be seen by user space
> > (see also the NOTE in the documentation of flush_dcache_page() in
> > cachetlb.txt). Thus, if you know that a page will not be mapped into
> > user space, please don't create the overhead of flushing it.
> > 
> 
> In the case of dentry unlike inline data, this is not mapped to user space, so dcache flush
> makes overhead. Do you mean that?

Yes. I suppose most architectures where D-cache aliasing is an issue
have optimizations that "defer" the actual flush when there are no
user space mappings.  But nevertheless, if you already now that there
can't be any aliases, there is no need to call these functions at
all.


- Simon 

------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk

Re: [f2fs-dev] [PATCH 1/3] f2fs: call flush_dcache_page when the page was updated

[Changman Lee]

Hi Simon,

Thanks very much for your interest.
It becomes more clear due to your explanation.

Regards,
Changman

On Tue, Nov 25, 2014 at 08:05:23PM +0100, Simon Baatz wrote:
> Hi Changman,
> 
> On Mon, Nov 24, 2014 at 11:46:46AM +0900, Changman Lee wrote:
> > Hi Simon,
> > Thanks for your explanation kindly.
> > 
> > On Sun, Nov 23, 2014 at 11:08:54AM +0100, Simon Baatz wrote:
> > > Hi Changman, Jaegeuk,
> > > 
> > > On Thu, Nov 20, 2014 at 05:47:29PM +0900, Changman Lee wrote:
> > > > On Wed, Nov 19, 2014 at 10:45:33PM -0800, Jaegeuk Kim wrote:
> > > > > On Thu, Nov 20, 2014 at 03:04:10PM +0900, Changman Lee wrote:
> > > > > > Hi Jaegeuk,
> > > > > > 
> > > > > > We should call flush_dcache_page before kunmap because the purpose of the cache flush is to address aliasing problem related to virtual address.
> > > > > 
> > > > > Oh, I just followed zero_user_segments below.
> > > > > 
> > > > > static inline void zero_user_segments(struct page *page,
> > > > > 	unsigned start1, unsigned end1,
> > > > > 	unsigned start2, unsigned end2)
> > > > > {
> > > > > 	void *kaddr = kmap_atomic(page);
> > > > > 
> > > > > 	BUG_ON(end1 > PAGE_SIZE || end2 > PAGE_SIZE);
> > > > > 
> > > > > 	if (end1 > start1)
> > > > > 		memset(kaddr + start1, 0, end1 - start1);
> > > > > 
> > > > > 	if (end2 > start2)
> > > > > 		memset(kaddr + start2, 0, end2 - start2);
> > > > > 
> > > > > 	kunmap_atomic(kaddr);
> > > > > 	flush_dcache_page(page);
> > > > > }
> > > > > 
> > > > > Is this a wrong reference? Or, a bug?
> > > > > 
> > > > 
> > > > Well.. Data in cache only have to be flushed until before other users read the data.
> > > > If so, it's not a bug.
> > > > 
> > > 
> > > Yes, it is not a bug, since flush_dcache_page() needs to be able to
> > > deal with non-kmapped pages. However, this may create overhead in
> > > some situations.
> > > 
> > 
> > Previously, I was vague but I thought that it should be different
> > according to vaddr exists or not. So I told jaegeuk that it should
> > be better to change an order between flush_dache_page and kunmap.
> > But actually, it doesn't matter the order between them except
> > the situation you said.
> > Could you explain the situation that makes overhead by flushing after kummap.
> > I can't imagine it by just seeing flush_dcache_page code.
> > 
> 
> I was a not very precise here. Yes, flush_dcache_page() on ARM does
> the same in both situations since it has no idea whether it is called
> before or after kunmap.  However, flush_kernel_dcache_page() can
> assume that it is called before kunmap and thus, for example, does not
> need to pin a highmem page by kmap_high_get() (apart from not having
> to care about flushing user space mappings)
> 
> > > According to documentation (see Documentation/cachetlb.txt), this is
> > > a use for flush_kernel_dcache_page(), since the page has been
> > > modified by the kernel only.  In contrast to flush_dcache_page(),
> > > this function must be called before kunmap().
> > > 
> > > flush_kernel_dcache_page() does not need to flush the user space
> > > aliases.  Additionally, at least on ARM, it does not flush at all
> > > when called within kmap_atomic()/kunmap_atomic(), when
> > > kunmap_atomic() is going to flush the page anyway.  (I know that
> > > almost no one uses flush_kernel_dcache_page() (probably because
> > > almost no one knows when to use which of the two functions), but it
> > > may save a few cache flushes on architectures which are affected by
> > > aliasing)
> > > 
> > > 
> > > > > Anyway I modified as below.
> > > > > 
> > > > > Thanks,
> > > > > 
> > > > > >From 7cb7b27c8cd2efc8a31d79239bef5b41c6e79216 Mon Sep 17 00:00:00 2001
> > > > > From: Jaegeuk Kim <jaegeuk@kernel.org>
> > > > > Date: Tue, 18 Nov 2014 10:50:21 -0800
> > > > > Subject: [PATCH] f2fs: call flush_dcache_page when the page was updated
> > > > > 
> > > > > Whenever f2fs updates mapped pages, it needs to call flush_dcache_page.
> > > > > 
> > > > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > > > > ---
> > > > >  fs/f2fs/dir.c    | 7 ++++++-
> > > > >  fs/f2fs/inline.c | 2 ++
> > > > >  2 files changed, 8 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
> > > > > index 5a49995..fabf4ee 100644
> > > > > --- a/fs/f2fs/dir.c
> > > > > +++ b/fs/f2fs/dir.c
> > > > > @@ -287,8 +287,10 @@ void f2fs_set_link(struct inode *dir, struct f2fs_dir_entry *de,
> > > > >  	f2fs_wait_on_page_writeback(page, type);
> > > > >  	de->ino = cpu_to_le32(inode->i_ino);
> > > > >  	set_de_type(de, inode);
> > > > > -	if (!f2fs_has_inline_dentry(dir))
> > > > > +	if (!f2fs_has_inline_dentry(dir)) {
> > > > > +		flush_dcache_page(page);
> > > > >  		kunmap(page);
> > > > > +	}
> > > 
> > > Is this a page that may be mapped into user space? (I may be
> > > completely wrong here, since I have no idea how this code works.  But
> > > it looks like as if the answer is "no" ;-) ).
> > > 
> > > It is not necessary to flush pages that cannot be seen by user space
> > > (see also the NOTE in the documentation of flush_dcache_page() in
> > > cachetlb.txt). Thus, if you know that a page will not be mapped into
> > > user space, please don't create the overhead of flushing it.
> > > 
> > 
> > In the case of dentry unlike inline data, this is not mapped to user space, so dcache flush
> > makes overhead. Do you mean that?
> 
> Yes. I suppose most architectures where D-cache aliasing is an issue
> have optimizations that "defer" the actual flush when there are no
> user space mappings.  But nevertheless, if you already now that there
> can't be any aliases, there is no need to call these functions at
> all.
> 
> 
> - Simon 

Re: [f2fs-dev] [PATCH 1/3] f2fs: call flush_dcache_page when the page was updated

[Jaegeuk Kim]

On Sun, Nov 23, 2014 at 11:08:54AM +0100, Simon Baatz wrote:
> Hi Changman, Jaegeuk,

Hi Simon,

> 
> On Thu, Nov 20, 2014 at 05:47:29PM +0900, Changman Lee wrote:
> > On Wed, Nov 19, 2014 at 10:45:33PM -0800, Jaegeuk Kim wrote:
> > > On Thu, Nov 20, 2014 at 03:04:10PM +0900, Changman Lee wrote:
> > > > Hi Jaegeuk,
> > > > 
> > > > We should call flush_dcache_page before kunmap because the purpose of the cache flush is to address aliasing problem related to virtual address.
> > > 
> > > Oh, I just followed zero_user_segments below.
> > > 
> > > static inline void zero_user_segments(struct page *page,
> > > 	unsigned start1, unsigned end1,
> > > 	unsigned start2, unsigned end2)
> > > {
> > > 	void *kaddr = kmap_atomic(page);
> > > 
> > > 	BUG_ON(end1 > PAGE_SIZE || end2 > PAGE_SIZE);
> > > 
> > > 	if (end1 > start1)
> > > 		memset(kaddr + start1, 0, end1 - start1);
> > > 
> > > 	if (end2 > start2)
> > > 		memset(kaddr + start2, 0, end2 - start2);
> > > 
> > > 	kunmap_atomic(kaddr);
> > > 	flush_dcache_page(page);
> > > }
> > > 
> > > Is this a wrong reference? Or, a bug?
> > > 
> > 
> > Well.. Data in cache only have to be flushed until before other users read the data.
> > If so, it's not a bug.
> > 
> 
> Yes, it is not a bug, since flush_dcache_page() needs to be able to
> deal with non-kmapped pages. However, this may create overhead in
> some situations.

Ok.

> 
> According to documentation (see Documentation/cachetlb.txt), this is
> a use for flush_kernel_dcache_page(), since the page has been
> modified by the kernel only.  In contrast to flush_dcache_page(),
> this function must be called before kunmap().
> 
> flush_kernel_dcache_page() does not need to flush the user space
> aliases.  Additionally, at least on ARM, it does not flush at all
> when called within kmap_atomic()/kunmap_atomic(), when
> kunmap_atomic() is going to flush the page anyway.  (I know that
> almost no one uses flush_kernel_dcache_page() (probably because
> almost no one knows when to use which of the two functions), but it
> may save a few cache flushes on architectures which are affected by
> aliasing)

Thank you very much for the explanation. :)

> 
> 
> > > Anyway I modified as below.
> > > 
> > > Thanks,
> > > 
> > > >From 7cb7b27c8cd2efc8a31d79239bef5b41c6e79216 Mon Sep 17 00:00:00 2001
> > > From: Jaegeuk Kim <jaegeuk@kernel.org>
> > > Date: Tue, 18 Nov 2014 10:50:21 -0800
> > > Subject: [PATCH] f2fs: call flush_dcache_page when the page was updated
> > > 
> > > Whenever f2fs updates mapped pages, it needs to call flush_dcache_page.
> > > 
> > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > > ---
> > >  fs/f2fs/dir.c    | 7 ++++++-
> > >  fs/f2fs/inline.c | 2 ++
> > >  2 files changed, 8 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
> > > index 5a49995..fabf4ee 100644
> > > --- a/fs/f2fs/dir.c
> > > +++ b/fs/f2fs/dir.c
> > > @@ -287,8 +287,10 @@ void f2fs_set_link(struct inode *dir, struct f2fs_dir_entry *de,
> > >  	f2fs_wait_on_page_writeback(page, type);
> > >  	de->ino = cpu_to_le32(inode->i_ino);
> > >  	set_de_type(de, inode);
> > > -	if (!f2fs_has_inline_dentry(dir))
> > > +	if (!f2fs_has_inline_dentry(dir)) {
> > > +		flush_dcache_page(page);
> > >  		kunmap(page);
> > > +	}
> 
> Is this a page that may be mapped into user space? (I may be
> completely wrong here, since I have no idea how this code works.  But
> it looks like as if the answer is "no" ;-) ).
> 
> It is not necessary to flush pages that cannot be seen by user space
> (see also the NOTE in the documentation of flush_dcache_page() in
> cachetlb.txt). Thus, if you know that a page will not be mapped into
> user space, please don't create the overhead of flushing it.

Right, users do not modify directory entry pages, so I should remove flushing
them. The only thing that I have to do is our inlined data, which are file-
backed pages likely modified by users.

Thanks,

> 
> 
> - Simon