delalloc blocks should be allocated before changing journal mode,
otherwise they can not be allocated and even more truncate on
delalloc blocks could triggre BUG by flushing delalloc buffers.
Signed-off-by: Yongqiang Yang <[email protected]>
---
fs/ext4/inode.c | 11 +++++++++++
1 files changed, 11 insertions(+), 0 deletions(-)
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index de05e86..384f8a7 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -4676,6 +4676,17 @@ int ext4_change_inode_journal_flag(struct inode *inode, int val)
return 0;
if (is_journal_aborted(journal))
return -EROFS;
+ /* We have to allocate physical blocks for delalloc blocks
+ * before flushing journal. otherwise delalloc blocks can not
+ * be allocated any more. even more truncate on delalloc blocks
+ * could trigger BUG by flushing delalloc blocks in journal.
+ * There is no delalloc block in non-journal data mode.
+ */
+ if (val && test_opt(inode->i_sb, DELALLOC)) {
+ err = ext4_alloc_da_blocks(inode);
+ if (err < 0)
+ return err;
+ }
jbd2_journal_lock_updates(journal);
jbd2_journal_flush(journal);
--
1.7.5.1
This patch lets ext4 journal deletion of data blocks. Besides this,
a unnecessary variable is removed.
Signed-off-by: Yongqiang Yang <[email protected]>
---
fs/ext4/mballoc.c | 7 ++-----
1 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index e2d8be8..2529efc 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -4562,19 +4562,16 @@ void ext4_free_blocks(handle_t *handle, struct inode *inode,
trace_ext4_free_blocks(inode, block, count, flags);
if (flags & EXT4_FREE_BLOCKS_FORGET) {
- struct buffer_head *tbh = bh;
int i;
BUG_ON(bh && (count > 1));
for (i = 0; i < count; i++) {
if (!bh)
- tbh = sb_find_get_block(inode->i_sb,
+ bh = sb_find_get_block(inode->i_sb,
block + i);
- if (unlikely(!tbh))
- continue;
ext4_forget(handle, flags & EXT4_FREE_BLOCKS_METADATA,
- inode, tbh, block + i);
+ inode, bh, block + i);
}
}
--
1.7.5.1
When switching from journal data mode, the data blocks
in journal will have no revoke record. Thus, data could be
corrupted during replay. However, there is no such problem in
switching to journal data mode. So we flush journal only in
the case that swithes from journal data mode.
Signed-off-by: Yongqiang Yang <[email protected]>
---
fs/ext4/inode.c | 7 ++++---
1 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 384f8a7..755f6c7 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -4689,7 +4689,6 @@ int ext4_change_inode_journal_flag(struct inode *inode, int val)
}
jbd2_journal_lock_updates(journal);
- jbd2_journal_flush(journal);
/*
* OK, there are no updates running now, and all cached data is
@@ -4699,10 +4698,12 @@ int ext4_change_inode_journal_flag(struct inode *inode, int val)
* the inode's in-core data-journaling state flag now.
*/
- if (val)
+ if (val) {
ext4_set_inode_flag(inode, EXT4_INODE_JOURNAL_DATA);
- else
+ } else {
+ jbd2_journal_flush(journal);
ext4_clear_inode_flag(inode, EXT4_INODE_JOURNAL_DATA);
+ }
ext4_set_aops(inode);
jbd2_journal_unlock_updates(journal);
--
1.7.5.1
We should not pass buffer whiose block number is not block + i.
Signed-off-by: Yongqiang Yang <[email protected]>
---
fs/ext4/mballoc.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 2529efc..a64b3b8 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -4572,6 +4572,7 @@ void ext4_free_blocks(handle_t *handle, struct inode *inode,
block + i);
ext4_forget(handle, flags & EXT4_FREE_BLOCKS_METADATA,
inode, bh, block + i);
+ bh = NULL;
}
}
--
1.7.5.1
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 <[email protected]>
---
fs/ext4/ext4_jbd2.c | 5 +++++
fs/jbd2/commit.c | 5 +++++
fs/jbd2/revoke.c | 28 ++++++++++++++++++++++++++++
include/linux/jbd2.h | 1 +
4 files changed, 39 insertions(+), 0 deletions(-)
diff --git a/fs/ext4/ext4_jbd2.c b/fs/ext4/ext4_jbd2.c
index aca1790..c69baa0 100644
--- a/fs/ext4/ext4_jbd2.c
+++ b/fs/ext4/ext4_jbd2.c
@@ -47,6 +47,11 @@ int __ext4_forget(const char *where, unsigned int line, handle_t *handle,
"data mode %x\n",
bh, is_metadata, inode->i_mode,
test_opt(inode->i_sb, DATA_FLAGS));
+ if (bh)
+ printk("forgetting bh %p: is_metadata = %d, mode %o, "
+ "data mode %x\n",
+ bh, is_metadata, inode->i_mode,
+ test_opt(inode->i_sb, DATA_FLAGS));
/* In the no journal case, we can just do a bforget and return */
if (!ext4_handle_valid(handle)) {
diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
index 264f0bb..ec63df5 100644
--- a/fs/jbd2/commit.c
+++ b/fs/jbd2/commit.c
@@ -430,6 +430,11 @@ void jbd2_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.
*/
jbd2_journal_switch_revoke_table(journal);
diff --git a/fs/jbd2/revoke.c b/fs/jbd2/revoke.c
index 1b67105..dca50c6 100644
--- a/fs/jbd2/revoke.c
+++ b/fs/jbd2/revoke.c
@@ -474,6 +474,34 @@ int jbd2_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 jbd2_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 jbd2_revoke_record_s *record;
+ struct buffer_head *bh;
+ record = (struct jbd2_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/jbd2.h b/include/linux/jbd2.h
index e44d114..4891a4d 100644
--- a/include/linux/jbd2.h
+++ b/include/linux/jbd2.h
@@ -1156,6 +1156,7 @@ extern int jbd2_journal_set_revoke(journal_t *, unsigned long long, tid_t);
extern int jbd2_journal_test_revoke(journal_t *, unsigned long long, tid_t);
extern void jbd2_journal_clear_revoke(journal_t *);
extern void jbd2_journal_switch_revoke_table(journal_t *journal);
+extern void journal_clear_revoked_flag(journal_t *journal);
/*
* The log thread user interface:
--
1.7.5.1
(2011/11/15 17:07), Yongqiang Yang wrote:
> delalloc blocks should be allocated before changing journal mode,
> otherwise they can not be allocated and even more truncate on
> delalloc blocks could triggre BUG by flushing delalloc buffers.
>
> Signed-off-by: Yongqiang Yang <[email protected]>
> ---
> fs/ext4/inode.c | 11 +++++++++++
> 1 files changed, 11 insertions(+), 0 deletions(-)
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index de05e86..384f8a7 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -4676,6 +4676,17 @@ int ext4_change_inode_journal_flag(struct inode *inode, int val)
> return 0;
> if (is_journal_aborted(journal))
> return -EROFS;
> + /* We have to allocate physical blocks for delalloc blocks
> + * before flushing journal. otherwise delalloc blocks can not
> + * be allocated any more. even more truncate on delalloc blocks
> + * could trigger BUG by flushing delalloc blocks in journal.
> + * There is no delalloc block in non-journal data mode.
> + */
> + if (val && test_opt(inode->i_sb, DELALLOC)) {
> + err = ext4_alloc_da_blocks(inode);
> + if (err < 0)
> + return err;
> + }
>
> jbd2_journal_lock_updates(journal);
> jbd2_journal_flush(journal);
Though I tested your patch by my reproducer that caused a panic which you
pointed out, a panic did not happen.
-------------------------------------------------------------------------------
#!/bin/sh
echo "a" > /tmp/a # file under an ext4 filesystem
chattr +j /tmp/a
echo "a" >> /tmp/a
chattr -j /tmp/a
-------------------------------------------------------------------------------
So, this patch looks good to me.
Tested-by: Toshiyuki Okajima <[email protected]>
On Tue, Nov 15, 2011 at 04:07:50PM +0800, Yongqiang Yang wrote:
> delalloc blocks should be allocated before changing journal mode,
> otherwise they can not be allocated and even more truncate on
> delalloc blocks could triggre BUG by flushing delalloc buffers.
>
> Signed-off-by: Yongqiang Yang <[email protected]>
Applied, thanks.
- Ted
On Tue, Nov 15, 2011 at 04:07:51PM +0800, Yongqiang Yang wrote:
> This patch lets ext4 journal deletion of data blocks. Besides this,
> a unnecessary variable is removed.
>
> Signed-off-by: Yongqiang Yang <[email protected]>
I don't see the point of this patch; it seems to be a code
simplification, but in fact it introduces a bug which has to get fixed
in patch 3/5 of this series.
The code here is a little arcane, because if bh is non-null, then
count must be 1. This is expressed in the BUG_ON found in the
function:
> BUG_ON(bh && (count > 1));
The reason for this bit of complexity is to avoid needing to call
sb_find_get_block() in those places where we have the buffer_head
already. This happens in exactly two locations: in an error cleanup
path in fs/ext4/indirect.c, and when releasing an xattr block in
ext4_xattr_release_block().
The better way of dealing with this is to drop the bh argument from
ext4_free_blocks() completely, and explicitly call ext4_forget() on
the bh in those two functions.
This will require changing all of the call sites of
ext4_free_blocks(), but it simplifies the function signature as well
as simplifying the code.
- Ted
> fs/ext4/mballoc.c | 7 ++-----
> 1 files changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index e2d8be8..2529efc 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -4562,19 +4562,16 @@ void ext4_free_blocks(handle_t *handle, struct inode *inode,
> trace_ext4_free_blocks(inode, block, count, flags);
>
> if (flags & EXT4_FREE_BLOCKS_FORGET) {
> - struct buffer_head *tbh = bh;
> int i;
>
> BUG_ON(bh && (count > 1));
>
> for (i = 0; i < count; i++) {
> if (!bh)
> - tbh = sb_find_get_block(inode->i_sb,
> + bh = sb_find_get_block(inode->i_sb,
> block + i);
> - if (unlikely(!tbh))
> - continue;
> ext4_forget(handle, flags & EXT4_FREE_BLOCKS_METADATA,
> - inode, tbh, block + i);
> + inode, bh, block + i);
> }
> }
>
> --
> 1.7.5.1
>
On Tue, Nov 15, 2011 at 04:07:52PM +0800, Yongqiang Yang wrote:
> We should not pass buffer whiose block number is not block + i.
>
> Signed-off-by: Yongqiang Yang <[email protected]>
This fixes a big in PATCH 2/5; see the comments for that patch.
- Ted
On Tue, Nov 15, 2011 at 04:07:53PM +0800, Yongqiang Yang wrote:
> When switching from journal data mode, the data blocks
> in journal will have no revoke record. Thus, data could be
> corrupted during replay. However, there is no such problem in
> switching to journal data mode. So we flush journal only in
> the case that swithes from journal data mode.
>
> Signed-off-by: Yongqiang Yang <[email protected]>
Applied, with a slightly different (and more explanatory commit
message):
ext4: flush journal when switching from data=journal mode
From: Yongqiang Yang <[email protected]>
It's necessary to flush the journal when switching away from
data=journal mode. This is because there are no revoke records when
we are data blocks are journalled, which are required in the other
journal modes.
However, it is not necessary to flush the journal when switching into
data=journal mode, and flushing the journal is expensive. So let's
avoid it in that case.
Signed-off-by: Yongqiang Yang <[email protected]>
Signed-off-by: "Theodore Ts'o" <[email protected]>
- Ted
On Tue, Nov 15, 2011 at 04:07:54PM +0800, 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.
>
> 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 <[email protected]>
Applied, thanks.
- Ted
On Wed, Dec 28, 2011 at 01:56:43PM -0500, Ted Ts'o wrote:
> On Tue, Nov 15, 2011 at 04:07:53PM +0800, Yongqiang Yang wrote:
> > When switching from journal data mode, the data blocks
> > in journal will have no revoke record. Thus, data could be
> > corrupted during replay. However, there is no such problem in
> > switching to journal data mode. So we flush journal only in
> > the case that swithes from journal data mode.
> >
> > Signed-off-by: Yongqiang Yang <[email protected]>
>
> Applied, with a slightly different (and more explanatory commit
> message):
>
> ext4: flush journal when switching from data=journal mode
>
> From: Yongqiang Yang <[email protected]>
>
> It's necessary to flush the journal when switching away from
> data=journal mode. This is because there are no revoke records when
> we are data blocks are journalled,
Minor nit, but did you mean "when data blocks are journalled" ?
--D
> which are required in the other journal modes.
>
> However, it is not necessary to flush the journal when switching into
> data=journal mode, and flushing the journal is expensive. So let's
> avoid it in that case.
>
> Signed-off-by: Yongqiang Yang <[email protected]>
> Signed-off-by: "Theodore Ts'o" <[email protected]>
>
> - Ted
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
On Fri, Dec 30, 2011 at 5:01 AM, Darrick J. Wong <[email protected]> wrote:
> On Wed, Dec 28, 2011 at 01:56:43PM -0500, Ted Ts'o wrote:
>> On Tue, Nov 15, 2011 at 04:07:53PM +0800, Yongqiang Yang wrote:
>> > When switching from journal data mode, the data blocks
>> > in journal will have no revoke record. ?Thus, data could be
>> > corrupted during replay. ?However, there is no such problem in
>> > switching to journal data mode. ?So we flush journal only in
>> > the case that swithes from journal data mode.
>> >
>> > Signed-off-by: Yongqiang Yang <[email protected]>
>>
>> Applied, with a slightly different (and more explanatory commit
>> message):
>>
>> ext4: flush journal when switching from data=journal mode
>>
>> From: Yongqiang Yang <[email protected]>
>>
>> It's necessary to flush the journal when switching away from
>> data=journal mode. ?This is because there are no revoke records when
>> we are data blocks are journalled,
>
> Minor nit, but did you mean "when data blocks are journalled" ?
Yes! chattr +j can change journal mode of a file to data mode.
Yongqiang.
>
> --D
>> which are required in the other journal modes.
>>
>> However, it is not necessary to flush the journal when switching into
>> data=journal mode, and flushing the journal is expensive. ?So let's
>> avoid it in that case.
>>
>> Signed-off-by: Yongqiang Yang <[email protected]>
>> Signed-off-by: "Theodore Ts'o" <[email protected]>
>>
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? - Ted
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
>> the body of a message to [email protected]
>> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
>>
>
--
Best Wishes
Yongqiang Yang
On Thu, Dec 29, 2011 at 01:01:53PM -0800, Darrick J. Wong wrote:
> > It's necessary to flush the journal when switching away from
> > data=journal mode. This is because there are no revoke records when
> > we are data blocks are journalled,
>
> Minor nit, but did you mean "when data blocks are journalled" ?
Thanks, I've made this fix in commit description.
- Ted
Hi Ted,
The 2nd and 3rd patch aim to let ext4_free_blocks work with journal
mode. Consider that journal mode of a file is changed from ordered
mode to journal mode and several data blocks are deleted, then bh
passed in is NULL and sb_find_get_block returns NULL, but we need
ext4_forget to handle the data blocks to record them in revoke table.
I am not sure status of ext4 with journal mode, according code here it
seems that ext4 with journal mode does not work.
Yongqiang.
On Thu, Dec 29, 2011 at 1:23 AM, Ted Ts'o <[email protected]> wrote:
> On Tue, Nov 15, 2011 at 04:07:51PM +0800, Yongqiang Yang wrote:
>> This patch lets ext4 journal deletion of data blocks. Besides this,
>> a unnecessary variable is removed.
>>
>> Signed-off-by: Yongqiang Yang <[email protected]>
>
> I don't see the point of this patch; it seems to be a code
> simplification, but in fact it introduces a bug which has to get fixed
> in patch 3/5 of this series.
>
> The code here is a little arcane, because if bh is non-null, then
> count must be 1. ?This is expressed in the BUG_ON found in the
> function:
>
>> ? ? ? ? ? ? ? BUG_ON(bh && (count > 1));
>
> The reason for this bit of complexity is to avoid needing to call
> sb_find_get_block() in those places where we have the buffer_head
> already. ?This happens in exactly two locations: in an error cleanup
> path in fs/ext4/indirect.c, and when releasing an xattr block in
> ext4_xattr_release_block().
>
> The better way of dealing with this is to drop the bh argument from
> ext4_free_blocks() completely, and explicitly call ext4_forget() on
> the bh in those two functions.
>
> This will require changing all of the call sites of
> ext4_free_blocks(), but it simplifies the function signature as well
> as simplifying the code.
>
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?- Ted
>
>> ?fs/ext4/mballoc.c | ? ?7 ++-----
>> ?1 files changed, 2 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
>> index e2d8be8..2529efc 100644
>> --- a/fs/ext4/mballoc.c
>> +++ b/fs/ext4/mballoc.c
>> @@ -4562,19 +4562,16 @@ void ext4_free_blocks(handle_t *handle, struct inode *inode,
>> ? ? ? trace_ext4_free_blocks(inode, block, count, flags);
>>
>> ? ? ? if (flags & EXT4_FREE_BLOCKS_FORGET) {
>> - ? ? ? ? ? ? struct buffer_head *tbh = bh;
>> ? ? ? ? ? ? ? int i;
>>
>> ? ? ? ? ? ? ? BUG_ON(bh && (count > 1));
>>
>> ? ? ? ? ? ? ? for (i = 0; i < count; i++) {
>> ? ? ? ? ? ? ? ? ? ? ? if (!bh)
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? tbh = sb_find_get_block(inode->i_sb,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? bh = sb_find_get_block(inode->i_sb,
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? block + i);
>> - ? ? ? ? ? ? ? ? ? ? if (unlikely(!tbh))
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? continue;
>> ? ? ? ? ? ? ? ? ? ? ? ext4_forget(handle, flags & EXT4_FREE_BLOCKS_METADATA,
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? inode, tbh, block + i);
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? inode, bh, block + i);
>> ? ? ? ? ? ? ? }
>> ? ? ? }
>>
>> --
>> 1.7.5.1
>>
--
Best Wishes
Yongqiang Yang
On Fri, Dec 30, 2011 at 10:59:48PM +0800, Yongqiang Yang wrote:
> Hi Ted,
>
> The 2nd and 3rd patch aim to let ext4_free_blocks work with journal
> mode. Consider that journal mode of a file is changed from ordered
> mode to journal mode and several data blocks are deleted, then bh
> passed in is NULL and sb_find_get_block returns NULL, but we need
> ext4_forget to handle the data blocks to record them in revoke table.
Ah, I see. This wasn't obvious from the commit description. Could
you combine patches #2 and #3, and add the above detail in the commit
description?
Many thanks!!
- Ted