[PATCH -next 0/6] Fix some issues about mmp

[PATCH -next 0/6] Fix some issues about mmp

[Ye Bin]

I test mmp function as follow steps:
1. Inject delay 5s in ext4_multi_mount_protect function after
"skip:" label.
2. Share HostA block device(sda) with HostB(nbd0) by NBD.
3. Enable mmp feature when mkfs.ext4 sda.
4. Mount sda and nbd0 at the same time.

I found kmmpd never trigger detect multi-mount. Reason is as follow:
1. Kmmpd init seq with 0, if two host have same nodename, will lead to
detect confliction very slow, even never detect confliction.
2. When detect confliction in kmmpd, we get 'check_bh' is same with 'bh'.
so we compare data with itself.
3. We only trigger detect when ”diff > mmp_check_interval * HZ“,
'mmp_check_interval' is double of 'mmp_update_interval', 'diff' is
about 'mmp_update_interval'. So 'diff' is little than 'mmp_check_interval * HZ'
normaly.

And i also found that 'check_interval' value store in disk  is not sure
after umount.

Ye Bin (6):
  ext4: init seq with random value in kmmpd
  ext4: introduce last_check_time record previous check time
  ext4: compare to local seq and nodename when check conflict
  ext4: avoid to re-read mmp check data get from page cache
  ext4: avoid to double free s_mmp_bh
  ext4: fix possible store wrong check interval value in disk when
    umount

 fs/ext4/mmp.c | 77 ++++++++++++++++++++++++++++-----------------------
 1 file changed, 43 insertions(+), 34 deletions(-)

-- 
2.31.1

[PATCH -next 1/6] ext4: init seq with random value in kmmpd

[Ye Bin]

If two host has the same nodename, and seq start from 0, May cause the
detection mechanism to fail.
So init seq with random value to accelerate conflict detection.

Signed-off-by: Ye Bin <yebin10@huawei.com>
---
 fs/ext4/mmp.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/fs/ext4/mmp.c b/fs/ext4/mmp.c
index cebea4270817..12af6dc8457b 100644
--- a/fs/ext4/mmp.c
+++ b/fs/ext4/mmp.c
@@ -122,6 +122,21 @@ void __dump_mmp_msg(struct super_block *sb, struct mmp_struct *mmp,
 		       (int)sizeof(mmp->mmp_bdevname), mmp->mmp_bdevname);
 }
 
+/*
+ * Get a random new sequence number but make sure it is not greater than
+ * EXT4_MMP_SEQ_MAX.
+ */
+static unsigned int mmp_new_seq(void)
+{
+	u32 new_seq;
+
+	do {
+		new_seq = prandom_u32();
+	} while (new_seq > EXT4_MMP_SEQ_MAX);
+
+	return new_seq;
+}
+
 /*
  * kmmpd will update the MMP sequence every s_mmp_update_interval seconds
  */
@@ -132,7 +147,7 @@ static int kmmpd(void *data)
 	struct buffer_head *bh = EXT4_SB(sb)->s_mmp_bh;
 	struct mmp_struct *mmp;
 	ext4_fsblk_t mmp_block;
-	u32 seq = 0;
+	u32 seq = mmp_new_seq();
 	unsigned long failed_writes = 0;
 	int mmp_update_interval = le16_to_cpu(es->s_mmp_update_interval);
 	unsigned mmp_check_interval;
@@ -258,21 +273,6 @@ void ext4_stop_mmpd(struct ext4_sb_info *sbi)
 	}
 }
 
-/*
- * Get a random new sequence number but make sure it is not greater than
- * EXT4_MMP_SEQ_MAX.
- */
-static unsigned int mmp_new_seq(void)
-{
-	u32 new_seq;
-
-	do {
-		new_seq = prandom_u32();
-	} while (new_seq > EXT4_MMP_SEQ_MAX);
-
-	return new_seq;
-}
-
 /*
  * Protect the filesystem from being mounted more than once.
  */
-- 
2.31.1

[PATCH -next 2/6] ext4: introduce last_check_time record previous check time

[Ye Bin]

kmmpd:
...
    diff = jiffies - last_update_time;
    if (diff > mmp_check_interval * HZ) {
...
As "mmp_check_interval = 2 * mmp_update_interval", 'diff' always little
than 'mmp_update_interval', so there will never trigger detection.
Introduce last_check_time record previous check time.

Signed-off-by: Ye Bin <yebin10@huawei.com>
---
 fs/ext4/mmp.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/fs/ext4/mmp.c b/fs/ext4/mmp.c
index 12af6dc8457b..89797f12a815 100644
--- a/fs/ext4/mmp.c
+++ b/fs/ext4/mmp.c
@@ -152,6 +152,7 @@ static int kmmpd(void *data)
 	int mmp_update_interval = le16_to_cpu(es->s_mmp_update_interval);
 	unsigned mmp_check_interval;
 	unsigned long last_update_time;
+	unsigned long last_check_time;
 	unsigned long diff;
 	int retval = 0;
 
@@ -170,6 +171,7 @@ static int kmmpd(void *data)
 
 	memcpy(mmp->mmp_nodename, init_utsname()->nodename,
 	       sizeof(mmp->mmp_nodename));
+	last_update_time = jiffies;
 
 	while (!kthread_should_stop() && !sb_rdonly(sb)) {
 		if (!ext4_has_feature_mmp(sb)) {
@@ -198,17 +200,18 @@ static int kmmpd(void *data)
 		}
 
 		diff = jiffies - last_update_time;
-		if (diff < mmp_update_interval * HZ)
+		if (diff < mmp_update_interval * HZ) {
 			schedule_timeout_interruptible(mmp_update_interval *
 						       HZ - diff);
+			diff = jiffies - last_update_time;
+		}
 
 		/*
 		 * We need to make sure that more than mmp_check_interval
-		 * seconds have not passed since writing. If that has happened
-		 * we need to check if the MMP block is as we left it.
+		 * seconds have not passed since check. If that has happened
+		 * we need to check if the MMP block is as we write it.
 		 */
-		diff = jiffies - last_update_time;
-		if (diff > mmp_check_interval * HZ) {
+		if (jiffies - last_check_time > mmp_check_interval * HZ) {
 			struct buffer_head *bh_check = NULL;
 			struct mmp_struct *mmp_check;
 
@@ -234,6 +237,7 @@ static int kmmpd(void *data)
 				goto wait_to_exit;
 			}
 			put_bh(bh_check);
+			last_check_time = jiffies;
 		}
 
 		 /*
-- 
2.31.1

Re: [PATCH -next 2/6] ext4: introduce last_check_time record previous check time

[Guoqing Jiang]

On 9/6/21 10:47 PM, Ye Bin wrote:
> kmmpd:
> ...
>      diff = jiffies - last_update_time;
>      if (diff > mmp_check_interval * HZ) {
> ...
> As "mmp_check_interval = 2 * mmp_update_interval", 'diff' always little
> than 'mmp_update_interval', so there will never trigger detection.
> Introduce last_check_time record previous check time.
>
> Signed-off-by: Ye Bin <yebin10@huawei.com>
> ---
>   fs/ext4/mmp.c | 14 +++++++++-----
>   1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/fs/ext4/mmp.c b/fs/ext4/mmp.c
> index 12af6dc8457b..89797f12a815 100644
> --- a/fs/ext4/mmp.c
> +++ b/fs/ext4/mmp.c
> @@ -152,6 +152,7 @@ static int kmmpd(void *data)
>   	int mmp_update_interval = le16_to_cpu(es->s_mmp_update_interval);
>   	unsigned mmp_check_interval;
>   	unsigned long last_update_time;
> +	unsigned long last_check_time;
>   	unsigned long diff;
>   	int retval = 0;
>   
> @@ -170,6 +171,7 @@ static int kmmpd(void *data)
>   
>   	memcpy(mmp->mmp_nodename, init_utsname()->nodename,
>   	       sizeof(mmp->mmp_nodename));
> +	last_update_time = jiffies;
>   
>   	while (!kthread_should_stop() && !sb_rdonly(sb)) {
>   		if (!ext4_has_feature_mmp(sb)) {
> @@ -198,17 +200,18 @@ static int kmmpd(void *data)
>   		}
>   
>   		diff = jiffies - last_update_time;
> -		if (diff < mmp_update_interval * HZ)
> +		if (diff < mmp_update_interval * HZ) {
>   			schedule_timeout_interruptible(mmp_update_interval *
>   						       HZ - diff);
> +			diff = jiffies - last_update_time;
> +		}
>   
>   		/*
>   		 * We need to make sure that more than mmp_check_interval
> -		 * seconds have not passed since writing. If that has happened
> -		 * we need to check if the MMP block is as we left it.
> +		 * seconds have not passed since check. If that has happened
> +		 * we need to check if the MMP block is as we write it.
>   		 */
> -		diff = jiffies - last_update_time;
> -		if (diff > mmp_check_interval * HZ) {
> +		if (jiffies - last_check_time > mmp_check_interval * HZ) {

Before the above checking, seems last_check_time is not initialized yet.
>   			struct buffer_head *bh_check = NULL;
>   			struct mmp_struct *mmp_check;
>   
> @@ -234,6 +237,7 @@ static int kmmpd(void *data)
>   				goto wait_to_exit;
>   			}
>   			put_bh(bh_check);
> +			last_check_time = jiffies;
>   		}
>   
>   		 /*

Thanks,
Guoqing

Re: [PATCH -next 2/6] ext4: introduce last_check_time record previous check time

[yebin]

On 2021/9/9 10:20, Guoqing Jiang wrote:
>
>
> On 9/6/21 10:47 PM, Ye Bin wrote:
>> kmmpd:
>> ...
>>      diff = jiffies - last_update_time;
>>      if (diff > mmp_check_interval * HZ) {
>> ...
>> As "mmp_check_interval = 2 * mmp_update_interval", 'diff' always little
>> than 'mmp_update_interval', so there will never trigger detection.
>> Introduce last_check_time record previous check time.
>>
>> Signed-off-by: Ye Bin <yebin10@huawei.com>
>> ---
>>   fs/ext4/mmp.c | 14 +++++++++-----
>>   1 file changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/ext4/mmp.c b/fs/ext4/mmp.c
>> index 12af6dc8457b..89797f12a815 100644
>> --- a/fs/ext4/mmp.c
>> +++ b/fs/ext4/mmp.c
>> @@ -152,6 +152,7 @@ static int kmmpd(void *data)
>>       int mmp_update_interval = le16_to_cpu(es->s_mmp_update_interval);
>>       unsigned mmp_check_interval;
>>       unsigned long last_update_time;
>> +    unsigned long last_check_time;
>>       unsigned long diff;
>>       int retval = 0;
>>   @@ -170,6 +171,7 @@ static int kmmpd(void *data)
>>         memcpy(mmp->mmp_nodename, init_utsname()->nodename,
>>              sizeof(mmp->mmp_nodename));
>> +    last_update_time = jiffies;
I'm sorry, actually i want to init 'last_check_time' at here,  I will 
fix it in v2.
Thanks,
YeBin
>>         while (!kthread_should_stop() && !sb_rdonly(sb)) {
>>           if (!ext4_has_feature_mmp(sb)) {
>> @@ -198,17 +200,18 @@ static int kmmpd(void *data)
>>           }
>>             diff = jiffies - last_update_time;
>> -        if (diff < mmp_update_interval * HZ)
>> +        if (diff < mmp_update_interval * HZ) {
>>               schedule_timeout_interruptible(mmp_update_interval *
>>                                  HZ - diff);
>> +            diff = jiffies - last_update_time;
>> +        }
>>             /*
>>            * We need to make sure that more than mmp_check_interval
>> -         * seconds have not passed since writing. If that has happened
>> -         * we need to check if the MMP block is as we left it.
>> +         * seconds have not passed since check. If that has happened
>> +         * we need to check if the MMP block is as we write it.
>>            */
>> -        diff = jiffies - last_update_time;
>> -        if (diff > mmp_check_interval * HZ) {
>> +        if (jiffies - last_check_time > mmp_check_interval * HZ) {
>
> Before the above checking, seems last_check_time is not initialized yet.
>>               struct buffer_head *bh_check = NULL;
>>               struct mmp_struct *mmp_check;
>>   @@ -234,6 +237,7 @@ static int kmmpd(void *data)
>>                   goto wait_to_exit;
>>               }
>>               put_bh(bh_check);
>> +            last_check_time = jiffies;
>>           }
>>              /*
>
> Thanks,
> Guoqing
> .
>

[PATCH -next 3/6] ext4: compare to local seq and nodename when check conflict

[Ye Bin]

As mmp and check_mmp is point to the same data, so there will never
detect conflict.
To solve this issue just compare to local data.

Signed-off-by: Ye Bin <yebin10@huawei.com>
---
 fs/ext4/mmp.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/mmp.c b/fs/ext4/mmp.c
index 89797f12a815..0fe7b9ac9db6 100644
--- a/fs/ext4/mmp.c
+++ b/fs/ext4/mmp.c
@@ -154,6 +154,7 @@ static int kmmpd(void *data)
 	unsigned long last_update_time;
 	unsigned long last_check_time;
 	unsigned long diff;
+	char nodename[64];
 	int retval = 0;
 
 	mmp_block = le64_to_cpu(es->s_mmp_block);
@@ -169,8 +170,8 @@ static int kmmpd(void *data)
 	BUILD_BUG_ON(sizeof(mmp->mmp_bdevname) < BDEVNAME_SIZE);
 	bdevname(bh->b_bdev, mmp->mmp_bdevname);
 
-	memcpy(mmp->mmp_nodename, init_utsname()->nodename,
-	       sizeof(mmp->mmp_nodename));
+	memcpy(nodename, init_utsname()->nodename, sizeof(nodename));
+	memcpy(mmp->mmp_nodename, nodename, sizeof(mmp->mmp_nodename));
 	last_update_time = jiffies;
 
 	while (!kthread_should_stop() && !sb_rdonly(sb)) {
@@ -224,8 +225,8 @@ static int kmmpd(void *data)
 			}
 
 			mmp_check = (struct mmp_struct *)(bh_check->b_data);
-			if (mmp->mmp_seq != mmp_check->mmp_seq ||
-			    memcmp(mmp->mmp_nodename, mmp_check->mmp_nodename,
+			if (seq != mmp_check->mmp_seq ||
+			    memcmp(nodename, mmp_check->mmp_nodename,
 				   sizeof(mmp->mmp_nodename))) {
 				dump_mmp_msg(sb, mmp_check,
 					     "Error while updating MMP info. "
-- 
2.31.1

[PATCH -next 4/6] ext4: avoid to re-read mmp check data get from page cache

[Ye Bin]

As call read_mmp_block pass bh_check which value is NULL, then call
sb_getblk to get buffer_head. But mmp_block's buffer_head is already exist
 which also is uptodate. Lead to compare the same data.

Signed-off-by: Ye Bin <yebin10@huawei.com>
---
 fs/ext4/mmp.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/fs/ext4/mmp.c b/fs/ext4/mmp.c
index 0fe7b9ac9db6..2a1473e4a9de 100644
--- a/fs/ext4/mmp.c
+++ b/fs/ext4/mmp.c
@@ -213,10 +213,7 @@ static int kmmpd(void *data)
 		 * we need to check if the MMP block is as we write it.
 		 */
 		if (jiffies - last_check_time > mmp_check_interval * HZ) {
-			struct buffer_head *bh_check = NULL;
-			struct mmp_struct *mmp_check;
-
-			retval = read_mmp_block(sb, &bh_check, mmp_block);
+			retval = read_mmp_block(sb, &bh, mmp_block);
 			if (retval) {
 				ext4_error_err(sb, -retval,
 					       "error reading MMP data: %d",
@@ -224,20 +221,18 @@ static int kmmpd(void *data)
 				goto wait_to_exit;
 			}
 
-			mmp_check = (struct mmp_struct *)(bh_check->b_data);
-			if (seq != mmp_check->mmp_seq ||
-			    memcmp(nodename, mmp_check->mmp_nodename,
-				   sizeof(mmp->mmp_nodename))) {
-				dump_mmp_msg(sb, mmp_check,
+			mmp = (struct mmp_struct *)(bh->b_data);
+			if (seq != le32_to_cpu(mmp->mmp_seq) ||
+			    memcmp(nodename, mmp->mmp_nodename,
+				    sizeof(nodename))) {
+				dump_mmp_msg(sb, mmp,
 					     "Error while updating MMP info. "
 					     "The filesystem seems to have been"
 					     " multiply mounted.");
 				ext4_error_err(sb, EBUSY, "abort");
-				put_bh(bh_check);
 				retval = -EBUSY;
 				goto wait_to_exit;
 			}
-			put_bh(bh_check);
 			last_check_time = jiffies;
 		}
 
-- 
2.31.1

[PATCH -next 5/6] ext4: avoid to double free s_mmp_bh

[Ye Bin]

If call read_mmp_block failed then s_mmp_bh will be freed in read_mmp_block.
Kmmpd wait to be killed, ext4_stop_mmpd stop kmmpd and also release s_mmp_bh.
To avoid double free, just set EXT4_SB(sb)->s_mmp_bh with NULL.

Signed-off-by: Ye Bin <yebin10@huawei.com>
---
 fs/ext4/mmp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/ext4/mmp.c b/fs/ext4/mmp.c
index 2a1473e4a9de..eed854bb6194 100644
--- a/fs/ext4/mmp.c
+++ b/fs/ext4/mmp.c
@@ -218,6 +218,7 @@ static int kmmpd(void *data)
 				ext4_error_err(sb, -retval,
 					       "error reading MMP data: %d",
 					       retval);
+				EXT4_SB(sb)->s_mmp_bh = NULL;
 				goto wait_to_exit;
 			}
 
-- 
2.31.1

[PATCH -next 6/6] ext4: fix possible store wrong check interval value in disk when umount

[Ye Bin]

Test follow steps:
1. mkfs.ext4 /dev/sda -O mmp
2. mount /dev/sda  /mnt
3. wait for about 1 minute
4. umount mnt
5. debugfs /dev/sda
6. dump_mmp
7. fsck.ext4 /dev/sda

I found 'check_interval' is range in [5, 10]. And sometime run fsck
print "MMP interval is 10 seconds and total wait time is 42 seconds.
Please wait...".
kmmpd:
...
	if (diff < mmp_update_interval * HZ)
		schedule_timeout_interruptible(mmp_update_interval * HZ - diff);
	 diff = jiffies - last_update_time;
...
	mmp_check_interval = max(min(EXT4_MMP_CHECK_MULT * diff / HZ,
				EXT4_MMP_MAX_CHECK_INTERVAL),
			        EXT4_MMP_MIN_CHECK_INTERVAL);
	mmp->mmp_check_interval = cpu_to_le16(mmp_check_interval);
...
We will call ext4_stop_mmpd to stop kmmpd kthread when umount, and
schedule_timeout_interruptible will be interrupted, so 'diff' maybe
little than mmp_update_interval. Then mmp_check_interval will range
in [EXT4_MMP_MAX_CHECK_INTERVAL, EXT4_MMP_CHECK_MULT * diff / HZ].
To solve this issue, if 'diff' little then mmp_update_interval * HZ
just break loop, don't update check interval.

Signed-off-by: Ye Bin <yebin10@huawei.com>
---
 fs/ext4/mmp.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/fs/ext4/mmp.c b/fs/ext4/mmp.c
index eed854bb6194..41c0a03019db 100644
--- a/fs/ext4/mmp.c
+++ b/fs/ext4/mmp.c
@@ -205,6 +205,14 @@ static int kmmpd(void *data)
 			schedule_timeout_interruptible(mmp_update_interval *
 						       HZ - diff);
 			diff = jiffies - last_update_time;
+			/* If 'diff' little 'than mmp_update_interval * HZ', it
+			 * means someone call ext4_stop_mmpd to stop kmmpd
+			 * kthread. We don't need to update mmp_check_interval
+			 * any more, as 'diff' is not exact value.
+			 */
+			if (unlikely(diff < mmp_update_interval * HZ &&
+			    kthread_should_stop()))
+				break;
 		}
 
 		/*
-- 
2.31.1