On Mon, 23 Jun 2008 20:00:51 +0900 Toshiyuki Okajima <toshi.okajima@xxxxxxxxxxxxxx> wrote:
> Hi.
>
> I found the possibility that the system has the page which has
> buffers (are marked 'BH_Freed') without mapping. As a result,
> the resource management software etc. might not operate correctly
> because there is a possibility that the page which cannot be
> estimated exists.
>
> I made a patch to positively release the pages that had the buffers
> which were marked 'BH_Freed'.
>
> Please confirm it.
>
> This patch is for 2.6.26-rc6.
> ---
> 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?
> Such all data
> buffers are marked 'BH_Freed'. So, also pages corresponding
> to them aren't released by JBD. The pages have data buffers
> without mapping. Due to this fact, we cannot estimate free
> memory correctly when there are many pages which have data
> buffers without mapping. Therefore the resource management
> software cannot work correctly.
> But it is not memory leakage because the pages can be released
> when the system has really few available memory.
>
> BTW,
> 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?
> ---
> 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
> +++ linux-2.6.26-rc6/fs/jbd/commit.c 2008-06-19 14:44:29.000000000 +0900
> @@ -42,11 +42,11 @@ static void journal_end_buffer_io_sync(s
> * by the VM, but their apparent absence upsets the VM accounting, and it makes
> * the numbers in /proc/meminfo look odd.
> *
> - * So here, we have a buffer which has just come off the forget list. Look to
> - * see if we can strip all buffers from the backing page.
> + * So here, we have a buffer which has just come off the list. Confirm if we
> + * can strip all buffers from the backing page.
> *
> - * Called under lock_journal(), and possibly under journal_datalist_lock. The
> - * caller provided us with a ref against the buffer, and we drop that here.
> + * Called under journal->j_list_lock. The caller provided us with a ref
> + * against the buffer, and we drop that here.
> */
> static void release_buffer_page(struct buffer_head *bh)
> {
> @@ -231,7 +231,16 @@ write_out_data:
> if (locked)
> unlock_buffer(bh);
> 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.
> + 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.
> continue;
> }
> if (locked && test_clear_buffer_dirty(bh)) {
> @@ -258,10 +267,17 @@ write_out_data:
> if (locked)
> unlock_buffer(bh);
> journal_remove_journal_head(bh);
> - /* Once for our safety reference, once for
> - * journal_remove_journal_head() */
> - put_bh(bh);
> - put_bh(bh);
> + put_bh(bh); /* for journal_remove_journal_head() */
> + if (buffer_freed(bh)) {
> + /* This bh has been marked 'BH_Feed'. */
> + 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);
> }
>
> if (need_resched() || spin_needbreak(&journal->j_list_lock)) {
> @@ -443,7 +459,16 @@ void journal_commit_transaction(journal_
> } else {
> jbd_unlock_bh_state(bh);
> }
> - put_bh(bh);
> + if (buffer_freed(bh)) {
> + /* This bh has been marked 'BH_Feed'. */
> + 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);
> cond_resched_lock(&journal->j_list_lock);
> }
> spin_unlock(&journal->j_list_lock);
--
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
