diff mbox series

ocfs2: call ocfs2_abort when journal abort

Message ID 20231030120057.928280-1-srivathsa.d.dara@oracle.com
State New
Headers
Series ocfs2: call ocfs2_abort when journal abort |

Commit Message

Srivathsa Dara Oct. 30, 2023, noon UTC 
  From: Ryan Ding <ryan.ding@oracle.com>


journal can not recover from abort state, so we should take following 
action to prevent file system from corruption:

1. change to readonly filesystem when local mount. We can not afford 
   further write, so change to RO state is reasonable.

2. panic when cluster mount. Because we can not release lock resource in
   this state, other node will hung when it require a lock owned by this
   node. So panic and remaster is a reasonable choise.

ocfs2_abort() will do all the above work.

Signed-off-by: Ryan Ding <ryan.ding@oracle.com>
Signed-off-by: Srivathsa Dara <srivathsa.d.dara@oracle.com>
---
 fs/ocfs2/journal.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

Comments

Joseph Qi Nov. 1, 2023, 1:13 a.m. UTC | #1 
On 10/30/23 8:00 PM, Srivathsa Dara wrote:
> From: Ryan Ding <ryan.ding@oracle.com>
> 
> 
> journal can not recover from abort state, so we should take following 
> action to prevent file system from corruption:
> 
> 1. change to readonly filesystem when local mount. We can not afford 
>    further write, so change to RO state is reasonable.
> 
> 2. panic when cluster mount. Because we can not release lock resource in
>    this state, other node will hung when it require a lock owned by this
>    node. So panic and remaster is a reasonable choise.
> 
> ocfs2_abort() will do all the above work.
> 
> Signed-off-by: Ryan Ding <ryan.ding@oracle.com>
> Signed-off-by: Srivathsa Dara <srivathsa.d.dara@oracle.com>
> ---
>  fs/ocfs2/journal.c | 27 +++++++++++++++------------
>  1 file changed, 15 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c
> index ce215565d061..6dace475f019 100644
> --- a/fs/ocfs2/journal.c
> +++ b/fs/ocfs2/journal.c
> @@ -14,7 +14,6 @@
>  #include <linux/kthread.h>
>  #include <linux/time.h>
>  #include <linux/random.h>
> -#include <linux/delay.h>
>  #include <linux/writeback.h>
>  
>  #include <cluster/masklog.h>
> @@ -2326,7 +2325,7 @@ static int __ocfs2_wait_on_mount(struct ocfs2_super *osb, int quota)
>  
>  static int ocfs2_commit_thread(void *arg)
>  {
> -	int status;
> +	int status = 0;
>  	struct ocfs2_super *osb = arg;
>  	struct ocfs2_journal *journal = osb->journal;
>  
> @@ -2340,21 +2339,25 @@ static int ocfs2_commit_thread(void *arg)
>  		wait_event_interruptible(osb->checkpoint_event,
>  					 atomic_read(&journal->j_num_trans)
>  					 || kthread_should_stop());
> +		if (status < 0) {
> +			/* As we can not terminate by ourself, just enter an
> +			 * empty loop to wait for stop.
> +			 */
> +			continue;
> +		}
>  
>  		status = ocfs2_commit_cache(osb);
>  		if (status < 0) {
> -			static unsigned long abort_warn_time;
> -
> -			/* Warn about this once per minute */
> -			if (printk_timed_ratelimit(&abort_warn_time, 60*HZ))
> -				mlog(ML_ERROR, "status = %d, journal is "
> -						"already aborted.\n", status);
>  			/*
> -			 * After ocfs2_commit_cache() fails, j_num_trans has a
> -			 * non-zero value.  Sleep here to avoid a busy-wait
> -			 * loop.
> +			 * journal can not recover from abort state, there is
> +			 * no need to keep commit cache. So we should either
> +			 * change to readonly(local mount) or just panic
> +			 * (cluster mount).
> +			 * We should also clear j_num_trans to prevent further
> +			 * commit.
>  			 */
> -			msleep_interruptible(1000);
> +			atomic_set(&journal->j_num_trans, 0);

Unconditionally clear 'j_num_trans' here seems buggy.
This may cause other nodes corrupt filesystem.

Thanks,
Joseph

> +			ocfs2_abort(osb->sb, "Detected aborted journal");
>  		}
>  
>  		if (kthread_should_stop() && atomic_read(&journal->j_num_trans)){
Heming Zhao Nov. 1, 2023, 2:42 a.m. UTC | #2 
On 11/1/23 09:13, Joseph Qi wrote:
> 
> 
> On 10/30/23 8:00 PM, Srivathsa Dara wrote:
>> From: Ryan Ding <ryan.ding@oracle.com>
>>
>>
>> journal can not recover from abort state, so we should take following
>> action to prevent file system from corruption:
>>
>> 1. change to readonly filesystem when local mount. We can not afford
>>     further write, so change to RO state is reasonable.
>>
>> 2. panic when cluster mount. Because we can not release lock resource in
>>     this state, other node will hung when it require a lock owned by this
>>     node. So panic and remaster is a reasonable choise.
>>
>> ocfs2_abort() will do all the above work.
>>
>> Signed-off-by: Ryan Ding <ryan.ding@oracle.com>
>> Signed-off-by: Srivathsa Dara <srivathsa.d.dara@oracle.com>
>> ---
>>   fs/ocfs2/journal.c | 27 +++++++++++++++------------
>>   1 file changed, 15 insertions(+), 12 deletions(-)
>>
>> diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c
>> index ce215565d061..6dace475f019 100644
>> --- a/fs/ocfs2/journal.c
>> +++ b/fs/ocfs2/journal.c
>> @@ -14,7 +14,6 @@
>>   #include <linux/kthread.h>
>>   #include <linux/time.h>
>>   #include <linux/random.h>
>> -#include <linux/delay.h>
>>   #include <linux/writeback.h>
>>   
>>   #include <cluster/masklog.h>
>> @@ -2326,7 +2325,7 @@ static int __ocfs2_wait_on_mount(struct ocfs2_super *osb, int quota)
>>   
>>   static int ocfs2_commit_thread(void *arg)
>>   {
>> -	int status;
>> +	int status = 0;
>>   	struct ocfs2_super *osb = arg;
>>   	struct ocfs2_journal *journal = osb->journal;
>>   
>> @@ -2340,21 +2339,25 @@ static int ocfs2_commit_thread(void *arg)
>>   		wait_event_interruptible(osb->checkpoint_event,
>>   					 atomic_read(&journal->j_num_trans)
>>   					 || kthread_should_stop());
>> +		if (status < 0) {
>> +			/* As we can not terminate by ourself, just enter an
>> +			 * empty loop to wait for stop.
>> +			 */
>> +			continue;
>> +		}
>>   
>>   		status = ocfs2_commit_cache(osb);
>>   		if (status < 0) {
>> -			static unsigned long abort_warn_time;
>> -
>> -			/* Warn about this once per minute */
>> -			if (printk_timed_ratelimit(&abort_warn_time, 60*HZ))
>> -				mlog(ML_ERROR, "status = %d, journal is "
>> -						"already aborted.\n", status);
>>   			/*
>> -			 * After ocfs2_commit_cache() fails, j_num_trans has a
>> -			 * non-zero value.  Sleep here to avoid a busy-wait
>> -			 * loop.
>> +			 * journal can not recover from abort state, there is
>> +			 * no need to keep commit cache. So we should either
>> +			 * change to readonly(local mount) or just panic
>> +			 * (cluster mount).
>> +			 * We should also clear j_num_trans to prevent further
>> +			 * commit.
>>   			 */
>> -			msleep_interruptible(1000);
>> +			atomic_set(&journal->j_num_trans, 0);
> 
> Unconditionally clear 'j_num_trans' here seems buggy.
> This may cause other nodes corrupt filesystem.

Yes, you described this buggy condition during review my patch:
https://lore.kernel.org/ocfs2-devel/2b90546e-d23e-7d45-22dd-a887a73f3e61@linux.alibaba.com/

btw, I ever send a v2 patch [1] about handling journal abort:
[1] https://lore.kernel.org/ocfs2-devel/20230626112453.22571-1-heming.zhao@suse.com/
This patch never get any feedback.

And I also send another mail [2] related with above patch:
[2] https://lore.kernel.org/ocfs2-devel/20230626150916.je3egonzb3crvbl5@p15/
please note, [2] is the problems from fsck.ocfs2, which is completely unrelated to [1].

Heming
> 
> Thanks,
> Joseph
> 
>> +			ocfs2_abort(osb->sb, "Detected aborted journal");
>>   		}
>>   
>>   		if (kthread_should_stop() && atomic_read(&journal->j_num_trans)){
>
diff mbox series

Patch

diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c
index ce215565d061..6dace475f019 100644
--- a/fs/ocfs2/journal.c
+++ b/fs/ocfs2/journal.c
@@ -14,7 +14,6 @@ 
 #include <linux/kthread.h>
 #include <linux/time.h>
 #include <linux/random.h>
-#include <linux/delay.h>
 #include <linux/writeback.h>
 
 #include <cluster/masklog.h>
@@ -2326,7 +2325,7 @@  static int __ocfs2_wait_on_mount(struct ocfs2_super *osb, int quota)
 
 static int ocfs2_commit_thread(void *arg)
 {
-	int status;
+	int status = 0;
 	struct ocfs2_super *osb = arg;
 	struct ocfs2_journal *journal = osb->journal;
 
@@ -2340,21 +2339,25 @@  static int ocfs2_commit_thread(void *arg)
 		wait_event_interruptible(osb->checkpoint_event,
 					 atomic_read(&journal->j_num_trans)
 					 || kthread_should_stop());
+		if (status < 0) {
+			/* As we can not terminate by ourself, just enter an
+			 * empty loop to wait for stop.
+			 */
+			continue;
+		}
 
 		status = ocfs2_commit_cache(osb);
 		if (status < 0) {
-			static unsigned long abort_warn_time;
-
-			/* Warn about this once per minute */
-			if (printk_timed_ratelimit(&abort_warn_time, 60*HZ))
-				mlog(ML_ERROR, "status = %d, journal is "
-						"already aborted.\n", status);
 			/*
-			 * After ocfs2_commit_cache() fails, j_num_trans has a
-			 * non-zero value.  Sleep here to avoid a busy-wait
-			 * loop.
+			 * journal can not recover from abort state, there is
+			 * no need to keep commit cache. So we should either
+			 * change to readonly(local mount) or just panic
+			 * (cluster mount).
+			 * We should also clear j_num_trans to prevent further
+			 * commit.
 			 */
-			msleep_interruptible(1000);
+			atomic_set(&journal->j_num_trans, 0);
+			ocfs2_abort(osb->sb, "Detected aborted journal");
 		}
 
 		if (kthread_should_stop() && atomic_read(&journal->j_num_trans)){