Fix some issues about mmp
cover Fix some issues about mmp
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(-)
Signature · 1 line
2.31.1
PATCH 1/6 ext4: init seq with random value in kmmpd
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.
--- fs/ext4/mmp.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-)
fs/ext4/mmp.c +16 -16
@@ -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. */
Signature · 1 line
2.31.1
PATCH 2/6 ext4: introduce last_check_time record previous check time
In reply to Ye Binkmmpd:
...
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.
--- fs/ext4/mmp.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)
fs/ext4/mmp.c +9 -5
@@ -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; }/*
Signature · 1 line
2.31.1
Re: [PATCH -next 2/6] ext4: introduce last_check_time record previous check time
Quoted 45 lines · 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. Introducelast_check_timerecord previous check time.Signed-off-by Ye Bin <[email protected]>--- fs/ext4/mmp.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)fs/ext4/mmp.c +5
-1@@ -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; + }
fs/ext4/mmp.c static int kmmpd(void *data) · line 214
Quoted 10 lines
fs/ext4/mmp.c +3
-4/* * 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.
fs/ext4/mmp.c static int kmmpd(void *data) · line 243
Quoted 11 lines
fs/ext4/mmp.c +1
-0struct 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
Quoted 17 lines · Guoqing Jiang
Quoted 14 lines · 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. Introducelast_check_timerecord previous check time.Signed-off-by Ye Bin <[email protected]>--- fs/ext4/mmp.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)
Quoted 15 lines
Quoted 14 lines
fs/ext4/mmp.c +1
-0@@ -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));Quoted 1 line
+
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
Quoted 37 lines
Quoted 11 lines
while (!kthread_should_stop() && !sb_rdonly(sb)) { if (!ext4_has_feature_mmp(sb)) {fs/ext4/mmp.c +3
-1@@ -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; + }fs/ext4/mmp.c static int kmmpd(void *data) · line 212
Quoted 10 lines
fs/ext4/mmp.c +3
-4/* * 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_timeis not initialized yet.Quoted 9 lines
fs/ext4/mmp.c +0
-0struct 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 3/6 ext4: compare to local seq and nodename when check conflict
In reply to Ye BinAs 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.
--- fs/ext4/mmp.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
fs/ext4/mmp.c +5 -4
@@ -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. "
Signature · 1 line
2.31.1
PATCH 4/6 ext4: avoid to re-read mmp check data get from page cache
In reply to Ye BinAs 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.
--- fs/ext4/mmp.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-)
fs/ext4/mmp.c +6 -11
@@ -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; }
Signature · 1 line
2.31.1
PATCH 5/6 ext4: avoid to double free s_mmp_bh
In reply to Ye BinIf 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.
--- fs/ext4/mmp.c | 1 + 1 file changed, 1 insertion(+)
fs/ext4/mmp.c +1 -0
@@ -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; }
Signature · 1 line
2.31.1
PATCH 6/6 ext4: fix possible store wrong check interval value in disk when umount
In reply to Ye BinTest 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.
--- fs/ext4/mmp.c | 8 ++++++++ 1 file changed, 8 insertions(+)
fs/ext4/mmp.c +8 -0
@@ -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 ; }/*
Signature · 1 line
2.31.1