From: Miklos Szeredi <[email protected]>
This function basically does:
remove_from_page_cache(old);
page_cache_release(old);
add_to_page_cache_locked(new);
Except it does this atomically, so there's no possibility for the
"add" to fail because of a race.
This is used by fuse to move pages into the page cache.
Signed-off-by: Miklos Szeredi <[email protected]>
---
fs/fuse/dev.c | 10 ++++------
include/linux/pagemap.h | 1 +
mm/filemap.c | 41 +++++++++++++++++++++++++++++++++++++++++
3 files changed, 46 insertions(+), 6 deletions(-)
Index: linux-2.6/mm/filemap.c
===================================================================
--- linux-2.6.orig/mm/filemap.c 2010-12-15 16:39:55.000000000 +0100
+++ linux-2.6/mm/filemap.c 2010-12-15 16:41:24.000000000 +0100
@@ -389,6 +389,47 @@ int filemap_write_and_wait_range(struct
}
EXPORT_SYMBOL(filemap_write_and_wait_range);
+int replace_page_cache_page(struct page *old, struct page *new, gfp_t gfp_mask)
+{
+ int error;
+
+ VM_BUG_ON(!PageLocked(old));
+ VM_BUG_ON(!PageLocked(new));
+ VM_BUG_ON(new->mapping);
+
+ error = mem_cgroup_cache_charge(new, current->mm,
+ gfp_mask & GFP_RECLAIM_MASK);
+ if (error)
+ goto out;
+
+ error = radix_tree_preload(gfp_mask & ~__GFP_HIGHMEM);
+ if (error == 0) {
+ struct address_space *mapping = old->mapping;
+ pgoff_t offset = old->index;
+
+ page_cache_get(new);
+ new->mapping = mapping;
+ new->index = offset;
+
+ spin_lock_irq(&mapping->tree_lock);
+ __remove_from_page_cache(old);
+ error = radix_tree_insert(&mapping->page_tree, offset, new);
+ BUG_ON(error);
+ mapping->nrpages++;
+ __inc_zone_page_state(new, NR_FILE_PAGES);
+ if (PageSwapBacked(new))
+ __inc_zone_page_state(new, NR_SHMEM);
+ spin_unlock_irq(&mapping->tree_lock);
+ radix_tree_preload_end();
+ mem_cgroup_uncharge_cache_page(old);
+ page_cache_release(old);
+ } else
+ mem_cgroup_uncharge_cache_page(new);
+out:
+ return error;
+}
+EXPORT_SYMBOL_GPL(replace_page_cache_page);
+
/**
* add_to_page_cache_locked - add a locked page to the pagecache
* @page: page to add
Index: linux-2.6/include/linux/pagemap.h
===================================================================
--- linux-2.6.orig/include/linux/pagemap.h 2010-12-15 16:39:39.000000000 +0100
+++ linux-2.6/include/linux/pagemap.h 2010-12-15 16:41:24.000000000 +0100
@@ -457,6 +457,7 @@ int add_to_page_cache_lru(struct page *p
pgoff_t index, gfp_t gfp_mask);
extern void remove_from_page_cache(struct page *page);
extern void __remove_from_page_cache(struct page *page);
+int replace_page_cache_page(struct page *old, struct page *new, gfp_t gfp_mask);
/*
* Like add_to_page_cache_locked, but used to add newly allocated pages:
Index: linux-2.6/fs/fuse/dev.c
===================================================================
--- linux-2.6.orig/fs/fuse/dev.c 2010-12-15 16:39:39.000000000 +0100
+++ linux-2.6/fs/fuse/dev.c 2010-12-15 16:41:24.000000000 +0100
@@ -729,14 +729,12 @@ static int fuse_try_move_page(struct fus
if (WARN_ON(PageMlocked(oldpage)))
goto out_fallback_unlock;
- remove_from_page_cache(oldpage);
- page_cache_release(oldpage);
-
- err = add_to_page_cache_locked(newpage, mapping, index, GFP_KERNEL);
+ err = replace_page_cache_page(oldpage, newpage, GFP_KERNEL);
if (err) {
- printk(KERN_WARNING "fuse_try_move_page: failed to add page");
- goto out_fallback_unlock;
+ unlock_page(newpage);
+ return err;
}
+
page_cache_get(newpage);
if (!(buf->flags & PIPE_BUF_FLAG_LRU))
On 12/15/2010 10:49 AM, Miklos Szeredi wrote:
> From: Miklos Szeredi<[email protected]>
>
> This function basically does:
>
> remove_from_page_cache(old);
> page_cache_release(old);
> add_to_page_cache_locked(new);
>
> Except it does this atomically, so there's no possibility for the
> "add" to fail because of a race.
>
> This is used by fuse to move pages into the page cache.
>
> Signed-off-by: Miklos Szeredi<[email protected]>
Acked-by: Rik van Riel <[email protected]>
--
All rights reversed
On Thu, Dec 16, 2010 at 12:49 AM, Miklos Szeredi <[email protected]> wrote:
> From: Miklos Szeredi <[email protected]>
>
> This function basically does:
>
> ? ? remove_from_page_cache(old);
> ? ? page_cache_release(old);
> ? ? add_to_page_cache_locked(new);
>
> Except it does this atomically, so there's no possibility for the
> "add" to fail because of a race.
>
> This is used by fuse to move pages into the page cache.
Please write down why fuse need this new atomic function in description.
>
> Signed-off-by: Miklos Szeredi <[email protected]>
> ---
> ?fs/fuse/dev.c ? ? ? ? ? | ? 10 ++++------
> ?include/linux/pagemap.h | ? ?1 +
> ?mm/filemap.c ? ? ? ? ? ?| ? 41 +++++++++++++++++++++++++++++++++++++++++
> ?3 files changed, 46 insertions(+), 6 deletions(-)
>
> Index: linux-2.6/mm/filemap.c
> ===================================================================
> --- linux-2.6.orig/mm/filemap.c 2010-12-15 16:39:55.000000000 +0100
> +++ linux-2.6/mm/filemap.c ? ? ?2010-12-15 16:41:24.000000000 +0100
> @@ -389,6 +389,47 @@ int filemap_write_and_wait_range(struct
> ?}
> ?EXPORT_SYMBOL(filemap_write_and_wait_range);
>
This function is exported.
Please, add function description
> +int replace_page_cache_page(struct page *old, struct page *new, gfp_t gfp_mask)
> +{
> + ? ? ? int error;
> +
> + ? ? ? VM_BUG_ON(!PageLocked(old));
> + ? ? ? VM_BUG_ON(!PageLocked(new));
> + ? ? ? VM_BUG_ON(new->mapping);
> +
> + ? ? ? error = mem_cgroup_cache_charge(new, current->mm,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? gfp_mask & GFP_RECLAIM_MASK);
> + ? ? ? if (error)
> + ? ? ? ? ? ? ? goto out;
> +
> + ? ? ? error = radix_tree_preload(gfp_mask & ~__GFP_HIGHMEM);
> + ? ? ? if (error == 0) {
> + ? ? ? ? ? ? ? struct address_space *mapping = old->mapping;
> + ? ? ? ? ? ? ? pgoff_t offset = old->index;
> +
> + ? ? ? ? ? ? ? page_cache_get(new);
> + ? ? ? ? ? ? ? new->mapping = mapping;
> + ? ? ? ? ? ? ? new->index = offset;
> +
> + ? ? ? ? ? ? ? spin_lock_irq(&mapping->tree_lock);
> + ? ? ? ? ? ? ? __remove_from_page_cache(old);
> + ? ? ? ? ? ? ? error = radix_tree_insert(&mapping->page_tree, offset, new);
> + ? ? ? ? ? ? ? BUG_ON(error);
> + ? ? ? ? ? ? ? mapping->nrpages++;
> + ? ? ? ? ? ? ? __inc_zone_page_state(new, NR_FILE_PAGES);
> + ? ? ? ? ? ? ? if (PageSwapBacked(new))
> + ? ? ? ? ? ? ? ? ? ? ? __inc_zone_page_state(new, NR_SHMEM);
> + ? ? ? ? ? ? ? spin_unlock_irq(&mapping->tree_lock);
> + ? ? ? ? ? ? ? radix_tree_preload_end();
> + ? ? ? ? ? ? ? mem_cgroup_uncharge_cache_page(old);
> + ? ? ? ? ? ? ? page_cache_release(old);
Why do you release reference of old?
> + ? ? ? } else
> + ? ? ? ? ? ? ? mem_cgroup_uncharge_cache_page(new);
> +out:
> + ? ? ? return error;
> +}
> +EXPORT_SYMBOL_GPL(replace_page_cache_page);
> +
> ?/**
> ?* add_to_page_cache_locked - add a locked page to the pagecache
> ?* @page: ? ? ?page to add
> Index: linux-2.6/include/linux/pagemap.h
> ===================================================================
> --- linux-2.6.orig/include/linux/pagemap.h ? ? ?2010-12-15 16:39:39.000000000 +0100
> +++ linux-2.6/include/linux/pagemap.h ? 2010-12-15 16:41:24.000000000 +0100
> @@ -457,6 +457,7 @@ int add_to_page_cache_lru(struct page *p
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?pgoff_t index, gfp_t gfp_mask);
> ?extern void remove_from_page_cache(struct page *page);
> ?extern void __remove_from_page_cache(struct page *page);
> +int replace_page_cache_page(struct page *old, struct page *new, gfp_t gfp_mask);
>
> ?/*
> ?* Like add_to_page_cache_locked, but used to add newly allocated pages:
> Index: linux-2.6/fs/fuse/dev.c
> ===================================================================
> --- linux-2.6.orig/fs/fuse/dev.c ? ? ? ?2010-12-15 16:39:39.000000000 +0100
> +++ linux-2.6/fs/fuse/dev.c ? ? 2010-12-15 16:41:24.000000000 +0100
> @@ -729,14 +729,12 @@ static int fuse_try_move_page(struct fus
> ? ? ? ?if (WARN_ON(PageMlocked(oldpage)))
> ? ? ? ? ? ? ? ?goto out_fallback_unlock;
>
> - ? ? ? remove_from_page_cache(oldpage);
> - ? ? ? page_cache_release(oldpage);
> -
> - ? ? ? err = add_to_page_cache_locked(newpage, mapping, index, GFP_KERNEL);
> + ? ? ? err = replace_page_cache_page(oldpage, newpage, GFP_KERNEL);
> ? ? ? ?if (err) {
> - ? ? ? ? ? ? ? printk(KERN_WARNING "fuse_try_move_page: failed to add page");
> - ? ? ? ? ? ? ? goto out_fallback_unlock;
> + ? ? ? ? ? ? ? unlock_page(newpage);
> + ? ? ? ? ? ? ? return err;
> ? ? ? ?}
> +
> ? ? ? ?page_cache_get(newpage);
>
> ? ? ? ?if (!(buf->flags & PIPE_BUF_FLAG_LRU))
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to [email protected]. ?For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
> Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
>
--
Kind regards,
Minchan Kim
On Wed, 15 Dec 2010 16:49:58 +0100
Miklos Szeredi <[email protected]> wrote:
> From: Miklos Szeredi <[email protected]>
>
> This function basically does:
>
> remove_from_page_cache(old);
> page_cache_release(old);
> add_to_page_cache_locked(new);
>
> Except it does this atomically, so there's no possibility for the
> "add" to fail because of a race.
>
> This is used by fuse to move pages into the page cache.
>
> Signed-off-by: Miklos Szeredi <[email protected]>
> ---
> fs/fuse/dev.c | 10 ++++------
> include/linux/pagemap.h | 1 +
> mm/filemap.c | 41 +++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 46 insertions(+), 6 deletions(-)
>
> Index: linux-2.6/mm/filemap.c
> ===================================================================
> --- linux-2.6.orig/mm/filemap.c 2010-12-15 16:39:55.000000000 +0100
> +++ linux-2.6/mm/filemap.c 2010-12-15 16:41:24.000000000 +0100
> @@ -389,6 +389,47 @@ int filemap_write_and_wait_range(struct
> }
> EXPORT_SYMBOL(filemap_write_and_wait_range);
>
> +int replace_page_cache_page(struct page *old, struct page *new, gfp_t gfp_mask)
> +{
> + int error;
> +
> + VM_BUG_ON(!PageLocked(old));
> + VM_BUG_ON(!PageLocked(new));
> + VM_BUG_ON(new->mapping);
> +
> + error = mem_cgroup_cache_charge(new, current->mm,
> + gfp_mask & GFP_RECLAIM_MASK);
Hmm, then, the page will be recharged to "current" instead of the memcg
where "old" was under control. Is this design ? If so, why ?
In mm/migrate.c, following is called.
charge = mem_cgroup_prepare_migration(page, newpage, &mem);
....do migration....
if (!charge)
mem_cgroup_end_migration(mem, page, newpage);
BTW, off topic, in fuse/dev.c
add_to_page_cache_locked(page)
is called and this page is "charged" to memory cgroup. But, IIUC, this page
will be never be on LRU and cannot be reclaimed by memory cgroup.
I think this looks like a memory leak at rmdir() of memory cgroup and
rmdir will fail wish -EBUSY always.
So, I'd like to change this call something like as
add_to_page_cache_locked_and_no_memory_cgroup_control().
So, I think just dropping this memory cgroup related code is okay for us
because this is a replacement for add_to_page_cache_locked() which seems
problematic.
This will put pages on fuse's private radix-tree out of control.
Or, is it possible to drain these radix-tree pages at rmdir() of memory
cgroup by some call ?
Thanks,
-Kame
> + if (error)
> + goto out;
> +
> + error = radix_tree_preload(gfp_mask & ~__GFP_HIGHMEM);
> + if (error == 0) {
> + struct address_space *mapping = old->mapping;
> + pgoff_t offset = old->index;
> +
> + page_cache_get(new);
> + new->mapping = mapping;
> + new->index = offset;
> +
> + spin_lock_irq(&mapping->tree_lock);
> + __remove_from_page_cache(old);
> + error = radix_tree_insert(&mapping->page_tree, offset, new);
> + BUG_ON(error);
> + mapping->nrpages++;
> + __inc_zone_page_state(new, NR_FILE_PAGES);
> + if (PageSwapBacked(new))
> + __inc_zone_page_state(new, NR_SHMEM);
> + spin_unlock_irq(&mapping->tree_lock);
> + radix_tree_preload_end();
> + mem_cgroup_uncharge_cache_page(old);
> + page_cache_release(old);
> + } else
> + mem_cgroup_uncharge_cache_page(new);
> +out:
> + return error;
> +}
> +EXPORT_SYMBOL_GPL(replace_page_cache_page);
> +
> /**
> * add_to_page_cache_locked - add a locked page to the pagecache
> * @page: page to add
> Index: linux-2.6/include/linux/pagemap.h
> ===================================================================
> --- linux-2.6.orig/include/linux/pagemap.h 2010-12-15 16:39:39.000000000 +0100
> +++ linux-2.6/include/linux/pagemap.h 2010-12-15 16:41:24.000000000 +0100
> @@ -457,6 +457,7 @@ int add_to_page_cache_lru(struct page *p
> pgoff_t index, gfp_t gfp_mask);
> extern void remove_from_page_cache(struct page *page);
> extern void __remove_from_page_cache(struct page *page);
> +int replace_page_cache_page(struct page *old, struct page *new, gfp_t gfp_mask);
>
> /*
> * Like add_to_page_cache_locked, but used to add newly allocated pages:
> Index: linux-2.6/fs/fuse/dev.c
> ===================================================================
> --- linux-2.6.orig/fs/fuse/dev.c 2010-12-15 16:39:39.000000000 +0100
> +++ linux-2.6/fs/fuse/dev.c 2010-12-15 16:41:24.000000000 +0100
> @@ -729,14 +729,12 @@ static int fuse_try_move_page(struct fus
> if (WARN_ON(PageMlocked(oldpage)))
> goto out_fallback_unlock;
>
> - remove_from_page_cache(oldpage);
> - page_cache_release(oldpage);
> -
> - err = add_to_page_cache_locked(newpage, mapping, index, GFP_KERNEL);
> + err = replace_page_cache_page(oldpage, newpage, GFP_KERNEL);
> if (err) {
> - printk(KERN_WARNING "fuse_try_move_page: failed to add page");
> - goto out_fallback_unlock;
> + unlock_page(newpage);
> + return err;
> }
> +
> page_cache_get(newpage);
>
> if (!(buf->flags & PIPE_BUF_FLAG_LRU))
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to [email protected]. For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
> Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
>
On Thu, 16 Dec 2010, Minchan Kim wrote:
> On Thu, Dec 16, 2010 at 12:49 AM, Miklos Szeredi <[email protected]> wrote:
> > From: Miklos Szeredi <[email protected]>
> >
> > This function basically does:
> >
> > remove_from_page_cache(old);
> > page_cache_release(old);
> > add_to_page_cache_locked(new);
> >
> > Except it does this atomically, so there's no possibility for the
> > "add" to fail because of a race.
> >
> > This is used by fuse to move pages into the page cache.
>
> Please write down why fuse need this new atomic function in description.
Okay.
> >
> > Signed-off-by: Miklos Szeredi <[email protected]>
> > ---
> > fs/fuse/dev.c | 10 ++++------
> > include/linux/pagemap.h | 1 +
> > mm/filemap.c | 41 +++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 46 insertions(+), 6 deletions(-)
> >
> > Index: linux-2.6/mm/filemap.c
> > ===================================================================
> > --- linux-2.6.orig/mm/filemap.c 2010-12-15 16:39:55.000000000 +0100
> > +++ linux-2.6/mm/filemap.c 2010-12-15 16:41:24.000000000 +0100
> > @@ -389,6 +389,47 @@ int filemap_write_and_wait_range(struct
> > }
> > EXPORT_SYMBOL(filemap_write_and_wait_range);
> >
>
> This function is exported.
> Please, add function description
Right, will do.
> > +int replace_page_cache_page(struct page *old, struct page *new, gfp_t gfp_mask)
> > +{
> > + int error;
> > +
> > + VM_BUG_ON(!PageLocked(old));
> > + VM_BUG_ON(!PageLocked(new));
> > + VM_BUG_ON(new->mapping);
> > +
> > + error = mem_cgroup_cache_charge(new, current->mm,
> > + gfp_mask & GFP_RECLAIM_MASK);
> > + if (error)
> > + goto out;
> > +
> > + error = radix_tree_preload(gfp_mask & ~__GFP_HIGHMEM);
> > + if (error == 0) {
> > + struct address_space *mapping = old->mapping;
> > + pgoff_t offset = old->index;
> > +
> > + page_cache_get(new);
> > + new->mapping = mapping;
> > + new->index = offset;
> > +
> > + spin_lock_irq(&mapping->tree_lock);
> > + __remove_from_page_cache(old);
> > + error = radix_tree_insert(&mapping->page_tree, offset, new);
> > + BUG_ON(error);
> > + mapping->nrpages++;
> > + __inc_zone_page_state(new, NR_FILE_PAGES);
> > + if (PageSwapBacked(new))
> > + __inc_zone_page_state(new, NR_SHMEM);
> > + spin_unlock_irq(&mapping->tree_lock);
> > + radix_tree_preload_end();
> > + mem_cgroup_uncharge_cache_page(old);
> > + page_cache_release(old);
>
> Why do you release reference of old?
That's the page cache reference we release. Just like we acquire the
page cache reference for "new" above.
I suspect it's historic that page_cache_release() doesn't drop the
page cache ref.
Thanks for the review.
Miklos
> > + } else
> > + mem_cgroup_uncharge_cache_page(new);
> > +out:
> > + return error;
> > +}
> > +EXPORT_SYMBOL_GPL(replace_page_cache_page);
> > +
> > /**
> > * add_to_page_cache_locked - add a locked page to the pagecache
> > * @page: page to add
> > Index: linux-2.6/include/linux/pagemap.h
> > ===================================================================
> > --- linux-2.6.orig/include/linux/pagemap.h 2010-12-15 16:39:39.000000000 +0100
> > +++ linux-2.6/include/linux/pagemap.h 2010-12-15 16:41:24.000000000 +0100
> > @@ -457,6 +457,7 @@ int add_to_page_cache_lru(struct page *p
> > pgoff_t index, gfp_t gfp_mask);
> > extern void remove_from_page_cache(struct page *page);
> > extern void __remove_from_page_cache(struct page *page);
> > +int replace_page_cache_page(struct page *old, struct page *new, gfp_t gfp_mask);
> >
> > /*
> > * Like add_to_page_cache_locked, but used to add newly allocated pages:
> > Index: linux-2.6/fs/fuse/dev.c
> > ===================================================================
> > --- linux-2.6.orig/fs/fuse/dev.c 2010-12-15 16:39:39.000000000 +0100
> > +++ linux-2.6/fs/fuse/dev.c 2010-12-15 16:41:24.000000000 +0100
> > @@ -729,14 +729,12 @@ static int fuse_try_move_page(struct fus
> > if (WARN_ON(PageMlocked(oldpage)))
> > goto out_fallback_unlock;
> >
> > - remove_from_page_cache(oldpage);
> > - page_cache_release(oldpage);
> > -
> > - err = add_to_page_cache_locked(newpage, mapping, index, GFP_KERNEL);
> > + err = replace_page_cache_page(oldpage, newpage, GFP_KERNEL);
> > if (err) {
> > - printk(KERN_WARNING "fuse_try_move_page: failed to add page");
> > - goto out_fallback_unlock;
> > + unlock_page(newpage);
> > + return err;
> > }
> > +
> > page_cache_get(newpage);
> >
> > if (!(buf->flags & PIPE_BUF_FLAG_LRU))
> >
> > --
> > To unsubscribe, send a message with 'unsubscribe linux-mm' in
> > the body to [email protected]. For more info on Linux MM,
> > see: http://www.linux-mm.org/ .
> > Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
> > Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
> >
>
>
>
> --
> Kind regards,
> Minchan Kim
>
On Thu, 16 Dec 2010, KAMEZAWA Hiroyuki wrote:
> On Wed, 15 Dec 2010 16:49:58 +0100
> Miklos Szeredi <[email protected]> wrote:
>
> > From: Miklos Szeredi <[email protected]>
> >
> > This function basically does:
> >
> > remove_from_page_cache(old);
> > page_cache_release(old);
> > add_to_page_cache_locked(new);
> >
> > Except it does this atomically, so there's no possibility for the
> > "add" to fail because of a race.
> >
> > This is used by fuse to move pages into the page cache.
> >
> > Signed-off-by: Miklos Szeredi <[email protected]>
> > ---
> > fs/fuse/dev.c | 10 ++++------
> > include/linux/pagemap.h | 1 +
> > mm/filemap.c | 41 +++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 46 insertions(+), 6 deletions(-)
> >
> > Index: linux-2.6/mm/filemap.c
> > ===================================================================
> > --- linux-2.6.orig/mm/filemap.c 2010-12-15 16:39:55.000000000 +0100
> > +++ linux-2.6/mm/filemap.c 2010-12-15 16:41:24.000000000 +0100
> > @@ -389,6 +389,47 @@ int filemap_write_and_wait_range(struct
> > }
> > EXPORT_SYMBOL(filemap_write_and_wait_range);
> >
> > +int replace_page_cache_page(struct page *old, struct page *new, gfp_t gfp_mask)
> > +{
> > + int error;
> > +
> > + VM_BUG_ON(!PageLocked(old));
> > + VM_BUG_ON(!PageLocked(new));
> > + VM_BUG_ON(new->mapping);
> > +
> > + error = mem_cgroup_cache_charge(new, current->mm,
> > + gfp_mask & GFP_RECLAIM_MASK);
>
> Hmm, then, the page will be recharged to "current" instead of the memcg
> where "old" was under control. Is this design ? If so, why ?
No, I just haven't thought about it.
Porbably charging "new" to where "old" was charged is the logical
thing to do here.
>
> In mm/migrate.c, following is called.
>
> charge = mem_cgroup_prepare_migration(page, newpage, &mem);
> ....do migration....
> if (!charge)
> mem_cgroup_end_migration(mem, page, newpage);
>
> BTW, off topic, in fuse/dev.c
>
> add_to_page_cache_locked(page)
This is the call which the above patch replaces with
replace_page_cache_page(). So if I fix replace_page_cache_page() to
charge "newpage" to the correct memory cgroup, that should solve all
problems, no?
Thanks for the review.
Miklos
On Thu, Dec 16, 2010 at 12:59:58PM +0100, Miklos Szeredi wrote:
> On Thu, 16 Dec 2010, Minchan Kim wrote:
> > > +int replace_page_cache_page(struct page *old, struct page *new, gfp_t gfp_mask)
> > > +{
> > > + ?? ?? ?? int error;
> > > +
> > > + ?? ?? ?? VM_BUG_ON(!PageLocked(old));
> > > + ?? ?? ?? VM_BUG_ON(!PageLocked(new));
> > > + ?? ?? ?? VM_BUG_ON(new->mapping);
> > > +
> > > + ?? ?? ?? error = mem_cgroup_cache_charge(new, current->mm,
> > > + ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? gfp_mask & GFP_RECLAIM_MASK);
> > > + ?? ?? ?? if (error)
> > > + ?? ?? ?? ?? ?? ?? ?? goto out;
> > > +
> > > + ?? ?? ?? error = radix_tree_preload(gfp_mask & ~__GFP_HIGHMEM);
> > > + ?? ?? ?? if (error == 0) {
> > > + ?? ?? ?? ?? ?? ?? ?? struct address_space *mapping = old->mapping;
> > > + ?? ?? ?? ?? ?? ?? ?? pgoff_t offset = old->index;
> > > +
> > > + ?? ?? ?? ?? ?? ?? ?? page_cache_get(new);
> > > + ?? ?? ?? ?? ?? ?? ?? new->mapping = mapping;
> > > + ?? ?? ?? ?? ?? ?? ?? new->index = offset;
> > > +
> > > + ?? ?? ?? ?? ?? ?? ?? spin_lock_irq(&mapping->tree_lock);
> > > + ?? ?? ?? ?? ?? ?? ?? __remove_from_page_cache(old);
> > > + ?? ?? ?? ?? ?? ?? ?? error = radix_tree_insert(&mapping->page_tree, offset, new);
> > > + ?? ?? ?? ?? ?? ?? ?? BUG_ON(error);
> > > + ?? ?? ?? ?? ?? ?? ?? mapping->nrpages++;
> > > + ?? ?? ?? ?? ?? ?? ?? __inc_zone_page_state(new, NR_FILE_PAGES);
> > > + ?? ?? ?? ?? ?? ?? ?? if (PageSwapBacked(new))
> > > + ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? __inc_zone_page_state(new, NR_SHMEM);
> > > + ?? ?? ?? ?? ?? ?? ?? spin_unlock_irq(&mapping->tree_lock);
> > > + ?? ?? ?? ?? ?? ?? ?? radix_tree_preload_end();
> > > + ?? ?? ?? ?? ?? ?? ?? mem_cgroup_uncharge_cache_page(old);
> > > + ?? ?? ?? ?? ?? ?? ?? page_cache_release(old);
> >
> > Why do you release reference of old?
>
> That's the page cache reference we release. Just like we acquire the
> page cache reference for "new" above.
I mean current page cache handling semantic and page reference counting semantic
is separeated. For example, remove_from_page_cache doesn't drop the reference of page.
That's because we need more works after drop the page from page cache.
Look at shmem_writepage, truncate_complete_page.
You makes the general API and caller might need works before the old page
is free. So how about this?
err = replace_page_cache_page(oldpage, newpage, GFP_KERNEL);
if (err) {
...
}
page_cache_release(oldpage); /* drop ref of page cache */
>
> I suspect it's historic that page_cache_release() doesn't drop the
> page cache ref.
Sorry I can't understand your words.
>
> Thanks for the review.
>
> Miklos
--
Kind regards,
Minchan Kim
On Thu, 16 Dec 2010 13:05:44 +0100
Miklos Szeredi <[email protected]> wrote:
> On Thu, 16 Dec 2010, KAMEZAWA Hiroyuki wrote:
> > Hmm, then, the page will be recharged to "current" instead of the memcg
> > where "old" was under control. Is this design ? If so, why ?
>
> No, I just haven't thought about it.
>
> Porbably charging "new" to where "old" was charged is the logical
> thing to do here.
>
> >
> > In mm/migrate.c, following is called.
> >
> > charge = mem_cgroup_prepare_migration(page, newpage, &mem);
> > ....do migration....
> > if (!charge)
> > mem_cgroup_end_migration(mem, page, newpage);
> >
> > BTW, off topic, in fuse/dev.c
> >
> > add_to_page_cache_locked(page)
>
> This is the call which the above patch replaces with
> replace_page_cache_page(). So if I fix replace_page_cache_page() to
> charge "newpage" to the correct memory cgroup, that should solve all
> problems, no?
>
No. memory cgroup expects all pages should be found on LRU. But, IIUC,
pages on this radix-tree will not be on LRU. So, memory cgroup can't find
it at destroying cgroup and can't reduce "usage" of resource to be 0.
This makes rmdir() returns -EBUSY.
I'm sorry if this page will be on LRU, somewhere.
Thanks,
-Kame
On Fri, 17 Dec 2010, Minchan Kim wrote:
> On Thu, Dec 16, 2010 at 12:59:58PM +0100, Miklos Szeredi wrote:
> > On Thu, 16 Dec 2010, Minchan Kim wrote:
> > >
> > > Why do you release reference of old?
> >
> > That's the page cache reference we release. Just like we acquire the
> > page cache reference for "new" above.
>
> I mean current page cache handling semantic and page reference counting semantic
> is separeated. For example, remove_from_page_cache doesn't drop the reference of page.
> That's because we need more works after drop the page from page cache.
> Look at shmem_writepage, truncate_complete_page.
I disagree with you there: I like the way Miklos made it symmetric,
I like the way delete_from_swap_cache drops the swap cache reference,
I dislike the way remove_from_page_cache does not - I did once try to
change that, but did a bad job, messed up reiserfs or reiser4 I forget
which, retreated in shame.
In both the examples you give, shmem_writepage and truncate_complete_page,
the caller has to be holding their own reference, in part because they
locked the page, and will need to unlock it before releasing their ref.
I think that would be true of any replace_page_cache_page caller.
>
> You makes the general API and caller might need works before the old page
> is free. So how about this?
>
> err = replace_page_cache_page(oldpage, newpage, GFP_KERNEL);
> if (err) {
> ...
> }
>
> page_cache_release(oldpage); /* drop ref of page cache */
>
>
> >
> > I suspect it's historic that page_cache_release() doesn't drop the
> > page cache ref.
>
> Sorry I can't understand your words.
Me neither: I believe Miklos meant __remove_from_page_cache() rather
than page_cache_release() in that instance.
Hugh
Hi Hugh,
On Fri, Dec 17, 2010 at 10:21 AM, Hugh Dickins <[email protected]> wrote:
> On Fri, 17 Dec 2010, Minchan Kim wrote:
>> On Thu, Dec 16, 2010 at 12:59:58PM +0100, Miklos Szeredi wrote:
>> > On Thu, 16 Dec 2010, Minchan Kim wrote:
>> > >
>> > > Why do you release reference of old?
>> >
>> > That's the page cache reference we release. ?Just like we acquire the
>> > page cache reference for "new" above.
>>
>> I mean current page cache handling semantic and page reference counting semantic
>> is separeated. For example, remove_from_page_cache doesn't drop the reference of page.
>> That's because we need more works after drop the page from page cache.
>> Look at shmem_writepage, truncate_complete_page.
>
> I disagree with you there: I like the way Miklos made it symmetric,
> I like the way delete_from_swap_cache drops the swap cache reference,
> I dislike the way remove_from_page_cache does not - I did once try to
> change that, but did a bad job, messed up reiserfs or reiser4 I forget
> which, retreated in shame.
I agree symmetric is good. I just said current fact which is that
remove_from_page_cache doesn't release ref.
So I thought we have to match current semantic to protect confusing.
Okay. I will not oppose current semantics.
Instead of it, please add it (ex, caller should hold the page
reference) in function description.
I am happy to hear that you tried it.
Although it is hard, I think it's very valuable thing.
Could you give me hint to googling your effort and why it is failed?
>
> In both the examples you give, shmem_writepage and truncate_complete_page,
> the caller has to be holding their own reference, in part because they
> locked the page, and will need to unlock it before releasing their ref.
> I think that would be true of any replace_page_cache_page caller.
Agree.
>
>>
>> You makes the general API and caller might need works before the old page
>> is free. So how about this?
>>
>> err = replace_page_cache_page(oldpage, newpage, GFP_KERNEL);
>> if (err) {
>> ? ? ? ? ...
>> }
>>
>> page_cache_release(oldpage); /* drop ref of page cache */
>>
>>
>> >
>> > I suspect it's historic that page_cache_release() doesn't drop the
>> > page cache ref.
>>
>> Sorry I can't understand your words.
>
> Me neither: I believe Miklos meant __remove_from_page_cache() rather
> than page_cache_release() in that instance.
Maybe. :)
Thanks for the comment, Hugh.
>
> Hugh
>
--
Kind regards,
Minchan Kim
On Fri, 17 Dec 2010, Minchan Kim wrote:
> On Fri, Dec 17, 2010 at 10:21 AM, Hugh Dickins <[email protected]> wrote:
> >
> > I disagree with you there: I like the way Miklos made it symmetric,
> > I like the way delete_from_swap_cache drops the swap cache reference,
> > I dislike the way remove_from_page_cache does not - I did once try to
> > change that, but did a bad job, messed up reiserfs or reiser4 I forget
> > which, retreated in shame.
>
> I agree symmetric is good. I just said current fact which is that
> remove_from_page_cache doesn't release ref.
> So I thought we have to match current semantic to protect confusing.
> Okay. I will not oppose current semantics.
> Instead of it, please add it (ex, caller should hold the page
> reference) in function description.
>
> I am happy to hear that you tried it.
> Although it is hard, I think it's very valuable thing.
> Could you give me hint to googling your effort and why it is failed?
http://lkml.org/lkml/2004/10/24/140
Hugh
On Fri, Dec 17, 2010 at 11:10 AM, Hugh Dickins <[email protected]> wrote:
> On Fri, 17 Dec 2010, Minchan Kim wrote:
>> On Fri, Dec 17, 2010 at 10:21 AM, Hugh Dickins <[email protected]> wrote:
>> >
>> > I disagree with you there: I like the way Miklos made it symmetric,
>> > I like the way delete_from_swap_cache drops the swap cache reference,
>> > I dislike the way remove_from_page_cache does not - I did once try to
>> > change that, but did a bad job, messed up reiserfs or reiser4 I forget
>> > which, retreated in shame.
>>
>> I agree symmetric is good. I just said current fact which is that
>> remove_from_page_cache doesn't release ref.
>> So I thought we have to match current semantic to protect confusing.
>> Okay. I will not oppose current semantics.
>> Instead of it, please add it (ex, caller should hold the page
>> reference) in function description.
>>
>> I am happy to hear that you tried it.
>> Although it is hard, I think it's very valuable thing.
>> Could you give me hint to googling your effort and why it is failed?
>
> http://lkml.org/lkml/2004/10/24/140
Thanks.
Now we have only 3 callers of remove_from_page_cache in mmtom.
1. truncate_huge_page
2. shmem_writepage
3. truncate_complete_page
4. fuse_try_move_page
It seems all of caller hold the page reference so It's ok to change
the semantic of remove_from_page_cache.
Okay. I will do that.
>
> Hugh
>
--
Kind regards,
Minchan Kim
On Fri, 17 Dec 2010, KAMEZAWA Hiroyuki wrote:
> No. memory cgroup expects all pages should be found on LRU. But, IIUC,
> pages on this radix-tree will not be on LRU. So, memory cgroup can't find
> it at destroying cgroup and can't reduce "usage" of resource to be 0.
> This makes rmdir() returns -EBUSY.
Oh, right. Yes, the page will be on the LRU (it needs to be,
otherwise the VM coulnd't reclaim it). After the
add_to_page_cache_locked is this:
if (!(buf->flags & PIPE_BUF_FLAG_LRU))
lru_cache_add_file(newpage);
It will add the page to the LRU, unless it's already on it.
Thanks,
Miklos
On Fri, 17 Dec 2010, Minchan Kim wrote:
> >> >
> >> > I suspect it's historic that page_cache_release() doesn't drop the
> >> > page cache ref.
> >>
> >> Sorry I can't understand your words.
> >
> > Me neither: I believe Miklos meant __remove_from_page_cache() rather
> > than page_cache_release() in that instance.
>
> Maybe. :)
Yeah, I did mean remove_from_page_cache :)
Thanks,
Miklos
On Fri, 17 Dec 2010 16:51:44 +0100
Miklos Szeredi <[email protected]> wrote:
> On Fri, 17 Dec 2010, KAMEZAWA Hiroyuki wrote:
> > No. memory cgroup expects all pages should be found on LRU. But, IIUC,
> > pages on this radix-tree will not be on LRU. So, memory cgroup can't find
> > it at destroying cgroup and can't reduce "usage" of resource to be 0.
> > This makes rmdir() returns -EBUSY.
>
> Oh, right. Yes, the page will be on the LRU (it needs to be,
> otherwise the VM coulnd't reclaim it). After the
> add_to_page_cache_locked is this:
>
> if (!(buf->flags & PIPE_BUF_FLAG_LRU))
> lru_cache_add_file(newpage);
>
> It will add the page to the LRU, unless it's already on it.
>
Thank you for clarification.
Thanks,
-Kame