Thank you for reviewing.
Andrew Morton wrote:
On Mon, 23 Jun 2008 20:00:51 +0900 Toshiyuki Okajima <toshi.okajima@xxxxxxxxxxxxxx> wrote:
Hi.
<SNIP>
On ordered mode, before journal_commit_transaction does with
t_locked_list, all data buffers which are requested to dispose
by journal_unmap_buffer aren't disposed by JBD.
Why not? What is it which prevents these buffers from being released
in the normal fashion?
When an ext3-ordered file is truncated, it is possible that many pages
are not successfully freed, because they are attached to a committing
transaction.
(This description is the comment of release_buffer_page())
So, journal_unmap_buffer() leaves it to journal_commit_transaction()
to release the buffers later.
(journal_unmap_buffer() puts the mark 'BH_Freed' to them
so that journal_commit_transaction() can identify whether they can be
released or not.)
But journal_commit_transaction() doesn't free them if they are treated
as data buffers because there is no code which releases the pages
(which have the data buffers (marked 'BH_Freed')) in it.
Therefore such the buffers and the pages remain by JBD.
(They remain till try_to_free_buffers() is called by someone.
For example, kswapd().)
Such all data
<SNIP>
all metadata buffers on such situation are disposed at
'JBD: commit phase 7' by JBD and pages corresponding to them
can be released on the same time.
Therefore, by applying the same statements as ones for disposing
metadata buffers (marked 'BH_Freed'), JBD in
journal_commit_transaction can release the pages which has data
buffers without mapping.
As a result, the page which cannot be estimated is lost.
Are you sure that this change cannot reintroduce the problem which the
addition of buffer_freed() fixed?
I think no problem happens because I only add
the code which releases the page (and buffer_heads) which should be
released originally.
I use release_buffer_page() to release the page.
The page can be released (try_to_free_buffers() can be executed in JBD)
only if the following is satisfied for a data buffer:
- it is demanded to be released by journal_unmap_buffer()
- its b_count is 1
- the page to which it belongs has no mapping
- the page is unlocked
And I have never experienced any problems while performing long run test.
---
fs/jbd/commit.c | 45 +++++++++++++++++++++++++++++++++++----------
1 files changed, 35 insertions(+), 10 deletions(-)
--- linux-2.6.26-rc6.org/fs/jbd/commit.c 2008-06-13 06:22:24.000000000 +0900
<SNIP>
BUFFER_TRACE(bh, "already cleaned up");
- put_bh(bh);
+ if (buffer_freed(bh)) {
+ /* This bh has been marked 'BH_Feed'. */
"BH_Freed".
But the comment is rather unnecessary anyway.
I see. I remove it.
+ clear_buffer_freed(bh);
+ /* Release the page if all other buffers
+ * that belong to the page that has this bh
+ * can be freed.
+ */
+ release_buffer_page(bh);
+ } else
+ put_bh(bh);
Perhaps the above code snippet shold be in a function rather than
repeated three times.
The patch adds new trailing whitespace.
OK. I will change these statements into one function.
I will send revised patch ASAP.
Thanks,
---
Toshiyuki Okajima
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
