[PATCH v4 0/2] md_cluster: bugs fix

[PATCH v4 0/2] md_cluster: bugs fix

[Zhao Heming]

v4:
- add new patch 'fix rmmod issue when md_cluster convert bitmap to none'
- add cover leter
- modify both patches comments.

v3:
- add restoring path for error case
- modify comments

v2:
- change setting path from location_store to md_setup_cluster
- add restoring path

v1:
- create: fix safemode_delay value when converting to clustered bitmap

Zhao Heming (2):
  md-cluster: fix safemode_delay value when converting to clustered
    bitmap
  md-cluster: fix rmmod issue when md_cluster convert bitmap to none

 drivers/md/md.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

-- 
2.25.0

[PATCH v4 1/2] md-cluster: fix safemode_delay value when converting to clustered bitmap

[Zhao Heming]

When array convert to clustered bitmap, the safe_mode_delay doesn't clean and vice versa.
the /sys/block/mdX/md/safe_mode_delay keep original value after changing bitmap type.
in safe_delay_store(), the code forbids setting mddev->safemode_delay when array is clustered.
So in cluster-md env, the expected safemode_delay value should be 0.

reproduction steps:
```
node1 # mdadm --zero-superblock /dev/sd{b,c,d}
node1 # mdadm -C /dev/md0 -b internal -e 1.2 -n 2 -l mirror /dev/sdb /dev/sdc
node1 # cat /sys/block/md0/md/safe_mode_delay
0.204
node1 # mdadm -G /dev/md0 -b none
node1 # mdadm --grow /dev/md0 --bitmap=clustered
node1 # cat /sys/block/md0/md/safe_mode_delay
0.204  <== doesn't change, should ZERO for cluster-md

node1 # mdadm --zero-superblock /dev/sd{b,c,d}
node1 # mdadm -C /dev/md0 -b clustered -e 1.2 -n 2 -l mirror /dev/sdb /dev/sdc
node1 # cat /sys/block/md0/md/safe_mode_delay
0.000
node1 # mdadm -G /dev/md0 -b none
node1 # cat /sys/block/md0/md/safe_mode_delay
0.000  <== doesn't change, should default value
```

md_setup_cluster() is a good place for setting, but md_cluster_stop is 
good for clean job when md_setup_cluster return err.

see below flow:
(user space)
mdadm -C /dev/md0 -b clustered -e 1.2 -n 2 -l mirror /dev/sda /dev/sdb
mdadm --grow /dev/md0 -b none
(kernel space)
SET_ARRAY_INFO
 update_array_info
  + mddev->bitmap_info.nodes = 0;
  + md_cluster_ops->leave(mddev)
  + md_bitmap_destroy
     md_bitmap_free //won't trigger md_cluster_stop() because above set 0.

Signed-off-by: Zhao Heming <heming.zhao@suse.com>
---
 drivers/md/md.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index f567f536b529..e20f1d5a5261 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -101,6 +101,8 @@ static void mddev_detach(struct mddev *mddev);
  * count by 2 for every hour elapsed between read errors.
  */
 #define MD_DEFAULT_MAX_CORRECTED_READ_ERRORS 20
+/* Default safemode delay: 200 msec */
+#define DEFAULT_SAFEMODE_DELAY ((200 * HZ)/1000 +1)
 /*
  * Current RAID-1,4,5 parallel reconstruction 'guaranteed speed limit'
  * is 1000 KB/sec, so the extra system load does not show up that much.
@@ -5982,7 +5984,7 @@ int md_run(struct mddev *mddev)
 	if (mddev_is_clustered(mddev))
 		mddev->safemode_delay = 0;
 	else
-		mddev->safemode_delay = (200 * HZ)/1000 +1; /* 200 msec delay */
+		mddev->safemode_delay = DEFAULT_SAFEMODE_DELAY;
 	mddev->in_sync = 1;
 	smp_wmb();
 	spin_lock(&mddev->lock);
@@ -7361,6 +7363,7 @@ static int update_array_info(struct mddev *mddev, mdu_array_info_t *info)
 
 				mddev->bitmap_info.nodes = 0;
 				md_cluster_ops->leave(mddev);
+				mddev->safemode_delay = DEFAULT_SAFEMODE_DELAY;
 			}
 			mddev_suspend(mddev);
 			md_bitmap_destroy(mddev);
@@ -8366,6 +8369,7 @@ int md_setup_cluster(struct mddev *mddev, int nodes)
 	}
 	spin_unlock(&pers_lock);
 
+	mddev->safemode_delay = 0;
 	return md_cluster_ops->join(mddev, nodes);
 }
 
@@ -8375,6 +8379,7 @@ void md_cluster_stop(struct mddev *mddev)
 		return;
 	md_cluster_ops->leave(mddev);
 	module_put(md_cluster_mod);
+	mddev->safemode_delay = DEFAULT_SAFEMODE_DELAY;
 }
 
 static int is_mddev_idle(struct mddev *mddev, int init)
-- 
2.25.0

[PATCH v4 2/2] md-cluster: fix rmmod issue when md_cluster convert bitmap to none

[Zhao Heming]

update_array_info misses calling module_put when removing cluster bitmap.

steps to reproduce:
```
node1 # mdadm -C /dev/md0 -b clustered -e 1.2 -n 2 -l mirror /dev/sda
/dev/sdb
mdadm: array /dev/md0 started.
node1 # lsmod | egrep "dlm|md_|raid1"
md_cluster             28672  1
dlm                   212992  14 md_cluster
configfs               57344  2 dlm
raid1                  53248  1
md_mod                176128  2 raid1,md_cluster
node1 # mdadm -G /dev/md0 -b none
node1 # lsmod | egrep "dlm|md_|raid1"
md_cluster             28672  1 <== should be zero
dlm                   212992  9 md_cluster
configfs               57344  2 dlm
raid1                  53248  1
md_mod                176128  2 raid1,md_cluster
node1 # mdadm -G /dev/md0 -b clustered
node1 # lsmod | egrep "dlm|md_|raid1"
md_cluster             28672  2 <== increase
dlm                   212992  14 md_cluster
configfs               57344  2 dlm
raid1                  53248  1
md_mod                176128  2 raid1,md_cluster
node1 # mdadm -G /dev/md0 -b none
node1 # mdadm -G /dev/md0 -b clustered
node1 # lsmod | egrep "dlm|md_|raid1"
md_cluster             28672  3 <== increase
dlm                   212992  14 md_cluster
configfs               57344  2 dlm
raid1                  53248  1
md_mod                176128  2 raid1,md_cluster
```

Signed-off-by: Zhao Heming <heming.zhao@suse.com>
---
 drivers/md/md.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index e20f1d5a5261..ca791387e54d 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -7363,6 +7363,7 @@ static int update_array_info(struct mddev *mddev, mdu_array_info_t *info)
 
 				mddev->bitmap_info.nodes = 0;
 				md_cluster_ops->leave(mddev);
+				module_put(md_cluster_mod);
 				mddev->safemode_delay = DEFAULT_SAFEMODE_DELAY;
 			}
 			mddev_suspend(mddev);
-- 
2.25.0

Re: [PATCH v4 0/2] md_cluster: bugs fix

[heming.zhao]

Very sorry, I mistakenly added Jes, CC Song in this mail.

Jes, please ignore these patches, sorry for CC you.

On 7/19/20 7:08 PM, Zhao Heming wrote:
> v4:
> - add new patch 'fix rmmod issue when md_cluster convert bitmap to none'
> - add cover leter
> - modify both patches comments.
> 
> v3:
> - add restoring path for error case
> - modify comments
> 
> v2:
> - change setting path from location_store to md_setup_cluster
> - add restoring path
> 
> v1:
> - create: fix safemode_delay value when converting to clustered bitmap
> 
> Zhao Heming (2):
>    md-cluster: fix safemode_delay value when converting to clustered
>      bitmap
>    md-cluster: fix rmmod issue when md_cluster convert bitmap to none
> 
>   drivers/md/md.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 

Re: [PATCH v4 1/2] md-cluster: fix safemode_delay value when converting to clustered bitmap

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 3649 bytes --]

On Sun, Jul 19 2020, Zhao Heming wrote:

> When array convert to clustered bitmap, the safe_mode_delay doesn't clean and vice versa.
> the /sys/block/mdX/md/safe_mode_delay keep original value after changing bitmap type.
> in safe_delay_store(), the code forbids setting mddev->safemode_delay when array is clustered.
> So in cluster-md env, the expected safemode_delay value should be 0.
>
> reproduction steps:
> ```
> node1 # mdadm --zero-superblock /dev/sd{b,c,d}
> node1 # mdadm -C /dev/md0 -b internal -e 1.2 -n 2 -l mirror /dev/sdb /dev/sdc
> node1 # cat /sys/block/md0/md/safe_mode_delay
> 0.204
> node1 # mdadm -G /dev/md0 -b none
> node1 # mdadm --grow /dev/md0 --bitmap=clustered
> node1 # cat /sys/block/md0/md/safe_mode_delay
> 0.204  <== doesn't change, should ZERO for cluster-md
>
> node1 # mdadm --zero-superblock /dev/sd{b,c,d}
> node1 # mdadm -C /dev/md0 -b clustered -e 1.2 -n 2 -l mirror /dev/sdb /dev/sdc
> node1 # cat /sys/block/md0/md/safe_mode_delay
> 0.000
> node1 # mdadm -G /dev/md0 -b none
> node1 # cat /sys/block/md0/md/safe_mode_delay
> 0.000  <== doesn't change, should default value
> ```
>
> md_setup_cluster() is a good place for setting, but md_cluster_stop is 
> good for clean job when md_setup_cluster return err.
>
> see below flow:
> (user space)
> mdadm -C /dev/md0 -b clustered -e 1.2 -n 2 -l mirror /dev/sda /dev/sdb
> mdadm --grow /dev/md0 -b none
> (kernel space)
> SET_ARRAY_INFO
>  update_array_info
>   + mddev->bitmap_info.nodes = 0;
>   + md_cluster_ops->leave(mddev)
>   + md_bitmap_destroy
>      md_bitmap_free //won't trigger md_cluster_stop() because above set 0.
>
> Signed-off-by: Zhao Heming <heming.zhao@suse.com>
> ---
>  drivers/md/md.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index f567f536b529..e20f1d5a5261 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -101,6 +101,8 @@ static void mddev_detach(struct mddev *mddev);
>   * count by 2 for every hour elapsed between read errors.
>   */
>  #define MD_DEFAULT_MAX_CORRECTED_READ_ERRORS 20
> +/* Default safemode delay: 200 msec */
> +#define DEFAULT_SAFEMODE_DELAY ((200 * HZ)/1000 +1)
>  /*
>   * Current RAID-1,4,5 parallel reconstruction 'guaranteed speed limit'
>   * is 1000 KB/sec, so the extra system load does not show up that much.
> @@ -5982,7 +5984,7 @@ int md_run(struct mddev *mddev)
>  	if (mddev_is_clustered(mddev))
>  		mddev->safemode_delay = 0;
>  	else
> -		mddev->safemode_delay = (200 * HZ)/1000 +1; /* 200 msec delay */
> +		mddev->safemode_delay = DEFAULT_SAFEMODE_DELAY;
>  	mddev->in_sync = 1;
>  	smp_wmb();
>  	spin_lock(&mddev->lock);
> @@ -7361,6 +7363,7 @@ static int update_array_info(struct mddev *mddev, mdu_array_info_t *info)
>  
>  				mddev->bitmap_info.nodes = 0;
>  				md_cluster_ops->leave(mddev);
> +				mddev->safemode_delay = DEFAULT_SAFEMODE_DELAY;
>  			}
>  			mddev_suspend(mddev);
>  			md_bitmap_destroy(mddev);
> @@ -8366,6 +8369,7 @@ int md_setup_cluster(struct mddev *mddev, int nodes)
>  	}
>  	spin_unlock(&pers_lock);
>  
> +	mddev->safemode_delay = 0;
>  	return md_cluster_ops->join(mddev, nodes);

I still don't think you should change safemode_delay here until after
->join succeeds.

NeilBrown

>  }
>  
> @@ -8375,6 +8379,7 @@ void md_cluster_stop(struct mddev *mddev)
>  		return;
>  	md_cluster_ops->leave(mddev);
>  	module_put(md_cluster_mod);
> +	mddev->safemode_delay = DEFAULT_SAFEMODE_DELAY;
>  }
>  
>  static int is_mddev_idle(struct mddev *mddev, int init)
> -- 
> 2.25.0

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH v4 2/2] md-cluster: fix rmmod issue when md_cluster convert bitmap to none

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2290 bytes --]

On Sun, Jul 19 2020, Zhao Heming wrote:

> update_array_info misses calling module_put when removing cluster bitmap.
>
> steps to reproduce:
> ```
> node1 # mdadm -C /dev/md0 -b clustered -e 1.2 -n 2 -l mirror /dev/sda
> /dev/sdb
> mdadm: array /dev/md0 started.
> node1 # lsmod | egrep "dlm|md_|raid1"
> md_cluster             28672  1
> dlm                   212992  14 md_cluster
> configfs               57344  2 dlm
> raid1                  53248  1
> md_mod                176128  2 raid1,md_cluster
> node1 # mdadm -G /dev/md0 -b none
> node1 # lsmod | egrep "dlm|md_|raid1"
> md_cluster             28672  1 <== should be zero
> dlm                   212992  9 md_cluster
> configfs               57344  2 dlm
> raid1                  53248  1
> md_mod                176128  2 raid1,md_cluster
> node1 # mdadm -G /dev/md0 -b clustered
> node1 # lsmod | egrep "dlm|md_|raid1"
> md_cluster             28672  2 <== increase
> dlm                   212992  14 md_cluster
> configfs               57344  2 dlm
> raid1                  53248  1
> md_mod                176128  2 raid1,md_cluster
> node1 # mdadm -G /dev/md0 -b none
> node1 # mdadm -G /dev/md0 -b clustered
> node1 # lsmod | egrep "dlm|md_|raid1"
> md_cluster             28672  3 <== increase
> dlm                   212992  14 md_cluster
> configfs               57344  2 dlm
> raid1                  53248  1
> md_mod                176128  2 raid1,md_cluster
> ```
>
> Signed-off-by: Zhao Heming <heming.zhao@suse.com>
> ---
>  drivers/md/md.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index e20f1d5a5261..ca791387e54d 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -7363,6 +7363,7 @@ static int update_array_info(struct mddev *mddev, mdu_array_info_t *info)
>  
>  				mddev->bitmap_info.nodes = 0;
>  				md_cluster_ops->leave(mddev);
> +				module_put(md_cluster_mod);
>  				mddev->safemode_delay = DEFAULT_SAFEMODE_DELAY;

I would make this a call to "md_cluster_stop()", and move the reset of
->safemode_delay" in there, but either way this is correct and needed.

  Reviewed-by: NeilBrown <neilb@suse.de>

Thanks,
NeilBrown


>  			}
>  			mddev_suspend(mddev);
> -- 
> 2.25.0

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH v4 2/2] md-cluster: fix rmmod issue when md_cluster convert bitmap to none

[heming.zhao]

On 7/20/20 7:26 AM, NeilBrown wrote:
> On Sun, Jul 19 2020, Zhao Heming wrote:
> 
>> update_array_info misses calling module_put when removing cluster bitmap.
>>
>> steps to reproduce:
>> ```
>> node1 # mdadm -C /dev/md0 -b clustered -e 1.2 -n 2 -l mirror /dev/sda
>> /dev/sdb
>> mdadm: array /dev/md0 started.
>> node1 # lsmod | egrep "dlm|md_|raid1"
>> md_cluster             28672  1
>> dlm                   212992  14 md_cluster
>> configfs               57344  2 dlm
>> raid1                  53248  1
>> md_mod                176128  2 raid1,md_cluster
>> node1 # mdadm -G /dev/md0 -b none
>> node1 # lsmod | egrep "dlm|md_|raid1"
>> md_cluster             28672  1 <== should be zero
>> dlm                   212992  9 md_cluster
>> configfs               57344  2 dlm
>> raid1                  53248  1
>> md_mod                176128  2 raid1,md_cluster
>> node1 # mdadm -G /dev/md0 -b clustered
>> node1 # lsmod | egrep "dlm|md_|raid1"
>> md_cluster             28672  2 <== increase
>> dlm                   212992  14 md_cluster
>> configfs               57344  2 dlm
>> raid1                  53248  1
>> md_mod                176128  2 raid1,md_cluster
>> node1 # mdadm -G /dev/md0 -b none
>> node1 # mdadm -G /dev/md0 -b clustered
>> node1 # lsmod | egrep "dlm|md_|raid1"
>> md_cluster             28672  3 <== increase
>> dlm                   212992  14 md_cluster
>> configfs               57344  2 dlm
>> raid1                  53248  1
>> md_mod                176128  2 raid1,md_cluster
>> ```
>>
>> Signed-off-by: Zhao Heming <heming.zhao@suse.com>
>> ---
>>   drivers/md/md.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>> index e20f1d5a5261..ca791387e54d 100644
>> --- a/drivers/md/md.c
>> +++ b/drivers/md/md.c
>> @@ -7363,6 +7363,7 @@ static int update_array_info(struct mddev *mddev, mdu_array_info_t *info)
>>   
>>   				mddev->bitmap_info.nodes = 0;
>>   				md_cluster_ops->leave(mddev);
>> +				module_put(md_cluster_mod);
>>   				mddev->safemode_delay = DEFAULT_SAFEMODE_DELAY;
> 
> I would make this a call to "md_cluster_stop()", and move the reset of
> ->safemode_delay" in there, but either way this is correct and needed.
> 
>    Reviewed-by: NeilBrown <neilb@suse.de>
> 
> Thanks,
> NeilBrown
> 
Thank you for your review.
If put module_put() in md_cluster_stop(), "mddev->bitmap_info.nodes = 0" also needs to put in md_cluster_stop().
the "bitmap_info.nodes = 0" makes mddev_is_clustered() false and will close the door to call md_cluster_stop().

> 
>>   			}
>>   			mddev_suspend(mddev);
>> -- 
>> 2.25.0

Re: [PATCH v4 1/2] md-cluster: fix safemode_delay value when converting to clustered bitmap

[heming.zhao]

On 7/20/20 7:24 AM, NeilBrown wrote:
> On Sun, Jul 19 2020, Zhao Heming wrote:
> 
>> When array convert to clustered bitmap, the safe_mode_delay doesn't clean and vice versa.
>> the /sys/block/mdX/md/safe_mode_delay keep original value after changing bitmap type.
>> in safe_delay_store(), the code forbids setting mddev->safemode_delay when array is clustered.
>> So in cluster-md env, the expected safemode_delay value should be 0.
>>
>> reproduction steps:
>> ```
>> node1 # mdadm --zero-superblock /dev/sd{b,c,d}
>> node1 # mdadm -C /dev/md0 -b internal -e 1.2 -n 2 -l mirror /dev/sdb /dev/sdc
>> node1 # cat /sys/block/md0/md/safe_mode_delay
>> 0.204
>> node1 # mdadm -G /dev/md0 -b none
>> node1 # mdadm --grow /dev/md0 --bitmap=clustered
>> node1 # cat /sys/block/md0/md/safe_mode_delay
>> 0.204  <== doesn't change, should ZERO for cluster-md
>>
>> node1 # mdadm --zero-superblock /dev/sd{b,c,d}
>> node1 # mdadm -C /dev/md0 -b clustered -e 1.2 -n 2 -l mirror /dev/sdb /dev/sdc
>> node1 # cat /sys/block/md0/md/safe_mode_delay
>> 0.000
>> node1 # mdadm -G /dev/md0 -b none
>> node1 # cat /sys/block/md0/md/safe_mode_delay
>> 0.000  <== doesn't change, should default value
>> ```
>>
>> md_setup_cluster() is a good place for setting, but md_cluster_stop is
>> good for clean job when md_setup_cluster return err.
>>
>> see below flow:
>> (user space)
>> mdadm -C /dev/md0 -b clustered -e 1.2 -n 2 -l mirror /dev/sda /dev/sdb
>> mdadm --grow /dev/md0 -b none
>> (kernel space)
>> SET_ARRAY_INFO
>>   update_array_info
>>    + mddev->bitmap_info.nodes = 0;
>>    + md_cluster_ops->leave(mddev)
>>    + md_bitmap_destroy
>>       md_bitmap_free //won't trigger md_cluster_stop() because above set 0.
>>
>> Signed-off-by: Zhao Heming <heming.zhao@suse.com>
>> ---
>>   drivers/md/md.c | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>> index f567f536b529..e20f1d5a5261 100644
>> --- a/drivers/md/md.c
>> +++ b/drivers/md/md.c
>> @@ -101,6 +101,8 @@ static void mddev_detach(struct mddev *mddev);
>>    * count by 2 for every hour elapsed between read errors.
>>    */
>>   #define MD_DEFAULT_MAX_CORRECTED_READ_ERRORS 20
>> +/* Default safemode delay: 200 msec */
>> +#define DEFAULT_SAFEMODE_DELAY ((200 * HZ)/1000 +1)
>>   /*
>>    * Current RAID-1,4,5 parallel reconstruction 'guaranteed speed limit'
>>    * is 1000 KB/sec, so the extra system load does not show up that much.
>> @@ -5982,7 +5984,7 @@ int md_run(struct mddev *mddev)
>>   	if (mddev_is_clustered(mddev))
>>   		mddev->safemode_delay = 0;
>>   	else
>> -		mddev->safemode_delay = (200 * HZ)/1000 +1; /* 200 msec delay */
>> +		mddev->safemode_delay = DEFAULT_SAFEMODE_DELAY;
>>   	mddev->in_sync = 1;
>>   	smp_wmb();
>>   	spin_lock(&mddev->lock);
>> @@ -7361,6 +7363,7 @@ static int update_array_info(struct mddev *mddev, mdu_array_info_t *info)
>>   
>>   				mddev->bitmap_info.nodes = 0;
>>   				md_cluster_ops->leave(mddev);
>> +				mddev->safemode_delay = DEFAULT_SAFEMODE_DELAY;
>>   			}
>>   			mddev_suspend(mddev);
>>   			md_bitmap_destroy(mddev);
>> @@ -8366,6 +8369,7 @@ int md_setup_cluster(struct mddev *mddev, int nodes)
>>   	}
>>   	spin_unlock(&pers_lock);
>>   
>> +	mddev->safemode_delay = 0;
>>   	return md_cluster_ops->join(mddev, nodes);
> 
> I still don't think you should change safemode_delay here until after
> ->join succeeds.
> 
> NeilBrown
you are right, it makes a gap to set delay to zero before join().
I will following your comments.
> 
>>   }
>>   
>> @@ -8375,6 +8379,7 @@ void md_cluster_stop(struct mddev *mddev)
>>   		return;
>>   	md_cluster_ops->leave(mddev);
>>   	module_put(md_cluster_mod);
>> +	mddev->safemode_delay = DEFAULT_SAFEMODE_DELAY;
>>   }
>>   
>>   static int is_mddev_idle(struct mddev *mddev, int init)
>> -- 
>> 2.25.0