From: Toshiyuki Okajima <toshi.okajima@jp.fujitsu.com>
Subject: Re: [BUG][PATCH 1/4] ext3: fix a cause of __schedule_bug via	blkdev_releasepage
Date: Mon, 15 Dec 2008 11:21:59 +0900
Message-ID: <4945BF47.5090508@jp.fujitsu.com>
References: <20081202200647.72cc5807.toshi.okajima@jp.fujitsu.com> <20081208140138.GA17700@mit.edu> <4941B63A.3090302@jp.fujitsu.com> <20081212062148.GJ10890@mit.edu>
Reply-To: toshi.okajima@jp.fujitsu.com
Mime-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Cc: aneesh.kumar@linux.vnet.ibm.com, balbir@linux.vnet.ibm.com,
	linux-ext4@vger.kernel.org
To: Theodore Tso <tytso@mit.edu>
In-Reply-To: <20081212062148.GJ10890@mit.edu>
Sender: linux-ext4-owner@vger.kernel.org

Ted-san,

Theodore Tso wrote:
 > On Fri, Dec 12, 2008 at 09:54:18AM +0900, Toshiyuki Okajima wrote:
 > > > To tell the truth, at first, I imagined the same patch as yours to fix this
 > > > problem. But I have made another patch because I thought that ext3(or ext4)
 > > > should not know the contents of the processing of journal_try_to_free_buffers
 > > >  in detail. (ext3 should not know there is a possibility to call
 > > > journal_wait_for_transaction_sync_data from journal_try_to_free_buffers.)
 >
 > I agree, but ext3 doesn't need to know that.  What my change did was
 > to mask off the _GFP_WAIT flag, which prohibits the function it calls
 > from blocking, because it knows that its caller is holding a spinlock.
 >
 > And actually, come to think of it.  We can do even better; the right
 > fix is to have blkdev_releasepage() mask off the _GFP_WAIT flag; this
 > is the function which is taking spinlock, and by masking off the
 > __GFP_WAIT flag, this is simply requesting all of the downstream
 > functions not to block, but to do the best job they can do without
 > blocking.  It doesn't need to know whether it's going to call
 > log_wait_commit(), or anything else; all it needs to do is request
 > "please don't block".

 > That means we only make the request once, in the function which is
 > taking spinlock, so all of the per-filesystem implementations of
 > release_metadata() don't need to know that its caller is holding a
 > spinlock.

OK. I agree your last change.

I also think blkdev_releasepage must do something so that downstream
functions of it don't sleep. Masking off the _GFP_WAIT flag is the
easiest achievement of it.
Besides, I think it is not valid implementation that brings us a care
about ei->client_releasepage's sleeping.

Additional Information:
I did an easy test with your last change and then I haven't experienced
any errors.

Regards,
Toshiyuki Okajima