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