From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail138.messagelabs.com (mail138.messagelabs.com [216.82.249.35]) by kanga.kvack.org (Postfix) with SMTP id 9059D6B0093 for ; Tue, 7 Dec 2010 10:26:36 -0500 (EST) Received: by pwi6 with SMTP id 6so24529pwi.14 for ; Tue, 07 Dec 2010 07:26:35 -0800 (PST) Date: Wed, 8 Dec 2010 00:26:25 +0900 From: Minchan Kim Subject: Re: [PATCH v4 2/7] deactivate invalidated pages Message-ID: <20101207152625.GB608@barrios-desktop> References: <20101207144923.GB2356@cmpxchg.org> <20101207150710.GA26613@barrios-desktop> <20101207151939.GF2356@cmpxchg.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20101207151939.GF2356@cmpxchg.org> Sender: owner-linux-mm@kvack.org To: Johannes Weiner Cc: Andrew Morton , Rik van Riel , KOSAKI Motohiro , linux-mm , LKML , Peter Zijlstra , Wu Fengguang , Nick Piggin , Mel Gorman List-ID: On Tue, Dec 07, 2010 at 04:19:39PM +0100, Johannes Weiner wrote: > On Wed, Dec 08, 2010 at 12:07:10AM +0900, Minchan Kim wrote: > > On Tue, Dec 07, 2010 at 03:49:24PM +0100, Johannes Weiner wrote: > > > On Mon, Dec 06, 2010 at 02:29:10AM +0900, Minchan Kim wrote: > > > > Changelog since v3: > > > > - Change function comments - suggested by Johannes > > > > - Change function name - suggested by Johannes > > > > - add only dirty/writeback pages to deactive pagevec > > > > > > Why the extra check? > > > > > > > @@ -359,8 +360,16 @@ unsigned long invalidate_mapping_pages(struct address_space *mapping, > > > > if (lock_failed) > > > > continue; > > > > > > > > - ret += invalidate_inode_page(page); > > > > - > > > > + ret = invalidate_inode_page(page); > > > > + /* > > > > + * If the page is dirty or under writeback, we can not > > > > + * invalidate it now. But we assume that attempted > > > > + * invalidation is a hint that the page is no longer > > > > + * of interest and try to speed up its reclaim. > > > > + */ > > > > + if (!ret && (PageDirty(page) || PageWriteback(page))) > > > > + deactivate_page(page); > > > > > > The writeback completion handler does not take the page lock, so you > > > can still miss pages that finish writeback before this test, no? > > > > Yes. but I think it's rare and even though it happens, it's not critical. > > > > > > Can you explain why you felt the need to add these checks? > > > > invalidate_inode_page can return 0 although the pages is !{dirty|writeback}. > > Look invalidate_complete_page. As easiest example, if the page has buffer and > > try_to_release_page can't release the buffer, it could return 0. > > Ok, but somebody still tried to truncate the page, so why shouldn't we > try to reclaim it? The reason for deactivating at this location is > that truncation is a strong hint for reclaim, not that it failed due > to dirty/writeback pages. > > What's the problem with deactivating pages where try_to_release_page() > failed? If try_to_release_page fails and the such pages stay long time in pagevec, pagevec drain often happens. I think such pages are rare so skip such pages doesn't hurt goal of this patch. > > I don't think we should add more logic than necessary. If there is a > good reason for it, it needs to get a code comment at least. Above my comment is enough to justify it? If you agree, I can add the comment. Thanks for careful review, Hannes. -- 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: email@kvack.org