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

[Simon Baatz <gmbnomis@gmail.com>]

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