Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757067AbZCSQ1r (ORCPT ); Thu, 19 Mar 2009 12:27:47 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1752481AbZCSQ1i (ORCPT ); Thu, 19 Mar 2009 12:27:38 -0400 Received: from smtp1.linux-foundation.org ([140.211.169.13]:36467 "EHLO smtp1.linux-foundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752068AbZCSQ1h (ORCPT ); Thu, 19 Mar 2009 12:27:37 -0400 Date: Thu, 19 Mar 2009 09:20:57 -0700 (PDT) From: Linus Torvalds X-X-Sender: torvalds@localhost.localdomain To: Nick Piggin cc: Ying Han , Jan Kara , Andrew Morton , linux-kernel , linux-mm , guichaz@gmail.com, Alex Khesin , Mike Waychison , Rohit Seth , Peter Zijlstra Subject: Re: ftruncate-mmap: pages are lost after writing to mmaped file. In-Reply-To: <200903200248.22623.nickpiggin@yahoo.com.au> Message-ID: References: <604427e00903181244w360c5519k9179d5c3e5cd6ab3@mail.gmail.com> <604427e00903181654y308d57d8w2cb32eab831cf45a@mail.gmail.com> <200903200248.22623.nickpiggin@yahoo.com.au> User-Agent: Alpine 2.00 (LFD 1167 2008-08-23) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3293 Lines: 107 On Fri, 20 Mar 2009, Nick Piggin wrote: > > But I think we do have a race in __set_page_dirty_buffers(): > > The page may not have buffers between the mapping->private_lock > critical section and the __set_page_dirty call there. So between > them, another thread might do a create_empty_buffers which can > see !PageDirty and thus it will create clean buffers. Hmm. Creating clean buffers is locked by the page lock, nothing else. And not all page dirtiers hold the page lock (in fact, most try to avoid it - the rule is that you either have to hold the page lock _or_ hold a reference to the 'mapping', and the latter is what the mmap code does, I think). So yeah, the page lock isn't sufficient. > Holding mapping->private_lock over the __set_page_dirty should > fix it, although I guess you'd want to release it before calling > __mark_inode_dirty so as not to put inode_lock under there. I > have a patch for this if it sounds reasonable. That would seem to make sense. Maybe moving the "TestSetPageDirty()" from inside __set_page_dirty() to the caller? Something like the appended? This is TOTALLY untested. Of course. Linus --- fs/buffer.c | 23 +++++++++++------------ 1 files changed, 11 insertions(+), 12 deletions(-) diff --git a/fs/buffer.c b/fs/buffer.c index 9f69741..891e1c7 100644 --- a/fs/buffer.c +++ b/fs/buffer.c @@ -760,15 +760,9 @@ EXPORT_SYMBOL(mark_buffer_dirty_inode); * If warn is true, then emit a warning if the page is not uptodate and has * not been truncated. */ -static int __set_page_dirty(struct page *page, +static void __set_page_dirty(struct page *page, struct address_space *mapping, int warn) { - if (unlikely(!mapping)) - return !TestSetPageDirty(page); - - if (TestSetPageDirty(page)) - return 0; - spin_lock_irq(&mapping->tree_lock); if (page->mapping) { /* Race with truncate? */ WARN_ON_ONCE(warn && !PageUptodate(page)); @@ -785,8 +779,6 @@ static int __set_page_dirty(struct page *page, } spin_unlock_irq(&mapping->tree_lock); __mark_inode_dirty(mapping->host, I_DIRTY_PAGES); - - return 1; } /* @@ -816,6 +808,7 @@ static int __set_page_dirty(struct page *page, */ int __set_page_dirty_buffers(struct page *page) { + int newly_dirty; struct address_space *mapping = page_mapping(page); if (unlikely(!mapping)) @@ -831,9 +824,12 @@ int __set_page_dirty_buffers(struct page *page) bh = bh->b_this_page; } while (bh != head); } + newly_dirty = !TestSetPageDirty(page); spin_unlock(&mapping->private_lock); - return __set_page_dirty(page, mapping, 1); + if (newly_dirty) + __set_page_dirty(page, mapping, 1); + return newly_dirty; } EXPORT_SYMBOL(__set_page_dirty_buffers); @@ -1262,8 +1258,11 @@ void mark_buffer_dirty(struct buffer_head *bh) return; } - if (!test_set_buffer_dirty(bh)) - __set_page_dirty(bh->b_page, page_mapping(bh->b_page), 0); + if (!test_set_buffer_dirty(bh)) { + struct page *page = bh->b_page; + if (!TestSetPageDirty(page)) + __set_page_dirty(page, page_mapping(page), 0); + } } /* -- 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/