Hi,

在 2023/03/07 17:53, Paul Menzel 写道:

Dear Yu,


Thank you for your patch.

Am 07.03.23 um 03:27 schrieb Yu Kuai:

quoted

From: Yu Kuai <redacted>

There is a small typo in the prefix of the commit message summary: raid10.

It also seems common to use md/raid10 as prefix.

quoted

raid10_sync_request() will add 'r10bio->remaining' for both rdev and
replacement rdev. However, if the read io failed,

fails (present tense for problem description/summary)

quoted

recovery_request_write() will return without issuring the write io, in

1.  return*s*
2.  assuring?

quoted

this case, end_sync_request() is only called once and 'remaining' is
leaked, which will cause io hang.

leaked, causing an io hang.

quoted

Fix the probleming by decreasing 'remaining' according to if 'bio' and

problem

Thanks for those advices, I'll send a new version after code changes is
reviewed.

Thanks,
Kuai

quoted

'repl_bio' is valid.

Fixes: 24afd80d99f8 ("md/raid10: handle recovery of replacement 
devices.")
Signed-off-by: Yu Kuai <redacted>
---
  drivers/md/raid10.c | 23 +++++++++++++----------
  1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index a8b5fecef136..f7002a1aa9cf 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c

@@ -2611,11 +2611,22 @@ static void recovery_request_write(struct 

mddev *mddev, struct r10bio *r10_bio)
  {
      struct r10conf *conf = mddev->private;
      int d;
-    struct bio *wbio, *wbio2;
+    struct bio *wbio = r10_bio->devs[1].bio;
+    struct bio *wbio2 = r10_bio->devs[1].repl_bio;
+
+    /* Need to test wbio2->bi_end_io before we call
+     * submit_bio_noacct as if the former is NULL,
+     * the latter is free to free wbio2.
+     */
+    if (wbio2 && !wbio2->bi_end_io)
+        wbio2 = NULL;
      if (!test_bit(R10BIO_Uptodate, &r10_bio->state)) {
          fix_recovery_read_error(r10_bio);
-        end_sync_request(r10_bio);
+        if (wbio->bi_end_io)
+            end_sync_request(r10_bio);
+        if (wbio2)
+            end_sync_request(r10_bio);
          return;
      }

@@ -2624,14 +2635,6 @@ static void recovery_request_write(struct mddev 

*mddev, struct r10bio *r10_bio)
       * and submit the write request
       */
      d = r10_bio->devs[1].devnum;
-    wbio = r10_bio->devs[1].bio;
-    wbio2 = r10_bio->devs[1].repl_bio;
-    /* Need to test wbio2->bi_end_io before we call
-     * submit_bio_noacct as if the former is NULL,
-     * the latter is free to free wbio2.
-     */
-    if (wbio2 && !wbio2->bi_end_io)
-        wbio2 = NULL;
      if (wbio->bi_end_io) {
          atomic_inc(&conf->mirrors[d].rdev->nr_pending);
          md_sync_acct(conf->mirrors[d].rdev->bdev, bio_sectors(wbio));

Kind regards,

Paul

.