linux-next: manual merge of the cleancache tree with Linus' tree

linux-next: manual merge of the cleancache tree with Linus' tree

[Stephen Rothwell]

Hi Dan,

Today's linux-next merge of the cleancache tree got a conflict in
mm/truncate.c between commit 5adc7b518b54 ("mm: truncate: change
remove_from_page_cache") from Linus' tree and commit 03e838947c8a
("mm/fs: add hooks to support cleancache") from the cleancache tree.

I fixed it up (see below) but am really not sure of the fix.  I can carry
this fix as necessary.

Is this stuff going to be merged into Linus' tree this time round?
-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

diff --cc mm/truncate.c
index a956675,cd94607..0000000
--- a/mm/truncate.c
+++ b/mm/truncate.c
@@@ -106,8 -108,13 +108,12 @@@ truncate_complete_page(struct address_s
  	cancel_dirty_page(page, PAGE_CACHE_SIZE);
  
  	clear_page_mlock(page);
 -	remove_from_page_cache(page);
  	ClearPageMappedToDisk(page);
 +	delete_from_page_cache(page);
+ 	/* this must be after the remove_from_page_cache which
+ 	 * calls cleancache_put_page (and note page->mapping is now NULL)
+ 	 */
+ 	cleancache_flush_page(mapping, page);
 -	page_cache_release(page);	/* pagecache ref */
  	return 0;
  }
  

Re: linux-next: manual merge of the cleancache tree with Linus' tree

[Andrew Morton]

On Thu, 24 Mar 2011 13:55:24 +1100 Stephen Rothwell <sfr@canb.auug.org.au> wrote:

> Hi Dan,
> 
> Today's linux-next merge of the cleancache tree got a conflict in
> mm/truncate.c between commit 5adc7b518b54 ("mm: truncate: change
> remove_from_page_cache") from Linus' tree and commit 03e838947c8a
> ("mm/fs: add hooks to support cleancache") from the cleancache tree.
> 
> I fixed it up (see below) but am really not sure of the fix.  I can carry
> this fix as necessary.
> 
> Is this stuff going to be merged into Linus' tree this time round?
> -- 
> Cheers,
> Stephen Rothwell                    sfr@canb.auug.org.au
> 
> diff --cc mm/truncate.c
> index a956675,cd94607..0000000
> --- a/mm/truncate.c
> +++ b/mm/truncate.c
> @@@ -106,8 -108,13 +108,12 @@@ truncate_complete_page(struct address_s
>   	cancel_dirty_page(page, PAGE_CACHE_SIZE);
>   
>   	clear_page_mlock(page);
>  -	remove_from_page_cache(page);
>   	ClearPageMappedToDisk(page);
>  +	delete_from_page_cache(page);
> + 	/* this must be after the remove_from_page_cache which
> + 	 * calls cleancache_put_page (and note page->mapping is now NULL)
> + 	 */
> + 	cleancache_flush_page(mapping, page);
>  -	page_cache_release(page);	/* pagecache ref */
>   	return 0;
>   }

I did the cleancache_flush_page() before the delete_from_page_cache(),
in case the delete_from_page_cache() freed the page.  I didn't actually
check whether that makes sense though.

Re: linux-next: manual merge of the cleancache tree with Linus' tree

[Minchan Kim]

On Thu, Mar 24, 2011 at 12:56 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Thu, 24 Mar 2011 13:55:24 +1100 Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>
>> Hi Dan,
>>
>> Today's linux-next merge of the cleancache tree got a conflict in
>> mm/truncate.c between commit 5adc7b518b54 ("mm: truncate: change
>> remove_from_page_cache") from Linus' tree and commit 03e838947c8a
>> ("mm/fs: add hooks to support cleancache") from the cleancache tree.
>>
>> I fixed it up (see below) but am really not sure of the fix.  I can carry
>> this fix as necessary.
>>
>> Is this stuff going to be merged into Linus' tree this time round?
>> --
>> Cheers,
>> Stephen Rothwell                    sfr@canb.auug.org.au
>>
>> diff --cc mm/truncate.c
>> index a956675,cd94607..0000000
>> --- a/mm/truncate.c
>> +++ b/mm/truncate.c
>> @@@ -106,8 -108,13 +108,12 @@@ truncate_complete_page(struct address_s
>>       cancel_dirty_page(page, PAGE_CACHE_SIZE);
>>
>>       clear_page_mlock(page);
>>  -    remove_from_page_cache(page);
>>       ClearPageMappedToDisk(page);
>>  +    delete_from_page_cache(page);
>> +     /* this must be after the remove_from_page_cache which
>> +      * calls cleancache_put_page (and note page->mapping is now NULL)
>> +      */
>> +     cleancache_flush_page(mapping, page);
>>  -    page_cache_release(page);       /* pagecache ref */
>>       return 0;
>>   }
>
> I did the cleancache_flush_page() before the delete_from_page_cache(),
> in case the delete_from_page_cache() freed the page.  I didn't actually
> check whether that makes sense though.

I am not sure cleancache's put and flush semantic.
If I understand rightly with old __remove_from_page_cache's comment,
maybe cleancache_flush_page is to invalidate the page.(If I understand
right, I hope the name is changed to cleancache_invalidate_page)

"        /*
         * if we're uptodate, flush out into the cleancache, otherwise
         * invalidate any existing cleancache entries.  We can't leave
         * stale data around in the cleancache once our page is gone
         */
        if (PageUptodate(page))
                cleancache_put_page(page);
        else
                cleancache_flush_page(mapping, page); "

So I think cleancache_flush_page should be done after
delete_from_page_cache because delete_from_page_cache calls
cleancache_put_page(maybe this function would flush the content of
memory into cleancache's target) before we invalidates the page.

And it should not be a problem in case the delete_from_page_cache
freed the page since cleancache should have a reference the page but I
didn't check cleancahe always has a reference of page. If it isn't,
it's a critical problem.

Dan, Could you comment this?


-- 
Kind regards,
Minchan Kim

Re: linux-next: manual merge of the cleancache tree with Linus' tree

[Minchan Kim]

On Thu, Mar 24, 2011 at 2:38 PM, Minchan Kim <minchan.kim@gmail.com> wrote:
> On Thu, Mar 24, 2011 at 12:56 PM, Andrew Morton
> <akpm@linux-foundation.org> wrote:
>> On Thu, 24 Mar 2011 13:55:24 +1100 Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>>
>>> Hi Dan,
>>>
>>> Today's linux-next merge of the cleancache tree got a conflict in
>>> mm/truncate.c between commit 5adc7b518b54 ("mm: truncate: change
>>> remove_from_page_cache") from Linus' tree and commit 03e838947c8a
>>> ("mm/fs: add hooks to support cleancache") from the cleancache tree.
>>>
>>> I fixed it up (see below) but am really not sure of the fix.  I can carry
>>> this fix as necessary.
>>>
>>> Is this stuff going to be merged into Linus' tree this time round?
>>> --
>>> Cheers,
>>> Stephen Rothwell                    sfr@canb.auug.org.au
>>>
>>> diff --cc mm/truncate.c
>>> index a956675,cd94607..0000000
>>> --- a/mm/truncate.c
>>> +++ b/mm/truncate.c
>>> @@@ -106,8 -108,13 +108,12 @@@ truncate_complete_page(struct address_s
>>>       cancel_dirty_page(page, PAGE_CACHE_SIZE);
>>>
>>>       clear_page_mlock(page);
>>>  -    remove_from_page_cache(page);
>>>       ClearPageMappedToDisk(page);
>>>  +    delete_from_page_cache(page);
>>> +     /* this must be after the remove_from_page_cache which
>>> +      * calls cleancache_put_page (and note page->mapping is now NULL)
>>> +      */
>>> +     cleancache_flush_page(mapping, page);
>>>  -    page_cache_release(page);       /* pagecache ref */
>>>       return 0;
>>>   }
>>
>> I did the cleancache_flush_page() before the delete_from_page_cache(),
>> in case the delete_from_page_cache() freed the page.  I didn't actually
>> check whether that makes sense though.
>
> I am not sure cleancache's put and flush semantic.
> If I understand rightly with old __remove_from_page_cache's comment,
> maybe cleancache_flush_page is to invalidate the page.(If I understand
> right, I hope the name is changed to cleancache_invalidate_page)
>
> "        /*
>         * if we're uptodate, flush out into the cleancache, otherwise
>         * invalidate any existing cleancache entries.  We can't leave
>         * stale data around in the cleancache once our page is gone
>         */
>        if (PageUptodate(page))
>                cleancache_put_page(page);
>        else
>                cleancache_flush_page(mapping, page); "
>
> So I think cleancache_flush_page should be done after
> delete_from_page_cache because delete_from_page_cache calls
> cleancache_put_page(maybe this function would flush the content of
> memory into cleancache's target) before we invalidates the page.
>
> And it should not be a problem in case the delete_from_page_cache
> freed the page since cleancache should have a reference the page but I
> didn't check cleancahe always has a reference of page. If it isn't,
> it's a critical problem.
>
> Dan, Could you comment this?

Dan, one more thing.

#define cleancache_fs_enabled_mapping(_mapping) \
        (mapping->host->i_sb->cleancache_poolid >= 0)

One is "_mapping", another is  "mapping"


-- 
Kind regards,
Minchan Kim

Re: linux-next: manual merge of the cleancache tree with Linus' tree

[Andrew Morton]

On Thu, 24 Mar 2011 14:58:06 +0900 Minchan Kim <minchan.kim@gmail.com> wrote:

> Dan, one more thing.
> 
> #define cleancache_fs_enabled_mapping(_mapping) \
>         (mapping->host->i_sb->cleancache_poolid >= 0)
> 
> One is "_mapping", another is  "mapping"

It should be implemented in C too.  This is the case for almost all
"functions" which are implemented as macros and it's rather a mystery
why we keep on typing #define!

It is not only for cleanliness and for typechecking, but also because
constructs such as

	{
		struct address_space *foo = ...;
		if (cleancache_fs_enabled_mapping(foo))
			...;
	}

will generate an unused-var warning against `foo' if CONFIG_CLEANCACHE=n. 
Implementing the function in C fixes that.  With current gcc, anyway.		

RE: linux-next: manual merge of the cleancache tree with Linus' tree

[Dan Magenheimer]

> >>> Is this stuff going to be merged into Linus' tree this time round?

Hi Stephen --

Still TBD.  Some discussion has occurred offlist.

> >> I did the cleancache_flush_page() before the
> delete_from_page_cache(),
> >> in case the delete_from_page_cache() freed the page.  I didn't
> actually
> >> check whether that makes sense though.
> >
> > I am not sure cleancache's put and flush semantic.
> > If I understand rightly with old __remove_from_page_cache's comment,
> > maybe cleancache_flush_page is to invalidate the page

Hi Minchan and Stephen --

I will take a close look at this and possibly ask Chris Mason to
take a look as well (since these hooks were placed by Chris in 2008
and this is the first significant change around the hooks since then).

I think as long as the page is still locked and the mapping
remains valid, the ordering may not matter, but will confirm
and test.
 
> Dan, one more thing.
> 
> #define cleancache_fs_enabled_mapping(_mapping) \
>         (mapping->host->i_sb->cleancache_poolid >= 0)
> 
> One is "_mapping", another is  "mapping"

Oops!  Nice catch, Minchan!  Will fix (using C, per
Andrew's reply).

Thanks,
Dan

RE: linux-next: manual merge of the cleancache tree with Linus' tree

[Dan Magenheimer]

> From: Minchan Kim [mailto:minchan.kim@gmail.com]
> Sent: Wednesday, March 23, 2011 11:38 PM
> To: Andrew Morton
> Cc: Stephen Rothwell; Dan Magenheimer; linux-next@vger.kernel.org;
> linux-kernel@vger.kernel.org; Linus
> Subject: Re: linux-next: manual merge of the cleancache tree with
> Linus' tree
> 
> On Thu, Mar 24, 2011 at 12:56 PM, Andrew Morton
> <akpm@linux-foundation.org> wrote:
> > On Thu, 24 Mar 2011 13:55:24 +1100 Stephen Rothwell
> <sfr@canb.auug.org.au> wrote:
> >
> >> Hi Dan,
> >>
> >> Today's linux-next merge of the cleancache tree got a conflict in
> >> mm/truncate.c between commit 5adc7b518b54 ("mm: truncate: change
> >> remove_from_page_cache") from Linus' tree and commit 03e838947c8a
> >> ("mm/fs: add hooks to support cleancache") from the cleancache tree.
> >>
> >> I fixed it up (see below) but am really not sure of the fix.  I can
> carry
> >> this fix as necessary.
> >>
> >> Is this stuff going to be merged into Linus' tree this time round?
> >> --
> >> Cheers,
> >> Stephen Rothwell                    sfr@canb.auug.org.au
> >>
> >> diff --cc mm/truncate.c
> >> index a956675,cd94607..0000000
> >> --- a/mm/truncate.c
> >> +++ b/mm/truncate.c
> >> @@@ -106,8 -108,13 +108,12 @@@ truncate_complete_page(struct
> address_s
> >>       cancel_dirty_page(page, PAGE_CACHE_SIZE);
> >>
> >>       clear_page_mlock(page);
> >>  -    remove_from_page_cache(page);
> >>       ClearPageMappedToDisk(page);
> >>  +    delete_from_page_cache(page);
> >> +     /* this must be after the remove_from_page_cache which
> >> +      * calls cleancache_put_page (and note page->mapping is now
> NULL)
> >> +      */
> >> +     cleancache_flush_page(mapping, page);
> >>  -    page_cache_release(page);       /* pagecache ref */
> >>       return 0;
> >>   }
> >
> > I did the cleancache_flush_page() before the
> delete_from_page_cache(),
> > in case the delete_from_page_cache() freed the page.  I didn't
> actually
> > check whether that makes sense though.
> 
> I am not sure cleancache's put and flush semantic.
> If I understand rightly with old __remove_from_page_cache's comment,
> maybe cleancache_flush_page is to invalidate the page.(If I understand
> right, I hope the name is changed to cleancache_invalidate_page)
> 
> "        /*
>          * if we're uptodate, flush out into the cleancache, otherwise
>          * invalidate any existing cleancache entries.  We can't leave
>          * stale data around in the cleancache once our page is gone
>          */
>         if (PageUptodate(page))
>                 cleancache_put_page(page);
>         else
>                 cleancache_flush_page(mapping, page); "
> 
> So I think cleancache_flush_page should be done after
> delete_from_page_cache because delete_from_page_cache calls
> cleancache_put_page(maybe this function would flush the content of
> memory into cleancache's target) before we invalidates the page.
> 
> And it should not be a problem in case the delete_from_page_cache
> freed the page since cleancache should have a reference the page but I
> didn't check cleancahe always has a reference of page. If it isn't,
> it's a critical problem.
> 
> Dan, Could you comment this?

In case anyone was waiting for a resolution on this, it
was resolved offlist.

The answer is that the order doesn't matter and the V8
cleancache patch will include a fix for this.

Thanks,
Dan

linux-next: manual merge of the cleancache tree with Linus' tree

[Stephen Rothwell]

Hi Dan,

Today's linux-next merge of the cleancache tree got a conflict in
fs/super.c between commit d863b50ab01333659314c2034890cb76d9fdc3c7 ("vfs:
call rcu_barrier after ->kill_sb()") from Linus' tree and commit
03e838947c8abe29a9d7abfaf7fd9125a46b70e9 ("mm/fs: add hooks to support
cleancache") from the cleancache tree.

I fixed it up (see below) and can carry the fix as necessary.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

diff --cc fs/super.c
index 7e9dd4c,30a54e0..0000000
--- a/fs/super.c
+++ b/fs/super.c
@@@ -30,7 -30,7 +30,8 @@@
  #include <linux/idr.h>
  #include <linux/mutex.h>
  #include <linux/backing-dev.h>
 +#include <linux/rculist_bl.h>
+ #include <linux/cleancache.h>
  #include "internal.h"
  
  
@@@ -177,11 -178,7 +179,12 @@@ void deactivate_locked_super(struct sup
  	struct file_system_type *fs = s->s_type;
  	if (atomic_dec_and_test(&s->s_active)) {
  		fs->kill_sb(s);
 +		/*
 +		 * We need to call rcu_barrier so all the delayed rcu free
 +		 * inodes are flushed before we release the fs module.
 +		 */
 +		rcu_barrier();
+ 		cleancache_flush_fs(s);
  		put_filesystem(fs);
  		put_super(s);
  	} else {

linux-next: manual merge of the cleancache tree with Linus' tree

[Stephen Rothwell]

Hi Dan,

Today's linux-next merge of the cleancache tree got a conflict in
mm/Kconfig between various transparent huge page commits from Linus' tree
and commit 83137a5649ec8a0bb769c68024b0532733087482 ("mm: cleancache core
ops functions and config") from the cleancache tree.

Just context changes.  I fixed it up (see below) and can carry the fix as
necessary.
-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

diff --cc mm/Kconfig
index 3ad483b,9ee0751..0000000
--- a/mm/Kconfig
+++ b/mm/Kconfig
@@@ -302,48 -302,24 +302,70 @@@ config NOMMU_INITIAL_TRIM_EXCES
  
  	  See Documentation/nommu-mmap.txt for more information.
  
 +config TRANSPARENT_HUGEPAGE
 +	bool "Transparent Hugepage Support"
 +	depends on X86 && MMU
 +	select COMPACTION
 +	help
 +	  Transparent Hugepages allows the kernel to use huge pages and
 +	  huge tlb transparently to the applications whenever possible.
 +	  This feature can improve computing performance to certain
 +	  applications by speeding up page faults during memory
 +	  allocation, by reducing the number of tlb misses and by speeding
 +	  up the pagetable walking.
 +
 +	  If memory constrained on embedded, you may want to say N.
 +
 +choice
 +	prompt "Transparent Hugepage Support sysfs defaults"
 +	depends on TRANSPARENT_HUGEPAGE
 +	default TRANSPARENT_HUGEPAGE_ALWAYS
 +	help
 +	  Selects the sysfs defaults for Transparent Hugepage Support.
 +
 +	config TRANSPARENT_HUGEPAGE_ALWAYS
 +		bool "always"
 +	help
 +	  Enabling Transparent Hugepage always, can increase the
 +	  memory footprint of applications without a guaranteed
 +	  benefit but it will work automatically for all applications.
 +
 +	config TRANSPARENT_HUGEPAGE_MADVISE
 +		bool "madvise"
 +	help
 +	  Enabling Transparent Hugepage madvise, will only provide a
 +	  performance improvement benefit to the applications using
 +	  madvise(MADV_HUGEPAGE) but it won't risk to increase the
 +	  memory footprint of applications without a guaranteed
 +	  benefit.
 +endchoice
 +
 +#
 +# UP and nommu archs use km based percpu allocator
 +#
 +config NEED_PER_CPU_KM
 +	depends on !SMP
 +	bool
 +	default y
++
+ config CLEANCACHE
+ 	bool "Enable cleancache pseudo-RAM driver to cache clean pages"
+ 	default y
+ 	help
+ 	  Cleancache can be thought of as a page-granularity victim cache
+ 	  for clean pages that the kernel's pageframe replacement algorithm
+ 	  (PFRA) would like to keep around, but can't since there isn't enough
+ 	  memory.  So when the PFRA "evicts" a page, it first attempts to put
+ 	  it into a synchronous concurrency-safe page-oriented pseudo-RAM
+ 	  device (such as Xen's Transcendent Memory, aka "tmem") which is not
+ 	  directly accessible or addressable by the kernel and is of unknown
+ 	  (and possibly time-varying) size.  And when a cleancache-enabled
+ 	  filesystem wishes to access a page in a file on disk, it first
+ 	  checks cleancache to see if it already contains it; if it does,
+ 	  the page is copied into the kernel and a disk access is avoided.
+ 	  When a pseudo-RAM device is available, a significant I/O reduction
+ 	  may be achieved.  When none is available, all cleancache calls
+ 	  are reduced to a single pointer-compare-against-NULL resulting
+ 	  in a negligible performance hit.
+ 
+ 	  If unsure, say Y to enable cleancache

linux-next: manual merge of the cleancache tree with Linus' tree

[Stephen Rothwell]

Hi Dan,

Today's linux-next merge of the cleancache tree got a conflict in
include/linux/fs.h between commit
c8aebb0c9f8c7471643d5f8ba68328de8013005f ("per-superblock default
->d_op") from Linus' tree and commit
8b53f238859d3fca74c4c52ee1af8d6b5daafcc5 ("fs: add field to superblock to
support cleancache") from the cleancache tree.

I fixed it up (see below) and can carry the fix as necessary.
-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

diff --cc include/linux/fs.h
index ed6ee47,c52bab0..0000000
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@@ -1422,8 -1384,12 +1422,13 @@@ struct super_block 
  	 * Saved mount options for lazy filesystems using
  	 * generic_show_options()
  	 */
 -	char *s_options;
 +	char __rcu *s_options;
 +	const struct dentry_operations *s_d_op; /* default d_op for dentries */
+ 
+ 	/*
+ 	 * Saved pool identifier for cleancache (-1 means none)
+ 	 */
+ 	int cleancache_poolid;
  };
  
  extern struct timespec current_fs_time(struct super_block *sb);