From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
Subject: Re: [PATCH -V2] ext4: Drop mapped buffer_head check during
	page_mkwrite
Date: Mon, 31 Aug 2009 12:00:06 +0530
Message-ID: <20090831063006.GA7711@skywalker.linux.vnet.ibm.com>
References: <1251264196-31382-1-git-send-email-aneesh.kumar@linux.vnet.ibm.com> <20090829022656.GK16732@mit.edu>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: cmm@us.ibm.com, sandeen@redhat.com, linux-ext4@vger.kernel.org
To: Theodore Tso <tytso@mit.edu>
Content-Disposition: inline
In-Reply-To: <20090829022656.GK16732@mit.edu>
Sender: linux-ext4-owner@vger.kernel.org

On Fri, Aug 28, 2009 at 10:26:56PM -0400, Theodore Tso wrote:
> On Wed, Aug 26, 2009 at 10:53:16AM +0530, Aneesh Kumar K.V wrote:
> > Inorder to check whether the buffer_heads are mapped we need
> > to hold page lock. Otherwise a reclaim can cleanup the attached
> > buffer_heads. Instead of taking page lock and check whether
> > buffer_heads are mapped we let the write_begin/write_end callback
> > does the equivalent. It does have a performance impact in that we
> > are doing more work if we the buffer_heads are already mapped.
> 
> So I started looking at all of the work that we need to do in
> write_begin/write_end; did you check both write paths depending on
> whether we are using delayed allocation or not?  It would seem to me
> that it might be better to use lock_page() and unlock_page() around
> the check, since in many work loads the buffer heads will already be
> mapped often, and it appears to me that write_begin() will end up
> locking the page anyway.
> 
> Am I missing something?
> 

Below are the possibilities i looked at

a) mmap with no parallel write to the same offset. That would mean
we don't have attached buffer heads because nobody attach buffer
heads to the page.

b) mmap happening to the hole. The buffer heads are not mapped.

c) mmap with parallel write to the same offset. The parallel write
did attach mapped buffer heads to the same page. So we should find
all buffer heads mapped in the above case. 

if we will find buffer heads already be mapped in many workloads then
i guess it make sense to add page lock. It will also avoid the
journal_start that we do in write_begin. I will redo the patch

-aneesh