I copy description of 1/5.
Now we add page reference on add_to_page_cache but doesn't drop it
in remove_from_page_cache. Such asymmetric makes confusing about
page reference so that caller should notice it and comment why they
release page reference. It's not good API.
Long time ago, Hugh tried it[1] but gave up of reason which
reiser4's drop_page had to unlock the page between removing it from
page cache and doing the page_cache_release. But now the situation is
changed. I think at least things in current mainline doesn't have any
obstacles. The problem is fs or somethings out of mainline.
If it has done such thing like reiser4, this patch could be a problem.
Do anyone know the such things? Do we care about things out of mainline?
[1] http://lkml.org/lkml/2004/10/24/140
Minchan Kim (5):
drop page reference on remove_from_page_cache
fuse: Remove unnecessary page release
tlbfs: Remove unnecessary page release
swap: Remove unnecessary page release
truncate: Remove unnecessary page release
fs/fuse/dev.c | 1 -
fs/hugetlbfs/inode.c | 1 -
mm/filemap.c | 12 ++++++++++++
mm/shmem.c | 1 -
mm/truncate.c | 1 -
5 files changed, 12 insertions(+), 4 deletions(-)
Now we add page reference on add_to_page_cache but doesn't drop it
in remove_from_page_cache. Such asymmetric makes confusing about
page reference so that caller should notice it and comment why they
release page reference. It's not good API.
Long time ago, Hugh tried it[1] but gave up of reason which
reiser4's drop_page had to unlock the page between removing it from
page cache and doing the page_cache_release. But now the situation is
changed. I think at least things in current mainline doesn't have any
obstacles. The problem is fs or somethings out of mainline.
If it has done such thing like reiser4, this patch could be a problem.
Do anyone know the such things? Do we care about things out of mainline?
Note :
The comment of remove_from_page_cache make by copy & paste & s/swap/page/
from delete_from_swap_cache.
[1] http://lkml.org/lkml/2004/10/24/140
Cc: Hugh Dickins <[email protected]>
Cc: [email protected]
Signed-off-by: Minchan Kim <[email protected]>
---
mm/filemap.c | 12 ++++++++++++
1 files changed, 12 insertions(+), 0 deletions(-)
diff --git a/mm/filemap.c b/mm/filemap.c
index 095c393..fb9db36 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -148,6 +148,16 @@ void __remove_from_page_cache(struct page *page)
}
}
+/**
+ * remove_from_page_cache - remove page from page cache
+ *
+ * @page: the page which the kernel is trying to remove from page cache
+ *
+ * This must be called only on pages that have
+ * been verified to be in the page cache and locked.
+ * It will never put the page into the free list,
+ * the caller has a reference on the page.
+ */
void remove_from_page_cache(struct page *page)
{
struct address_space *mapping = page->mapping;
@@ -163,6 +173,8 @@ void remove_from_page_cache(struct page *page)
if (freepage)
freepage(page);
+
+ page_cache_release(page);
}
EXPORT_SYMBOL(remove_from_page_cache);
--
1.7.0.4
This patch series changes remove_from_page_cache's page ref counting
rule. page cache ref count is decreased in remove_from_page_cache.
So we don't need call again in caller context.
Cc:Hugh Dickins <[email protected]>
Cc: [email protected]
Signed-off-by: Minchan Kim <[email protected]>
---
mm/shmem.c | 1 -
1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/mm/shmem.c b/mm/shmem.c
index fa9acc9..16800f2 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -1091,7 +1091,6 @@ static int shmem_writepage(struct page *page, struct writeback_control *wbc)
spin_unlock(&info->lock);
swap_shmem_alloc(swap);
BUG_ON(page_mapped(page));
- page_cache_release(page); /* pagecache ref */
swap_writepage(page, wbc);
if (inode) {
mutex_lock(&shmem_swaplist_mutex);
--
1.7.0.4
This patch series changes remove_from_page_cache's page ref counting
rule. page cache ref count is decreased in remove_from_page_cache.
So we don't need call again in caller context.
Cc: Nick Piggin <[email protected]>
Cc: Al Viro <[email protected]>
Cc: [email protected]
Signed-off-by: Minchan Kim <[email protected]>
---
mm/truncate.c | 1 -
1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/mm/truncate.c b/mm/truncate.c
index 9ee5673..8decb93 100644
--- a/mm/truncate.c
+++ b/mm/truncate.c
@@ -114,7 +114,6 @@ truncate_complete_page(struct address_space *mapping, struct page *page)
* calls cleancache_put_page (and note page->mapping is now NULL)
*/
cleancache_flush_page(mapping, page);
- page_cache_release(page); /* pagecache ref */
return 0;
}
--
1.7.0.4
This patch series changes remove_from_page_cache's page ref counting
rule. page cache ref count is decreased in remove_from_page_cache.
So we don't need call again in caller context.
Cc: Miklos Szeredi <[email protected]>
Cc: [email protected]
Signed-off-by: Minchan Kim <[email protected]>
---
fs/fuse/dev.c | 1 -
1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index cf8d28d..4adaf4b 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -738,7 +738,6 @@ static int fuse_try_move_page(struct fuse_copy_state *cs, struct page **pagep)
goto out_fallback_unlock;
remove_from_page_cache(oldpage);
- page_cache_release(oldpage);
err = add_to_page_cache_locked(newpage, mapping, index, GFP_KERNEL);
if (err) {
--
1.7.0.4
This patch series changes remove_from_page_cache's page ref counting
rule. page cache ref count is decreased in remove_from_page_cache.
So we don't need call again in caller context.
Cc: William Irwin <[email protected]>
Signed-off-by: Minchan Kim <[email protected]>
---
fs/hugetlbfs/inode.c | 1 -
1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index 9885082..4f32fb6 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -333,7 +333,6 @@ static void truncate_huge_page(struct page *page)
cancel_dirty_page(page, /* No IO accounting for huge pages? */0);
ClearPageUptodate(page);
remove_from_page_cache(page);
- put_page(page);
}
static void truncate_hugepages(struct inode *inode, loff_t lstart)
--
1.7.0.4
On Sat, 18 Dec 2010 02:13:36 +0900
Minchan Kim <[email protected]> wrote:
> Now we add page reference on add_to_page_cache but doesn't drop it
> in remove_from_page_cache. Such asymmetric makes confusing about
> page reference so that caller should notice it and comment why they
> release page reference. It's not good API.
>
> Long time ago, Hugh tried it[1] but gave up of reason which
> reiser4's drop_page had to unlock the page between removing it from
> page cache and doing the page_cache_release. But now the situation is
> changed. I think at least things in current mainline doesn't have any
> obstacles. The problem is fs or somethings out of mainline.
> If it has done such thing like reiser4, this patch could be a problem.
>
> Do anyone know the such things? Do we care about things out of mainline?
>
> Note :
> The comment of remove_from_page_cache make by copy & paste & s/swap/page/
> from delete_from_swap_cache.
>
> [1] http://lkml.org/lkml/2004/10/24/140
>
> Cc: Hugh Dickins <[email protected]>
> Cc: [email protected]
> Signed-off-by: Minchan Kim <[email protected]>
I like this.
Reviewd-by: KAMEZAWA Hiroyuki <[email protected]>
On Sat, 18 Dec 2010 02:13:37 +0900
Minchan Kim <[email protected]> wrote:
> This patch series changes remove_from_page_cache's page ref counting
> rule. page cache ref count is decreased in remove_from_page_cache.
> So we don't need call again in caller context.
>
> Cc: Miklos Szeredi <[email protected]>
> Cc: [email protected]
> Signed-off-by: Minchan Kim <[email protected]>
Reviewed-by: KAMEZAWA Hiroyuki <[email protected]>
On Sat, 18 Dec 2010 02:13:39 +0900
Minchan Kim <[email protected]> wrote:
> This patch series changes remove_from_page_cache's page ref counting
> rule. page cache ref count is decreased in remove_from_page_cache.
> So we don't need call again in caller context.
>
> Cc:Hugh Dickins <[email protected]>
> Cc: [email protected]
> Signed-off-by: Minchan Kim <[email protected]>
Reviewed-by: KAMEZAWA Hiroyuki <[email protected]>
On Sat, 18 Dec 2010 02:13:40 +0900
Minchan Kim <[email protected]> wrote:
> This patch series changes remove_from_page_cache's page ref counting
> rule. page cache ref count is decreased in remove_from_page_cache.
> So we don't need call again in caller context.
>
> Cc: Nick Piggin <[email protected]>
> Cc: Al Viro <[email protected]>
> Cc: [email protected]
> Signed-off-by: Minchan Kim <[email protected]>
Reviewed-by: KAMEZAWA Hiroyuki <[email protected]>
On Sat, 18 Dec 2010 02:13:38 +0900
Minchan Kim <[email protected]> wrote:
> This patch series changes remove_from_page_cache's page ref counting
> rule. page cache ref count is decreased in remove_from_page_cache.
> So we don't need call again in caller context.
>
> Cc: William Irwin <[email protected]>
> Signed-off-by: Minchan Kim <[email protected]>
Reviewed-by: KAMEZAWA Hiroyuki <[email protected]>
> This patch series changes remove_from_page_cache's page ref counting
> rule. page cache ref count is decreased in remove_from_page_cache.
> So we don't need call again in caller context.
>
> Cc: Nick Piggin <[email protected]>
> Cc: Al Viro <[email protected]>
> Cc: [email protected]
> Signed-off-by: Minchan Kim <[email protected]>
> ---
> mm/truncate.c | 1 -
> 1 files changed, 0 insertions(+), 1 deletions(-)
>
> diff --git a/mm/truncate.c b/mm/truncate.c
> index 9ee5673..8decb93 100644
> --- a/mm/truncate.c
> +++ b/mm/truncate.c
> @@ -114,7 +114,6 @@ truncate_complete_page(struct address_space *mapping, struct page *page)
> * calls cleancache_put_page (and note page->mapping is now NULL)
> */
> cleancache_flush_page(mapping, page);
> - page_cache_release(page); /* pagecache ref */
> return 0;
Do we _always_ have stable page reference here? IOW, I can assume
cleancache_flush_page() doesn't cause NULL deref?
On Mon, Dec 20, 2010 at 11:21 AM, KOSAKI Motohiro
<[email protected]> wrote:
>> This patch series changes remove_from_page_cache's page ref counting
>> rule. page cache ref count is decreased in remove_from_page_cache.
>> So we don't need call again in caller context.
>>
>> Cc: Nick Piggin <[email protected]>
>> Cc: Al Viro <[email protected]>
>> Cc: [email protected]
>> Signed-off-by: Minchan Kim <[email protected]>
>> ---
>> ?mm/truncate.c | ? ?1 -
>> ?1 files changed, 0 insertions(+), 1 deletions(-)
>>
>> diff --git a/mm/truncate.c b/mm/truncate.c
>> index 9ee5673..8decb93 100644
>> --- a/mm/truncate.c
>> +++ b/mm/truncate.c
>> @@ -114,7 +114,6 @@ truncate_complete_page(struct address_space *mapping, struct page *page)
>> ? ? ? ?* calls cleancache_put_page (and note page->mapping is now NULL)
>> ? ? ? ?*/
>> ? ? ? cleancache_flush_page(mapping, page);
>> - ? ? page_cache_release(page); ? ? ? /* pagecache ref */
>> ? ? ? return 0;
>
> Do we _always_ have stable page reference here? IOW, I can assume
I think so.
Because the page is locked so caller have to hold a ref to unlock it.
> cleancache_flush_page() doesn't cause NULL deref?
>
>
>
>
--
Kind regards,
Minchan Kim
> On Mon, Dec 20, 2010 at 11:21 AM, KOSAKI Motohiro
> <[email protected]> wrote:
> >> This patch series changes remove_from_page_cache's page ref counting
> >> rule. page cache ref count is decreased in remove_from_page_cache.
> >> So we don't need call again in caller context.
> >>
> >> Cc: Nick Piggin <[email protected]>
> >> Cc: Al Viro <[email protected]>
> >> Cc: [email protected]
> >> Signed-off-by: Minchan Kim <[email protected]>
> >> ---
> >> ?mm/truncate.c | ? ?1 -
> >> ?1 files changed, 0 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/mm/truncate.c b/mm/truncate.c
> >> index 9ee5673..8decb93 100644
> >> --- a/mm/truncate.c
> >> +++ b/mm/truncate.c
> >> @@ -114,7 +114,6 @@ truncate_complete_page(struct address_space *mapping, struct page *page)
> >> ? ? ? ?* calls cleancache_put_page (and note page->mapping is now NULL)
> >> ? ? ? ?*/
> >> ? ? ? cleancache_flush_page(mapping, page);
> >> - ? ? page_cache_release(page); ? ? ? /* pagecache ref */
> >> ? ? ? return 0;
> >
> > Do we _always_ have stable page reference here? IOW, I can assume
>
> I think so.
> Because the page is locked so caller have to hold a ref to unlock it.
Hmm...
Perhaps, I'm missing something. But I think __memory_failure() only lock
compaund_head page. not all. example.
>
> > cleancache_flush_page() doesn't cause NULL deref?
On Mon, 20 Dec 2010 11:21:52 +0900 (JST)
KOSAKI Motohiro <[email protected]> wrote:
> > This patch series changes remove_from_page_cache's page ref counting
> > rule. page cache ref count is decreased in remove_from_page_cache.
> > So we don't need call again in caller context.
> >
> > Cc: Nick Piggin <[email protected]>
> > Cc: Al Viro <[email protected]>
> > Cc: [email protected]
> > Signed-off-by: Minchan Kim <[email protected]>
> > ---
> > mm/truncate.c | 1 -
> > 1 files changed, 0 insertions(+), 1 deletions(-)
> >
> > diff --git a/mm/truncate.c b/mm/truncate.c
> > index 9ee5673..8decb93 100644
> > --- a/mm/truncate.c
> > +++ b/mm/truncate.c
> > @@ -114,7 +114,6 @@ truncate_complete_page(struct address_space *mapping, struct page *page)
> > * calls cleancache_put_page (and note page->mapping is now NULL)
> > */
> > cleancache_flush_page(mapping, page);
> > - page_cache_release(page); /* pagecache ref */
> > return 0;
>
> Do we _always_ have stable page reference here? IOW, I can assume
> cleancache_flush_page() doesn't cause NULL deref?
>
Hmm, my review was bad.
I think cleancache_flush_page() here should eat (mapping, index) as argument
rather than "page".
BTW, I can't understand
==
void __cleancache_flush_page(struct address_space *mapping, struct page *page)
{
/* careful... page->mapping is NULL sometimes when this is called */
int pool_id = mapping->host->i_sb->cleancache_poolid;
struct cleancache_filekey key = { .u.key = { 0 } };
==
Why above is safe...
I think (mapping,index) should be passed instead of page.
-Kame
On Mon, Dec 20, 2010 at 11:32 AM, KOSAKI Motohiro
<[email protected]> wrote:
>> On Mon, Dec 20, 2010 at 11:21 AM, KOSAKI Motohiro
>> <[email protected]> wrote:
>> >> This patch series changes remove_from_page_cache's page ref counting
>> >> rule. page cache ref count is decreased in remove_from_page_cache.
>> >> So we don't need call again in caller context.
>> >>
>> >> Cc: Nick Piggin <[email protected]>
>> >> Cc: Al Viro <[email protected]>
>> >> Cc: [email protected]
>> >> Signed-off-by: Minchan Kim <[email protected]>
>> >> ---
>> >> ?mm/truncate.c | ? ?1 -
>> >> ?1 files changed, 0 insertions(+), 1 deletions(-)
>> >>
>> >> diff --git a/mm/truncate.c b/mm/truncate.c
>> >> index 9ee5673..8decb93 100644
>> >> --- a/mm/truncate.c
>> >> +++ b/mm/truncate.c
>> >> @@ -114,7 +114,6 @@ truncate_complete_page(struct address_space *mapping, struct page *page)
>> >> ? ? ? ?* calls cleancache_put_page (and note page->mapping is now NULL)
>> >> ? ? ? ?*/
>> >> ? ? ? cleancache_flush_page(mapping, page);
>> >> - ? ? page_cache_release(page); ? ? ? /* pagecache ref */
>> >> ? ? ? return 0;
>> >
>> > Do we _always_ have stable page reference here? IOW, I can assume
>>
>> I think so.
>> Because the page is locked so caller have to hold a ref to unlock it.
>
> Hmm...
>
> Perhaps, I'm missing something. But I think ?__memory_failure() only lock
> compaund_head page. not all. example.
The page passed truncate_complete_page is only head page?
Is it possible to pass the page which isn't head of compound in
truncate_complete_page?
>
>>
>> > cleancache_flush_page() doesn't cause NULL deref?
>
>
>
>
--
Kind regards,
Minchan Kim
On Mon, Dec 20, 2010 at 11:27 AM, KAMEZAWA Hiroyuki
<[email protected]> wrote:
> On Mon, 20 Dec 2010 11:21:52 +0900 (JST)
> KOSAKI Motohiro <[email protected]> wrote:
>
>> > This patch series changes remove_from_page_cache's page ref counting
>> > rule. page cache ref count is decreased in remove_from_page_cache.
>> > So we don't need call again in caller context.
>> >
>> > Cc: Nick Piggin <[email protected]>
>> > Cc: Al Viro <[email protected]>
>> > Cc: [email protected]
>> > Signed-off-by: Minchan Kim <[email protected]>
>> > ---
>> > ?mm/truncate.c | ? ?1 -
>> > ?1 files changed, 0 insertions(+), 1 deletions(-)
>> >
>> > diff --git a/mm/truncate.c b/mm/truncate.c
>> > index 9ee5673..8decb93 100644
>> > --- a/mm/truncate.c
>> > +++ b/mm/truncate.c
>> > @@ -114,7 +114,6 @@ truncate_complete_page(struct address_space *mapping, struct page *page)
>> > ? ? ?* calls cleancache_put_page (and note page->mapping is now NULL)
>> > ? ? ?*/
>> > ? ? cleancache_flush_page(mapping, page);
>> > - ? page_cache_release(page); ? ? ? /* pagecache ref */
>> > ? ? return 0;
>>
>> Do we _always_ have stable page reference here? IOW, I can assume
>> cleancache_flush_page() doesn't cause NULL deref?
>>
> Hmm, my review was bad.
>
> I think cleancache_flush_page() here should eat (mapping, index) as argument
> rather than "page".
>
> BTW, ?I can't understand
> ==
> void __cleancache_flush_page(struct address_space *mapping, struct page *page)
> {
> ? ? ? ?/* careful... page->mapping is NULL sometimes when this is called */
> ? ? ? ?int pool_id = mapping->host->i_sb->cleancache_poolid;
> ? ? ? ?struct cleancache_filekey key = { .u.key = { 0 } };
> ==
>
> Why above is safe...
> I think (mapping,index) should be passed instead of page.
I don't think current code isn't safe.
void __cleancache_flush_page(struct address_space *mapping, struct page *page)
{
/* careful... page->mapping is NULL sometimes when this is called */
int pool_id = mapping->host->i_sb->cleancache_poolid;
struct cleancache_filekey key = { .u.key = { 0 } };
if (pool_id >= 0) {
VM_BUG_ON(!PageLocked(page));
it does check PageLocked. So caller should hold a page reference to
prevent freeing ramined PG_locked
If the caller doesn't hold a ref of page, I think it's BUG of caller.
In our case, caller calls truncate_complete_page have to make sure it, I think.
>
>
> -Kame
>
>
--
Kind regards,
Minchan Kim
> On Mon, Dec 20, 2010 at 11:32 AM, KOSAKI Motohiro
> <[email protected]> wrote:
> >> On Mon, Dec 20, 2010 at 11:21 AM, KOSAKI Motohiro
> >> <[email protected]> wrote:
> >> >> This patch series changes remove_from_page_cache's page ref counting
> >> >> rule. page cache ref count is decreased in remove_from_page_cache.
> >> >> So we don't need call again in caller context.
> >> >>
> >> >> Cc: Nick Piggin <[email protected]>
> >> >> Cc: Al Viro <[email protected]>
> >> >> Cc: [email protected]
> >> >> Signed-off-by: Minchan Kim <[email protected]>
> >> >> ---
> >> >> ?mm/truncate.c | ? ?1 -
> >> >> ?1 files changed, 0 insertions(+), 1 deletions(-)
> >> >>
> >> >> diff --git a/mm/truncate.c b/mm/truncate.c
> >> >> index 9ee5673..8decb93 100644
> >> >> --- a/mm/truncate.c
> >> >> +++ b/mm/truncate.c
> >> >> @@ -114,7 +114,6 @@ truncate_complete_page(struct address_space *mapping, struct page *page)
> >> >> ? ? ? ?* calls cleancache_put_page (and note page->mapping is now NULL)
> >> >> ? ? ? ?*/
> >> >> ? ? ? cleancache_flush_page(mapping, page);
> >> >> - ? ? page_cache_release(page); ? ? ? /* pagecache ref */
> >> >> ? ? ? return 0;
> >> >
> >> > Do we _always_ have stable page reference here? IOW, I can assume
> >>
> >> I think so.
> >> Because the page is locked so caller have to hold a ref to unlock it.
> >
> > Hmm...
> >
> > Perhaps, I'm missing something. But I think ?__memory_failure() only lock
> > compaund_head page. not all. example.
>
> The page passed truncate_complete_page is only head page?
> Is it possible to pass the page which isn't head of compound in
> truncate_complete_page?
I dunno, really. My five miniture grep found following logic. therefore I asked you.
__memory_failure()
{
p = pfn_to_page(pfn);
hpage = compound_head(p);
(snip)
res = -EBUSY;
for (ps = error_states;; ps++) {
if ((p->flags & ps->mask) == ps->res) {
res = page_action(ps, p, pfn); // call truncate here
break;
}
}
out:
unlock_page(hpage);
}
On Mon, Dec 20, 2010 at 12:03 PM, KOSAKI Motohiro
<[email protected]> wrote:
>> On Mon, Dec 20, 2010 at 11:32 AM, KOSAKI Motohiro
>> <[email protected]> wrote:
>> >> On Mon, Dec 20, 2010 at 11:21 AM, KOSAKI Motohiro
>> >> <[email protected]> wrote:
>> >> >> This patch series changes remove_from_page_cache's page ref counting
>> >> >> rule. page cache ref count is decreased in remove_from_page_cache.
>> >> >> So we don't need call again in caller context.
>> >> >>
>> >> >> Cc: Nick Piggin <[email protected]>
>> >> >> Cc: Al Viro <[email protected]>
>> >> >> Cc: [email protected]
>> >> >> Signed-off-by: Minchan Kim <[email protected]>
>> >> >> ---
>> >> >> ?mm/truncate.c | ? ?1 -
>> >> >> ?1 files changed, 0 insertions(+), 1 deletions(-)
>> >> >>
>> >> >> diff --git a/mm/truncate.c b/mm/truncate.c
>> >> >> index 9ee5673..8decb93 100644
>> >> >> --- a/mm/truncate.c
>> >> >> +++ b/mm/truncate.c
>> >> >> @@ -114,7 +114,6 @@ truncate_complete_page(struct address_space *mapping, struct page *page)
>> >> >> ? ? ? ?* calls cleancache_put_page (and note page->mapping is now NULL)
>> >> >> ? ? ? ?*/
>> >> >> ? ? ? cleancache_flush_page(mapping, page);
>> >> >> - ? ? page_cache_release(page); ? ? ? /* pagecache ref */
>> >> >> ? ? ? return 0;
>> >> >
>> >> > Do we _always_ have stable page reference here? IOW, I can assume
>> >>
>> >> I think so.
>> >> Because the page is locked so caller have to hold a ref to unlock it.
>> >
>> > Hmm...
>> >
>> > Perhaps, I'm missing something. But I think ?__memory_failure() only lock
>> > compaund_head page. not all. example.
>>
>> The page passed truncate_complete_page is only head page?
>> Is it possible to pass the page which isn't head of compound in
>> truncate_complete_page?
>
> I dunno, really. My five miniture grep found following logic. therefore I asked you.
>
>
>
> __memory_failure()
> {
> ? ? ? ?p = pfn_to_page(pfn);
> ? ? ? ?hpage = compound_head(p);
> (snip)
> ? ? ? ?res = -EBUSY;
> ? ? ? ?for (ps = error_states;; ps++) {
> ? ? ? ? ? ? ? ?if ((p->flags & ps->mask) == ps->res) {
> ? ? ? ? ? ? ? ? ? ? ? ?res = page_action(ps, p, pfn); ?// call truncate here
> ? ? ? ? ? ? ? ? ? ? ? ?break;
> ? ? ? ? ? ? ? ?}
> ? ? ? ?}
> out:
> ? ? ? ?unlock_page(hpage);
> }
>
>
AFAIK, We have to handle head page when we handle compound page.
Internal page handling logic about tail pages is hidden by compound
page internal.
So I think memory_failure also don't have a problem.
For needing double check, Cced Andi.
Thanks for the review, KOSAKI.
--
Kind regards,
Minchan Kim
On Mon, 20 Dec 2010 11:58:50 +0900
Minchan Kim <[email protected]> wrote:
> On Mon, Dec 20, 2010 at 11:27 AM, KAMEZAWA Hiroyuki
> <[email protected]> wrote:
> > On Mon, 20 Dec 2010 11:21:52 +0900 (JST)
> > KOSAKI Motohiro <[email protected]> wrote:
> >
> >> > This patch series changes remove_from_page_cache's page ref counting
> >> > rule. page cache ref count is decreased in remove_from_page_cache.
> >> > So we don't need call again in caller context.
> >> >
> >> > Cc: Nick Piggin <[email protected]>
> >> > Cc: Al Viro <[email protected]>
> >> > Cc: [email protected]
> >> > Signed-off-by: Minchan Kim <[email protected]>
> >> > ---
> >> > mm/truncate.c | 1 -
> >> > 1 files changed, 0 insertions(+), 1 deletions(-)
> >> >
> >> > diff --git a/mm/truncate.c b/mm/truncate.c
> >> > index 9ee5673..8decb93 100644
> >> > --- a/mm/truncate.c
> >> > +++ b/mm/truncate.c
> >> > @@ -114,7 +114,6 @@ truncate_complete_page(struct address_space *mapping, struct page *page)
> >> > * calls cleancache_put_page (and note page->mapping is now NULL)
> >> > */
> >> > cleancache_flush_page(mapping, page);
> >> > - page_cache_release(page); /* pagecache ref */
> >> > return 0;
> >>
> >> Do we _always_ have stable page reference here? IOW, I can assume
> >> cleancache_flush_page() doesn't cause NULL deref?
> >>
> > Hmm, my review was bad.
> >
> > I think cleancache_flush_page() here should eat (mapping, index) as argument
> > rather than "page".
> >
> > BTW, I can't understand
> > ==
> > void __cleancache_flush_page(struct address_space *mapping, struct page *page)
> > {
> > /* careful... page->mapping is NULL sometimes when this is called */
> > int pool_id = mapping->host->i_sb->cleancache_poolid;
> > struct cleancache_filekey key = { .u.key = { 0 } };
> > ==
> >
> > Why above is safe...
> > I think (mapping,index) should be passed instead of page.
>
> I don't think current code isn't safe.
>
> void __cleancache_flush_page(struct address_space *mapping, struct page *page)
> {
> /* careful... page->mapping is NULL sometimes when this is called */
> int pool_id = mapping->host->i_sb->cleancache_poolid;
> struct cleancache_filekey key = { .u.key = { 0 } };
>
> if (pool_id >= 0) {
> VM_BUG_ON(!PageLocked(page));
>
> it does check PageLocked. So caller should hold a page reference to
> prevent freeing ramined PG_locked
> If the caller doesn't hold a ref of page, I think it's BUG of caller.
>
> In our case, caller calls truncate_complete_page have to make sure it, I think.
>
Ah, my point is that this function trust page->index even if page->mapping is
reset to NULL. And I'm not sure that there are any race that an other thread
add a replacement page for (mapping, index) while a thread call this function.
Thanks,
-Kame
On Mon, Dec 20, 2010 at 1:35 PM, KAMEZAWA Hiroyuki
<[email protected]> wrote:
> On Mon, 20 Dec 2010 11:58:50 +0900
> Minchan Kim <[email protected]> wrote:
>
>> On Mon, Dec 20, 2010 at 11:27 AM, KAMEZAWA Hiroyuki
>> <[email protected]> wrote:
>> > On Mon, 20 Dec 2010 11:21:52 +0900 (JST)
>> > KOSAKI Motohiro <[email protected]> wrote:
>> >
>> >> > This patch series changes remove_from_page_cache's page ref counting
>> >> > rule. page cache ref count is decreased in remove_from_page_cache.
>> >> > So we don't need call again in caller context.
>> >> >
>> >> > Cc: Nick Piggin <[email protected]>
>> >> > Cc: Al Viro <[email protected]>
>> >> > Cc: [email protected]
>> >> > Signed-off-by: Minchan Kim <[email protected]>
>> >> > ---
>> >> > ?mm/truncate.c | ? ?1 -
>> >> > ?1 files changed, 0 insertions(+), 1 deletions(-)
>> >> >
>> >> > diff --git a/mm/truncate.c b/mm/truncate.c
>> >> > index 9ee5673..8decb93 100644
>> >> > --- a/mm/truncate.c
>> >> > +++ b/mm/truncate.c
>> >> > @@ -114,7 +114,6 @@ truncate_complete_page(struct address_space *mapping, struct page *page)
>> >> > ? ? ?* calls cleancache_put_page (and note page->mapping is now NULL)
>> >> > ? ? ?*/
>> >> > ? ? cleancache_flush_page(mapping, page);
>> >> > - ? page_cache_release(page); ? ? ? /* pagecache ref */
>> >> > ? ? return 0;
>> >>
>> >> Do we _always_ have stable page reference here? IOW, I can assume
>> >> cleancache_flush_page() doesn't cause NULL deref?
>> >>
>> > Hmm, my review was bad.
>> >
>> > I think cleancache_flush_page() here should eat (mapping, index) as argument
>> > rather than "page".
>> >
>> > BTW, ?I can't understand
>> > ==
>> > void __cleancache_flush_page(struct address_space *mapping, struct page *page)
>> > {
>> > ? ? ? ?/* careful... page->mapping is NULL sometimes when this is called */
>> > ? ? ? ?int pool_id = mapping->host->i_sb->cleancache_poolid;
>> > ? ? ? ?struct cleancache_filekey key = { .u.key = { 0 } };
>> > ==
>> >
>> > Why above is safe...
>> > I think (mapping,index) should be passed instead of page.
>>
>> I don't think current code isn't safe.
>>
>> void __cleancache_flush_page(struct address_space *mapping, struct page *page)
>> {
>> ? ? ? ? /* careful... page->mapping is NULL sometimes when this is called */
>> ? ? ? ? int pool_id = mapping->host->i_sb->cleancache_poolid;
>> ? ? ? ? struct cleancache_filekey key = { .u.key = { 0 } };
>>
>> ? ? ? ? if (pool_id >= 0) {
>> ? ? ? ? ? ? ? ? VM_BUG_ON(!PageLocked(page));
>>
>> it does check PageLocked. So caller should hold a page reference to
>> prevent freeing ramined PG_locked
>> If the caller doesn't hold a ref of page, I think it's BUG of caller.
>>
>> In our case, caller calls truncate_complete_page have to make sure it, I think.
>>
>
> Ah, my point is that this function trust page->index even if page->mapping is
> reset to NULL. And I'm not sure that there are any race that an other thread
> add a replacement page for (mapping, index) while a thread call this function.
Because the page is locked and is detached from page cache, I guess
it's no problem.
Anyway, I think It's off-topic.
>
> Thanks,
> -Kame
>
>
>
>
--
Kind regards,
Minchan Kim
On Mon, 20 Dec 2010 17:09:11 +0900
Minchan Kim <[email protected]> wrote:
> Because the page is locked and is detached from page cache, I guess
> it's no problem.
> Anyway, I think It's off-topic.
>
yes.
Thanks,
-Kame
You'll need to merge all patches into one, otherwise you create really
nasty memory leaks when bisecting between them.
On Mon, Dec 20, 2010 at 7:33 PM, Christoph Hellwig <[email protected]> wrote:
> You'll need to merge all patches into one, otherwise you create really
> nasty memory leaks when bisecting between them.
>
Okay. I will resend.
Thanks for the notice, Christoph.
--
Kind regards,
Minchan Kim
On Tue, 21 Dec 2010, Minchan Kim wrote:
> On Mon, Dec 20, 2010 at 7:33 PM, Christoph Hellwig <[email protected]> wrote:
> > You'll need to merge all patches into one, otherwise you create really
> > nasty memory leaks when bisecting between them.
> >
>
> Okay. I will resend.
>
> Thanks for the notice, Christoph.
Good point from hch, but I feel even more strongly: if you're going to
do this now, please rename remove_from_page_cache (delete_from_page_cache
was what I chose back when I misdid it) - you're changing an EXPORTed
function in a subtle (well, subtlish) confusing way, which could easily
waste people's time down the line, whether in not-yet-in-tree filesystems
or backports of fixes. I'd much rather you break someone's build,
forcing them to look at what changed, than crash or leak at runtime.
If you do rename, you can keep your patch structure, introducing the
new function as a wrapper to the old at the beginning, then removing
the old function at the end.
(As you know, I do agree that it's right to decrement the reference
count at the point of removing from page cache.)
Hugh
On Tue, Dec 21, 2010 at 2:07 PM, Hugh Dickins <[email protected]> wrote:
> On Tue, 21 Dec 2010, Minchan Kim wrote:
>> On Mon, Dec 20, 2010 at 7:33 PM, Christoph Hellwig <[email protected]> wrote:
>> > You'll need to merge all patches into one, otherwise you create really
>> > nasty memory leaks when bisecting between them.
>> >
>>
>> Okay. I will resend.
>>
>> Thanks for the notice, Christoph.
>
> Good point from hch, but I feel even more strongly: if you're going to
> do this now, please rename remove_from_page_cache (delete_from_page_cache
> was what I chose back when I misdid it) - you're changing an EXPORTed
> function in a subtle (well, subtlish) confusing way, which could easily
> waste people's time down the line, whether in not-yet-in-tree filesystems
> or backports of fixes. ?I'd much rather you break someone's build,
> forcing them to look at what changed, than crash or leak at runtime.
>
> If you do rename, you can keep your patch structure, introducing the
> new function as a wrapper to the old at the beginning, then removing
> the old function at the end.
It is very good idea!!
Thanks for good suggestion, Hugh.
>
> (As you know, I do agree that it's right to decrement the reference
> count at the point of removing from page cache.)
>
> Hugh
>
--
Kind regards,
Minchan Kim