* [PATCH] mm: add replace_page_cache_page() function
@ 2010-12-15 15:49 ` Miklos Szeredi
0 siblings, 0 replies; 42+ messages in thread
From: Miklos Szeredi @ 2010-12-15 15:49 UTC (permalink / raw)
To: akpm; +Cc: linux-fsdevel, linux-mm, linux-kernel
From: Miklos Szeredi <mszeredi@suse.cz>
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 <mszeredi@suse.cz>
---
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))
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH] mm: add replace_page_cache_page() function
@ 2010-12-15 15:49 ` Miklos Szeredi
0 siblings, 0 replies; 42+ messages in thread
From: Miklos Szeredi @ 2010-12-15 15:49 UTC (permalink / raw)
To: akpm; +Cc: linux-fsdevel, linux-mm, linux-kernel
From: Miklos Szeredi <mszeredi@suse.cz>
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 <mszeredi@suse.cz>
---
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))
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. 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:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] mm: add replace_page_cache_page() function
2010-12-15 15:49 ` Miklos Szeredi
@ 2010-12-15 16:49 ` Rik van Riel
-1 siblings, 0 replies; 42+ messages in thread
From: Rik van Riel @ 2010-12-15 16:49 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: akpm, linux-fsdevel, linux-mm, linux-kernel
On 12/15/2010 10:49 AM, Miklos Szeredi wrote:
> From: Miklos Szeredi<mszeredi@suse.cz>
>
> 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<mszeredi@suse.cz>
Acked-by: Rik van Riel <riel@redhat.com>
--
All rights reversed
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] mm: add replace_page_cache_page() function
@ 2010-12-15 16:49 ` Rik van Riel
0 siblings, 0 replies; 42+ messages in thread
From: Rik van Riel @ 2010-12-15 16:49 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: akpm, linux-fsdevel, linux-mm, linux-kernel
On 12/15/2010 10:49 AM, Miklos Szeredi wrote:
> From: Miklos Szeredi<mszeredi@suse.cz>
>
> 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<mszeredi@suse.cz>
Acked-by: Rik van Riel <riel@redhat.com>
--
All rights reversed
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. 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:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] mm: add replace_page_cache_page() function
2010-12-15 15:49 ` Miklos Szeredi
(?)
@ 2010-12-15 23:22 ` Minchan Kim
-1 siblings, 0 replies; 42+ messages in thread
From: Minchan Kim @ 2010-12-15 23:22 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: akpm, linux-fsdevel, linux-mm, linux-kernel
On Thu, Dec 16, 2010 at 12:49 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> From: Miklos Szeredi <mszeredi@suse.cz>
>
> 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 <mszeredi@suse.cz>
> ---
> 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 majordomo@kvack.org. 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:"dont@kvack.org"> email@kvack.org </a>
>
--
Kind regards,
Minchan Kim
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] mm: add replace_page_cache_page() function
@ 2010-12-15 23:22 ` Minchan Kim
0 siblings, 0 replies; 42+ messages in thread
From: Minchan Kim @ 2010-12-15 23:22 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: akpm, linux-fsdevel, linux-mm, linux-kernel
On Thu, Dec 16, 2010 at 12:49 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> From: Miklos Szeredi <mszeredi@suse.cz>
>
> 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 <mszeredi@suse.cz>
> ---
> 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 majordomo@kvack.org. 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:"dont@kvack.org"> email@kvack.org </a>
>
--
Kind regards,
Minchan Kim
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] mm: add replace_page_cache_page() function
@ 2010-12-15 23:22 ` Minchan Kim
0 siblings, 0 replies; 42+ messages in thread
From: Minchan Kim @ 2010-12-15 23:22 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: akpm, linux-fsdevel, linux-mm, linux-kernel
On Thu, Dec 16, 2010 at 12:49 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> From: Miklos Szeredi <mszeredi@suse.cz>
>
> 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 <mszeredi@suse.cz>
> ---
> 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 majordomo@kvack.org. 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:"dont@kvack.org"> email@kvack.org </a>
>
--
Kind regards,
Minchan Kim
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. 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:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] mm: add replace_page_cache_page() function
2010-12-15 15:49 ` Miklos Szeredi
@ 2010-12-16 1:07 ` KAMEZAWA Hiroyuki
-1 siblings, 0 replies; 42+ messages in thread
From: KAMEZAWA Hiroyuki @ 2010-12-16 1:07 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: akpm, linux-fsdevel, linux-mm, linux-kernel
On Wed, 15 Dec 2010 16:49:58 +0100
Miklos Szeredi <miklos@szeredi.hu> wrote:
> From: Miklos Szeredi <mszeredi@suse.cz>
>
> 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 <mszeredi@suse.cz>
> ---
> 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 majordomo@kvack.org. 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:"dont@kvack.org"> email@kvack.org </a>
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] mm: add replace_page_cache_page() function
@ 2010-12-16 1:07 ` KAMEZAWA Hiroyuki
0 siblings, 0 replies; 42+ messages in thread
From: KAMEZAWA Hiroyuki @ 2010-12-16 1:07 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: akpm, linux-fsdevel, linux-mm, linux-kernel
On Wed, 15 Dec 2010 16:49:58 +0100
Miklos Szeredi <miklos@szeredi.hu> wrote:
> From: Miklos Szeredi <mszeredi@suse.cz>
>
> 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 <mszeredi@suse.cz>
> ---
> 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 majordomo@kvack.org. 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:"dont@kvack.org"> email@kvack.org </a>
>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. 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:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] mm: add replace_page_cache_page() function
2010-12-15 23:22 ` Minchan Kim
@ 2010-12-16 11:59 ` Miklos Szeredi
-1 siblings, 0 replies; 42+ messages in thread
From: Miklos Szeredi @ 2010-12-16 11:59 UTC (permalink / raw)
To: Minchan Kim; +Cc: miklos, akpm, linux-fsdevel, linux-mm, linux-kernel
On Thu, 16 Dec 2010, Minchan Kim wrote:
> On Thu, Dec 16, 2010 at 12:49 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> > From: Miklos Szeredi <mszeredi@suse.cz>
> >
> > 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 <mszeredi@suse.cz>
> > ---
> > 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 majordomo@kvack.org. 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:"dont@kvack.org"> email@kvack.org </a>
> >
>
>
>
> --
> Kind regards,
> Minchan Kim
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] mm: add replace_page_cache_page() function
@ 2010-12-16 11:59 ` Miklos Szeredi
0 siblings, 0 replies; 42+ messages in thread
From: Miklos Szeredi @ 2010-12-16 11:59 UTC (permalink / raw)
To: Minchan Kim; +Cc: miklos, akpm, linux-fsdevel, linux-mm, linux-kernel
On Thu, 16 Dec 2010, Minchan Kim wrote:
> On Thu, Dec 16, 2010 at 12:49 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> > From: Miklos Szeredi <mszeredi@suse.cz>
> >
> > This function basically does:
> >
> > A A remove_from_page_cache(old);
> > A A page_cache_release(old);
> > A A 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 <mszeredi@suse.cz>
> > ---
> > A fs/fuse/dev.c A A A A A | A 10 ++++------
> > A include/linux/pagemap.h | A A 1 +
> > A mm/filemap.c A A A A A A | A 41 +++++++++++++++++++++++++++++++++++++++++
> > A 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 A A A 2010-12-15 16:41:24.000000000 +0100
> > @@ -389,6 +389,47 @@ int filemap_write_and_wait_range(struct
> > A }
> > A 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)
> > +{
> > + A A A int error;
> > +
> > + A A A VM_BUG_ON(!PageLocked(old));
> > + A A A VM_BUG_ON(!PageLocked(new));
> > + A A A VM_BUG_ON(new->mapping);
> > +
> > + A A A error = mem_cgroup_cache_charge(new, current->mm,
> > + A A A A A A A A A A A A A A A A A A A gfp_mask & GFP_RECLAIM_MASK);
> > + A A A if (error)
> > + A A A A A A A goto out;
> > +
> > + A A A error = radix_tree_preload(gfp_mask & ~__GFP_HIGHMEM);
> > + A A A if (error == 0) {
> > + A A A A A A A struct address_space *mapping = old->mapping;
> > + A A A A A A A pgoff_t offset = old->index;
> > +
> > + A A A A A A A page_cache_get(new);
> > + A A A A A A A new->mapping = mapping;
> > + A A A A A A A new->index = offset;
> > +
> > + A A A A A A A spin_lock_irq(&mapping->tree_lock);
> > + A A A A A A A __remove_from_page_cache(old);
> > + A A A A A A A error = radix_tree_insert(&mapping->page_tree, offset, new);
> > + A A A A A A A BUG_ON(error);
> > + A A A A A A A mapping->nrpages++;
> > + A A A A A A A __inc_zone_page_state(new, NR_FILE_PAGES);
> > + A A A A A A A if (PageSwapBacked(new))
> > + A A A A A A A A A A A __inc_zone_page_state(new, NR_SHMEM);
> > + A A A A A A A spin_unlock_irq(&mapping->tree_lock);
> > + A A A A A A A radix_tree_preload_end();
> > + A A A A A A A mem_cgroup_uncharge_cache_page(old);
> > + A A A A A A A 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
> > + A A A } else
> > + A A A A A A A mem_cgroup_uncharge_cache_page(new);
> > +out:
> > + A A A return error;
> > +}
> > +EXPORT_SYMBOL_GPL(replace_page_cache_page);
> > +
> > A /**
> > A * add_to_page_cache_locked - add a locked page to the pagecache
> > A * @page: A A A page to add
> > Index: linux-2.6/include/linux/pagemap.h
> > ===================================================================
> > --- linux-2.6.orig/include/linux/pagemap.h A A A 2010-12-15 16:39:39.000000000 +0100
> > +++ linux-2.6/include/linux/pagemap.h A 2010-12-15 16:41:24.000000000 +0100
> > @@ -457,6 +457,7 @@ int add_to_page_cache_lru(struct page *p
> > A A A A A A A A A A A A A A A A pgoff_t index, gfp_t gfp_mask);
> > A extern void remove_from_page_cache(struct page *page);
> > A extern void __remove_from_page_cache(struct page *page);
> > +int replace_page_cache_page(struct page *old, struct page *new, gfp_t gfp_mask);
> >
> > A /*
> > A * 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 A A A A 2010-12-15 16:39:39.000000000 +0100
> > +++ linux-2.6/fs/fuse/dev.c A A 2010-12-15 16:41:24.000000000 +0100
> > @@ -729,14 +729,12 @@ static int fuse_try_move_page(struct fus
> > A A A A if (WARN_ON(PageMlocked(oldpage)))
> > A A A A A A A A goto out_fallback_unlock;
> >
> > - A A A remove_from_page_cache(oldpage);
> > - A A A page_cache_release(oldpage);
> > -
> > - A A A err = add_to_page_cache_locked(newpage, mapping, index, GFP_KERNEL);
> > + A A A err = replace_page_cache_page(oldpage, newpage, GFP_KERNEL);
> > A A A A if (err) {
> > - A A A A A A A printk(KERN_WARNING "fuse_try_move_page: failed to add page");
> > - A A A A A A A goto out_fallback_unlock;
> > + A A A A A A A unlock_page(newpage);
> > + A A A A A A A return err;
> > A A A A }
> > +
> > A A A A page_cache_get(newpage);
> >
> > A A A A if (!(buf->flags & PIPE_BUF_FLAG_LRU))
> >
> > --
> > To unsubscribe, send a message with 'unsubscribe linux-mm' in
> > the body to majordomo@kvack.org. A 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:"dont@kvack.org"> email@kvack.org </a>
> >
>
>
>
> --
> Kind regards,
> Minchan Kim
>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. 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:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] mm: add replace_page_cache_page() function
2010-12-16 1:07 ` KAMEZAWA Hiroyuki
@ 2010-12-16 12:05 ` Miklos Szeredi
-1 siblings, 0 replies; 42+ messages in thread
From: Miklos Szeredi @ 2010-12-16 12:05 UTC (permalink / raw)
To: KAMEZAWA Hiroyuki; +Cc: miklos, akpm, linux-fsdevel, linux-mm, linux-kernel
On Thu, 16 Dec 2010, KAMEZAWA Hiroyuki wrote:
> On Wed, 15 Dec 2010 16:49:58 +0100
> Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> > From: Miklos Szeredi <mszeredi@suse.cz>
> >
> > 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 <mszeredi@suse.cz>
> > ---
> > 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
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] mm: add replace_page_cache_page() function
@ 2010-12-16 12:05 ` Miklos Szeredi
0 siblings, 0 replies; 42+ messages in thread
From: Miklos Szeredi @ 2010-12-16 12:05 UTC (permalink / raw)
To: KAMEZAWA Hiroyuki; +Cc: miklos, akpm, linux-fsdevel, linux-mm, linux-kernel
On Thu, 16 Dec 2010, KAMEZAWA Hiroyuki wrote:
> On Wed, 15 Dec 2010 16:49:58 +0100
> Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> > From: Miklos Szeredi <mszeredi@suse.cz>
> >
> > 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 <mszeredi@suse.cz>
> > ---
> > 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
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. 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:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] mm: add replace_page_cache_page() function
2010-12-16 11:59 ` Miklos Szeredi
@ 2010-12-16 22:04 ` Minchan Kim
-1 siblings, 0 replies; 42+ messages in thread
From: Minchan Kim @ 2010-12-16 22:04 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: akpm, linux-fsdevel, linux-mm, linux-kernel
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
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] mm: add replace_page_cache_page() function
@ 2010-12-16 22:04 ` Minchan Kim
0 siblings, 0 replies; 42+ messages in thread
From: Minchan Kim @ 2010-12-16 22:04 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: akpm, linux-fsdevel, linux-mm, linux-kernel
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
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. 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:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] mm: add replace_page_cache_page() function
2010-12-16 12:05 ` Miklos Szeredi
@ 2010-12-17 0:01 ` KAMEZAWA Hiroyuki
-1 siblings, 0 replies; 42+ messages in thread
From: KAMEZAWA Hiroyuki @ 2010-12-17 0:01 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: akpm, linux-fsdevel, linux-mm, linux-kernel
On Thu, 16 Dec 2010 13:05:44 +0100
Miklos Szeredi <miklos@szeredi.hu> 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
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] mm: add replace_page_cache_page() function
@ 2010-12-17 0:01 ` KAMEZAWA Hiroyuki
0 siblings, 0 replies; 42+ messages in thread
From: KAMEZAWA Hiroyuki @ 2010-12-17 0:01 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: akpm, linux-fsdevel, linux-mm, linux-kernel
On Thu, 16 Dec 2010 13:05:44 +0100
Miklos Szeredi <miklos@szeredi.hu> 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
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. 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:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] mm: add replace_page_cache_page() function
2010-12-16 22:04 ` Minchan Kim
@ 2010-12-17 1:21 ` Hugh Dickins
-1 siblings, 0 replies; 42+ messages in thread
From: Hugh Dickins @ 2010-12-17 1:21 UTC (permalink / raw)
To: Minchan Kim; +Cc: Miklos Szeredi, akpm, linux-fsdevel, linux-mm, linux-kernel
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
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] mm: add replace_page_cache_page() function
@ 2010-12-17 1:21 ` Hugh Dickins
0 siblings, 0 replies; 42+ messages in thread
From: Hugh Dickins @ 2010-12-17 1:21 UTC (permalink / raw)
To: Minchan Kim; +Cc: Miklos Szeredi, akpm, linux-fsdevel, linux-mm, linux-kernel
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
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. 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:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] mm: add replace_page_cache_page() function
2010-12-17 1:21 ` Hugh Dickins
@ 2010-12-17 1:40 ` Minchan Kim
-1 siblings, 0 replies; 42+ messages in thread
From: Minchan Kim @ 2010-12-17 1:40 UTC (permalink / raw)
To: Hugh Dickins; +Cc: Miklos Szeredi, akpm, linux-fsdevel, linux-mm, linux-kernel
Hi Hugh,
On Fri, Dec 17, 2010 at 10:21 AM, Hugh Dickins <hughd@google.com> 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
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] mm: add replace_page_cache_page() function
@ 2010-12-17 1:40 ` Minchan Kim
0 siblings, 0 replies; 42+ messages in thread
From: Minchan Kim @ 2010-12-17 1:40 UTC (permalink / raw)
To: Hugh Dickins; +Cc: Miklos Szeredi, akpm, linux-fsdevel, linux-mm, linux-kernel
Hi Hugh,
On Fri, Dec 17, 2010 at 10:21 AM, Hugh Dickins <hughd@google.com> 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
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. 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:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] mm: add replace_page_cache_page() function
2010-12-17 1:40 ` Minchan Kim
@ 2010-12-17 2:10 ` Hugh Dickins
-1 siblings, 0 replies; 42+ messages in thread
From: Hugh Dickins @ 2010-12-17 2:10 UTC (permalink / raw)
To: Minchan Kim; +Cc: Miklos Szeredi, akpm, linux-fsdevel, linux-mm, linux-kernel
On Fri, 17 Dec 2010, Minchan Kim wrote:
> On Fri, Dec 17, 2010 at 10:21 AM, Hugh Dickins <hughd@google.com> 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
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] mm: add replace_page_cache_page() function
@ 2010-12-17 2:10 ` Hugh Dickins
0 siblings, 0 replies; 42+ messages in thread
From: Hugh Dickins @ 2010-12-17 2:10 UTC (permalink / raw)
To: Minchan Kim; +Cc: Miklos Szeredi, akpm, linux-fsdevel, linux-mm, linux-kernel
On Fri, 17 Dec 2010, Minchan Kim wrote:
> On Fri, Dec 17, 2010 at 10:21 AM, Hugh Dickins <hughd@google.com> 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
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. 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:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] mm: add replace_page_cache_page() function
2010-12-17 2:10 ` Hugh Dickins
@ 2010-12-17 4:37 ` Minchan Kim
-1 siblings, 0 replies; 42+ messages in thread
From: Minchan Kim @ 2010-12-17 4:37 UTC (permalink / raw)
To: Hugh Dickins; +Cc: Miklos Szeredi, akpm, linux-fsdevel, linux-mm, linux-kernel
On Fri, Dec 17, 2010 at 11:10 AM, Hugh Dickins <hughd@google.com> wrote:
> On Fri, 17 Dec 2010, Minchan Kim wrote:
>> On Fri, Dec 17, 2010 at 10:21 AM, Hugh Dickins <hughd@google.com> 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
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] mm: add replace_page_cache_page() function
@ 2010-12-17 4:37 ` Minchan Kim
0 siblings, 0 replies; 42+ messages in thread
From: Minchan Kim @ 2010-12-17 4:37 UTC (permalink / raw)
To: Hugh Dickins; +Cc: Miklos Szeredi, akpm, linux-fsdevel, linux-mm, linux-kernel
On Fri, Dec 17, 2010 at 11:10 AM, Hugh Dickins <hughd@google.com> wrote:
> On Fri, 17 Dec 2010, Minchan Kim wrote:
>> On Fri, Dec 17, 2010 at 10:21 AM, Hugh Dickins <hughd@google.com> 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
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. 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:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] mm: add replace_page_cache_page() function
2010-12-17 0:01 ` KAMEZAWA Hiroyuki
@ 2010-12-17 15:51 ` Miklos Szeredi
-1 siblings, 0 replies; 42+ messages in thread
From: Miklos Szeredi @ 2010-12-17 15:51 UTC (permalink / raw)
To: KAMEZAWA Hiroyuki; +Cc: miklos, akpm, linux-fsdevel, linux-mm, linux-kernel
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
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] mm: add replace_page_cache_page() function
@ 2010-12-17 15:51 ` Miklos Szeredi
0 siblings, 0 replies; 42+ messages in thread
From: Miklos Szeredi @ 2010-12-17 15:51 UTC (permalink / raw)
To: KAMEZAWA Hiroyuki; +Cc: miklos, akpm, linux-fsdevel, linux-mm, linux-kernel
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
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. 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:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] mm: add replace_page_cache_page() function
2010-12-17 1:40 ` Minchan Kim
@ 2010-12-17 15:53 ` Miklos Szeredi
-1 siblings, 0 replies; 42+ messages in thread
From: Miklos Szeredi @ 2010-12-17 15:53 UTC (permalink / raw)
To: Minchan Kim; +Cc: hughd, miklos, akpm, linux-fsdevel, linux-mm, linux-kernel
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
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] mm: add replace_page_cache_page() function
@ 2010-12-17 15:53 ` Miklos Szeredi
0 siblings, 0 replies; 42+ messages in thread
From: Miklos Szeredi @ 2010-12-17 15:53 UTC (permalink / raw)
To: Minchan Kim; +Cc: hughd, miklos, akpm, linux-fsdevel, linux-mm, linux-kernel
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
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. 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:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] mm: add replace_page_cache_page() function
2010-12-17 15:51 ` Miklos Szeredi
@ 2010-12-19 23:54 ` KAMEZAWA Hiroyuki
-1 siblings, 0 replies; 42+ messages in thread
From: KAMEZAWA Hiroyuki @ 2010-12-19 23:54 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: akpm, linux-fsdevel, linux-mm, linux-kernel
On Fri, 17 Dec 2010 16:51:44 +0100
Miklos Szeredi <miklos@szeredi.hu> 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
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] mm: add replace_page_cache_page() function
@ 2010-12-19 23:54 ` KAMEZAWA Hiroyuki
0 siblings, 0 replies; 42+ messages in thread
From: KAMEZAWA Hiroyuki @ 2010-12-19 23:54 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: akpm, linux-fsdevel, linux-mm, linux-kernel
On Fri, 17 Dec 2010 16:51:44 +0100
Miklos Szeredi <miklos@szeredi.hu> 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
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. 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:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] mm: add replace_page_cache_page() function
2011-01-11 3:53 ` Daisuke Nishimura
(?)
@ 2011-01-11 9:38 ` Miklos Szeredi
-1 siblings, 0 replies; 42+ messages in thread
From: Miklos Szeredi @ 2011-01-11 9:38 UTC (permalink / raw)
To: Daisuke Nishimura
Cc: kamezawa.hiroyu, miklos, akpm, minchan.kim, linux-fsdevel,
linux-mm, linux-kernel, nishimura
On Tue, 11 Jan 2011, Daisuke Nishimura wrote:
> > What I recommend is below. (Please see the newest -mm because of a bug fix for
> > mem cgroup) Considering page management on radix-tree, it can be considerd as
> > a kind of page-migration, which replaces pages on radix-tree.
> >
> > ==
> >
> > > +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);
> > > +
> > struct mem_cgroup *memcg;
> >
> I think it should be initialized to NULL.
>
> > error = mem_cgroup_prepare_migration(old, new, &memcg);
>
> I want some comments like:
>
> /*
> * This is not page migration, but prepare_migration and end_migration
> * does enough work for charge replacement.
> */
>
> > #
> > # This function will charge against "newpage". But this expects
> > # the caller allows GFP_KERNEL gfp_mask.
> > # After this, the newpage is in "charged" state.
> > if (error)
> > return -ENOMEM;
> >
> > > + error = radix_tree_preload(gfp_mask & ~__GFP_HIGHMEM);
> > > + if (!error) {
> > > + 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_replace_cache_page(old, new); <== remove this.
> >
> > mem_cgroup_end_migraton(memcg, old, new, true);
> >
> > > + page_cache_release(old);
> > > + }
> > else
> > mem_cgroup_end_migration(memcg, old, new, false);
> >
> > # Here, if the 4th argument is true, old page is uncharged.
> > # if the 4th argument is false, the new page is uncharged.
> > # Then, "charge" of the old page will be migrated onto the new page
> > # if replacement is done.
> >
> >
> >
> > > +
> > > + return error;
> > > +}
> > > +EXPORT_SYMBOL_GPL(replace_page_cache_page);
> > > +
> >
> > ==
> >
> > I think this is enough simple and this covers all memory cgroup's racy
> > problems.
> >
> I agree.
Thanks for the comments.
Yeah, using existing infrastructure is undoubtedly simpler and less
prone to bugs. So going with this for a first implementation might
do.
However, replace_page_cache_page() is meant to be very efficient,
otherwise any performance won by not copying the page contents are
lost to the cost of page replacement. My guess is,
mem_cgroup_prepare_migration()/end_migration() are to heavyweight for
this.
Thanks,
Miklos
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] mm: add replace_page_cache_page() function
@ 2011-01-11 9:38 ` Miklos Szeredi
0 siblings, 0 replies; 42+ messages in thread
From: Miklos Szeredi @ 2011-01-11 9:38 UTC (permalink / raw)
To: Daisuke Nishimura
Cc: kamezawa.hiroyu, miklos, akpm, minchan.kim, linux-fsdevel,
linux-mm, linux-kernel, nishimura
On Tue, 11 Jan 2011, Daisuke Nishimura wrote:
> > What I recommend is below. (Please see the newest -mm because of a bug fix for
> > mem cgroup) Considering page management on radix-tree, it can be considerd as
> > a kind of page-migration, which replaces pages on radix-tree.
> >
> > ==
> >
> > > +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);
> > > +
> > struct mem_cgroup *memcg;
> >
> I think it should be initialized to NULL.
>
> > error = mem_cgroup_prepare_migration(old, new, &memcg);
>
> I want some comments like:
>
> /*
> * This is not page migration, but prepare_migration and end_migration
> * does enough work for charge replacement.
> */
>
> > #
> > # This function will charge against "newpage". But this expects
> > # the caller allows GFP_KERNEL gfp_mask.
> > # After this, the newpage is in "charged" state.
> > if (error)
> > return -ENOMEM;
> >
> > > + error = radix_tree_preload(gfp_mask & ~__GFP_HIGHMEM);
> > > + if (!error) {
> > > + 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_replace_cache_page(old, new); <== remove this.
> >
> > mem_cgroup_end_migraton(memcg, old, new, true);
> >
> > > + page_cache_release(old);
> > > + }
> > else
> > mem_cgroup_end_migration(memcg, old, new, false);
> >
> > # Here, if the 4th argument is true, old page is uncharged.
> > # if the 4th argument is false, the new page is uncharged.
> > # Then, "charge" of the old page will be migrated onto the new page
> > # if replacement is done.
> >
> >
> >
> > > +
> > > + return error;
> > > +}
> > > +EXPORT_SYMBOL_GPL(replace_page_cache_page);
> > > +
> >
> > ==
> >
> > I think this is enough simple and this covers all memory cgroup's racy
> > problems.
> >
> I agree.
Thanks for the comments.
Yeah, using existing infrastructure is undoubtedly simpler and less
prone to bugs. So going with this for a first implementation might
do.
However, replace_page_cache_page() is meant to be very efficient,
otherwise any performance won by not copying the page contents are
lost to the cost of page replacement. My guess is,
mem_cgroup_prepare_migration()/end_migration() are to heavyweight for
this.
Thanks,
Miklos
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. 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:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] mm: add replace_page_cache_page() function
@ 2011-01-11 9:38 ` Miklos Szeredi
0 siblings, 0 replies; 42+ messages in thread
From: Miklos Szeredi @ 2011-01-11 9:38 UTC (permalink / raw)
To: Daisuke Nishimura
Cc: kamezawa.hiroyu, miklos, akpm, minchan.kim, linux-fsdevel,
linux-mm, linux-kernel
On Tue, 11 Jan 2011, Daisuke Nishimura wrote:
> > What I recommend is below. (Please see the newest -mm because of a bug fix for
> > mem cgroup) Considering page management on radix-tree, it can be considerd as
> > a kind of page-migration, which replaces pages on radix-tree.
> >
> > ==
> >
> > > +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);
> > > +
> > struct mem_cgroup *memcg;
> >
> I think it should be initialized to NULL.
>
> > error = mem_cgroup_prepare_migration(old, new, &memcg);
>
> I want some comments like:
>
> /*
> * This is not page migration, but prepare_migration and end_migration
> * does enough work for charge replacement.
> */
>
> > #
> > # This function will charge against "newpage". But this expects
> > # the caller allows GFP_KERNEL gfp_mask.
> > # After this, the newpage is in "charged" state.
> > if (error)
> > return -ENOMEM;
> >
> > > + error = radix_tree_preload(gfp_mask & ~__GFP_HIGHMEM);
> > > + if (!error) {
> > > + 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_replace_cache_page(old, new); <== remove this.
> >
> > mem_cgroup_end_migraton(memcg, old, new, true);
> >
> > > + page_cache_release(old);
> > > + }
> > else
> > mem_cgroup_end_migration(memcg, old, new, false);
> >
> > # Here, if the 4th argument is true, old page is uncharged.
> > # if the 4th argument is false, the new page is uncharged.
> > # Then, "charge" of the old page will be migrated onto the new page
> > # if replacement is done.
> >
> >
> >
> > > +
> > > + return error;
> > > +}
> > > +EXPORT_SYMBOL_GPL(replace_page_cache_page);
> > > +
> >
> > ==
> >
> > I think this is enough simple and this covers all memory cgroup's racy
> > problems.
> >
> I agree.
Thanks for the comments.
Yeah, using existing infrastructure is undoubtedly simpler and less
prone to bugs. So going with this for a first implementation might
do.
However, replace_page_cache_page() is meant to be very efficient,
otherwise any performance won by not copying the page contents are
lost to the cost of page replacement. My guess is,
mem_cgroup_prepare_migration()/end_migration() are to heavyweight for
this.
Thanks,
Miklos
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. 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:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] mm: add replace_page_cache_page() function
2011-01-07 18:22 ` Miklos Szeredi
@ 2011-01-11 5:41 ` Minchan Kim
-1 siblings, 0 replies; 42+ messages in thread
From: Minchan Kim @ 2011-01-11 5:41 UTC (permalink / raw)
To: Miklos Szeredi
Cc: akpm, kamezawa.hiroyu, linux-fsdevel, linux-mm, linux-kernel
Hi Miklos,
On Fri, Jan 07, 2011 at 07:22:41PM +0100, Miklos Szeredi wrote:
> Here's an updated patch, addressing the review comments.
>
> Hiroyuki-san, can you please review the newly introduced
> mem_cgroup_replace_cache_page(), as I'm not fully familiar with the
> memory cgroup code.
>
> Thanks,
> Miklos
> ---
>
> From: Miklos Szeredi <mszeredi@suse.cz>
> Subject: mm: add replace_page_cache_page() function
>
> 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 <mszeredi@suse.cz>
> ---
> fs/fuse/dev.c | 10 +++------
> include/linux/memcontrol.h | 8 +++++++
> include/linux/pagemap.h | 1
> mm/filemap.c | 50 +++++++++++++++++++++++++++++++++++++++++++++
> mm/memcontrol.c | 38 ++++++++++++++++++++++++++++++++++
> 5 files changed, 101 insertions(+), 6 deletions(-)
>
> Index: linux-2.6/mm/filemap.c
> ===================================================================
> --- linux-2.6.orig/mm/filemap.c 2011-01-07 17:53:39.000000000 +0100
> +++ linux-2.6/mm/filemap.c 2011-01-07 19:14:45.000000000 +0100
> @@ -390,6 +390,56 @@ int filemap_write_and_wait_range(struct
> EXPORT_SYMBOL(filemap_write_and_wait_range);
>
> /**
> + * replace_page_cache_page - replace a pagecache page with a new one
> + * @old: page to be replaced
> + * @new: page to replace with
> + * @gfp_mask: page allocation mode
> + *
> + * This function replaces a page in the pagecache with a new one. On
> + * success it acquires the pagecache reference for the new page and
> + * drop it for the old page. Both the old and new pages must be
> + * locked. This function does not add the new page to the LRU, the
> + * caller must do that.
> + *
> + * The remove + add is atomic. The only way this function can fail is
> + * memory allocation failure.
> + */
> +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 = radix_tree_preload(gfp_mask & ~__GFP_HIGHMEM);
> + if (!error) {
> + 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);
Since v1, I tried to change page reference accouting semantics of
remove_from_page_cache. At last, it changes API name from __remove_from_page_cache
to __delete_from_page_cache.
So this conflicts my series.
If you are not urgent, could you resend based on my series after Andrew merges it?
Or if Andrew has a concern about my series and he merged your patch, I will resend
my series include fixing this part of your patch.
Please wait Andrew's next step.
--
Kind regards,
Minchan Kim
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] mm: add replace_page_cache_page() function
@ 2011-01-11 5:41 ` Minchan Kim
0 siblings, 0 replies; 42+ messages in thread
From: Minchan Kim @ 2011-01-11 5:41 UTC (permalink / raw)
To: Miklos Szeredi
Cc: akpm, kamezawa.hiroyu, linux-fsdevel, linux-mm, linux-kernel
Hi Miklos,
On Fri, Jan 07, 2011 at 07:22:41PM +0100, Miklos Szeredi wrote:
> Here's an updated patch, addressing the review comments.
>
> Hiroyuki-san, can you please review the newly introduced
> mem_cgroup_replace_cache_page(), as I'm not fully familiar with the
> memory cgroup code.
>
> Thanks,
> Miklos
> ---
>
> From: Miklos Szeredi <mszeredi@suse.cz>
> Subject: mm: add replace_page_cache_page() function
>
> 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 <mszeredi@suse.cz>
> ---
> fs/fuse/dev.c | 10 +++------
> include/linux/memcontrol.h | 8 +++++++
> include/linux/pagemap.h | 1
> mm/filemap.c | 50 +++++++++++++++++++++++++++++++++++++++++++++
> mm/memcontrol.c | 38 ++++++++++++++++++++++++++++++++++
> 5 files changed, 101 insertions(+), 6 deletions(-)
>
> Index: linux-2.6/mm/filemap.c
> ===================================================================
> --- linux-2.6.orig/mm/filemap.c 2011-01-07 17:53:39.000000000 +0100
> +++ linux-2.6/mm/filemap.c 2011-01-07 19:14:45.000000000 +0100
> @@ -390,6 +390,56 @@ int filemap_write_and_wait_range(struct
> EXPORT_SYMBOL(filemap_write_and_wait_range);
>
> /**
> + * replace_page_cache_page - replace a pagecache page with a new one
> + * @old: page to be replaced
> + * @new: page to replace with
> + * @gfp_mask: page allocation mode
> + *
> + * This function replaces a page in the pagecache with a new one. On
> + * success it acquires the pagecache reference for the new page and
> + * drop it for the old page. Both the old and new pages must be
> + * locked. This function does not add the new page to the LRU, the
> + * caller must do that.
> + *
> + * The remove + add is atomic. The only way this function can fail is
> + * memory allocation failure.
> + */
> +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 = radix_tree_preload(gfp_mask & ~__GFP_HIGHMEM);
> + if (!error) {
> + 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);
Since v1, I tried to change page reference accouting semantics of
remove_from_page_cache. At last, it changes API name from __remove_from_page_cache
to __delete_from_page_cache.
So this conflicts my series.
If you are not urgent, could you resend based on my series after Andrew merges it?
Or if Andrew has a concern about my series and he merged your patch, I will resend
my series include fixing this part of your patch.
Please wait Andrew's next step.
--
Kind regards,
Minchan Kim
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. 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:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] mm: add replace_page_cache_page() function
2011-01-11 2:29 ` KAMEZAWA Hiroyuki
@ 2011-01-11 3:53 ` Daisuke Nishimura
-1 siblings, 0 replies; 42+ messages in thread
From: Daisuke Nishimura @ 2011-01-11 3:53 UTC (permalink / raw)
To: KAMEZAWA Hiroyuki
Cc: Miklos Szeredi, akpm, minchan.kim, linux-fsdevel, linux-mm,
linux-kernel, Daisuke Nishimura
> What I recommend is below. (Please see the newest -mm because of a bug fix for
> mem cgroup) Considering page management on radix-tree, it can be considerd as
> a kind of page-migration, which replaces pages on radix-tree.
>
> ==
>
> > +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);
> > +
> struct mem_cgroup *memcg;
>
I think it should be initialized to NULL.
> error = mem_cgroup_prepare_migration(old, new, &memcg);
I want some comments like:
/*
* This is not page migration, but prepare_migration and end_migration
* does enough work for charge replacement.
*/
> #
> # This function will charge against "newpage". But this expects
> # the caller allows GFP_KERNEL gfp_mask.
> # After this, the newpage is in "charged" state.
> if (error)
> return -ENOMEM;
>
> > + error = radix_tree_preload(gfp_mask & ~__GFP_HIGHMEM);
> > + if (!error) {
> > + 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_replace_cache_page(old, new); <== remove this.
>
> mem_cgroup_end_migraton(memcg, old, new, true);
>
> > + page_cache_release(old);
> > + }
> else
> mem_cgroup_end_migration(memcg, old, new, false);
>
> # Here, if the 4th argument is true, old page is uncharged.
> # if the 4th argument is false, the new page is uncharged.
> # Then, "charge" of the old page will be migrated onto the new page
> # if replacement is done.
>
>
>
> > +
> > + return error;
> > +}
> > +EXPORT_SYMBOL_GPL(replace_page_cache_page);
> > +
>
> ==
>
> I think this is enough simple and this covers all memory cgroup's racy
> problems.
>
I agree.
Thanks,
Daisuke Nishimura.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] mm: add replace_page_cache_page() function
@ 2011-01-11 3:53 ` Daisuke Nishimura
0 siblings, 0 replies; 42+ messages in thread
From: Daisuke Nishimura @ 2011-01-11 3:53 UTC (permalink / raw)
To: KAMEZAWA Hiroyuki
Cc: Miklos Szeredi, akpm, minchan.kim, linux-fsdevel, linux-mm,
linux-kernel, Daisuke Nishimura
> What I recommend is below. (Please see the newest -mm because of a bug fix for
> mem cgroup) Considering page management on radix-tree, it can be considerd as
> a kind of page-migration, which replaces pages on radix-tree.
>
> ==
>
> > +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);
> > +
> struct mem_cgroup *memcg;
>
I think it should be initialized to NULL.
> error = mem_cgroup_prepare_migration(old, new, &memcg);
I want some comments like:
/*
* This is not page migration, but prepare_migration and end_migration
* does enough work for charge replacement.
*/
> #
> # This function will charge against "newpage". But this expects
> # the caller allows GFP_KERNEL gfp_mask.
> # After this, the newpage is in "charged" state.
> if (error)
> return -ENOMEM;
>
> > + error = radix_tree_preload(gfp_mask & ~__GFP_HIGHMEM);
> > + if (!error) {
> > + 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_replace_cache_page(old, new); <== remove this.
>
> mem_cgroup_end_migraton(memcg, old, new, true);
>
> > + page_cache_release(old);
> > + }
> else
> mem_cgroup_end_migration(memcg, old, new, false);
>
> # Here, if the 4th argument is true, old page is uncharged.
> # if the 4th argument is false, the new page is uncharged.
> # Then, "charge" of the old page will be migrated onto the new page
> # if replacement is done.
>
>
>
> > +
> > + return error;
> > +}
> > +EXPORT_SYMBOL_GPL(replace_page_cache_page);
> > +
>
> ==
>
> I think this is enough simple and this covers all memory cgroup's racy
> problems.
>
I agree.
Thanks,
Daisuke Nishimura.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. 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:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] mm: add replace_page_cache_page() function
2011-01-07 18:22 ` Miklos Szeredi
@ 2011-01-11 2:29 ` KAMEZAWA Hiroyuki
-1 siblings, 0 replies; 42+ messages in thread
From: KAMEZAWA Hiroyuki @ 2011-01-11 2:29 UTC (permalink / raw)
To: Miklos Szeredi
Cc: akpm, minchan.kim, linux-fsdevel, linux-mm, linux-kernel, nishimura
On Fri, 07 Jan 2011 19:22:41 +0100
Miklos Szeredi <miklos@szeredi.hu> wrote:
> Here's an updated patch, addressing the review comments.
>
> Hiroyuki-san, can you please review the newly introduced
> mem_cgroup_replace_cache_page(), as I'm not fully familiar with the
> memory cgroup code.
>
Ok. Ccing Nishimura. see below.
> Thanks,
> Miklos
> ---
>
> From: Miklos Szeredi <mszeredi@suse.cz>
> Subject: mm: add replace_page_cache_page() function
>
> 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 <mszeredi@suse.cz>
> ---
> fs/fuse/dev.c | 10 +++------
> include/linux/memcontrol.h | 8 +++++++
> include/linux/pagemap.h | 1
> mm/filemap.c | 50 +++++++++++++++++++++++++++++++++++++++++++++
> mm/memcontrol.c | 38 ++++++++++++++++++++++++++++++++++
> 5 files changed, 101 insertions(+), 6 deletions(-)
>
> Index: linux-2.6/mm/filemap.c
> ===================================================================
> --- linux-2.6.orig/mm/filemap.c 2011-01-07 17:53:39.000000000 +0100
> +++ linux-2.6/mm/filemap.c 2011-01-07 19:14:45.000000000 +0100
> @@ -390,6 +390,56 @@ int filemap_write_and_wait_range(struct
> EXPORT_SYMBOL(filemap_write_and_wait_range);
>
> /**
> + * replace_page_cache_page - replace a pagecache page with a new one
> + * @old: page to be replaced
> + * @new: page to replace with
> + * @gfp_mask: page allocation mode
> + *
> + * This function replaces a page in the pagecache with a new one. On
> + * success it acquires the pagecache reference for the new page and
> + * drop it for the old page. Both the old and new pages must be
> + * locked. This function does not add the new page to the LRU, the
> + * caller must do that.
> + *
> + * The remove + add is atomic. The only way this function can fail is
> + * memory allocation failure.
> + */
> +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 = radix_tree_preload(gfp_mask & ~__GFP_HIGHMEM);
> + if (!error) {
> + 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_replace_cache_page(old, new);
> + page_cache_release(old);
> + }
> +
> + 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
> * @mapping: the page's address_space
> Index: linux-2.6/include/linux/pagemap.h
> ===================================================================
> --- linux-2.6.orig/include/linux/pagemap.h 2011-01-07 17:53:39.000000000 +0100
> +++ linux-2.6/include/linux/pagemap.h 2011-01-07 19:14:45.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 2011-01-07 17:53:39.000000000 +0100
> +++ linux-2.6/fs/fuse/dev.c 2011-01-07 19:14:45.000000000 +0100
> @@ -737,14 +737,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))
> Index: linux-2.6/include/linux/memcontrol.h
> ===================================================================
> --- linux-2.6.orig/include/linux/memcontrol.h 2011-01-07 17:53:39.000000000 +0100
> +++ linux-2.6/include/linux/memcontrol.h 2011-01-07 19:14:45.000000000 +0100
> @@ -95,6 +95,9 @@ mem_cgroup_prepare_migration(struct page
> extern void mem_cgroup_end_migration(struct mem_cgroup *mem,
> struct page *oldpage, struct page *newpage);
>
> +extern void mem_cgroup_replace_cache_page(struct page *oldpage,
> + struct page *newpage);
> +
> /*
> * For memory reclaim.
> */
> @@ -236,6 +239,11 @@ static inline void mem_cgroup_end_migrat
> {
> }
>
> +static inline void mem_cgroup_replace_cache_page(struct page *oldpage,
> + struct page *newpage)
> +{
> +}
> +
> static inline int mem_cgroup_get_reclaim_priority(struct mem_cgroup *mem)
> {
> return 0;
> Index: linux-2.6/mm/memcontrol.c
> ===================================================================
> --- linux-2.6.orig/mm/memcontrol.c 2011-01-07 17:53:39.000000000 +0100
> +++ linux-2.6/mm/memcontrol.c 2011-01-07 19:20:41.000000000 +0100
> @@ -2905,6 +2905,44 @@ void mem_cgroup_end_migration(struct mem
> }
>
> /*
> + * This function moves the charge from oldpage to newpage. The new
> + * page must not be already charged.
> + */
> +void mem_cgroup_replace_cache_page(struct page *oldpage, struct page *newpage)
> +{
> + struct page_cgroup *old_pc;
> + struct page_cgroup *new_pc;
> + struct mem_cgroup *mem;
> +
> + if (mem_cgroup_disabled())
> + return;
> +
> + old_pc = lookup_page_cgroup(oldpage);
> + lock_page_cgroup(old_pc);
> + if (!PageCgroupUsed(old_pc)) {
> + unlock_page_cgroup(old_pc);
> + return;
> + }
> +
> + mem = old_pc->mem_cgroup;
> + css_get(&mem->css);
> + ClearPageCgroupUsed(old_pc);
> + unlock_page_cgroup(old_pc);
> +
> + new_pc = lookup_page_cgroup(newpage);
> + lock_page_cgroup(new_pc);
> + BUG_ON(PageCgroupUsed(new_pc));
> +
> + new_pc->mem_cgroup = mem;
> + smp_wmb();
> + SetPageCgroupCache(new_pc);
> + SetPageCgroupUsed(new_pc);
> + unlock_page_cgroup(new_pc);
> + css_put(&mem->css);
> +}
I think some of moving flags are lacked and this new function is not necessary.
What I recommend is below. (Please see the newest -mm because of a bug fix for
mem cgroup) Considering page management on radix-tree, it can be considerd as
a kind of page-migration, which replaces pages on radix-tree.
==
> +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);
> +
struct mem_cgroup *memcg;
error = mem_cgroup_prepare_migration(old, new, &memcg);
#
# This function will charge against "newpage". But this expects
# the caller allows GFP_KERNEL gfp_mask.
# After this, the newpage is in "charged" state.
if (error)
return -ENOMEM;
> + error = radix_tree_preload(gfp_mask & ~__GFP_HIGHMEM);
> + if (!error) {
> + 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_replace_cache_page(old, new); <== remove this.
mem_cgroup_end_migraton(memcg, old, new, true);
> + page_cache_release(old);
> + }
else
mem_cgroup_end_migration(memcg, old, new, false);
# Here, if the 4th argument is true, old page is uncharged.
# if the 4th argument is false, the new page is uncharged.
# Then, "charge" of the old page will be migrated onto the new page
# if replacement is done.
> +
> + return error;
> +}
> +EXPORT_SYMBOL_GPL(replace_page_cache_page);
> +
==
I think this is enough simple and this covers all memory cgroup's racy
problems.
Thanks,
-Kame
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] mm: add replace_page_cache_page() function
@ 2011-01-11 2:29 ` KAMEZAWA Hiroyuki
0 siblings, 0 replies; 42+ messages in thread
From: KAMEZAWA Hiroyuki @ 2011-01-11 2:29 UTC (permalink / raw)
To: Miklos Szeredi
Cc: akpm, minchan.kim, linux-fsdevel, linux-mm, linux-kernel, nishimura
On Fri, 07 Jan 2011 19:22:41 +0100
Miklos Szeredi <miklos@szeredi.hu> wrote:
> Here's an updated patch, addressing the review comments.
>
> Hiroyuki-san, can you please review the newly introduced
> mem_cgroup_replace_cache_page(), as I'm not fully familiar with the
> memory cgroup code.
>
Ok. Ccing Nishimura. see below.
> Thanks,
> Miklos
> ---
>
> From: Miklos Szeredi <mszeredi@suse.cz>
> Subject: mm: add replace_page_cache_page() function
>
> 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 <mszeredi@suse.cz>
> ---
> fs/fuse/dev.c | 10 +++------
> include/linux/memcontrol.h | 8 +++++++
> include/linux/pagemap.h | 1
> mm/filemap.c | 50 +++++++++++++++++++++++++++++++++++++++++++++
> mm/memcontrol.c | 38 ++++++++++++++++++++++++++++++++++
> 5 files changed, 101 insertions(+), 6 deletions(-)
>
> Index: linux-2.6/mm/filemap.c
> ===================================================================
> --- linux-2.6.orig/mm/filemap.c 2011-01-07 17:53:39.000000000 +0100
> +++ linux-2.6/mm/filemap.c 2011-01-07 19:14:45.000000000 +0100
> @@ -390,6 +390,56 @@ int filemap_write_and_wait_range(struct
> EXPORT_SYMBOL(filemap_write_and_wait_range);
>
> /**
> + * replace_page_cache_page - replace a pagecache page with a new one
> + * @old: page to be replaced
> + * @new: page to replace with
> + * @gfp_mask: page allocation mode
> + *
> + * This function replaces a page in the pagecache with a new one. On
> + * success it acquires the pagecache reference for the new page and
> + * drop it for the old page. Both the old and new pages must be
> + * locked. This function does not add the new page to the LRU, the
> + * caller must do that.
> + *
> + * The remove + add is atomic. The only way this function can fail is
> + * memory allocation failure.
> + */
> +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 = radix_tree_preload(gfp_mask & ~__GFP_HIGHMEM);
> + if (!error) {
> + 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_replace_cache_page(old, new);
> + page_cache_release(old);
> + }
> +
> + 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
> * @mapping: the page's address_space
> Index: linux-2.6/include/linux/pagemap.h
> ===================================================================
> --- linux-2.6.orig/include/linux/pagemap.h 2011-01-07 17:53:39.000000000 +0100
> +++ linux-2.6/include/linux/pagemap.h 2011-01-07 19:14:45.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 2011-01-07 17:53:39.000000000 +0100
> +++ linux-2.6/fs/fuse/dev.c 2011-01-07 19:14:45.000000000 +0100
> @@ -737,14 +737,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))
> Index: linux-2.6/include/linux/memcontrol.h
> ===================================================================
> --- linux-2.6.orig/include/linux/memcontrol.h 2011-01-07 17:53:39.000000000 +0100
> +++ linux-2.6/include/linux/memcontrol.h 2011-01-07 19:14:45.000000000 +0100
> @@ -95,6 +95,9 @@ mem_cgroup_prepare_migration(struct page
> extern void mem_cgroup_end_migration(struct mem_cgroup *mem,
> struct page *oldpage, struct page *newpage);
>
> +extern void mem_cgroup_replace_cache_page(struct page *oldpage,
> + struct page *newpage);
> +
> /*
> * For memory reclaim.
> */
> @@ -236,6 +239,11 @@ static inline void mem_cgroup_end_migrat
> {
> }
>
> +static inline void mem_cgroup_replace_cache_page(struct page *oldpage,
> + struct page *newpage)
> +{
> +}
> +
> static inline int mem_cgroup_get_reclaim_priority(struct mem_cgroup *mem)
> {
> return 0;
> Index: linux-2.6/mm/memcontrol.c
> ===================================================================
> --- linux-2.6.orig/mm/memcontrol.c 2011-01-07 17:53:39.000000000 +0100
> +++ linux-2.6/mm/memcontrol.c 2011-01-07 19:20:41.000000000 +0100
> @@ -2905,6 +2905,44 @@ void mem_cgroup_end_migration(struct mem
> }
>
> /*
> + * This function moves the charge from oldpage to newpage. The new
> + * page must not be already charged.
> + */
> +void mem_cgroup_replace_cache_page(struct page *oldpage, struct page *newpage)
> +{
> + struct page_cgroup *old_pc;
> + struct page_cgroup *new_pc;
> + struct mem_cgroup *mem;
> +
> + if (mem_cgroup_disabled())
> + return;
> +
> + old_pc = lookup_page_cgroup(oldpage);
> + lock_page_cgroup(old_pc);
> + if (!PageCgroupUsed(old_pc)) {
> + unlock_page_cgroup(old_pc);
> + return;
> + }
> +
> + mem = old_pc->mem_cgroup;
> + css_get(&mem->css);
> + ClearPageCgroupUsed(old_pc);
> + unlock_page_cgroup(old_pc);
> +
> + new_pc = lookup_page_cgroup(newpage);
> + lock_page_cgroup(new_pc);
> + BUG_ON(PageCgroupUsed(new_pc));
> +
> + new_pc->mem_cgroup = mem;
> + smp_wmb();
> + SetPageCgroupCache(new_pc);
> + SetPageCgroupUsed(new_pc);
> + unlock_page_cgroup(new_pc);
> + css_put(&mem->css);
> +}
I think some of moving flags are lacked and this new function is not necessary.
What I recommend is below. (Please see the newest -mm because of a bug fix for
mem cgroup) Considering page management on radix-tree, it can be considerd as
a kind of page-migration, which replaces pages on radix-tree.
==
> +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);
> +
struct mem_cgroup *memcg;
error = mem_cgroup_prepare_migration(old, new, &memcg);
#
# This function will charge against "newpage". But this expects
# the caller allows GFP_KERNEL gfp_mask.
# After this, the newpage is in "charged" state.
if (error)
return -ENOMEM;
> + error = radix_tree_preload(gfp_mask & ~__GFP_HIGHMEM);
> + if (!error) {
> + 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_replace_cache_page(old, new); <== remove this.
mem_cgroup_end_migraton(memcg, old, new, true);
> + page_cache_release(old);
> + }
else
mem_cgroup_end_migration(memcg, old, new, false);
# Here, if the 4th argument is true, old page is uncharged.
# if the 4th argument is false, the new page is uncharged.
# Then, "charge" of the old page will be migrated onto the new page
# if replacement is done.
> +
> + return error;
> +}
> +EXPORT_SYMBOL_GPL(replace_page_cache_page);
> +
==
I think this is enough simple and this covers all memory cgroup's racy
problems.
Thanks,
-Kame
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. 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:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH] mm: add replace_page_cache_page() function
@ 2011-01-07 18:22 ` Miklos Szeredi
0 siblings, 0 replies; 42+ messages in thread
From: Miklos Szeredi @ 2011-01-07 18:22 UTC (permalink / raw)
To: akpm; +Cc: minchan.kim, kamezawa.hiroyu, linux-fsdevel, linux-mm, linux-kernel
Here's an updated patch, addressing the review comments.
Hiroyuki-san, can you please review the newly introduced
mem_cgroup_replace_cache_page(), as I'm not fully familiar with the
memory cgroup code.
Thanks,
Miklos
---
From: Miklos Szeredi <mszeredi@suse.cz>
Subject: mm: add replace_page_cache_page() function
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 <mszeredi@suse.cz>
---
fs/fuse/dev.c | 10 +++------
include/linux/memcontrol.h | 8 +++++++
include/linux/pagemap.h | 1
mm/filemap.c | 50 +++++++++++++++++++++++++++++++++++++++++++++
mm/memcontrol.c | 38 ++++++++++++++++++++++++++++++++++
5 files changed, 101 insertions(+), 6 deletions(-)
Index: linux-2.6/mm/filemap.c
===================================================================
--- linux-2.6.orig/mm/filemap.c 2011-01-07 17:53:39.000000000 +0100
+++ linux-2.6/mm/filemap.c 2011-01-07 19:14:45.000000000 +0100
@@ -390,6 +390,56 @@ int filemap_write_and_wait_range(struct
EXPORT_SYMBOL(filemap_write_and_wait_range);
/**
+ * replace_page_cache_page - replace a pagecache page with a new one
+ * @old: page to be replaced
+ * @new: page to replace with
+ * @gfp_mask: page allocation mode
+ *
+ * This function replaces a page in the pagecache with a new one. On
+ * success it acquires the pagecache reference for the new page and
+ * drop it for the old page. Both the old and new pages must be
+ * locked. This function does not add the new page to the LRU, the
+ * caller must do that.
+ *
+ * The remove + add is atomic. The only way this function can fail is
+ * memory allocation failure.
+ */
+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 = radix_tree_preload(gfp_mask & ~__GFP_HIGHMEM);
+ if (!error) {
+ 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_replace_cache_page(old, new);
+ page_cache_release(old);
+ }
+
+ 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
* @mapping: the page's address_space
Index: linux-2.6/include/linux/pagemap.h
===================================================================
--- linux-2.6.orig/include/linux/pagemap.h 2011-01-07 17:53:39.000000000 +0100
+++ linux-2.6/include/linux/pagemap.h 2011-01-07 19:14:45.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 2011-01-07 17:53:39.000000000 +0100
+++ linux-2.6/fs/fuse/dev.c 2011-01-07 19:14:45.000000000 +0100
@@ -737,14 +737,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))
Index: linux-2.6/include/linux/memcontrol.h
===================================================================
--- linux-2.6.orig/include/linux/memcontrol.h 2011-01-07 17:53:39.000000000 +0100
+++ linux-2.6/include/linux/memcontrol.h 2011-01-07 19:14:45.000000000 +0100
@@ -95,6 +95,9 @@ mem_cgroup_prepare_migration(struct page
extern void mem_cgroup_end_migration(struct mem_cgroup *mem,
struct page *oldpage, struct page *newpage);
+extern void mem_cgroup_replace_cache_page(struct page *oldpage,
+ struct page *newpage);
+
/*
* For memory reclaim.
*/
@@ -236,6 +239,11 @@ static inline void mem_cgroup_end_migrat
{
}
+static inline void mem_cgroup_replace_cache_page(struct page *oldpage,
+ struct page *newpage)
+{
+}
+
static inline int mem_cgroup_get_reclaim_priority(struct mem_cgroup *mem)
{
return 0;
Index: linux-2.6/mm/memcontrol.c
===================================================================
--- linux-2.6.orig/mm/memcontrol.c 2011-01-07 17:53:39.000000000 +0100
+++ linux-2.6/mm/memcontrol.c 2011-01-07 19:20:41.000000000 +0100
@@ -2905,6 +2905,44 @@ void mem_cgroup_end_migration(struct mem
}
/*
+ * This function moves the charge from oldpage to newpage. The new
+ * page must not be already charged.
+ */
+void mem_cgroup_replace_cache_page(struct page *oldpage, struct page *newpage)
+{
+ struct page_cgroup *old_pc;
+ struct page_cgroup *new_pc;
+ struct mem_cgroup *mem;
+
+ if (mem_cgroup_disabled())
+ return;
+
+ old_pc = lookup_page_cgroup(oldpage);
+ lock_page_cgroup(old_pc);
+ if (!PageCgroupUsed(old_pc)) {
+ unlock_page_cgroup(old_pc);
+ return;
+ }
+
+ mem = old_pc->mem_cgroup;
+ css_get(&mem->css);
+ ClearPageCgroupUsed(old_pc);
+ unlock_page_cgroup(old_pc);
+
+ new_pc = lookup_page_cgroup(newpage);
+ lock_page_cgroup(new_pc);
+ BUG_ON(PageCgroupUsed(new_pc));
+
+ new_pc->mem_cgroup = mem;
+ smp_wmb();
+ SetPageCgroupCache(new_pc);
+ SetPageCgroupUsed(new_pc);
+ unlock_page_cgroup(new_pc);
+ css_put(&mem->css);
+}
+
+
+/*
* A call to try to shrink memory usage on charge failure at shmem's swapin.
* Calling hierarchical_reclaim is not enough because we should update
* last_oom_jiffies to prevent pagefault_out_of_memory from invoking global OOM.
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH] mm: add replace_page_cache_page() function
@ 2011-01-07 18:22 ` Miklos Szeredi
0 siblings, 0 replies; 42+ messages in thread
From: Miklos Szeredi @ 2011-01-07 18:22 UTC (permalink / raw)
To: akpm; +Cc: minchan.kim, kamezawa.hiroyu, linux-fsdevel, linux-mm, linux-kernel
Here's an updated patch, addressing the review comments.
Hiroyuki-san, can you please review the newly introduced
mem_cgroup_replace_cache_page(), as I'm not fully familiar with the
memory cgroup code.
Thanks,
Miklos
---
From: Miklos Szeredi <mszeredi@suse.cz>
Subject: mm: add replace_page_cache_page() function
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 <mszeredi@suse.cz>
---
fs/fuse/dev.c | 10 +++------
include/linux/memcontrol.h | 8 +++++++
include/linux/pagemap.h | 1
mm/filemap.c | 50 +++++++++++++++++++++++++++++++++++++++++++++
mm/memcontrol.c | 38 ++++++++++++++++++++++++++++++++++
5 files changed, 101 insertions(+), 6 deletions(-)
Index: linux-2.6/mm/filemap.c
===================================================================
--- linux-2.6.orig/mm/filemap.c 2011-01-07 17:53:39.000000000 +0100
+++ linux-2.6/mm/filemap.c 2011-01-07 19:14:45.000000000 +0100
@@ -390,6 +390,56 @@ int filemap_write_and_wait_range(struct
EXPORT_SYMBOL(filemap_write_and_wait_range);
/**
+ * replace_page_cache_page - replace a pagecache page with a new one
+ * @old: page to be replaced
+ * @new: page to replace with
+ * @gfp_mask: page allocation mode
+ *
+ * This function replaces a page in the pagecache with a new one. On
+ * success it acquires the pagecache reference for the new page and
+ * drop it for the old page. Both the old and new pages must be
+ * locked. This function does not add the new page to the LRU, the
+ * caller must do that.
+ *
+ * The remove + add is atomic. The only way this function can fail is
+ * memory allocation failure.
+ */
+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 = radix_tree_preload(gfp_mask & ~__GFP_HIGHMEM);
+ if (!error) {
+ 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_replace_cache_page(old, new);
+ page_cache_release(old);
+ }
+
+ 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
* @mapping: the page's address_space
Index: linux-2.6/include/linux/pagemap.h
===================================================================
--- linux-2.6.orig/include/linux/pagemap.h 2011-01-07 17:53:39.000000000 +0100
+++ linux-2.6/include/linux/pagemap.h 2011-01-07 19:14:45.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 2011-01-07 17:53:39.000000000 +0100
+++ linux-2.6/fs/fuse/dev.c 2011-01-07 19:14:45.000000000 +0100
@@ -737,14 +737,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))
Index: linux-2.6/include/linux/memcontrol.h
===================================================================
--- linux-2.6.orig/include/linux/memcontrol.h 2011-01-07 17:53:39.000000000 +0100
+++ linux-2.6/include/linux/memcontrol.h 2011-01-07 19:14:45.000000000 +0100
@@ -95,6 +95,9 @@ mem_cgroup_prepare_migration(struct page
extern void mem_cgroup_end_migration(struct mem_cgroup *mem,
struct page *oldpage, struct page *newpage);
+extern void mem_cgroup_replace_cache_page(struct page *oldpage,
+ struct page *newpage);
+
/*
* For memory reclaim.
*/
@@ -236,6 +239,11 @@ static inline void mem_cgroup_end_migrat
{
}
+static inline void mem_cgroup_replace_cache_page(struct page *oldpage,
+ struct page *newpage)
+{
+}
+
static inline int mem_cgroup_get_reclaim_priority(struct mem_cgroup *mem)
{
return 0;
Index: linux-2.6/mm/memcontrol.c
===================================================================
--- linux-2.6.orig/mm/memcontrol.c 2011-01-07 17:53:39.000000000 +0100
+++ linux-2.6/mm/memcontrol.c 2011-01-07 19:20:41.000000000 +0100
@@ -2905,6 +2905,44 @@ void mem_cgroup_end_migration(struct mem
}
/*
+ * This function moves the charge from oldpage to newpage. The new
+ * page must not be already charged.
+ */
+void mem_cgroup_replace_cache_page(struct page *oldpage, struct page *newpage)
+{
+ struct page_cgroup *old_pc;
+ struct page_cgroup *new_pc;
+ struct mem_cgroup *mem;
+
+ if (mem_cgroup_disabled())
+ return;
+
+ old_pc = lookup_page_cgroup(oldpage);
+ lock_page_cgroup(old_pc);
+ if (!PageCgroupUsed(old_pc)) {
+ unlock_page_cgroup(old_pc);
+ return;
+ }
+
+ mem = old_pc->mem_cgroup;
+ css_get(&mem->css);
+ ClearPageCgroupUsed(old_pc);
+ unlock_page_cgroup(old_pc);
+
+ new_pc = lookup_page_cgroup(newpage);
+ lock_page_cgroup(new_pc);
+ BUG_ON(PageCgroupUsed(new_pc));
+
+ new_pc->mem_cgroup = mem;
+ smp_wmb();
+ SetPageCgroupCache(new_pc);
+ SetPageCgroupUsed(new_pc);
+ unlock_page_cgroup(new_pc);
+ css_put(&mem->css);
+}
+
+
+/*
* A call to try to shrink memory usage on charge failure at shmem's swapin.
* Calling hierarchical_reclaim is not enough because we should update
* last_oom_jiffies to prevent pagefault_out_of_memory from invoking global OOM.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. 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:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 42+ messages in thread
end of thread, other threads:[~2011-01-11 9:38 UTC | newest]
Thread overview: 42+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-12-15 15:49 [PATCH] mm: add replace_page_cache_page() function Miklos Szeredi
2010-12-15 15:49 ` Miklos Szeredi
2010-12-15 16:49 ` Rik van Riel
2010-12-15 16:49 ` Rik van Riel
2010-12-15 23:22 ` Minchan Kim
2010-12-15 23:22 ` Minchan Kim
2010-12-15 23:22 ` Minchan Kim
2010-12-16 11:59 ` Miklos Szeredi
2010-12-16 11:59 ` Miklos Szeredi
2010-12-16 22:04 ` Minchan Kim
2010-12-16 22:04 ` Minchan Kim
2010-12-17 1:21 ` Hugh Dickins
2010-12-17 1:21 ` Hugh Dickins
2010-12-17 1:40 ` Minchan Kim
2010-12-17 1:40 ` Minchan Kim
2010-12-17 2:10 ` Hugh Dickins
2010-12-17 2:10 ` Hugh Dickins
2010-12-17 4:37 ` Minchan Kim
2010-12-17 4:37 ` Minchan Kim
2010-12-17 15:53 ` Miklos Szeredi
2010-12-17 15:53 ` Miklos Szeredi
2010-12-16 1:07 ` KAMEZAWA Hiroyuki
2010-12-16 1:07 ` KAMEZAWA Hiroyuki
2010-12-16 12:05 ` Miklos Szeredi
2010-12-16 12:05 ` Miklos Szeredi
2010-12-17 0:01 ` KAMEZAWA Hiroyuki
2010-12-17 0:01 ` KAMEZAWA Hiroyuki
2010-12-17 15:51 ` Miklos Szeredi
2010-12-17 15:51 ` Miklos Szeredi
2010-12-19 23:54 ` KAMEZAWA Hiroyuki
2010-12-19 23:54 ` KAMEZAWA Hiroyuki
2011-01-07 18:22 Miklos Szeredi
2011-01-07 18:22 ` Miklos Szeredi
2011-01-11 2:29 ` KAMEZAWA Hiroyuki
2011-01-11 2:29 ` KAMEZAWA Hiroyuki
2011-01-11 3:53 ` Daisuke Nishimura
2011-01-11 3:53 ` Daisuke Nishimura
2011-01-11 9:38 ` Miklos Szeredi
2011-01-11 9:38 ` Miklos Szeredi
2011-01-11 9:38 ` Miklos Szeredi
2011-01-11 5:41 ` Minchan Kim
2011-01-11 5:41 ` Minchan Kim
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.