Fix some issues about mmp
v2 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.
v1->v2:
Fix 'last_check_time' not initialized before checking.
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 v2 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
Jan Kara
reviewerRe: [PATCH -next v2 1/6] ext4: init seq with random value in kmmpd
Quoted 5 lines · 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 <[email protected]>
Thanks for the patch. I agree the code in kmmpd looks suspicious. The sequence number is initialized to a random number in ext4_multi_mount_protect() which is called before kmmpd is started. I think kmmpd() should initialize its 'seq' to a number fetched from the mmp block, instead of 0 as is currently in the code. I don't think generating a new random number as you do is really needed...
Honza
Quoted 64 lines
--- 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
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Re: [PATCH -next v2 1/6] ext4: init seq with random value in kmmpd
Quoted 6 lines · Jan Kara
Quoted 5 lines · 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 <[email protected]>
Quoted 8 lines
Thanks for the patch. I agree the code in kmmpd looks suspicious. The sequence number is initialized to a random number in
ext4_multi_mount_protect()which is called before kmmpd is started. I thinkkmmpd()should initialize its 'seq' to a number fetched from the mmp block, instead of 0 as is currently in the code. I don't think generating a new random number as you do is really needed...Honza
Thank you for your advice，i will fix it.
Quoted 65 lines
Quoted 65 lines
--- 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 v2 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_check_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
Jan Kara
reviewerRe: [PATCH -next v2 2/6] ext4: introduce last_check_time record previous check time
Quoted 10 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]>
I think the check is there only for the case where write_mmp_block() + sleep took longer than mmp_check_interval. I agree that should rarely happen but on a really busy system it is possible and in that case we would miss updating mmp block for too long and so another node could have started using the filesystem. I actually don't see a reason why kmmpd should be checking the block each mmp_check_interval as you do - mmp_check_interval is just for ext4_multi_mount_protect() to know how long it should wait before considering mmp block stale... Am I missing something?
Honza
Quoted 59 lines
--- 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_check_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
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Re: [PATCH -next v2 2/6] ext4: introduce last_check_time record previous check time
Quoted 20 lines · Jan Kara
Quoted 10 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]>I think the check is there only for the case where
write_mmp_block()+ sleep took longer thanmmp_check_interval. I agree that should rarely happen but on a really busy system it is possible and in that case we would miss updating mmp block for too long and so another node could have started using the filesystem. I actually don't see a reason why kmmpd should be checking the block eachmmp_check_intervalas you do -mmp_check_intervalis just forext4_multi_mount_protect()to know how long it should wait before considering mmp block stale... Am I missing something?
Quoted 1 line
Honza
I'm sorry, I didn't understand the detection mechanism here before. Now I understand the detection mechanism here.
As you said, it's just an abnormal protection. There's really no problem.
Quoted 59 lines
Quoted 59 lines
--- 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_check_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 v2 2/6] ext4: introduce last_check_time record previous check time
Quoted 23 lines · yebin
Quoted 20 lines · Jan Kara
Quoted 10 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]>I think the check is there only for the case where
write_mmp_block()+ sleep took longer thanmmp_check_interval. I agree that should rarely happen but on a really busy system it is possible and in that case we would miss updating mmp block for too long and so another node could have started using the filesystem. I actually don't see a reason why kmmpd should be checking the block eachmmp_check_intervalas you do -mmp_check_intervalis just forext4_multi_mount_protect()to know how long it should wait before considering mmp block stale... Am I missing something?
Quoted 5 lines
Quoted 1 line
Honza
I'm sorry, I didn't understand the detection mechanism here before. Now I understand the detection mechanism here.
As you said, it's just an abnormal protection. There's really no problem.
Yeah, i did test as following steps
hostA hostB
mount
ext4_multi_mount_protect -> seq == EXT4_MMP_SEQ_CLEAN
delay 5s after label "skip" so hostB will see seq isEXT4_MMP_SEQ_CLEAN
mount
ext4_multi_mount_protect -> seq == EXT4_MMP_SEQ_CLEAN
run kmmpd
run kmmpd
Actually，in this situation kmmpd will not detect confliction.
In ext4_multi_mount_protect function we write mmp data fisrt and wait 'wait_time * HZ' seconds, read mmp data do check.Most of the time, If 'wait_time' is zero, it can pass check.
Quoted 53 lines
Quoted 52 lines
Quoted 52 lines
--- fs/ext4/mmp.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)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));fs/ext4/mmp.c +2
-1+ last_check_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);fs/ext4/mmp.c +6
-4+ 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
Jan Kara
reviewerRe: [PATCH -next v2 2/6] ext4: introduce last_check_time record previous check time
Quoted 42 lines · yebin
Quoted 24 lines · yebin
Quoted 23 lines · Jan Kara
Quoted 10 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]>I think the check is there only for the case where
write_mmp_block()+ sleep took longer thanmmp_check_interval. I agree that should rarely happen but on a really busy system it is possible and in that case we would
miss updating mmp block for too long and so another node could have started
using the filesystem. I actually don't see a reason why kmmpd should be checking the block eachmmp_check_intervalas you do -mmp_check_interval
is just forext4_multi_mount_protect()to know how long it should wait before considering mmp block stale... Am I missing something?Quoted 6 lines
Quoted 1 line
Honza
I'm sorry, I didn't understand the detection mechanism here before. Now I understand
the detection mechanism here.
As you said, it's just an abnormal protection. There's really no problem.Yeah, i did test as following steps
hostA hostB
mount
ext4_multi_mount_protect-> seq ==EXT4_MMP_SEQ_CLEAN
delay 5s after label "skip" so hostB will see seq isEXT4_MMP_SEQ_CLEANmount ext4_multi_mount_protect -> seq == EXT4_MMP_SEQ_CLEAN run kmmpd run kmmpd
Quoted 5 lines
Actually，in this situation kmmpd will not detect confliction. In
ext4_multi_mount_protectfunction we write mmp data first and wait 'wait_time* HZ' seconds,
read mmp data do check. Most of the time, If 'wait_time' is zero, it can pass check.
But how can be wait_time zero? As far as I'm reading the code, wait_time must be at least EXT4_MMP_MIN_CHECK_INTERVAL...
Honza
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Re: [PATCH -next v2 2/6] ext4: introduce last_check_time record previous check time
Quoted 48 lines · Jan Kara
Quoted 42 lines · yebin
Quoted 24 lines · yebin
Quoted 23 lines · Jan Kara
Quoted 10 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]>I think the check is there only for the case where
write_mmp_block()+ sleep took longer thanmmp_check_interval. I agree that should rarely happen but on a really busy system it is possible and in that case we would
miss updating mmp block for too long and so another node could have started
using the filesystem. I actually don't see a reason why kmmpd should be checking the block eachmmp_check_intervalas you do -mmp_check_interval
is just forext4_multi_mount_protect()to know how long it should wait before considering mmp block stale... Am I missing something?Quoted 6 lines
Quoted 1 line
Honza
I'm sorry, I didn't understand the detection mechanism here before. Now I understand
the detection mechanism here.
As you said, it's just an abnormal protection. There's really no problem.Yeah, i did test as following steps
hostA hostB
mount
ext4_multi_mount_protect-> seq ==EXT4_MMP_SEQ_CLEAN
delay 5s after label "skip" so hostB will see seq isEXT4_MMP_SEQ_CLEANmount ext4_multi_mount_protect -> seq == EXT4_MMP_SEQ_CLEAN run kmmpd run kmmpdQuoted 5 lines
Actually，in this situation kmmpd will not detect confliction. In
ext4_multi_mount_protectfunction we write mmp data first and wait 'wait_time* HZ' seconds,
read mmp data do check. Most of the time, If 'wait_time' is zero, it can pass check.
Quoted 4 lines
But how can be
wait_timezero? As far as I'm reading the code,wait_timemust be at leastEXT4_MMP_MIN_CHECK_INTERVAL...Honza
int ext4_multi_mount_protect(struct super_block *sb,
ext4_fsblk_t mmp_block)
{
struct ext4_super_block *es = EXT4_SB(sb)->s_es;
struct buffer_head *bh = NULL;
struct mmp_struct *mmp = NULL;
u32 seq;
unsigned int mmp_check_interval =
le16_to_cpu(es->s_mmp_update_interval);
unsigned int wait_time = 0; --> wait_time
is equal with zero
int retval;
if (mmp_block < le32_to_cpu(es->s_first_data_block) ||
mmp_block >= ext4_blocks_count(es)) {
ext4_warning(sb, "Invalid MMP block in superblock");
goto failed;
}
retval = read_mmp_block(sb, &bh, mmp_block);
if (retval)
goto failed;
mmp = (struct mmp_struct *)(bh->b_data);
if (mmp_check_interval < EXT4_MMP_MIN_CHECK_INTERVAL)
mmp_check_interval = EXT4_MMP_MIN_CHECK_INTERVAL;
/*
* If check_interval in MMP block is larger, use that instead of
* update_interval from the superblock.
*/
if (le16_to_cpu(mmp->mmp_check_interval) > mmp_check_interval)
mmp_check_interval = le16_to_cpu(mmp->mmp_check_interval);
seq = le32_to_cpu(mmp->mmp_seq);
if (seq == EXT4_MMP_SEQ_CLEAN) --> If hostA and hostB mount
the same block device at the same time,
--> HostA and hostB maybe get 'seq' with the same value EXT4_MMP_SEQ_CLEAN.
goto skip;
...
skip:
/*
* write a new random sequence number.
*/
seq = mmp_new_seq();
mmp->mmp_seq = cpu_to_le32(seq);
retval = write_mmp_block(sb, bh);
if (retval)
goto failed;
/*
* wait for MMP interval and check mmp_seq.
*/
if (schedule_timeout_interruptible(HZ * wait_time) != 0)
{ --> If seq is equal with EXT4_MMP_SEQ_CLEAN, wait_time is zero.
ext4_warning(sb, "MMP startup interrupted, failing mount");
goto failed;
}
retval = read_mmp_block(sb, &bh, mmp_block); -->We may get the
same data with which we wrote, so we can't detect conflict at here.
if (retval)
goto failed;
mmp = (struct mmp_struct *)(bh->b_data);
if (seq != le32_to_cpu(mmp->mmp_seq)) {
dump_mmp_msg(sb, mmp,
"Device is already active on another node.");
goto failed;
}...
}
Jan Kara
reviewerRe: [PATCH -next v2 2/6] ext4: introduce last_check_time record previous check time
Quoted 85 lines · yebin
Quoted 48 lines · Jan Kara
Quoted 42 lines · yebin
Quoted 24 lines · yebin
Quoted 23 lines · Jan Kara
Quoted 10 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]>I think the check is there only for the case where
write_mmp_block()+ sleep took longer thanmmp_check_interval. I agree that should rarely happen but on a really busy system it is possible and in that case we would
miss updating mmp block for too long and so another node could have started
using the filesystem. I actually don't see a reason why kmmpd should be checking the block eachmmp_check_intervalas you do -mmp_check_interval
is just forext4_multi_mount_protect()to know how long it should wait before considering mmp block stale... Am I missing something?Quoted 6 lines
Quoted 1 line
Honza
I'm sorry, I didn't understand the detection mechanism here before. Now I understand
the detection mechanism here.
As you said, it's just an abnormal protection. There's really no problem.Yeah, i did test as following steps
hostA hostB
mount
ext4_multi_mount_protect-> seq ==EXT4_MMP_SEQ_CLEAN
delay 5s after label "skip" so hostB will see seq isEXT4_MMP_SEQ_CLEANmount ext4_multi_mount_protect -> seq == EXT4_MMP_SEQ_CLEAN run kmmpd run kmmpdQuoted 5 lines
Actually，in this situation kmmpd will not detect confliction. In
ext4_multi_mount_protectfunction we write mmp data first and wait 'wait_time* HZ' seconds,
read mmp data do check. Most of the time, If 'wait_time' is zero, it can pass check.Quoted 4 lines
But how can be
wait_timezero? As far as I'm reading the code,wait_timemust be at leastEXT4_MMP_MIN_CHECK_INTERVAL...Honza
int ext4_multi_mount_protect(struct
super_block*sb,
ext4_fsblk_tmmp_block){ struct ext4_super_block *es = EXT4_SB(sb)->s_es; struct buffer_head *bh = NULL; struct mmp_struct *mmp = NULL; u32 seq; unsigned int mmp_check_interval = le16_to_cpu(es->s_mmp_update_interval); unsigned int wait_time = 0; --> wait_time is equal with zero int retval; if (mmp_block < le32_to_cpu(es->s_first_data_block) || mmp_block >= ext4_blocks_count(es)) { ext4_warning(sb, "Invalid MMP block in superblock"); goto failed; } retval = read_mmp_block(sb, &bh, mmp_block); if (retval) goto failed; mmp = (struct mmp_struct *)(bh->b_data); if (mmp_check_interval < EXT4_MMP_MIN_CHECK_INTERVAL) mmp_check_interval = EXT4_MMP_MIN_CHECK_INTERVAL; /* * If check_interval in MMP block is larger, use that instead of * update_interval from the superblock. */
Quoted 8 lines
if (le16_to_cpu(mmp->mmp_check_interval) > mmp_check_interval)
mmp_check_interval = le16_to_cpu(mmp->mmp_check_interval);
seq = le32_to_cpu(mmp->mmp_seq);
if (seq == EXT4_MMP_SEQ_CLEAN) --> If hostA and hostB mount the
same block device at the same time,
--> HostA and hostB maybe get 'seq' with the same value EXT4_MMP_SEQ_CLEAN.
goto skip;Oh, I see. Thanks for explanation.
Quoted 22 lines
...
skip:/* * write a new random sequence number. */ seq = mmp_new_seq(); mmp->mmp_seq = cpu_to_le32(seq); retval = write_mmp_block(sb, bh); if (retval) goto failed; /* * wait for MMP interval and check mmp_seq. */ if (schedule_timeout_interruptible(HZ * wait_time) != 0) { --> If seq is equal with EXT4_MMP_SEQ_CLEAN, wait_time is zero. ext4_warning(sb, "MMP startup interrupted, failing mount"); goto failed; } retval = read_mmp_block(sb, &bh, mmp_block); -->We may get the same
Quoted 1 line
data with which we wrote, so we can't detect conflict at here.
OK, I see. So the race in ext4_multi_mount_protect() goes like:
hostA hostB
read_mmp_block() read_mmp_block()
- sees EXT4_MMP_SEQ_CLEAN - sees EXT4_MMP_SEQ_CLEANwrite_mmp_block()wait_time == 0 -> no waitread_mmp_block()
- all OK, mount
write_mmp_block() wait_time == 0 -> no wait read_mmp_block() - all OK, mount
Do I get it right? Actually, if we passed seq we wrote in ext4_multi_mount_protect() to kmmpd (probably in sb), then kmmpd would notice the conflict on its first invocation but still that would be a bit late because there would be a time window where hostA and hostB would be both using the fs.
We could reduce the likelyhood of this race by always waiting in ext4_multi_mount_protect() between write & read but I guess that is undesirable as it would slow down all clean mounts. Ted?
Honza
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Re: [PATCH -next v2 2/6] ext4: introduce last_check_time record previous check time
Quoted 129 lines · Jan Kara
Quoted 85 lines · yebin
Quoted 48 lines · Jan Kara
Quoted 42 lines · yebin
Quoted 24 lines · yebin
Quoted 23 lines · Jan Kara
Quoted 10 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]>I think the check is there only for the case where
write_mmp_block()+ sleep took longer thanmmp_check_interval. I agree that should rarely happen but on a really busy system it is possible and in that case we would
miss updating mmp block for too long and so another node could have started
using the filesystem. I actually don't see a reason why kmmpd should be checking the block eachmmp_check_intervalas you do -mmp_check_interval
is just forext4_multi_mount_protect()to know how long it should wait before considering mmp block stale... Am I missing something?Quoted 6 lines
Quoted 1 line
Honza
I'm sorry, I didn't understand the detection mechanism here before. Now I understand
the detection mechanism here.
As you said, it's just an abnormal protection. There's really no problem.Yeah, i did test as following steps
hostA hostB
mount
ext4_multi_mount_protect-> seq ==EXT4_MMP_SEQ_CLEAN
delay 5s after label "skip" so hostB will see seq isEXT4_MMP_SEQ_CLEANmount ext4_multi_mount_protect -> seq == EXT4_MMP_SEQ_CLEAN run kmmpd run kmmpdQuoted 5 lines
Actually，in this situation kmmpd will not detect confliction. In
ext4_multi_mount_protectfunction we write mmp data first and wait 'wait_time* HZ' seconds,
read mmp data do check. Most of the time, If 'wait_time' is zero, it can pass check.Quoted 4 lines
But how can be
wait_timezero? As far as I'm reading the code,wait_timemust be at leastEXT4_MMP_MIN_CHECK_INTERVAL...Honza
int ext4_multi_mount_protect(struct
super_block*sb,
ext4_fsblk_tmmp_block){ struct ext4_super_block *es = EXT4_SB(sb)->s_es; struct buffer_head *bh = NULL; struct mmp_struct *mmp = NULL; u32 seq; unsigned int mmp_check_interval = le16_to_cpu(es->s_mmp_update_interval); unsigned int wait_time = 0; --> wait_time is equal with zero int retval; if (mmp_block < le32_to_cpu(es->s_first_data_block) || mmp_block >= ext4_blocks_count(es)) { ext4_warning(sb, "Invalid MMP block in superblock"); goto failed; } retval = read_mmp_block(sb, &bh, mmp_block); if (retval) goto failed; mmp = (struct mmp_struct *)(bh->b_data); if (mmp_check_interval < EXT4_MMP_MIN_CHECK_INTERVAL) mmp_check_interval = EXT4_MMP_MIN_CHECK_INTERVAL; /* * If check_interval in MMP block is larger, use that instead of * update_interval from the superblock. */Quoted 8 lines
if (le16_to_cpu(mmp->mmp_check_interval) > mmp_check_interval) mmp_check_interval = le16_to_cpu(mmp->mmp_check_interval); seq = le32_to_cpu(mmp->mmp_seq); if (seq == EXT4_MMP_SEQ_CLEAN) --> If hostA and hostB mount the same block device at the same time, --> HostA and hostB maybe get 'seq' with the same value EXT4_MMP_SEQ_CLEAN. goto skip;Oh, I see. Thanks for explanation.
Quoted 22 lines
...
skip:/* * write a new random sequence number. */ seq = mmp_new_seq(); mmp->mmp_seq = cpu_to_le32(seq); retval = write_mmp_block(sb, bh); if (retval) goto failed; /* * wait for MMP interval and check mmp_seq. */ if (schedule_timeout_interruptible(HZ * wait_time) != 0) { --> If seq is equal with EXT4_MMP_SEQ_CLEAN, wait_time is zero. ext4_warning(sb, "MMP startup interrupted, failing mount"); goto failed; } retval = read_mmp_block(sb, &bh, mmp_block); -->We may get the sameQuoted 1 line
data with which we wrote, so we can't detect conflict at here.
OK, I see. So the race in
ext4_multi_mount_protect()goes like:hostA hostB
read_mmp_block()read_mmp_block()
- seesEXT4_MMP_SEQ_CLEAN- seesEXT4_MMP_SEQ_CLEANwrite_mmp_block()wait_time== 0 -> no waitread_mmp_block()
- all OK, mount
Quoted 4 lines
write_mmp_block() wait_time == 0 -> no wait read_mmp_block() - all OK, mount
Yes, that's what i mean.
Quoted 12 lines
Do I get it right? Actually, if we passed seq we wrote in
ext4_multi_mount_protect()to kmmpd (probably in sb), then kmmpd would notice the conflict on its first invocation but still that would be a bit late because there would be a time window where hostA and hostB would be both using the fs.We could reduce the likelyhood of this race by always waiting in
ext4_multi_mount_protect()between write & read but I guess that is undesirable as it would slow down all clean mounts. Ted?Honza
Theodore Ts'o
maintainerRe: [PATCH -next v2 2/6] ext4: introduce last_check_time record previous check time
In reply to Jan KaraQuoted 22 lines · Jan Kara
OK, I see. So the race in
ext4_multi_mount_protect()goes like:hostA hostB
read_mmp_block()read_mmp_block()
- seesEXT4_MMP_SEQ_CLEAN- seesEXT4_MMP_SEQ_CLEANwrite_mmp_block()wait_time== 0 -> no waitread_mmp_block()
- all OK, mountwrite_mmp_block() wait_time == 0 -> no wait read_mmp_block() - all OK, mountDo I get it right? Actually, if we passed seq we wrote in
ext4_multi_mount_protect()to kmmpd (probably in sb), then kmmpd would notice the conflict on its first invocation but still that would be a bit late because there would be a time window where hostA and hostB would be both using the fs.
Quoted 3 lines
We could reduce the likelyhood of this race by always waiting in
ext4_multi_mount_protect()between write & read but I guess that is undesirable as it would slow down all clean mounts. Ted?
I'd like Andreas to comment here. My understanding is that MMP originally intended as a safety mechanism which would be used as part of a primary/backup high availability system, but not as the *primary* system where you might try to have two servers simultaneously try to mount the file system and use MMP as the "election" mechanism to decide which server is going to be the primary system, and which would be the backup system.
The cost of being able to handle this particular race is it would slow down the mounts of cleanly unmounted systems.
There *are* better systems to implement leader elections[1] than using MMP. Most of these more efficient leader elections assume that you have a working IP network, and so if you have a separate storage network (including a shared SCSI bus) from your standard IP network, then MMP is a useful failsafe in the face of a network partition of your IP network. The question is whether MMP should be useful for more than that. And if it isn't, then we should probably document what MMP is and isn't good for, and give advice in the form of an application note for how MMP should be used in the context of a larger system.
[1] https://en.wikipedia.org/wiki/Leader_election
- Ted
Re: [PATCH -next v2 2/6] ext4: introduce last_check_time record previous check time
Quoted 18 lines · Theodore Ts'o
Quoted 16 lines · Jan Kara
OK, I see. So the race in
ext4_multi_mount_protect()goes like:hostA hostB
read_mmp_block()read_mmp_block()
- seesEXT4_MMP_SEQ_CLEAN- seesEXT4_MMP_SEQ_CLEANwrite_mmp_block()wait_time== 0 -> no waitread_mmp_block()
- all OK, mountwrite_mmp_block() wait_time == 0 -> no wait read_mmp_block() - all OK, mount
Quoted 5 lines
Quoted 5 lines
Do I get it right? Actually, if we passed seq we wrote in
ext4_multi_mount_protect()to kmmpd (probably in sb), then kmmpd would notice the conflict on its first invocation but still that would be a bit late because there would be a time window where hostA and hostB would be both using the fs.
It would be enough to have even a short delay between write and read to detect this case. I _thought_ there should be a delay in this case, but maybe it was removed after the patch was originally submitted?
Quoted 12 lines
Quoted 3 lines
We could reduce the likelyhood of this race by always waiting in
ext4_multi_mount_protect()between write & read but I guess that is undesirable as it would slow down all clean mounts. Ted?I'd like Andreas to comment here. My understanding is that MMP originally intended as a safety mechanism which would be used as part of a primary/backup high availability system, but not as the *primary* system where you might try to have two servers simultaneously try to mount the file system and use MMP as the "election" mechanism to decide which server is going to be the primary system, and which would be the backup system.
Quoted 2 lines
The cost of being able to handle this particular race is it would slow down the mounts of cleanly unmounted systems.
Ted's understanding is correct - MMP is intended to be a backup mechanism to prevent filesystem corruption in the case where external HA methods do the wrong thing. This has avoided problems countless times on systems with multi-port access to the same storage, and can also be useful in the case of shared VM images accessed over the network, and similar.
When MMP was implemented for ZFS, a slightly different mechanism was used. Rather than having the delay to detect concurrent mounts, it instead writes to multiple different blocks in a random order, and then reads them all. If two nodes try to mount the filesystem concurrently, they would pick different block orders, and the chance of them having the same order (and one clobbering all of the blocks of the other) would be 1/2^num_blocks. The drawback is that this would consume more space in the filesystem, but it wouldn't be a huge deal these days.
Quoted 10 lines
There *are* better systems to implement leader elections[1] than using MMP. Most of these more efficient leader elections assume that you have a working IP network, and so if you have a separate storage network (including a shared SCSI bus) from your standard IP network, then MMP is a useful failsafe in the face of a network partition of your IP network. The question is whether MMP should be useful for more than that. And if it isn't, then we should probably document what MMP is and isn't good for, and give advice in the form of an application note for how MMP should be used in the context of a larger system.
One of the existing failure cases with HA that MMP detects is loss of network connection, so I wouldn't want to depend on that.
Cheers, Andreas
Re: [PATCH -next v2 2/6] ext4: introduce last_check_time record previous check time
In reply to Theodore Ts'oQuoted 18 lines · Theodore Ts'o
Quoted 16 lines · Jan Kara
OK, I see. So the race in
ext4_multi_mount_protect()goes like:hostA hostB
read_mmp_block()read_mmp_block()
- seesEXT4_MMP_SEQ_CLEAN- seesEXT4_MMP_SEQ_CLEANwrite_mmp_block()wait_time== 0 -> no waitread_mmp_block()
- all OK, mountwrite_mmp_block() wait_time == 0 -> no wait read_mmp_block() - all OK, mount
Quoted 5 lines
Quoted 5 lines
Do I get it right? Actually, if we passed seq we wrote in
ext4_multi_mount_protect()to kmmpd (probably in sb), then kmmpd would notice the conflict on its first invocation but still that would be a bit late because there would be a time window where hostA and hostB would be both using the fs.
It would be enough to have even a short delay between write and read to detect this case. I _thought_ there should be a delay in this case, but maybe it was removed after the patch was originally submitted?
Quoted 12 lines
Quoted 3 lines
We could reduce the likelyhood of this race by always waiting in
ext4_multi_mount_protect()between write & read but I guess that is undesirable as it would slow down all clean mounts. Ted?I'd like Andreas to comment here. My understanding is that MMP originally intended as a safety mechanism which would be used as part of a primary/backup high availability system, but not as the *primary* system where you might try to have two servers simultaneously try to mount the file system and use MMP as the "election" mechanism to decide which server is going to be the primary system, and which would be the backup system.
Quoted 2 lines
The cost of being able to handle this particular race is it would slow down the mounts of cleanly unmounted systems.
Ted's understanding is correct - MMP is intended to be a backup mechanism to prevent filesystem corruption in the case where external HA methods do the wrong thing. This has avoided problems countless times on systems with multi-port access to the same storage, and can also be useful in the case of shared VM images accessed over the network, and similar.
When MMP was implemented for ZFS, a slightly different mechanism was used. Rather than having the delay to detect concurrent mounts, it instead writes to multiple different blocks in a random order, and then reads them all. If two nodes try to mount the filesystem concurrently, they would pick different block orders, and the chance of them having the same order (and one clobbering all of the blocks of the other) would be 1/2^num_blocks. The drawback is that this would consume more space in the filesystem, but it wouldn't be a huge deal these days.
Quoted 10 lines
There *are* better systems to implement leader elections[1] than using MMP. Most of these more efficient leader elections assume that you have a working IP network, and so if you have a separate storage network (including a shared SCSI bus) from your standard IP network, then MMP is a useful failsafe in the face of a network partition of your IP network. The question is whether MMP should be useful for more than that. And if it isn't, then we should probably document what MMP is and isn't good for, and give advice in the form of an application note for how MMP should be used in the context of a larger system.
One of the existing failure cases with HA that MMP detects is loss of network connection, so I wouldn't want to depend on that.
Cheers, Andreas
PATCH v2 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_check_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
Jan Kara
reviewerRe: [PATCH -next v2 3/6] ext4: compare to local seq and nodename when check conflict
Quoted 5 lines · Ye Bin
As mmp and
check_mmpis 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 <[email protected]>
Good spotting! Just one nit below.
fs/ext4/mmp.c static int kmmpd(void *data) · line 157
Quoted 13 lines
--- fs/ext4/mmp.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)fs/ext4/mmp.c +1
-0@@ -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 ];
Perhaps define a constant for the length of nodename and use it here as well as in the declaration of mmp_struct->mmp_nodename?
Honza
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
PATCH v2 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
Jan Kara
reviewerRe: [PATCH -next v2 4/6] ext4: avoid to re-read mmp check data get from page cache
Quoted 5 lines · Ye Bin
As call
read_mmp_blockpassbh_checkwhich value is NULL, then callsb_getblkto getbuffer_head. But mmp_block'sbuffer_headis already exist
which also is uptodate. Lead to compare the same data.Signed-off-by Ye Bin <[email protected]>
This looks good, except that read_mmp_block() also releases bh it is passed in case of error. So it can free buffer head which is still referenced from EXT4_SB(sb)->s_mmp_bh and cause use-after-free issues.
I guess I would just get rid of sb_getblk() in read_mmp_block() and always expect valid bh passed. The only place that passes NULL bh after this patch is one case in ext4_multi_mount_protect() and that can call sb_getblk() on its own. That way we can also simplify read_mmp_block() prototype to:
static int read_mmp_block(struct super_block *sb, struct buffer_head *bh);
Honza
Quoted 49 lines
--- 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
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Re: [PATCH -next v2 4/6] ext4: avoid to re-read mmp check data get from page cache
Quoted 69 lines · Jan Kara
Quoted 5 lines · Ye Bin
As call
read_mmp_blockpassbh_checkwhich value is NULL, then callsb_getblkto getbuffer_head. But mmp_block'sbuffer_headis already exist
which also is uptodate. Lead to compare the same data.Signed-off-by Ye Bin <[email protected]>This looks good, except that
read_mmp_block()also releases bh it is passed in case of error. So it can free buffer head which is still referenced from EXT4_SB(sb)->s_mmp_bh and cause use-after-free issues.I guess I would just get rid of
sb_getblk()inread_mmp_block()and always expect valid bh passed. The only place that passes NULL bh after this patch is one case inext4_multi_mount_protect()and that can callsb_getblk()on its own. That way we can also simplifyread_mmp_block()prototype to:static int
read_mmp_block(struct super_block *sb, struct buffer_head *bh);Honza
Yeah, I will refactor 'read_mmp_block'.Quoted 49 lines
--- 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 v2 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 v2 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
Jan Kara
reviewerRe: [PATCH -next v2 6/6] ext4: fix possible store wrong check interval value in disk when umount
Quoted 42 lines · 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/sdaI 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 callext4_stop_mmpdto stop kmmpd kthread when umount, andschedule_timeout_interruptiblewill be interrupted, so 'diff' maybe little thanmmp_update_interval. Thenmmp_check_intervalwill range in [EXT4_MMP_MAX_CHECK_INTERVAL,EXT4_MMP_CHECK_MULT* diff / HZ]. To solve this issue, if 'diff' little thenmmp_update_interval* HZ just break loop, don't update check interval.Signed-off-by Ye Bin <[email protected]>--- fs/ext4/mmp.c | 8 ++++++++ 1 file changed, 8 insertions(+)fs/ext4/mmp.c +0
-0@@ -205,6 +205,14 @@ static int kmmpd(void *data)schedule_timeout_interruptible (mmp_update_interval *HZ - diff);
fs/ext4/mmp.c static int kmmpd(void *data) · line 216
Quoted 10 lines
fs/ext4/mmp.c +8
-0diff = 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 ; }
So in this case, mmp_check_interval would be EXT4_MMP_MIN_CHECK_INTERVAL. I don't quite understand what the practical problem is - the fsck message? That will happen anytime mmp_check_interval is >= 10 AFAICT and I don't quite see how that is connected to this condition... Can you explain a bit more please?
Honza
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Re: [PATCH -next v2 6/6] ext4: fix possible store wrong check interval value in disk when umount
Quoted 59 lines · Jan Kara
Quoted 42 lines · 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/sdaI 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 callext4_stop_mmpdto stop kmmpd kthread when umount, andschedule_timeout_interruptiblewill be interrupted, so 'diff' maybe little thanmmp_update_interval. Thenmmp_check_intervalwill range in [EXT4_MMP_MAX_CHECK_INTERVAL,EXT4_MMP_CHECK_MULT* diff / HZ]. To solve this issue, if 'diff' little thenmmp_update_interval* HZ just break loop, don't update check interval.Signed-off-by Ye Bin <[email protected]>--- fs/ext4/mmp.c | 8 ++++++++ 1 file changed, 8 insertions(+)fs/ext4/mmp.c +0
-0@@ -205,6 +205,14 @@ static int kmmpd(void *data)schedule_timeout_interruptible (mmp_update_interval *HZ - diff);fs/ext4/mmp.c static int kmmpd(void *data) · line 216
Quoted 10 lines
fs/ext4/mmp.c +8
-0diff = 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 ; }So in this case,
mmp_check_intervalwould beEXT4_MMP_MIN_CHECK_INTERVAL. I don't quite understand what the practical problem is - the fsck message? That will happen anytimemmp_check_intervalis >= 10 AFAICT and I don't quite see how that is connected to this condition... Can you explain a bit more please?
Quoted 1 line
Honza
I just think 'mmp_check_interval' is not reflect real check interval, and also sometime run fsck print "MMP interval is 10 seconds and total wait time is 42 seconds. Please wait...", but sometime not.