2011-11-19 12:08:06

by Yongqiang Yang

[permalink] [raw]
Subject: [PATCH v2] jbd: clear revoked flag on buffers before a new transaction started

Currently, we clear revoked flag only when a block is reused. However,
this can tigger a false journal error. Consider a situation when 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. 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 the revoked status more uptodate - we clear
revoked flag when switching revoke tables to reflect there is no revoked
buffers in current transaction any more.

Signed-off-by: Yongqiang Yang <[email protected]>
---
fs/jbd/commit.c | 6 ++++++
fs/jbd/revoke.c | 34 ++++++++++++++++++++++++++++++++++
include/linux/jbd.h | 1 +
3 files changed, 41 insertions(+), 0 deletions(-)

diff --git a/fs/jbd/commit.c b/fs/jbd/commit.c
index 8799207..f2b9a57 100644
--- a/fs/jbd/commit.c
+++ b/fs/jbd/commit.c
@@ -392,6 +392,12 @@ void journal_commit_transaction(journal_t *journal)
jbd_debug (3, "JBD: commit phase 1\n");

/*
+ * Clear revoked flag to reflect there is no revoked buffers
+ * in the next transaction which is going to be started.
+ */
+ journal_clear_buffer_revoked_flags(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..501d363 100644
--- a/fs/jbd/revoke.c
+++ b/fs/jbd/revoke.c
@@ -47,6 +47,10 @@
* overwriting the new data. We don't even need to clear the revoke
* bit here.
*
+ * We cache revoke status of a buffer in the current transaction in b_states
+ * bits. As the name says, revokevalid flag indicates that the cached revoke
+ * status of a buffer is valid and we can rely on the cached status.
+ *
* Revoke information on buffers is a tri-state value:
*
* RevokeValid clear: no cached revoke status, need to look it up
@@ -474,6 +478,36 @@ 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 to reflect there is no revoked buffers in the next
+ * transaction which is going to be started.
+ */
+void journal_clear_buffer_revoked_flags(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..5aa964e 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_buffer_revoked_flags(journal_t *journal);

/*
* The log thread user interface:
--
1.7.5.1



2011-11-22 00:23:29

by Jan Kara

[permalink] [raw]
Subject: Re: [PATCH v2] jbd: clear revoked flag on buffers before a new transaction started

On Sat 19-11-11 17:34:29, Yongqiang Yang wrote:
> Currently, we clear revoked flag only when a block is reused. However,
> this can tigger a false journal error. Consider a situation when 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. 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 the revoked status more uptodate - we clear
> revoked flag when switching revoke tables to reflect there is no revoked
> buffers in current transaction any more.
Thanks. Merged into my tree. Will you create the same patch for JBD2 as
well?

Honza

>
> Signed-off-by: Yongqiang Yang <[email protected]>
> ---
> fs/jbd/commit.c | 6 ++++++
> fs/jbd/revoke.c | 34 ++++++++++++++++++++++++++++++++++
> include/linux/jbd.h | 1 +
> 3 files changed, 41 insertions(+), 0 deletions(-)
>
> diff --git a/fs/jbd/commit.c b/fs/jbd/commit.c
> index 8799207..f2b9a57 100644
> --- a/fs/jbd/commit.c
> +++ b/fs/jbd/commit.c
> @@ -392,6 +392,12 @@ void journal_commit_transaction(journal_t *journal)
> jbd_debug (3, "JBD: commit phase 1\n");
>
> /*
> + * Clear revoked flag to reflect there is no revoked buffers
> + * in the next transaction which is going to be started.
> + */
> + journal_clear_buffer_revoked_flags(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..501d363 100644
> --- a/fs/jbd/revoke.c
> +++ b/fs/jbd/revoke.c
> @@ -47,6 +47,10 @@
> * overwriting the new data. We don't even need to clear the revoke
> * bit here.
> *
> + * We cache revoke status of a buffer in the current transaction in b_states
> + * bits. As the name says, revokevalid flag indicates that the cached revoke
> + * status of a buffer is valid and we can rely on the cached status.
> + *
> * Revoke information on buffers is a tri-state value:
> *
> * RevokeValid clear: no cached revoke status, need to look it up
> @@ -474,6 +478,36 @@ 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 to reflect there is no revoked buffers in the next
> + * transaction which is going to be started.
> + */
> +void journal_clear_buffer_revoked_flags(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..5aa964e 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_buffer_revoked_flags(journal_t *journal);
>
> /*
> * The log thread user interface:
> --
> 1.7.5.1
>
--
Jan Kara <[email protected]>
SUSE Labs, CR

2011-11-22 01:55:18

by Yongqiang Yang

[permalink] [raw]
Subject: Re: [PATCH v2] jbd: clear revoked flag on buffers before a new transaction started

On Tue, Nov 22, 2011 at 8:23 AM, Jan Kara <[email protected]> wrote:
> On Sat 19-11-11 17:34:29, Yongqiang Yang wrote:
>> Currently, we clear revoked flag only when a block is reused. ?However,
>> this can tigger a false journal error. ?Consider a situation when 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. ?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 the revoked status more uptodate - we clear
>> revoked flag when switching revoke tables to reflect there is no revoked
>> buffers in current transaction any more.
> ?Thanks. Merged into my tree. Will you create the same patch for JBD2 as
> well?
YES! I had sent out the patch for jbd2. It seems that Ted is busy these days.

Yongqiang.
>
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?Honza
>
>>
>> Signed-off-by: Yongqiang Yang <[email protected]>
>> ---
>> ?fs/jbd/commit.c ? ? | ? ?6 ++++++
>> ?fs/jbd/revoke.c ? ? | ? 34 ++++++++++++++++++++++++++++++++++
>> ?include/linux/jbd.h | ? ?1 +
>> ?3 files changed, 41 insertions(+), 0 deletions(-)
>>
>> diff --git a/fs/jbd/commit.c b/fs/jbd/commit.c
>> index 8799207..f2b9a57 100644
>> --- a/fs/jbd/commit.c
>> +++ b/fs/jbd/commit.c
>> @@ -392,6 +392,12 @@ void journal_commit_transaction(journal_t *journal)
>> ? ? ? jbd_debug (3, "JBD: commit phase 1\n");
>>
>> ? ? ? /*
>> + ? ? ?* Clear revoked flag to reflect there is no revoked buffers
>> + ? ? ?* in the next transaction which is going to be started.
>> + ? ? ?*/
>> + ? ? journal_clear_buffer_revoked_flags(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..501d363 100644
>> --- a/fs/jbd/revoke.c
>> +++ b/fs/jbd/revoke.c
>> @@ -47,6 +47,10 @@
>> ? * ? overwriting the new data. ?We don't even need to clear the revoke
>> ? * ? bit here.
>> ? *
>> + * We cache revoke status of a buffer in the current transaction in b_states
>> + * bits. ?As the name says, revokevalid flag indicates that the cached revoke
>> + * status of a buffer is valid and we can rely on the cached status.
>> + *
>> ? * Revoke information on buffers is a tri-state value:
>> ? *
>> ? * RevokeValid clear: ? ? ? ?no cached revoke status, need to look it up
>> @@ -474,6 +478,36 @@ 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 to reflect there is no revoked buffers in the next
>> + * transaction which is going to be started.
>> + */
>> +void journal_clear_buffer_revoked_flags(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..5aa964e 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_buffer_revoked_flags(journal_t *journal);
>>
>> ?/*
>> ? * The log thread user interface:
>> --
>> 1.7.5.1
>>
> --
> Jan Kara <[email protected]>
> SUSE Labs, CR
>



--
Best Wishes
Yongqiang Yang

2011-11-22 12:52:00

by Theodore Ts'o

[permalink] [raw]
Subject: Re: [PATCH v2] jbd: clear revoked flag on buffers before a new transaction started


On Nov 21, 2011, at 8:55 PM, Yongqiang Yang wrote:
> YES! I had sent out the patch for jbd2. It seems that Ted is busy these days.

I generally start integrating patches for the next merge window after -rc3.

I've currently been more focused on the 1.42 e2fsprogs release.

-- Ted