[PATCH] JBD: positively dispose the unmapped data buffers in journal_commit_transaction

[PATCH] JBD: positively dispose the unmapped data buffers in journal_commit_transaction

[Toshiyuki Okajima]

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. 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.

Signed-off-by: Toshiyuki Okajima <toshi.okajima@jp.fujitsu.com>
---
 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'. */
+				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);	
 			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);

Re: [PATCH] JBD: positively dispose the unmapped data buffers in journal_commit_transaction

[Andrew Morton]

On Mon, 23 Jun 2008 20:00:51 +0900 Toshiyuki Okajima <toshi.okajima@jp.fujitsu.com> 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);

Re: [PATCH] JBD: positively dispose the unmapped data buffers in journal_commit_transaction

[Toshiyuki Okajima]

Thank you for reviewing.

Andrew Morton wrote:
> On Mon, 23 Jun 2008 20:00:51 +0900 Toshiyuki Okajima <toshi.okajima@jp.fujitsu.com> 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

(take 2)[PATCH] JBD: positively dispose the unmapped data buffers in journal_commit_transaction

[Toshiyuki Okajima]

Hi.

I updated my patch and introduction article for it by reflecting
the comment of Andrew's.

I think that this fix doesn't introduce other problems because
I only add the code which releases the pages (and buffers) which
should be released originally, and I have never experienced any
problems while performing long run test.

Please confirm them.

This patch is for 2.6.26-rc6.
---
[Problem]
After ext3-ordered files are truncated, there is a possibility
that the pages which cannot be estimated still remain. Remaining
pages can be released when the system has really few memory.
So, it is not memory leakage. But the resource management
software etc. may not work correctly.

[Description]
It is possible that journal_unmap_buffer() cannot release
the buffers, and the pages to which they belong because
they are attached to a commiting transaction and
journal_unmap_buffer() cannot release them.
To release such the buffers and the pages later,
journal_unmap_buffer() leaves it to journal_commit_transaction().
(journal_unmap_buffer() puts the mark 'BH_Freed' to the buffers
so that journal_commit_transaction() can identify whether they
can be released or not.)

In the journalled mode and the writeback mode, jbd does with only
metadata buffers.  But in the ordered mode, jbd does with metadata
buffers and also data buffers.

Actually, journal_commit_transaction() releases only the metadata
buffers of which release is demanded by journal_unmap_buffer(),
and also releases the pages to which they belong if possible.

As a result, the data buffers of which release is demanded
by journal_unmap_buffer() remain after a transaction commits.
And also the pages to which they belong remain.

Such the remained pages don't have mapping any longer.
Due to this fact, there is a possibility that the pages which
cannot be estimated remain.

[How to fix]
The metadata buffers marked 'BH_Freed' and the pages to which
they belong can be released at 'JBD: commit phase 7'.

Therefore, by applying the same code into 'JBD: commit phase 2'
(where the data buffers are done with),
journal_commit_transaction() can also release the data buffers
marked 'BH_Freed' and the pages to which they belong.

As a result, all the buffers marked 'BH_Freed' can be released,
and also all the pages to which these buffers belong can be
released at journal_commit_transaction().
So, the page which cannot be estimated is lost.

<<Excerpt of code at 'JBD: commit phase 7'>>
 >         spin_lock(&journal->j_list_lock);
 >         while (commit_transaction->t_forget) {
 >                 transaction_t *cp_transaction;
 >                 struct buffer_head *bh;
 >
 >                 jh = commit_transaction->t_forget;
 >...
 >                 if (buffer_freed(bh)) {
 >                 ^^^^^^^^^^^^^^^^^^^^^^^^
 >                         clear_buffer_freed(bh);
 >                        ^^^^^^^^^^^^^^^^^^^^^^^^
 >                         clear_buffer_jbddirty(bh);
 >                 }
 >
 >                 if (buffer_jbddirty(bh)) {
 >                         JBUFFER_TRACE(jh, "add to new checkpointing trans");
 >                         __journal_insert_checkpoint(jh, commit_transaction);
 >                         JBUFFER_TRACE(jh, "refile for checkpoint writeback");
 >                         __journal_refile_buffer(jh);
 >                         jbd_unlock_bh_state(bh);
 >                 } else {
 >                         J_ASSERT_BH(bh, !buffer_dirty(bh));
 > ...
 >                         JBUFFER_TRACE(jh, "refile or unfile freed buffer");
 >                         __journal_refile_buffer(jh);
 >                         if (!jh->b_transaction) {
 >                                 jbd_unlock_bh_state(bh);
 >                                  /* needs a brelse */
 >                                 journal_remove_journal_head(bh);
 >                                 release_buffer_page(bh);
 >                                 ^^^^^^^^^^^^^^^^^^^^^^^^
 >                         } else
 >                 }
****************************************************************
* Apply the code of "^^^^^^" lines into 'JBD: commit phase 2' *
****************************************************************

[Additional information]
At journal_commit_transaction() code,
there is one extra message in the series of jbd debug messages.
("JBD: commit phase 2")
This patch fixes it, too.

Signed-off-by: Toshiyuki Okajima <toshi.okajima@jp.fujitsu.com>
---
  fs/jbd/commit.c |   29 ++++++++++++++++++++---------
  1 file changed, 20 insertions(+), 9 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-25 14:51:55.000000000 +0900
@@ -36,7 +36,7 @@ static void journal_end_buffer_io_sync(s

  /*
   * When an ext3-ordered file is truncated, it is possible that many pages are
- * not sucessfully freed, because they are attached to a committing transaction.
+ * not successfully freed, because they are attached to a committing transaction.
   * After the transaction commits, these pages are left on the LRU, with no
   * ->mapping, and with attached buffers.  These pages are trivially reclaimable
   * by the VM, but their apparent absence upsets the VM accounting, and it makes
@@ -45,8 +45,8 @@ static void journal_end_buffer_io_sync(s
   * 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.
   *
- * 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)
  {
@@ -78,6 +78,19 @@ nope:
  }

  /*
+ * Decrement reference counter for data buffer. If it has been marked
+ * 'BH_Freed', release it and the page to which it belongs if possible.
+ */
+static void release_data_buffer(struct buffer_head *bh)
+{
+	if (buffer_freed(bh)) {
+		clear_buffer_freed(bh);
+		release_buffer_page(bh);
+	} else
+		put_bh(bh);
+}
+
+/*
   * Try to acquire jbd_lock_bh_state() against the buffer, when j_list_lock is
   * held.  For ranking reasons we must trylock.  If we lose, schedule away and
   * return 0.  j_list_lock is dropped in this case.
@@ -231,7 +244,7 @@ write_out_data:
  			if (locked)
  				unlock_buffer(bh);
  			BUFFER_TRACE(bh, "already cleaned up");
-			put_bh(bh);
+			release_data_buffer(bh);
  			continue;
  		}
  		if (locked && test_clear_buffer_dirty(bh)) {
@@ -258,10 +271,10 @@ write_out_data:
  			if (locked)
  				unlock_buffer(bh);
  			journal_remove_journal_head(bh);
-			/* Once for our safety reference, once for
+			/* One for our safety reference, other for
  			 * journal_remove_journal_head() */
  			put_bh(bh);
-			put_bh(bh);
+			release_data_buffer(bh);
  		}

  		if (need_resched() || spin_needbreak(&journal->j_list_lock)) {
@@ -443,7 +456,7 @@ void journal_commit_transaction(journal_
  		} else {
  			jbd_unlock_bh_state(bh);
  		}
-		put_bh(bh);
+		release_data_buffer(bh);
  		cond_resched_lock(&journal->j_list_lock);
  	}
  	spin_unlock(&journal->j_list_lock);
@@ -453,8 +466,6 @@ void journal_commit_transaction(journal_

  	journal_write_revoke_records(journal, commit_transaction);

-	jbd_debug(3, "JBD: commit phase 2\n");

Re: (take 2)[PATCH] JBD: positively dispose the unmapped data buffers in journal_commit_transaction

[Jan Kara]

  Hi,

> I updated my patch and introduction article for it by reflecting
> the comment of Andrew's.
> 
> I think that this fix doesn't introduce other problems because
> I only add the code which releases the pages (and buffers) which
> should be released originally, and I have never experienced any
> problems while performing long run test.
> 
> Please confirm them.
> 
> This patch is for 2.6.26-rc6.
> ---
> [Problem]
> After ext3-ordered files are truncated, there is a possibility
> that the pages which cannot be estimated still remain. Remaining
> pages can be released when the system has really few memory.
> So, it is not memory leakage. But the resource management
> software etc. may not work correctly.
> 
> [Description]
> It is possible that journal_unmap_buffer() cannot release
> the buffers, and the pages to which they belong because
> they are attached to a commiting transaction and
> journal_unmap_buffer() cannot release them.
> To release such the buffers and the pages later,
> journal_unmap_buffer() leaves it to journal_commit_transaction().
> (journal_unmap_buffer() puts the mark 'BH_Freed' to the buffers
> so that journal_commit_transaction() can identify whether they
> can be released or not.)
> 
> In the journalled mode and the writeback mode, jbd does with only
> metadata buffers.  But in the ordered mode, jbd does with metadata
> buffers and also data buffers.
> 
> Actually, journal_commit_transaction() releases only the metadata
> buffers of which release is demanded by journal_unmap_buffer(),
> and also releases the pages to which they belong if possible.
> 
> As a result, the data buffers of which release is demanded
> by journal_unmap_buffer() remain after a transaction commits.
> And also the pages to which they belong remain.
> 
> Such the remained pages don't have mapping any longer.
> Due to this fact, there is a possibility that the pages which
> cannot be estimated remain.
> 
> [How to fix]
> The metadata buffers marked 'BH_Freed' and the pages to which
> they belong can be released at 'JBD: commit phase 7'.
> 
> Therefore, by applying the same code into 'JBD: commit phase 2'
> (where the data buffers are done with),
> journal_commit_transaction() can also release the data buffers
> marked 'BH_Freed' and the pages to which they belong.
> 
> As a result, all the buffers marked 'BH_Freed' can be released,
> and also all the pages to which these buffers belong can be
> released at journal_commit_transaction().
> So, the page which cannot be estimated is lost.
> 
  <snip>

> [Additional information]
> At journal_commit_transaction() code,
> there is one extra message in the series of jbd debug messages.
> ("JBD: commit phase 2")
> This patch fixes it, too.
> 
> Signed-off-by: Toshiyuki Okajima <toshi.okajima@jp.fujitsu.com>
  I agree with the change. It's true that we can leave some anonymous
pages behind and it's nicer to the MM to release them earlier when we
know they will be never needed again. The patch looks fine to me, you
can add
  Acked-by: Jan Kara <jack@suse.cz>

  How much have you stressed the patched kernel? I suggest you use
fsxlinux and put some memory pressure to the system...

								Honza

> ---
>  fs/jbd/commit.c |   29 ++++++++++++++++++++---------
>  1 file changed, 20 insertions(+), 9 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-25 14:51:55.000000000 +0900
> @@ -36,7 +36,7 @@ static void journal_end_buffer_io_sync(s
> 
>  /*
>   * When an ext3-ordered file is truncated, it is possible that many pages 
>   are
> - * not sucessfully freed, because they are attached to a committing 
> transaction.
> + * not successfully freed, because they are attached to a committing 
> transaction.
>   * After the transaction commits, these pages are left on the LRU, with no
>   * ->mapping, and with attached buffers.  These pages are trivially 
>   reclaimable
>   * by the VM, but their apparent absence upsets the VM accounting, and it 
>   makes
> @@ -45,8 +45,8 @@ static void journal_end_buffer_io_sync(s
>   * 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.
>   *
> - * 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)
>  {
> @@ -78,6 +78,19 @@ nope:
>  }
> 
>  /*
> + * Decrement reference counter for data buffer. If it has been marked
> + * 'BH_Freed', release it and the page to which it belongs if possible.
> + */
> +static void release_data_buffer(struct buffer_head *bh)
> +{
> +	if (buffer_freed(bh)) {
> +		clear_buffer_freed(bh);
> +		release_buffer_page(bh);
> +	} else
> +		put_bh(bh);
> +}
> +
> +/*
>   * Try to acquire jbd_lock_bh_state() against the buffer, when j_list_lock 
>   is
>   * held.  For ranking reasons we must trylock.  If we lose, schedule away 
>   and
>   * return 0.  j_list_lock is dropped in this case.
> @@ -231,7 +244,7 @@ write_out_data:
>  			if (locked)
>  				unlock_buffer(bh);
>  			BUFFER_TRACE(bh, "already cleaned up");
> -			put_bh(bh);
> +			release_data_buffer(bh);
>  			continue;
>  		}
>  		if (locked && test_clear_buffer_dirty(bh)) {
> @@ -258,10 +271,10 @@ write_out_data:
>  			if (locked)
>  				unlock_buffer(bh);
>  			journal_remove_journal_head(bh);
> -			/* Once for our safety reference, once for
> +			/* One for our safety reference, other for
>  			 * journal_remove_journal_head() */
>  			put_bh(bh);
> -			put_bh(bh);
> +			release_data_buffer(bh);
>  		}
> 
>  		if (need_resched() || spin_needbreak(&journal->j_list_lock)) 
>  		{
> @@ -443,7 +456,7 @@ void journal_commit_transaction(journal_
>  		} else {
>  			jbd_unlock_bh_state(bh);
>  		}
> -		put_bh(bh);
> +		release_data_buffer(bh);
>  		cond_resched_lock(&journal->j_list_lock);
>  	}
>  	spin_unlock(&journal->j_list_lock);
> @@ -453,8 +466,6 @@ void journal_commit_transaction(journal_
> 
>  	journal_write_revoke_records(journal, commit_transaction);
> 
> -	jbd_debug(3, "JBD: commit phase 2\n");
> -
>  	/*
>  	 * If we found any dirty or locked buffers, then we should have
>  	 * looped back up to the write_out_data label.  If there weren't
-- 
Jan Kara <jack@suse.cz>
SuSE CR Labs

Re: (take 2)[PATCH] JBD: positively dispose the unmapped data buffers in journal_commit_transaction

[Toshiyuki Okajima]

Hi,

Jan Kara wrote:
>   Hi,
> 
>> I updated my patch and introduction article for it by reflecting
>> the comment of Andrew's.
<SNIP>
>>
>> Signed-off-by: Toshiyuki Okajima <toshi.okajima@jp.fujitsu.com>
>   I agree with the change. It's true that we can leave some anonymous
> pages behind and it's nicer to the MM to release them earlier when we
> know they will be never needed again. The patch looks fine to me, you
> can add
>   Acked-by: Jan Kara <jack@suse.cz>

Thank you for confirming.

>   How much have you stressed the patched kernel? I suggest you use
> fsxlinux and put some memory pressure to the system...

I have stressed it for 72 or more hours.
Stresser does:
  - allocates/frees big memory(1.7GB) which was almost system
    memory size(2GB) repeatedly.
Confirmation of integrity of patched Filesystem(jbd) does:
  - creates files, and copies 3 files from created each file
    (3 copies run concurrently), and confirms whether there is
    no difference between created files and copied files.
  (20 processes runs these works concurrently and repeatedly.)
Above 2 jobs run concurrently.

I haven't ever used 'fsxlinux'.
Next time, I will use it.

Thanks,
---
Toshiyuki Okajima

Re: (take 2)[PATCH] JBD: positively dispose the unmapped data buffers in journal_commit_transaction

[Jan Kara]

> Hi,
> 
> Jan Kara wrote:
> >  Hi,
> >
> >>I updated my patch and introduction article for it by reflecting
> >>the comment of Andrew's.
> <SNIP>
> >>
> >>Signed-off-by: Toshiyuki Okajima <toshi.okajima@jp.fujitsu.com>
> >  I agree with the change. It's true that we can leave some anonymous
> >pages behind and it's nicer to the MM to release them earlier when we
> >know they will be never needed again. The patch looks fine to me, you
> >can add
> >  Acked-by: Jan Kara <jack@suse.cz>
> 
> Thank you for confirming.
  Please keep me CCed (use group reply), thanks. I sometimes don't have
time for reading mailing lists or just skim through them so I can easily
miss replies...

> >  How much have you stressed the patched kernel? I suggest you use
> >fsxlinux and put some memory pressure to the system...
> 
> I have stressed it for 72 or more hours.
> Stresser does:
>  - allocates/frees big memory(1.7GB) which was almost system
>    memory size(2GB) repeatedly.
  OK, I suppose you also wrote something to the memory (otherwise it
won't be really allocated).

> Confirmation of integrity of patched Filesystem(jbd) does:
>  - creates files, and copies 3 files from created each file
>    (3 copies run concurrently), and confirms whether there is
>    no difference between created files and copied files.
>  (20 processes runs these works concurrently and repeatedly.)
> Above 2 jobs run concurrently.
  This sounds reasonable. fsxlinux does actually something similar but
it also stresses mmaped accesses and truncate patch. In this case, what
you did should be enough.

								Honza
-- 
Jan Kara <jack@suse.cz>
SuSE CR Labs

Re: (take 2)[PATCH] JBD: positively dispose the unmapped data buffers in journal_commit_transaction

[Toshiyuki Okajima]

Hi,

Jan Kara wrote:
>> Hi,
>>
>> Jan Kara wrote:
>>>  Hi,
>>>
>>>> I updated my patch and introduction article for it by reflecting
>>>> the comment of Andrew's.
>> <SNIP>
>>>> Signed-off-by: Toshiyuki Okajima <toshi.okajima@jp.fujitsu.com>
>>>  I agree with the change. It's true that we can leave some anonymous
>>> pages behind and it's nicer to the MM to release them earlier when we
>>> know they will be never needed again. The patch looks fine to me, you
>>> can add
>>>  Acked-by: Jan Kara <jack@suse.cz>
>> Thank you for confirming.
>   Please keep me CCed (use group reply), thanks. I sometimes don't have
> time for reading mailing lists or just skim through them so I can easily
> miss replies...

OK, I will keep you CCed next time.

>>>  How much have you stressed the patched kernel? I suggest you use
>>> fsxlinux and put some memory pressure to the system...
>> I have stressed it for 72 or more hours.
>> Stresser does:
>>  - allocates/frees big memory(1.7GB) which was almost system
>>    memory size(2GB) repeatedly.
>   OK, I suppose you also wrote something to the memory (otherwise it
> won't be really allocated).

I forgot to explain. You are right.
It writes something to each of all pages.

>> Confirmation of integrity of patched Filesystem(jbd) does:
>>  - creates files, and copies 3 files from created each file
>>    (3 copies run concurrently), and confirms whether there is
>>    no difference between created files and copied files.
>>  (20 processes runs these works concurrently and repeatedly.)
>> Above 2 jobs run concurrently.
>   This sounds reasonable. fsxlinux does actually something similar but
> it also stresses mmaped accesses and truncate patch. In this case, what
> you did should be enough.
> 
> 								Honza

I wrote only abstract of my long run test at previous mail,
but my test does also mmaped accesses.
So, it seems my test works really the same as fsxlinux...

Thanks,
---
Toshiyuki Okajima