LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Jan Kara <jack@suse.cz>
To: Josef Bacik <jbacik@redhat.com>
Cc: linux-kernel@vger.kernel.org, linux-ext4@vger.kernel.org,
	Andrew Morton <akpm@linux-foundation.org>
Subject: Re: [PATCH 2/2] fix possible journal overflow issues
Date: Mon, 3 Mar 2008 12:24:20 +0100	[thread overview]
Message-ID: <20080303112420.GB13725@duck.suse.cz> (raw)
In-Reply-To: <200802271351.54026.jbacik@redhat.com>

On Wed 27-02-08 13:51:53, Josef Bacik wrote:
> Hello,
> 
> There are several cases where the running transaction can get buffers added to 
> its BJ_Metadata list which it never dirtied, which makes its t_nr_buffers 
> counter end up larger than its t_outstanding_credits counter.  This will cause 
> issues when starting new transactions as while we are logging buffers we 
> decrement t_outstanding_buffers, so when t_outstanding_buffers goes negative, 
> we will report that we need less space in the journal than we actually need, so 
> transactions will be started even though there may not be enough room for them.  
> In the worst case scenario (which admittedly is almost impossible to reproduce) 
> this will result in the journal running out of space.  The fix is to only 
> refile buffers from the committing transaction to the running transactions 
> BJ_Modified list when b_modified is set on that journal, which is the only way 
> to be sure if the running transaction has modified that buffer.  This patch 
> also fixes an accounting error in journal_forget, it is possible that we can 
> call journal_forget on a buffer without having modified it, only gotten write 
> access to it, so instead of freeing a credit, we only do so if the buffer was 
> modified.  The assert will help catch if this problem occurs.  Without these 
> two patches I could hit this assert within minutes of running postmark, with 
> them this issue no longer arises.  Thank you,
> 
> Signed-off-by: Josef Bacik <jbacik@redhat.com>
  Acked-by: Jan Kara <jack@suse.cz>

									Honza
> 
> Index: linux-2.6/fs/jbd/transaction.c
> ===================================================================
> --- linux-2.6.orig/fs/jbd/transaction.c
> +++ linux-2.6/fs/jbd/transaction.c
> @@ -1234,6 +1234,7 @@ int journal_forget (handle_t *handle, st
>  	struct journal_head *jh;
>  	int drop_reserve = 0;
>  	int err = 0;
> +	int was_modified = 0;
>  
>  	BUFFER_TRACE(bh, "entry");
>  
> @@ -1252,6 +1253,9 @@ int journal_forget (handle_t *handle, st
>  		goto not_jbd;
>  	}
>  
> +	/* keep track of wether or not this transaction modified us */
> +	was_modified = jh->b_modified;
> +
>  	/*
>  	 * The buffer's going from the transaction, we must drop
>  	 * all references -bzzz
> @@ -1269,7 +1273,12 @@ int journal_forget (handle_t *handle, st
>  
>  		JBUFFER_TRACE(jh, "belongs to current transaction: unfile");
>  
> -		drop_reserve = 1;
> +		/*
> +		 * we only want to drop a reference if this transaction
> +		 * modified the buffer
> +		 */
> +		if (was_modified)
> +			drop_reserve = 1;
>  
>  		/*
>  		 * We are no longer going to journal this buffer.
> @@ -1309,7 +1318,13 @@ int journal_forget (handle_t *handle, st
>  		if (jh->b_next_transaction) {
>  			J_ASSERT(jh->b_next_transaction == transaction);
>  			jh->b_next_transaction = NULL;
> -			drop_reserve = 1;
> +
> +			/*
> +			 * only drop a reference if this transaction modified
> +			 * the buffer
> +			 */
> +			if (was_modified)
> +				drop_reserve = 1;
>  		}
>  	}
>  
> @@ -2081,7 +2096,7 @@ void __journal_refile_buffer(struct jour
>  	jh->b_transaction = jh->b_next_transaction;
>  	jh->b_next_transaction = NULL;
>  	__journal_file_buffer(jh, jh->b_transaction,
> -				was_dirty ? BJ_Metadata : BJ_Reserved);
> +				jh->b_modified ? BJ_Metadata : BJ_Reserved);
>  	J_ASSERT_JH(jh, jh->b_transaction->t_state == T_RUNNING);
>  
>  	if (was_dirty)
> Index: linux-2.6/fs/jbd2/transaction.c
> ===================================================================
> --- linux-2.6.orig/fs/jbd2/transaction.c
> +++ linux-2.6/fs/jbd2/transaction.c
> @@ -1243,6 +1243,7 @@ int jbd2_journal_forget (handle_t *handl
>  	struct journal_head *jh;
>  	int drop_reserve = 0;
>  	int err = 0;
> +	int was_modified = 0;
>  
>  	BUFFER_TRACE(bh, "entry");
>  
> @@ -1261,6 +1262,9 @@ int jbd2_journal_forget (handle_t *handl
>  		goto not_jbd;
>  	}
>  
> +	/* keep track of wether or not this transaction modified us */
> +	was_modified = jh->b_modified;
> +
>  	/*
>  	 * The buffer's going from the transaction, we must drop
>  	 * all references -bzzz
> @@ -1278,7 +1282,12 @@ int jbd2_journal_forget (handle_t *handl
>  
>  		JBUFFER_TRACE(jh, "belongs to current transaction: unfile");
>  
> -		drop_reserve = 1;
> +		/*
> +		 * we only want to drop a reference if this transaction
> +		 * modified the buffer
> +		 */
> +		if (was_modified)
> +			drop_reserve = 1;
>  
>  		/*
>  		 * We are no longer going to journal this buffer.
> @@ -1318,7 +1327,13 @@ int jbd2_journal_forget (handle_t *handl
>  		if (jh->b_next_transaction) {
>  			J_ASSERT(jh->b_next_transaction == transaction);
>  			jh->b_next_transaction = NULL;
> -			drop_reserve = 1;
> +
> +			/*
> +			 * only drop a reference if this transaction modified
> +			 * the buffer
> +			 */
> +			if (was_modified)
> +				drop_reserve = 1;
>  		}
>  	}
>  
> @@ -2090,7 +2105,7 @@ void __jbd2_journal_refile_buffer(struct
>  	jh->b_transaction = jh->b_next_transaction;
>  	jh->b_next_transaction = NULL;
>  	__jbd2_journal_file_buffer(jh, jh->b_transaction,
> -				was_dirty ? BJ_Metadata : BJ_Reserved);
> +				jh->b_modified ? BJ_Metadata : BJ_Reserved);
>  	J_ASSERT_JH(jh, jh->b_transaction->t_state == T_RUNNING);
>  
>  	if (was_dirty)
> Index: linux-2.6/fs/jbd/commit.c
> ===================================================================
> --- linux-2.6.orig/fs/jbd/commit.c
> +++ linux-2.6/fs/jbd/commit.c
> @@ -472,6 +472,9 @@ void journal_commit_transaction(journal_
>  	 */
>  	commit_transaction->t_state = T_COMMIT;
>  
> +	J_ASSERT(commit_transaction->t_nr_buffers <=
> +		 commit_transaction->t_outstanding_credits);
> +
>  	descriptor = NULL;
>  	bufs = 0;
>  	while (commit_transaction->t_buffers) {
> Index: linux-2.6/fs/jbd2/commit.c
> ===================================================================
> --- linux-2.6.orig/fs/jbd2/commit.c
> +++ linux-2.6/fs/jbd2/commit.c
> @@ -568,6 +568,9 @@ void jbd2_journal_commit_transaction(jou
>  	stats.u.run.rs_blocks = commit_transaction->t_outstanding_credits;
>  	stats.u.run.rs_blocks_logged = 0;
>  
> +	J_ASSERT(commit_transaction->t_nr_buffers <=
> +		 commit_transaction->t_outstanding_credits);
> +
>  	descriptor = NULL;
>  	bufs = 0;
>  	while (commit_transaction->t_buffers) {
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

      reply	other threads:[~2008-03-03 11:24 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-02-27 18:51 Josef Bacik
2008-03-03 11:24 ` Jan Kara [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20080303112420.GB13725@duck.suse.cz \
    --to=jack@suse.cz \
    --cc=akpm@linux-foundation.org \
    --cc=jbacik@redhat.com \
    --cc=linux-ext4@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --subject='Re: [PATCH 2/2] fix possible journal overflow issues' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).