[Ocfs2-devel] [PATCH] ocfs2: clear journal dirty flag after shutdown journal

[Ocfs2-devel] [PATCH] ocfs2: clear journal dirty flag after shutdown journal

[Junxiao Bi]

Dirty flag of the journal should be cleared at the last stage of umount,
if do it before jbd2_journal_destroy(), then some metadata in uncommitted
transaction could be lost due to io error, but as dirty flag of journal
was already cleared, we can't find that until run a full fsck. This may
cause system panic or other corruption.

Signed-off-by: Junxiao Bi <junxiao.bi@oracle.com>
---
 fs/ocfs2/journal.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c
index b63c97f4318e..25d678c92fbb 100644
--- a/fs/ocfs2/journal.c
+++ b/fs/ocfs2/journal.c
@@ -1017,6 +1017,10 @@ void ocfs2_journal_shutdown(struct ocfs2_super *osb)
 			mlog_errno(status);
 	}
 
+	/* Shutdown the kernel journal system */
+	jbd2_journal_destroy(journal->j_journal);
+	journal->j_journal = NULL;
+
 	if (status == 0) {
 		/*
 		 * Do not toggle if flush was unsuccessful otherwise
@@ -1027,10 +1031,6 @@ void ocfs2_journal_shutdown(struct ocfs2_super *osb)
 			mlog_errno(status);
 	}
 
-	/* Shutdown the kernel journal system */
-	jbd2_journal_destroy(journal->j_journal);
-	journal->j_journal = NULL;
-
 	OCFS2_I(inode)->ip_open_count--;
 
 	/* unlock our journal */
-- 
2.17.1

[Ocfs2-devel] [PATCH] ocfs2: clear journal dirty flag after shutdown journal

[piaojun]

Hi Junxiao,

I'm very interested in this bug, and it seems causing read-only if
involving metadata. Could you help show how to reproduce this problem?

Thanks,
Jun

On 2018/11/16 16:58, Junxiao Bi wrote:
> Dirty flag of the journal should be cleared at the last stage of umount,
> if do it before jbd2_journal_destroy(), then some metadata in uncommitted
> transaction could be lost due to io error, but as dirty flag of journal
> was already cleared, we can't find that until run a full fsck. This may
> cause system panic or other corruption.
> 
> Signed-off-by: Junxiao Bi <junxiao.bi@oracle.com>
> ---
>  fs/ocfs2/journal.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c
> index b63c97f4318e..25d678c92fbb 100644
> --- a/fs/ocfs2/journal.c
> +++ b/fs/ocfs2/journal.c
> @@ -1017,6 +1017,10 @@ void ocfs2_journal_shutdown(struct ocfs2_super *osb)
>  			mlog_errno(status);
>  	}
>  
> +	/* Shutdown the kernel journal system */
> +	jbd2_journal_destroy(journal->j_journal);
> +	journal->j_journal = NULL;
> +
>  	if (status == 0) {
>  		/*
>  		 * Do not toggle if flush was unsuccessful otherwise
> @@ -1027,10 +1031,6 @@ void ocfs2_journal_shutdown(struct ocfs2_super *osb)
>  			mlog_errno(status);
>  	}
>  
> -	/* Shutdown the kernel journal system */
> -	jbd2_journal_destroy(journal->j_journal);
> -	journal->j_journal = NULL;
> -
>  	OCFS2_I(inode)->ip_open_count--;
>  
>  	/* unlock our journal */
> 

[Ocfs2-devel] [PATCH] ocfs2: clear journal dirty flag after shutdown journal

[Junxiao Bi]

Hi Jun,

ocfs2_journal_shutdown() was only invoked during umount, jbd2 will force all uncommitted metadata to disk, there is no set fs readonly for io error in that case.
To reproduce, you need cause storage return io error between ocfs2_journal_toggle_dirty() and jbd2_journal_destroy().
  
1020         if (status == 0) {
1021                 /*
1022                  * Do not toggle if flush was unsuccessful otherwise
1023                  * will leave dirty metadata in a "clean" journal
1024                  */
1025                 status = ocfs2_journal_toggle_dirty(osb, 0, 0);
1026                 if (status < 0)
1027                         mlog_errno(status);
1028         }
1029
1030         /* Shutdown the kernel journal system */
1031         jbd2_journal_destroy(journal->j_journal);

Thanks,
Junxiao.

On 11/19/18 9:32 AM, piaojun wrote:
> Hi Junxiao,
>
> I'm very interested in this bug, and it seems causing read-only if
> involving metadata. Could you help show how to reproduce this problem?
>
> Thanks,
> Jun
>
> On 2018/11/16 16:58, Junxiao Bi wrote:
>> Dirty flag of the journal should be cleared at the last stage of umount,
>> if do it before jbd2_journal_destroy(), then some metadata in uncommitted
>> transaction could be lost due to io error, but as dirty flag of journal
>> was already cleared, we can't find that until run a full fsck. This may
>> cause system panic or other corruption.
>>
>> Signed-off-by: Junxiao Bi <junxiao.bi@oracle.com>
>> ---
>>   fs/ocfs2/journal.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c
>> index b63c97f4318e..25d678c92fbb 100644
>> --- a/fs/ocfs2/journal.c
>> +++ b/fs/ocfs2/journal.c
>> @@ -1017,6 +1017,10 @@ void ocfs2_journal_shutdown(struct ocfs2_super *osb)
>>   			mlog_errno(status);
>>   	}
>>   
>> +	/* Shutdown the kernel journal system */
>> +	jbd2_journal_destroy(journal->j_journal);
>> +	journal->j_journal = NULL;
>> +
>>   	if (status == 0) {
>>   		/*
>>   		 * Do not toggle if flush was unsuccessful otherwise
>> @@ -1027,10 +1031,6 @@ void ocfs2_journal_shutdown(struct ocfs2_super *osb)
>>   			mlog_errno(status);
>>   	}
>>   
>> -	/* Shutdown the kernel journal system */
>> -	jbd2_journal_destroy(journal->j_journal);
>> -	journal->j_journal = NULL;
>> -
>>   	OCFS2_I(inode)->ip_open_count--;
>>   
>>   	/* unlock our journal */
>>

[Ocfs2-devel] [PATCH] ocfs2: clear journal dirty flag after shutdown journal

[jiangyiwen]

On 2018/11/16 16:58, Junxiao Bi wrote:
> Dirty flag of the journal should be cleared at the last stage of umount,
> if do it before jbd2_journal_destroy(), then some metadata in uncommitted
> transaction could be lost due to io error, but as dirty flag of journal
> was already cleared, we can't find that until run a full fsck. This may
> cause system panic or other corruption.
> 
> Signed-off-by: Junxiao Bi <junxiao.bi@oracle.com>
> ---
>  fs/ocfs2/journal.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c
> index b63c97f4318e..25d678c92fbb 100644
> --- a/fs/ocfs2/journal.c
> +++ b/fs/ocfs2/journal.c
> @@ -1017,6 +1017,10 @@ void ocfs2_journal_shutdown(struct ocfs2_super *osb)
>  			mlog_errno(status);
>  	}
>  
> +	/* Shutdown the kernel journal system */
> +	jbd2_journal_destroy(journal->j_journal);
> +	journal->j_journal = NULL;
> +

Hi Junxiao,

I feel this adjustment doesn't make any sense. When jbd2_journal_destroy() is
done, it still call ocfs2_journal_toggle_dirty() to clean dirty flag. Am I
wrong or understand error ?

Thanks.
Yiwen.

>  	if (status == 0) {
>  		/*
>  		 * Do not toggle if flush was unsuccessful otherwise
> @@ -1027,10 +1031,6 @@ void ocfs2_journal_shutdown(struct ocfs2_super *osb)
>  			mlog_errno(status);
>  	}
>  
> -	/* Shutdown the kernel journal system */
> -	jbd2_journal_destroy(journal->j_journal);
> -	journal->j_journal = NULL;
> -
>  	OCFS2_I(inode)->ip_open_count--;
>  
>  	/* unlock our journal */
> 

[Ocfs2-devel] [PATCH] ocfs2: clear journal dirty flag after shutdown journal

[Junxiao Bi]

On 11/19/18 10:28 AM, jiangyiwen wrote:
> On 2018/11/16 16:58, Junxiao Bi wrote:
>> Dirty flag of the journal should be cleared at the last stage of umount,
>> if do it before jbd2_journal_destroy(), then some metadata in uncommitted
>> transaction could be lost due to io error, but as dirty flag of journal
>> was already cleared, we can't find that until run a full fsck. This may
>> cause system panic or other corruption.
>>
>> Signed-off-by: Junxiao Bi <junxiao.bi@oracle.com>
>> ---
>>   fs/ocfs2/journal.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c
>> index b63c97f4318e..25d678c92fbb 100644
>> --- a/fs/ocfs2/journal.c
>> +++ b/fs/ocfs2/journal.c
>> @@ -1017,6 +1017,10 @@ void ocfs2_journal_shutdown(struct ocfs2_super *osb)
>>   			mlog_errno(status);
>>   	}
>>   
>> +	/* Shutdown the kernel journal system */
>> +	jbd2_journal_destroy(journal->j_journal);
>> +	journal->j_journal = NULL;
>> +
> Hi Junxiao,
>
> I feel this adjustment doesn't make any sense. When jbd2_journal_destroy() is
> done, it still call ocfs2_journal_toggle_dirty() to clean dirty flag. Am I
> wrong or understand error ?

Oh, missed checking the return value of jbd2_journal_destroy(), if 
failed, should not call ocfs2_journal_toggle_dirty(). Thanks for 
pointing this.

Thanks,

Junxiao.

>
> Thanks.
> Yiwen.
>
>>   	if (status == 0) {
>>   		/*
>>   		 * Do not toggle if flush was unsuccessful otherwise
>> @@ -1027,10 +1031,6 @@ void ocfs2_journal_shutdown(struct ocfs2_super *osb)
>>   			mlog_errno(status);
>>   	}
>>   
>> -	/* Shutdown the kernel journal system */
>> -	jbd2_journal_destroy(journal->j_journal);
>> -	journal->j_journal = NULL;
>> -
>>   	OCFS2_I(inode)->ip_open_count--;
>>   
>>   	/* unlock our journal */
>>
>