[RESEND,2/2] ext3: flush journal only when journal mode is changed from journaled
diff mbox

Message ID 1321344289-13125-1-git-send-email-xiaoqiangnk@gmail.com
State New
Headers show

Commit Message

Yongqiang Yang Nov. 15, 2011, 8:04 a.m. UTC
To prevent data from corruption during repaly, we need to flush journal
before changing journal mode from journaled.  However flushing journal is
not needed when changing journal to journaled.

Signed-off-by: Yongqiang Yang <xiaoqiangnk@gmail.com>
---
 fs/ext3/inode.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

Comments

Jan Kara Nov. 15, 2011, 1:33 p.m. UTC | #1
On Tue 15-11-11 16:04:49, Yongqiang Yang wrote:
> To prevent data from corruption during repaly, we need to flush journal
> before changing journal mode from journaled.  However flushing journal is
> not needed when changing journal to journaled.
  We flush the journal for other reasons as well - switching inodes
->i_aops is rather complex thing so a filesystem should better be in a
quiescent state. You'd definitely at least need to commit the running
transaction otherwise a hell would break loose if you tried to modify a
buffer both as ordered data and as metadata in one transaction.

  So I strongly prefer to keep the code as is unless you can really
convincingly *prove* that nothing breaks and show benefit of such change.

								Honza
> 
> Signed-off-by: Yongqiang Yang <xiaoqiangnk@gmail.com>
> ---
>  fs/ext3/inode.c |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/ext3/inode.c b/fs/ext3/inode.c
> index 04da6ac..5577358 100644
> --- a/fs/ext3/inode.c
> +++ b/fs/ext3/inode.c
> @@ -3546,7 +3546,6 @@ int ext3_change_inode_journal_flag(struct inode *inode, int val)
>  		return -EROFS;
>  
>  	journal_lock_updates(journal);
> -	journal_flush(journal);
>  
>  	/*
>  	 * OK, there are no updates running now, and all cached data is
> @@ -3556,10 +3555,12 @@ int ext3_change_inode_journal_flag(struct inode *inode, int val)
>  	 * the inode's in-core data-journaling state flag now.
>  	 */
>  
> -	if (val)
> +	if (val) {
>  		EXT3_I(inode)->i_flags |= EXT3_JOURNAL_DATA_FL;
> -	else
> +	} else {
> +		journal_flush(journal);
>  		EXT3_I(inode)->i_flags &= ~EXT3_JOURNAL_DATA_FL;
> +	}
>  	ext3_set_aops(inode);
>  
>  	journal_unlock_updates(journal);
> -- 
> 1.7.5.1
>
Yongqiang Yang Nov. 15, 2011, 1:57 p.m. UTC | #2
On Tue, Nov 15, 2011 at 9:33 PM, Jan Kara <jack@suse.cz> wrote:
> On Tue 15-11-11 16:04:49, Yongqiang Yang wrote:
>> To prevent data from corruption during repaly, we need to flush journal
>> before changing journal mode from journaled.  However flushing journal is
>> not needed when changing journal to journaled.
>  We flush the journal for other reasons as well - switching inodes
> ->i_aops is rather complex thing so a filesystem should better be in a
> quiescent state. You'd definitely at least need to commit the running
> transaction otherwise a hell would break loose if you tried to modify a
> buffer both as ordered data and as metadata in one transaction.
>
>  So I strongly prefer to keep the code as is unless you can really
> convincingly *prove* that nothing breaks and show benefit of such change.
Well.  The only benefit is that we can switch journal mode of a file
from ordered to journaled without flushing journal, thus, it is much
faster.
Before switching journal mode of a file from ordered to journaled, the
metadata has been in journal while data not.  After switching, both
metadata and data will be go to journal.  I am not sure if there is
any problem:-).

I am not very insistent on the patch.  It seems that the benefit can be ignored.

Yongqiang.
>
>                                                                Honza
>>
>> Signed-off-by: Yongqiang Yang <xiaoqiangnk@gmail.com>
>> ---
>>  fs/ext3/inode.c |    7 ++++---
>>  1 files changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/ext3/inode.c b/fs/ext3/inode.c
>> index 04da6ac..5577358 100644
>> --- a/fs/ext3/inode.c
>> +++ b/fs/ext3/inode.c
>> @@ -3546,7 +3546,6 @@ int ext3_change_inode_journal_flag(struct inode *inode, int val)
>>               return -EROFS;
>>
>>       journal_lock_updates(journal);
>> -     journal_flush(journal);
>>
>>       /*
>>        * OK, there are no updates running now, and all cached data is
>> @@ -3556,10 +3555,12 @@ int ext3_change_inode_journal_flag(struct inode *inode, int val)
>>        * the inode's in-core data-journaling state flag now.
>>        */
>>
>> -     if (val)
>> +     if (val) {
>>               EXT3_I(inode)->i_flags |= EXT3_JOURNAL_DATA_FL;
>> -     else
>> +     } else {
>> +             journal_flush(journal);
>>               EXT3_I(inode)->i_flags &= ~EXT3_JOURNAL_DATA_FL;
>> +     }
>>       ext3_set_aops(inode);
>>
>>       journal_unlock_updates(journal);
>> --
>> 1.7.5.1
>>
> --
> Jan Kara <jack@suse.cz>
> SUSE Labs, CR
>

Patch
diff mbox

diff --git a/fs/ext3/inode.c b/fs/ext3/inode.c
index 04da6ac..5577358 100644
--- a/fs/ext3/inode.c
+++ b/fs/ext3/inode.c
@@ -3546,7 +3546,6 @@  int ext3_change_inode_journal_flag(struct inode *inode, int val)
 		return -EROFS;
 
 	journal_lock_updates(journal);
-	journal_flush(journal);
 
 	/*
 	 * OK, there are no updates running now, and all cached data is
@@ -3556,10 +3555,12 @@  int ext3_change_inode_journal_flag(struct inode *inode, int val)
 	 * the inode's in-core data-journaling state flag now.
 	 */
 
-	if (val)
+	if (val) {
 		EXT3_I(inode)->i_flags |= EXT3_JOURNAL_DATA_FL;
-	else
+	} else {
+		journal_flush(journal);
 		EXT3_I(inode)->i_flags &= ~EXT3_JOURNAL_DATA_FL;
+	}
 	ext3_set_aops(inode);
 
 	journal_unlock_updates(journal);