[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Fri, 12 Dec 2008 09:54:18 +0900
From: Toshiyuki Okajima <toshi.okajima@...fujitsu.com>
To: Theodore Tso <tytso@....edu>
CC: aneesh.kumar@...ux.vnet.ibm.com, balbir@...ux.vnet.ibm.com,
linux-ext4@...r.kernel.org
Subject: Re: [BUG][PATCH 1/4] ext3: fix a cause of __schedule_bug via blkdev_releasepage
Ted-san,
Thank you for your reviewing.
> Toshiyuki-san,
>
> My apologies for not having a chance to review your patches; things
> have been rather busy for me. There were a couple of shortcomings in
> your patch series, and I think there is a better way of solving the
> issue at hand. First of all, patches #1 and #2 use a new function
> which is not actually defined until patches #3 and #4, respectively.
> This can make it difficult for people who are trying to use "git
> bisect" to try to localize the root cause of a problem.
>
> Secondly, the introduction of a large number of wrapper functions
> increases stack utilization, and makes the call depth deeper (and in
> the long run, each increase in call depth makes the code that much
> harder to trace through and understand), and so it should be done only
> as last resort. Fortunately, there is a simpler way of fixing this
> problem. I include the inter-diff below, but I will fold this into
> the current blkdev_releasepage() patches that are in the ext4 patch
> queue.
>
> Best regards,
>
> - Ted
>
> P.S. Note that this patch is functionally identical to what you
> proposed in your patch series, but since the gfp_wait mask already
> controlls whether or not log_wait_commit() is called, instead of
> introducing a new functional parameter, we just mask off __GFP_WAIT
> before calling jbd2_journal_try_to_free_buffers. I'll make a similar
> change to the ext3 patch, and attach the two revised patches to this
> mail thread.
Through the idea as follows, I agree to your change.
-----------------------------------------------------------------------------
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.)
So, I have made a new function, journal_try_to_free_metadata_buffers
to release only metadata buffer_heads.
(I wanted a function which is the almost same as journal_try_to_free_buffers
except calling journal_wait_for_transaction_sync_data from it.)
However, this new function needed big changes, you know.
I reconsidered what was the most suitable patch to fix this problem
after I read your mail (patch).
And then, I thought that it was important to make the most concise patch
to fix only a root cause. Big patch is not easy to understand even if it is
more logical one.
Therefore, there is the fact that ext3_release_metadata must not sleep because
it can get a spinlock, and then, only changing ext3_release_metadata to the
logic to make it not sleep is the simplest fix for this problem.
-----------------------------------------------------------------------------
Best Regards,
Toshiyuki Okajima
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists