Patchwork [1/2] jbd: clear revoked flag on buffers before a new transaction started

login
register
mail settings
Submitter Yongqiang Yang
Date Nov. 15, 2011, 8:02 a.m.
Message ID <1321344152-13033-1-git-send-email-xiaoqiangnk@gmail.com>
Download mbox | patch
Permalink /patch/125719/
State New
Headers show

Comments

Yongqiang Yang - Nov. 15, 2011, 8:02 a.m.
A revoked block in a transaction means the block is deleted by filesystem
in the transaction.  If the block is reused in the same transaction, we
need to cancel revoke status of the block.  We also should prohibit a block
from being revoked more than once in a transaction.  So we need to look up
the revoke table to check if a given block is revoked, to acceletate the
looking up, jbd/jbd2 use revoked flag to cache status of a block.

Ok, we should clear revoked flag once the transaction is not running. Because
the revoking and cancelling revoke operate on a running transaction. Once
a transaction is non-running, revoked flag is useless.

Without this patch, the following case triggers a false journal error.
Given that a block is used as a meta block and is deleted(revoked) in ordered
mode, then the block is allocated as a data block to a file.  Up to now,
user changes the file's journal mode from ordered to journaled, then truncates
the file.  The block will be considered re-revoked by journal because it
has revoked flag in last transaction.

Signed-off-by: Yongqiang Yang <xiaoqiangnk@gmail.com>
---
 fs/jbd/commit.c     |    5 +++++
 fs/jbd/revoke.c     |   28 ++++++++++++++++++++++++++++
 include/linux/jbd.h |    1 +
 3 files changed, 34 insertions(+), 0 deletions(-)
Jan Kara - Nov. 15, 2011, 1:27 p.m.
On Tue 15-11-11 16:02:31, Yongqiang Yang wrote:
> A revoked block in a transaction means the block is deleted by filesystem
> in the transaction.  If the block is reused in the same transaction, we
> need to cancel revoke status of the block.  We also should prohibit a block
> from being revoked more than once in a transaction.  So we need to look up
> the revoke table to check if a given block is revoked, to acceletate the
> looking up, jbd/jbd2 use revoked flag to cache status of a block.
> 
  I'd alter the change log here:
Currently, we clear revoked status only when a block is reused. However,
this can trigger a false journal error: Consider a situation when a block
is used as a metadata block, is deleted (revoked) in ordered mode, and then
the block is allocated as a data block to a file. At this moment user
changes the file's journal mode from ordered to journaled, and truncates
the file. The block will be considered re-revoked by journal because it
has revoked flag still pending from the last transaction and an assertion
triggers.

We fix the problem by keeping revoked status more uptodate - we clear
buffers' revoke bit when switching revoke tables to reflect that no buffers
are revoked in the current transaction anymore.

> Signed-off-by: Yongqiang Yang <xiaoqiangnk@gmail.com>
> ---
>  fs/jbd/commit.c     |    5 +++++
>  fs/jbd/revoke.c     |   28 ++++++++++++++++++++++++++++
>  include/linux/jbd.h |    1 +
>  3 files changed, 34 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/jbd/commit.c b/fs/jbd/commit.c
> index 8799207..075b7e8 100644
> --- a/fs/jbd/commit.c
> +++ b/fs/jbd/commit.c
> @@ -392,6 +392,11 @@ void journal_commit_transaction(journal_t *journal)
>  	jbd_debug (3, "JBD: commit phase 1\n");
>  
>  	/*
> +	 * Clear revoked flag.
  This comment does not explain nothing not obvious from the code. But we
should explain here *why* we need to clear the flags:
  We clear revoke status cached in b_state here to reflect that no buffer
is revoked in the next transaction which is going to be started.
> +	 */
> +	journal_clear_revoked_flag(journal);
  Maybe call the function journal_clear_buffer_revoked_flags()?

> +
> +	/*
>  	 * Switch to a new revoke table.
>  	 */
>  	journal_switch_revoke_table(journal);
> diff --git a/fs/jbd/revoke.c b/fs/jbd/revoke.c
> index 034eb82..cf73b67 100644
> --- a/fs/jbd/revoke.c
> +++ b/fs/jbd/revoke.c
> @@ -474,6 +474,34 @@ int journal_cancel_revoke(handle_t *handle, struct journal_head *jh)
>  	return did_revoke;
>  }
>  
> +/* journal_clear_revoked_flag clears revoked flag of buffers in
> + * revoke table.
> + */
  Comment style is:
/*
 * some text
 * more text
 */

> +void journal_clear_revoked_flag(journal_t *journal)
> +{
> +	struct jbd_revoke_table_s *revoke = journal->j_revoke;
> +	int i = 0;
> +
> +	for (i = 0; i < revoke->hash_size; i++) {
> +		struct list_head *hash_list;
> +		struct list_head *list_entry;
> +		hash_list = &revoke->hash_table[i];
> +
> +		list_for_each(list_entry, hash_list) {
> +			struct jbd_revoke_record_s *record;
> +			struct buffer_head *bh;
> +			record = (struct jbd_revoke_record_s *)list_entry;
> +			bh = __find_get_block(journal->j_fs_dev,
> +					      record->blocknr,
> +					      journal->j_blocksize);
> +			if (bh) {
> +				clear_buffer_revoked(bh);
> +				__brelse(bh);
> +			}
> +		}
> +	}
> +}
> +
>  /* journal_switch_revoke table select j_revoke for next transaction
>   * we do not want to suspend any processing until all revokes are
>   * written -bzzz
> diff --git a/include/linux/jbd.h b/include/linux/jbd.h
> index 0f9f0b6..de59ac0 100644
> --- a/include/linux/jbd.h
> +++ b/include/linux/jbd.h
> @@ -918,6 +918,7 @@ extern int	journal_set_revoke(journal_t *, unsigned int, tid_t);
>  extern int	journal_test_revoke(journal_t *, unsigned int, tid_t);
>  extern void	journal_clear_revoke(journal_t *);
>  extern void	journal_switch_revoke_table(journal_t *journal);
> +extern void	journal_clear_revoked_flag(journal_t *journal);
>  
>  /*
>   * The log thread user interface:
  Also please extend a comment (around line 50 in fs/jbd/journal.c) by:
We cache revoke status of a buffer in the current transaction in b_state
bits. Revoke information on buffers...

									Honza

Patch

diff --git a/fs/jbd/commit.c b/fs/jbd/commit.c
index 8799207..075b7e8 100644
--- a/fs/jbd/commit.c
+++ b/fs/jbd/commit.c
@@ -392,6 +392,11 @@  void journal_commit_transaction(journal_t *journal)
 	jbd_debug (3, "JBD: commit phase 1\n");
 
 	/*
+	 * Clear revoked flag.
+	 */
+	journal_clear_revoked_flag(journal);
+
+	/*
 	 * Switch to a new revoke table.
 	 */
 	journal_switch_revoke_table(journal);
diff --git a/fs/jbd/revoke.c b/fs/jbd/revoke.c
index 034eb82..cf73b67 100644
--- a/fs/jbd/revoke.c
+++ b/fs/jbd/revoke.c
@@ -474,6 +474,34 @@  int journal_cancel_revoke(handle_t *handle, struct journal_head *jh)
 	return did_revoke;
 }
 
+/* journal_clear_revoked_flag clears revoked flag of buffers in
+ * revoke table.
+ */
+void journal_clear_revoked_flag(journal_t *journal)
+{
+	struct jbd_revoke_table_s *revoke = journal->j_revoke;
+	int i = 0;
+
+	for (i = 0; i < revoke->hash_size; i++) {
+		struct list_head *hash_list;
+		struct list_head *list_entry;
+		hash_list = &revoke->hash_table[i];
+
+		list_for_each(list_entry, hash_list) {
+			struct jbd_revoke_record_s *record;
+			struct buffer_head *bh;
+			record = (struct jbd_revoke_record_s *)list_entry;
+			bh = __find_get_block(journal->j_fs_dev,
+					      record->blocknr,
+					      journal->j_blocksize);
+			if (bh) {
+				clear_buffer_revoked(bh);
+				__brelse(bh);
+			}
+		}
+	}
+}
+
 /* journal_switch_revoke table select j_revoke for next transaction
  * we do not want to suspend any processing until all revokes are
  * written -bzzz
diff --git a/include/linux/jbd.h b/include/linux/jbd.h
index 0f9f0b6..de59ac0 100644
--- a/include/linux/jbd.h
+++ b/include/linux/jbd.h
@@ -918,6 +918,7 @@  extern int	journal_set_revoke(journal_t *, unsigned int, tid_t);
 extern int	journal_test_revoke(journal_t *, unsigned int, tid_t);
 extern void	journal_clear_revoke(journal_t *);
 extern void	journal_switch_revoke_table(journal_t *journal);
+extern void	journal_clear_revoked_flag(journal_t *journal);
 
 /*
  * The log thread user interface: