Hi Dan,
Today's linux-next merge of the cleancache tree got a conflict in
mm/truncate.c between commit 5adc7b518b54 ("mm: truncate: change
remove_from_page_cache") from Linus' tree and commit 03e838947c8a
("mm/fs: add hooks to support cleancache") from the cleancache tree.
I fixed it up (see below) but am really not sure of the fix. I can carry
this fix as necessary.
Is this stuff going to be merged into Linus' tree this time round?
--
Cheers,
Stephen Rothwell [email protected]
diff --cc mm/truncate.c
index a956675,cd94607..0000000
--- a/mm/truncate.c
+++ b/mm/truncate.c
@@@ -106,8 -108,13 +108,12 @@@ truncate_complete_page(struct address_s
cancel_dirty_page(page, PAGE_CACHE_SIZE);
clear_page_mlock(page);
- remove_from_page_cache(page);
ClearPageMappedToDisk(page);
+ delete_from_page_cache(page);
+ /* this must be after the remove_from_page_cache which
+ * calls cleancache_put_page (and note page->mapping is now NULL)
+ */
+ cleancache_flush_page(mapping, page);
- page_cache_release(page); /* pagecache ref */
return 0;
}
On Thu, 24 Mar 2011 13:55:24 +1100 Stephen Rothwell <[email protected]> wrote:
> Hi Dan,
>
> Today's linux-next merge of the cleancache tree got a conflict in
> mm/truncate.c between commit 5adc7b518b54 ("mm: truncate: change
> remove_from_page_cache") from Linus' tree and commit 03e838947c8a
> ("mm/fs: add hooks to support cleancache") from the cleancache tree.
>
> I fixed it up (see below) but am really not sure of the fix. I can carry
> this fix as necessary.
>
> Is this stuff going to be merged into Linus' tree this time round?
> --
> Cheers,
> Stephen Rothwell [email protected]
>
> diff --cc mm/truncate.c
> index a956675,cd94607..0000000
> --- a/mm/truncate.c
> +++ b/mm/truncate.c
> @@@ -106,8 -108,13 +108,12 @@@ truncate_complete_page(struct address_s
> cancel_dirty_page(page, PAGE_CACHE_SIZE);
>
> clear_page_mlock(page);
> - remove_from_page_cache(page);
> ClearPageMappedToDisk(page);
> + delete_from_page_cache(page);
> + /* this must be after the remove_from_page_cache which
> + * calls cleancache_put_page (and note page->mapping is now NULL)
> + */
> + cleancache_flush_page(mapping, page);
> - page_cache_release(page); /* pagecache ref */
> return 0;
> }
I did the cleancache_flush_page() before the delete_from_page_cache(),
in case the delete_from_page_cache() freed the page. I didn't actually
check whether that makes sense though.
On Thu, Mar 24, 2011 at 12:56 PM, Andrew Morton
<[email protected]> wrote:
> On Thu, 24 Mar 2011 13:55:24 +1100 Stephen Rothwell <[email protected]> wrote:
>
>> Hi Dan,
>>
>> Today's linux-next merge of the cleancache tree got a conflict in
>> mm/truncate.c between commit 5adc7b518b54 ("mm: truncate: change
>> remove_from_page_cache") from Linus' tree and commit 03e838947c8a
>> ("mm/fs: add hooks to support cleancache") from the cleancache tree.
>>
>> I fixed it up (see below) but am really not sure of the fix. I can carry
>> this fix as necessary.
>>
>> Is this stuff going to be merged into Linus' tree this time round?
>> --
>> Cheers,
>> Stephen Rothwell [email protected]
>>
>> diff --cc mm/truncate.c
>> index a956675,cd94607..0000000
>> --- a/mm/truncate.c
>> +++ b/mm/truncate.c
>> @@@ -106,8 -108,13 +108,12 @@@ truncate_complete_page(struct address_s
>> cancel_dirty_page(page, PAGE_CACHE_SIZE);
>>
>> clear_page_mlock(page);
>> - remove_from_page_cache(page);
>> ClearPageMappedToDisk(page);
>> + delete_from_page_cache(page);
>> + /* this must be after the remove_from_page_cache which
>> + * calls cleancache_put_page (and note page->mapping is now NULL)
>> + */
>> + cleancache_flush_page(mapping, page);
>> - page_cache_release(page); /* pagecache ref */
>> return 0;
>> }
>
> I did the cleancache_flush_page() before the delete_from_page_cache(),
> in case the delete_from_page_cache() freed the page. I didn't actually
> check whether that makes sense though.
I am not sure cleancache's put and flush semantic.
If I understand rightly with old __remove_from_page_cache's comment,
maybe cleancache_flush_page is to invalidate the page.(If I understand
right, I hope the name is changed to cleancache_invalidate_page)
" /*
* if we're uptodate, flush out into the cleancache, otherwise
* invalidate any existing cleancache entries. We can't leave
* stale data around in the cleancache once our page is gone
*/
if (PageUptodate(page))
cleancache_put_page(page);
else
cleancache_flush_page(mapping, page); "
So I think cleancache_flush_page should be done after
delete_from_page_cache because delete_from_page_cache calls
cleancache_put_page(maybe this function would flush the content of
memory into cleancache's target) before we invalidates the page.
And it should not be a problem in case the delete_from_page_cache
freed the page since cleancache should have a reference the page but I
didn't check cleancahe always has a reference of page. If it isn't,
it's a critical problem.
Dan, Could you comment this?
--
Kind regards,
Minchan Kim
On Thu, Mar 24, 2011 at 2:38 PM, Minchan Kim <[email protected]> wrote:
> On Thu, Mar 24, 2011 at 12:56 PM, Andrew Morton
> <[email protected]> wrote:
>> On Thu, 24 Mar 2011 13:55:24 +1100 Stephen Rothwell <[email protected]> wrote:
>>
>>> Hi Dan,
>>>
>>> Today's linux-next merge of the cleancache tree got a conflict in
>>> mm/truncate.c between commit 5adc7b518b54 ("mm: truncate: change
>>> remove_from_page_cache") from Linus' tree and commit 03e838947c8a
>>> ("mm/fs: add hooks to support cleancache") from the cleancache tree.
>>>
>>> I fixed it up (see below) but am really not sure of the fix. I can carry
>>> this fix as necessary.
>>>
>>> Is this stuff going to be merged into Linus' tree this time round?
>>> --
>>> Cheers,
>>> Stephen Rothwell [email protected]
>>>
>>> diff --cc mm/truncate.c
>>> index a956675,cd94607..0000000
>>> --- a/mm/truncate.c
>>> +++ b/mm/truncate.c
>>> @@@ -106,8 -108,13 +108,12 @@@ truncate_complete_page(struct address_s
>>> cancel_dirty_page(page, PAGE_CACHE_SIZE);
>>>
>>> clear_page_mlock(page);
>>> - remove_from_page_cache(page);
>>> ClearPageMappedToDisk(page);
>>> + delete_from_page_cache(page);
>>> + /* this must be after the remove_from_page_cache which
>>> + * calls cleancache_put_page (and note page->mapping is now NULL)
>>> + */
>>> + cleancache_flush_page(mapping, page);
>>> - page_cache_release(page); /* pagecache ref */
>>> return 0;
>>> }
>>
>> I did the cleancache_flush_page() before the delete_from_page_cache(),
>> in case the delete_from_page_cache() freed the page. I didn't actually
>> check whether that makes sense though.
>
> I am not sure cleancache's put and flush semantic.
> If I understand rightly with old __remove_from_page_cache's comment,
> maybe cleancache_flush_page is to invalidate the page.(If I understand
> right, I hope the name is changed to cleancache_invalidate_page)
>
> " /*
> * if we're uptodate, flush out into the cleancache, otherwise
> * invalidate any existing cleancache entries. We can't leave
> * stale data around in the cleancache once our page is gone
> */
> if (PageUptodate(page))
> cleancache_put_page(page);
> else
> cleancache_flush_page(mapping, page); "
>
> So I think cleancache_flush_page should be done after
> delete_from_page_cache because delete_from_page_cache calls
> cleancache_put_page(maybe this function would flush the content of
> memory into cleancache's target) before we invalidates the page.
>
> And it should not be a problem in case the delete_from_page_cache
> freed the page since cleancache should have a reference the page but I
> didn't check cleancahe always has a reference of page. If it isn't,
> it's a critical problem.
>
> Dan, Could you comment this?
Dan, one more thing.
#define cleancache_fs_enabled_mapping(_mapping) \
(mapping->host->i_sb->cleancache_poolid >= 0)
One is "_mapping", another is "mapping"
--
Kind regards,
Minchan Kim
On Thu, 24 Mar 2011 14:58:06 +0900 Minchan Kim <[email protected]> wrote:
> Dan, one more thing.
>
> #define cleancache_fs_enabled_mapping(_mapping) \
> (mapping->host->i_sb->cleancache_poolid >= 0)
>
> One is "_mapping", another is "mapping"
It should be implemented in C too. This is the case for almost all
"functions" which are implemented as macros and it's rather a mystery
why we keep on typing #define!
It is not only for cleanliness and for typechecking, but also because
constructs such as
{
struct address_space *foo = ...;
if (cleancache_fs_enabled_mapping(foo))
...;
}
will generate an unused-var warning against `foo' if CONFIG_CLEANCACHE=n.
Implementing the function in C fixes that. With current gcc, anyway.
> >>> Is this stuff going to be merged into Linus' tree this time round?
Hi Stephen --
Still TBD. Some discussion has occurred offlist.
> >> I did the cleancache_flush_page() before the
> delete_from_page_cache(),
> >> in case the delete_from_page_cache() freed the page. I didn't
> actually
> >> check whether that makes sense though.
> >
> > I am not sure cleancache's put and flush semantic.
> > If I understand rightly with old __remove_from_page_cache's comment,
> > maybe cleancache_flush_page is to invalidate the page
Hi Minchan and Stephen --
I will take a close look at this and possibly ask Chris Mason to
take a look as well (since these hooks were placed by Chris in 2008
and this is the first significant change around the hooks since then).
I think as long as the page is still locked and the mapping
remains valid, the ordering may not matter, but will confirm
and test.
> Dan, one more thing.
>
> #define cleancache_fs_enabled_mapping(_mapping) \
> (mapping->host->i_sb->cleancache_poolid >= 0)
>
> One is "_mapping", another is "mapping"
Oops! Nice catch, Minchan! Will fix (using C, per
Andrew's reply).
Thanks,
Dan
> From: Minchan Kim [mailto:[email protected]]
> Sent: Wednesday, March 23, 2011 11:38 PM
> To: Andrew Morton
> Cc: Stephen Rothwell; Dan Magenheimer; [email protected];
> [email protected]; Linus
> Subject: Re: linux-next: manual merge of the cleancache tree with
> Linus' tree
>
> On Thu, Mar 24, 2011 at 12:56 PM, Andrew Morton
> <[email protected]> wrote:
> > On Thu, 24 Mar 2011 13:55:24 +1100 Stephen Rothwell
> <[email protected]> wrote:
> >
> >> Hi Dan,
> >>
> >> Today's linux-next merge of the cleancache tree got a conflict in
> >> mm/truncate.c between commit 5adc7b518b54 ("mm: truncate: change
> >> remove_from_page_cache") from Linus' tree and commit 03e838947c8a
> >> ("mm/fs: add hooks to support cleancache") from the cleancache tree.
> >>
> >> I fixed it up (see below) but am really not sure of the fix. I can
> carry
> >> this fix as necessary.
> >>
> >> Is this stuff going to be merged into Linus' tree this time round?
> >> --
> >> Cheers,
> >> Stephen Rothwell [email protected]
> >>
> >> diff --cc mm/truncate.c
> >> index a956675,cd94607..0000000
> >> --- a/mm/truncate.c
> >> +++ b/mm/truncate.c
> >> @@@ -106,8 -108,13 +108,12 @@@ truncate_complete_page(struct
> address_s
> >> cancel_dirty_page(page, PAGE_CACHE_SIZE);
> >>
> >> clear_page_mlock(page);
> >> - remove_from_page_cache(page);
> >> ClearPageMappedToDisk(page);
> >> + delete_from_page_cache(page);
> >> + /* this must be after the remove_from_page_cache which
> >> + * calls cleancache_put_page (and note page->mapping is now
> NULL)
> >> + */
> >> + cleancache_flush_page(mapping, page);
> >> - page_cache_release(page); /* pagecache ref */
> >> return 0;
> >> }
> >
> > I did the cleancache_flush_page() before the
> delete_from_page_cache(),
> > in case the delete_from_page_cache() freed the page. I didn't
> actually
> > check whether that makes sense though.
>
> I am not sure cleancache's put and flush semantic.
> If I understand rightly with old __remove_from_page_cache's comment,
> maybe cleancache_flush_page is to invalidate the page.(If I understand
> right, I hope the name is changed to cleancache_invalidate_page)
>
> " /*
> * if we're uptodate, flush out into the cleancache, otherwise
> * invalidate any existing cleancache entries. We can't leave
> * stale data around in the cleancache once our page is gone
> */
> if (PageUptodate(page))
> cleancache_put_page(page);
> else
> cleancache_flush_page(mapping, page); "
>
> So I think cleancache_flush_page should be done after
> delete_from_page_cache because delete_from_page_cache calls
> cleancache_put_page(maybe this function would flush the content of
> memory into cleancache's target) before we invalidates the page.
>
> And it should not be a problem in case the delete_from_page_cache
> freed the page since cleancache should have a reference the page but I
> didn't check cleancahe always has a reference of page. If it isn't,
> it's a critical problem.
>
> Dan, Could you comment this?
In case anyone was waiting for a resolution on this, it
was resolved offlist.
The answer is that the order doesn't matter and the V8
cleancache patch will include a fix for this.
Thanks,
Dan