[PATCH v2 0/4] raid10 bugfix

[PATCH v2 0/4] raid10 bugfix

[linan666]

From: Li Nan <linan122@huawei.com>

Changes in v2:
 - add patch "md/raid10: improve code of mrdev in raid10_sync_request".
 - in patch 3, break lines longer than 80 columns and improve log.

Li Nan (4):
  md/raid10: fix null-ptr-deref of mreplace in raid10_sync_request
  md/raid10: improve code of mrdev in raid10_sync_request
  md/raid10: fix incorrect done of recovery
  md/raid10: fix io loss while replacement replace rdev

 drivers/md/raid10.c | 73 +++++++++++++++++++++++++++++++--------------
 1 file changed, 51 insertions(+), 22 deletions(-)

-- 
2.31.1

[PATCH v2 1/4] md/raid10: fix null-ptr-deref of mreplace in raid10_sync_request

[linan666]

From: Li Nan <linan122@huawei.com>

need_replace will be set to 1 if no-Faulty mreplace exists, and mreplace
will be deref later. However, the latter check of mreplace might set
mreplace to NULL, null-ptr-deref occurs if need_replace is 1 at this time.

Fix it by merging two checks into one. And replace 'need_replace' with
'mreplace' because their values are always the same.

Fixes: ee37d7314a32 ("md/raid10: Fix raid10 replace hang when new added disk faulty")
Signed-off-by: Li Nan <linan122@huawei.com>
---
 drivers/md/raid10.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 4fcfcb350d2b..e21502c03b45 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -3438,7 +3438,6 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
 			int must_sync;
 			int any_working;
 			int need_recover = 0;
-			int need_replace = 0;
 			struct raid10_info *mirror = &conf->mirrors[i];
 			struct md_rdev *mrdev, *mreplace;
 
@@ -3451,10 +3450,10 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
 			    !test_bit(In_sync, &mrdev->flags))
 				need_recover = 1;
 			if (mreplace != NULL &&
-			    !test_bit(Faulty, &mreplace->flags))
-				need_replace = 1;
+			    test_bit(Faulty, &mreplace->flags))
+				mreplace = NULL;
 
-			if (!need_recover && !need_replace) {
+			if (!need_recover && !mreplace) {
 				rcu_read_unlock();
 				continue;
 			}
@@ -3470,8 +3469,6 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
 				rcu_read_unlock();
 				continue;
 			}
-			if (mreplace && test_bit(Faulty, &mreplace->flags))
-				mreplace = NULL;
 			/* Unless we are doing a full sync, or a replacement
 			 * we only need to recover the block if it is set in
 			 * the bitmap
@@ -3594,11 +3591,11 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
 				bio = r10_bio->devs[1].repl_bio;
 				if (bio)
 					bio->bi_end_io = NULL;
-				/* Note: if need_replace, then bio
+				/* Note: if replace is not NULL, then bio
 				 * cannot be NULL as r10buf_pool_alloc will
 				 * have allocated it.
 				 */
-				if (!need_replace)
+				if (!mreplace)
 					break;
 				bio->bi_next = biolist;
 				biolist = bio;
-- 
2.31.1

[PATCH v2 2/4] md/raid10: improve code of mrdev in raid10_sync_request

[linan666]

From: Li Nan <linan122@huawei.com>

'need_recover' and 'mrdev' are equivalent in raid10_sync_request(), and
inc mrdev->nr_pending is unreasonable if don't need recovery. Replace
'need_recover' with 'mrdev', and only inc nr_pending when needed.

Suggested-by: Yu Kuai <yukuai3@huawei.com>
Signed-off-by: Li Nan <linan122@huawei.com>
---
 drivers/md/raid10.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index e21502c03b45..9de9eabff209 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -3437,7 +3437,6 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
 			sector_t sect;
 			int must_sync;
 			int any_working;
-			int need_recover = 0;
 			struct raid10_info *mirror = &conf->mirrors[i];
 			struct md_rdev *mrdev, *mreplace;
 
@@ -3446,14 +3445,14 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
 			mreplace = rcu_dereference(mirror->replacement);
 
 			if (mrdev != NULL &&
-			    !test_bit(Faulty, &mrdev->flags) &&
-			    !test_bit(In_sync, &mrdev->flags))
-				need_recover = 1;
+			    (test_bit(Faulty, &mrdev->flags) ||
+			    test_bit(In_sync, &mrdev->flags)))
+				mrdev = NULL;
 			if (mreplace != NULL &&
 			    test_bit(Faulty, &mreplace->flags))
 				mreplace = NULL;
 
-			if (!need_recover && !mreplace) {
+			if (!mrdev && !mreplace) {
 				rcu_read_unlock();
 				continue;
 			}
@@ -3487,7 +3486,8 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
 				rcu_read_unlock();
 				continue;
 			}
-			atomic_inc(&mrdev->nr_pending);
+			if (mrdev)
+				atomic_inc(&mrdev->nr_pending);
 			if (mreplace)
 				atomic_inc(&mreplace->nr_pending);
 			rcu_read_unlock();
@@ -3574,7 +3574,7 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
 				r10_bio->devs[1].devnum = i;
 				r10_bio->devs[1].addr = to_addr;
 
-				if (need_recover) {
+				if (mrdev) {
 					bio = r10_bio->devs[1].bio;
 					bio->bi_next = biolist;
 					biolist = bio;
@@ -3619,7 +3619,7 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
 					for (k = 0; k < conf->copies; k++)
 						if (r10_bio->devs[k].devnum == i)
 							break;
-					if (!test_bit(In_sync,
+					if (mrdev && !test_bit(In_sync,
 						      &mrdev->flags)
 					    && !rdev_set_badblocks(
 						    mrdev,
@@ -3645,12 +3645,14 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
 				if (rb2)
 					atomic_dec(&rb2->remaining);
 				r10_bio = rb2;
-				rdev_dec_pending(mrdev, mddev);
+				if (mrdev)
+					rdev_dec_pending(mrdev, mddev);
 				if (mreplace)
 					rdev_dec_pending(mreplace, mddev);
 				break;
 			}
-			rdev_dec_pending(mrdev, mddev);
+			if (mrdev)
+				rdev_dec_pending(mrdev, mddev);
 			if (mreplace)
 				rdev_dec_pending(mreplace, mddev);
 			if (r10_bio->devs[0].bio->bi_opf & MD_FAILFAST) {
-- 
2.31.1

[PATCH v2 3/4] md/raid10: fix incorrect done of recovery

[linan666]

From: Li Nan <linan122@huawei.com>

Recovery will go to giveup and let chunks_skipped++ in
raid10_sync_request() if there are some bad_blocks, and it will return
max_sector when chunks_skipped >= geo.raid_disks. Now, recovery fail and
data is inconsistent but user think recovery is done, it is wrong.

Fix it by set mirror's recovery_disabled, spare device will not  be added
to here. The same issue alos exists on resync, it will be fixd in future.

Signed-off-by: Li Nan <linan122@huawei.com>
---
 drivers/md/raid10.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 9de9eabff209..aa22782a7330 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -3303,6 +3303,7 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
 	int chunks_skipped = 0;
 	sector_t chunk_mask = conf->geo.chunk_mask;
 	int page_idx = 0;
+	int error_disk = -1;
 
 	/*
 	 * Allow skipping a full rebuild for incremental assembly
@@ -3386,7 +3387,20 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
 		return reshape_request(mddev, sector_nr, skipped);
 
 	if (chunks_skipped >= conf->geo.raid_disks) {
-		/* if there has been nothing to do on any drive,
+		pr_err("md/raid10:%s: %s fail\n", mdname(mddev),
+			test_bit(MD_RECOVERY_SYNC, &mddev->recovery) ?  "resync" : "recovery");
+		if (error_disk >= 0 &&
+		    !test_bit(MD_RECOVERY_SYNC, &mddev->recovery)) {
+			/*
+			 * recovery fail, set mirrors.recovory_disabled,
+			 * device shouldn't be added to there.
+			 */
+			conf->mirrors[error_disk].recovery_disabled =
+						mddev->recovery_disabled;
+			return 0;
+		}
+		/*
+		 * if there has been nothing to do on any drive,
 		 * then there is nothing to do at all..
 		 */
 		*skipped = 1;
@@ -3640,6 +3654,8 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
 						       mdname(mddev));
 					mirror->recovery_disabled
 						= mddev->recovery_disabled;
+				} else {
+					error_disk = i;
 				}
 				put_buf(r10_bio);
 				if (rb2)
-- 
2.31.1

[PATCH v2 4/4] md/raid10: fix io loss while replacement replace rdev

[linan666]

From: Li Nan <linan122@huawei.com>

When we remove a disk which has replacement, first set rdev to NULL
and then set replacement to rdev, finally set replacement to NULL (see
raid10_remove_disk()). If io is submitted during the same time, it might
read both rdev and replacement as NULL, and io will not be submitted.

  rdev -> NULL
			read rdev
  replacement -> NULL
			read replacement

Fix it by reading replacement first and rdev later, meanwhile, use smp_mb()
to prevent memory reordering.

Fixes: 475b0321a4df ("md/raid10: writes should get directed to replacement as well as original.")
Signed-off-by: Li Nan <linan122@huawei.com>
Reviewed-by: Yu Kuai <yukuai3@huawei.com>
---
 drivers/md/raid10.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index aa22782a7330..88e4bae15143 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -779,8 +779,16 @@ static struct md_rdev *read_balance(struct r10conf *conf,
 		disk = r10_bio->devs[slot].devnum;
 		rdev = rcu_dereference(conf->mirrors[disk].replacement);
 		if (rdev == NULL || test_bit(Faulty, &rdev->flags) ||
-		    r10_bio->devs[slot].addr + sectors > rdev->recovery_offset)
+		    r10_bio->devs[slot].addr + sectors >
+		    rdev->recovery_offset) {
+			/*
+			 * Read replacement first to prevent reading both rdev
+			 * and replacement as NULL during replacement replace
+			 * rdev.
+			 */
+			smp_mb();
 			rdev = rcu_dereference(conf->mirrors[disk].rdev);
+		}
 		if (rdev == NULL ||
 		    test_bit(Faulty, &rdev->flags))
 			continue;
@@ -1479,9 +1487,15 @@ static void raid10_write_request(struct mddev *mddev, struct bio *bio,
 
 	for (i = 0;  i < conf->copies; i++) {
 		int d = r10_bio->devs[i].devnum;
-		struct md_rdev *rdev = rcu_dereference(conf->mirrors[d].rdev);
-		struct md_rdev *rrdev = rcu_dereference(
-			conf->mirrors[d].replacement);
+		struct md_rdev *rdev, *rrdev;
+
+		rrdev = rcu_dereference(conf->mirrors[d].replacement);
+		/*
+		 * Read replacement first to Prevent reading both rdev and
+		 * replacement as NULL during replacement replace rdev.
+		 */
+		smp_mb();
+		rdev = rcu_dereference(conf->mirrors[d].rdev);
 		if (rdev == rrdev)
 			rrdev = NULL;
 		if (rdev && (test_bit(Faulty, &rdev->flags)))
-- 
2.31.1

Re: [PATCH v2 1/4] md/raid10: fix null-ptr-deref of mreplace in raid10_sync_request

[Song Liu]

On Fri, May 26, 2023 at 12:47 AM <linan666@huaweicloud.com> wrote:
>
> From: Li Nan <linan122@huawei.com>
>
> need_replace will be set to 1 if no-Faulty mreplace exists, and mreplace
> will be deref later. However, the latter check of mreplace might set
> mreplace to NULL, null-ptr-deref occurs if need_replace is 1 at this time.
>
> Fix it by merging two checks into one. And replace 'need_replace' with
> 'mreplace' because their values are always the same.
>
> Fixes: ee37d7314a32 ("md/raid10: Fix raid10 replace hang when new added disk faulty")
> Signed-off-by: Li Nan <linan122@huawei.com>
> ---
>  drivers/md/raid10.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index 4fcfcb350d2b..e21502c03b45 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -3438,7 +3438,6 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
>                         int must_sync;
>                         int any_working;
>                         int need_recover = 0;
> -                       int need_replace = 0;
>                         struct raid10_info *mirror = &conf->mirrors[i];
>                         struct md_rdev *mrdev, *mreplace;
>
> @@ -3451,10 +3450,10 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
>                             !test_bit(In_sync, &mrdev->flags))
>                                 need_recover = 1;
>                         if (mreplace != NULL &&
> -                           !test_bit(Faulty, &mreplace->flags))
> -                               need_replace = 1;
> +                           test_bit(Faulty, &mreplace->flags))
> +                               mreplace = NULL;
>
> -                       if (!need_recover && !need_replace) {
> +                       if (!need_recover && !mreplace) {
>                                 rcu_read_unlock();
>                                 continue;
>                         }
> @@ -3470,8 +3469,6 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
>                                 rcu_read_unlock();
>                                 continue;
>                         }

To make sure I understand the issue correctly:

The null-ptr-deref only happens when the Faulty bit was set after the
last check and before this check below, right?

> -                       if (mreplace && test_bit(Faulty, &mreplace->flags))
> -                               mreplace = NULL;
>                         /* Unless we are doing a full sync, or a replacement
>                          * we only need to recover the block if it is set in
>                          * the bitmap

Thanks,
Song

> @@ -3594,11 +3591,11 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
>                                 bio = r10_bio->devs[1].repl_bio;
>                                 if (bio)
>                                         bio->bi_end_io = NULL;
> -                               /* Note: if need_replace, then bio
> +                               /* Note: if replace is not NULL, then bio
>                                  * cannot be NULL as r10buf_pool_alloc will
>                                  * have allocated it.
>                                  */
> -                               if (!need_replace)
> +                               if (!mreplace)
>                                         break;
>                                 bio->bi_next = biolist;
>                                 biolist = bio;
> --
> 2.31.1
>

Re: [PATCH v2 1/4] md/raid10: fix null-ptr-deref of mreplace in raid10_sync_request

[Yu Kuai]

Hi,

在 2023/05/27 5:38, Song Liu 写道:
> On Fri, May 26, 2023 at 12:47 AM <linan666@huaweicloud.com> wrote:                    }
> 
> To make sure I understand the issue correctly:
> 
> The null-ptr-deref only happens when the Faulty bit was set after the
> last check and before this check below, right?

Yes, you're right.

Thanks,
Kuai
> 
>> -                       if (mreplace && test_bit(Faulty, &mreplace->flags))
>> -                               mreplace = NULL;
>>                          /* Unless we are doing a full sync, or a replacement
>>                           * we only need to recover the block if it is set in
>>                           * the bitmap
> 
> Thanks,
> Song
> 

Re: [PATCH v2 1/4] md/raid10: fix null-ptr-deref of mreplace in raid10_sync_request

[Yu Kuai]

Hi,

在 2023/05/26 15:45, linan666@huaweicloud.com 写道:
> From: Li Nan <linan122@huawei.com>
> 
> need_replace will be set to 1 if no-Faulty mreplace exists, and mreplace
> will be deref later. However, the latter check of mreplace might set
> mreplace to NULL, null-ptr-deref occurs if need_replace is 1 at this time.
> 
> Fix it by merging two checks into one. And replace 'need_replace' with
> 'mreplace' because their values are always the same.
> 
> Fixes: ee37d7314a32 ("md/raid10: Fix raid10 replace hang when new added disk faulty")
> Signed-off-by: Li Nan <linan122@huawei.com>

Other than some nits below, this patch looks good to me, feel free too
add:

Reviewed-by: Yu Kuai <yukuai3@huawei.com>
> ---
>   drivers/md/raid10.c | 13 +++++--------
>   1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index 4fcfcb350d2b..e21502c03b45 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -3438,7 +3438,6 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
>   			int must_sync;
>   			int any_working;
>   			int need_recover = 0;
> -			int need_replace = 0;
>   			struct raid10_info *mirror = &conf->mirrors[i];
>   			struct md_rdev *mrdev, *mreplace;
>   
> @@ -3451,10 +3450,10 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
>   			    !test_bit(In_sync, &mrdev->flags))
>   				need_recover = 1;
>   			if (mreplace != NULL &&
> -			    !test_bit(Faulty, &mreplace->flags))
> -				need_replace = 1;
> +			    test_bit(Faulty, &mreplace->flags))
This can be keeped in one line.

> +				mreplace = NULL;
>   
> -			if (!need_recover && !need_replace) {
> +			if (!need_recover && !mreplace) {
>   				rcu_read_unlock();
>   				continue;
>   			}
> @@ -3470,8 +3469,6 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
>   				rcu_read_unlock();
>   				continue;
>   			}
> -			if (mreplace && test_bit(Faulty, &mreplace->flags))
> -				mreplace = NULL;
>   			/* Unless we are doing a full sync, or a replacement
>   			 * we only need to recover the block if it is set in
>   			 * the bitmap
> @@ -3594,11 +3591,11 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
>   				bio = r10_bio->devs[1].repl_bio;
>   				if (bio)
>   					bio->bi_end_io = NULL;
> -				/* Note: if need_replace, then bio
> +				/* Note: if replace is not NULL, then bio
>   				 * cannot be NULL as r10buf_pool_alloc will
>   				 * have allocated it.
>   				 */
> -				if (!need_replace)
> +				if (!mreplace)
>   					break;
>   				bio->bi_next = biolist;
>   				biolist = bio;
> 

Re: [PATCH v2 1/4] md/raid10: fix null-ptr-deref of mreplace in raid10_sync_request

[Li Nan]

在 2023/5/27 5:38, Song Liu 写道:
> On Fri, May 26, 2023 at 12:47 AM <linan666@huaweicloud.com> wrote:
>>
>> From: Li Nan <linan122@huawei.com>
>>
>> need_replace will be set to 1 if no-Faulty mreplace exists, and mreplace
>> will be deref later. However, the latter check of mreplace might set
>> mreplace to NULL, null-ptr-deref occurs if need_replace is 1 at this time.
>>
>> Fix it by merging two checks into one. And replace 'need_replace' with
>> 'mreplace' because their values are always the same.
>>
>> Fixes: ee37d7314a32 ("md/raid10: Fix raid10 replace hang when new added disk faulty")
>> Signed-off-by: Li Nan <linan122@huawei.com>
>> ---
>>   drivers/md/raid10.c | 13 +++++--------
>>   1 file changed, 5 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
>> index 4fcfcb350d2b..e21502c03b45 100644
>> --- a/drivers/md/raid10.c
>> +++ b/drivers/md/raid10.c
>> @@ -3438,7 +3438,6 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
>>                          int must_sync;
>>                          int any_working;
>>                          int need_recover = 0;
>> -                       int need_replace = 0;
>>                          struct raid10_info *mirror = &conf->mirrors[i];
>>                          struct md_rdev *mrdev, *mreplace;
>>
>> @@ -3451,10 +3450,10 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
>>                              !test_bit(In_sync, &mrdev->flags))
>>                                  need_recover = 1;
>>                          if (mreplace != NULL &&
>> -                           !test_bit(Faulty, &mreplace->flags))
>> -                               need_replace = 1;
>> +                           test_bit(Faulty, &mreplace->flags))
>> +                               mreplace = NULL;
>>
>> -                       if (!need_recover && !need_replace) {
>> +                       if (!need_recover && !mreplace) {
>>                                  rcu_read_unlock();
>>                                  continue;
>>                          }
>> @@ -3470,8 +3469,6 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
>>                                  rcu_read_unlock();
>>                                  continue;
>>                          }
> 
> To make sure I understand the issue correctly:
> 
> The null-ptr-deref only happens when the Faulty bit was set after the
> last check and before this check below, right?
> 

Yes. I will improve log in next version.

-- 
Thanks,
Nan

Re: [PATCH v2 1/4] md/raid10: fix null-ptr-deref of mreplace in raid10_sync_request

[Li Nan]

在 2023/5/27 9:21, Yu Kuai 写道:
> Hi,
> 
> 在 2023/05/26 15:45, linan666@huaweicloud.com 写道:
>> From: Li Nan <linan122@huawei.com>
>>
>> need_replace will be set to 1 if no-Faulty mreplace exists, and mreplace
>> will be deref later. However, the latter check of mreplace might set
>> mreplace to NULL, null-ptr-deref occurs if need_replace is 1 at this 
>> time.
>>
>> Fix it by merging two checks into one. And replace 'need_replace' with
>> 'mreplace' because their values are always the same.
>>
>> Fixes: ee37d7314a32 ("md/raid10: Fix raid10 replace hang when new 
>> added disk faulty")
>> Signed-off-by: Li Nan <linan122@huawei.com>
> 
> Other than some nits below, this patch looks good to me, feel free too
> add:
> 
> Reviewed-by: Yu Kuai <yukuai3@huawei.com>
>> ---
>>   drivers/md/raid10.c | 13 +++++--------
>>   1 file changed, 5 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
>> index 4fcfcb350d2b..e21502c03b45 100644
>> --- a/drivers/md/raid10.c
>> +++ b/drivers/md/raid10.c
>> @@ -3438,7 +3438,6 @@ static sector_t raid10_sync_request(struct mddev 
>> *mddev, sector_t sector_nr,
>>               int must_sync;
>>               int any_working;
>>               int need_recover = 0;
>> -            int need_replace = 0;
>>               struct raid10_info *mirror = &conf->mirrors[i];
>>               struct md_rdev *mrdev, *mreplace;
>> @@ -3451,10 +3450,10 @@ static sector_t raid10_sync_request(struct 
>> mddev *mddev, sector_t sector_nr,
>>                   !test_bit(In_sync, &mrdev->flags))
>>                   need_recover = 1;
>>               if (mreplace != NULL &&
>> -                !test_bit(Faulty, &mreplace->flags))
>> -                need_replace = 1;
>> +                test_bit(Faulty, &mreplace->flags))
> This can be keeped in one line.
> 

OK, I will change it.
Thanks for your review.

-- 
Thanks,
Nan

Re: [PATCH v2 2/4] md/raid10: improve code of mrdev in raid10_sync_request

[Yu Kuai]

在 2023/05/26 15:45, linan666@huaweicloud.com 写道:
> From: Li Nan <linan122@huawei.com>
> 
> 'need_recover' and 'mrdev' are equivalent in raid10_sync_request(), and
> inc mrdev->nr_pending is unreasonable if don't need recovery. Replace
> 'need_recover' with 'mrdev', and only inc nr_pending when needed.

LGTM, feel free to add:

Reviewed-by: Yu Kuai <yukuai3@huawei.com>
> 
> Suggested-by: Yu Kuai <yukuai3@huawei.com>
> Signed-off-by: Li Nan <linan122@huawei.com>
> ---
>   drivers/md/raid10.c | 22 ++++++++++++----------
>   1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index e21502c03b45..9de9eabff209 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -3437,7 +3437,6 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
>   			sector_t sect;
>   			int must_sync;
>   			int any_working;
> -			int need_recover = 0;
>   			struct raid10_info *mirror = &conf->mirrors[i];
>   			struct md_rdev *mrdev, *mreplace;
>   
> @@ -3446,14 +3445,14 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
>   			mreplace = rcu_dereference(mirror->replacement);
>   
>   			if (mrdev != NULL &&
> -			    !test_bit(Faulty, &mrdev->flags) &&
> -			    !test_bit(In_sync, &mrdev->flags))
> -				need_recover = 1;
> +			    (test_bit(Faulty, &mrdev->flags) ||
> +			    test_bit(In_sync, &mrdev->flags)))
> +				mrdev = NULL;
>   			if (mreplace != NULL &&
>   			    test_bit(Faulty, &mreplace->flags))
>   				mreplace = NULL;
>   
> -			if (!need_recover && !mreplace) {
> +			if (!mrdev && !mreplace) {
>   				rcu_read_unlock();
>   				continue;
>   			}
> @@ -3487,7 +3486,8 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
>   				rcu_read_unlock();
>   				continue;
>   			}
> -			atomic_inc(&mrdev->nr_pending);
> +			if (mrdev)
> +				atomic_inc(&mrdev->nr_pending);
>   			if (mreplace)
>   				atomic_inc(&mreplace->nr_pending);
>   			rcu_read_unlock();
> @@ -3574,7 +3574,7 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
>   				r10_bio->devs[1].devnum = i;
>   				r10_bio->devs[1].addr = to_addr;
>   
> -				if (need_recover) {
> +				if (mrdev) {
>   					bio = r10_bio->devs[1].bio;
>   					bio->bi_next = biolist;
>   					biolist = bio;
> @@ -3619,7 +3619,7 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
>   					for (k = 0; k < conf->copies; k++)
>   						if (r10_bio->devs[k].devnum == i)
>   							break;
> -					if (!test_bit(In_sync,
> +					if (mrdev && !test_bit(In_sync,
>   						      &mrdev->flags)
>   					    && !rdev_set_badblocks(
>   						    mrdev,
> @@ -3645,12 +3645,14 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
>   				if (rb2)
>   					atomic_dec(&rb2->remaining);
>   				r10_bio = rb2;
> -				rdev_dec_pending(mrdev, mddev);
> +				if (mrdev)
> +					rdev_dec_pending(mrdev, mddev);
>   				if (mreplace)
>   					rdev_dec_pending(mreplace, mddev);
>   				break;
>   			}
> -			rdev_dec_pending(mrdev, mddev);
> +			if (mrdev)
> +				rdev_dec_pending(mrdev, mddev);
>   			if (mreplace)
>   				rdev_dec_pending(mreplace, mddev);
>   			if (r10_bio->devs[0].bio->bi_opf & MD_FAILFAST) {
>