On 11/1/23 6:24 PM, Yu Kuai wrote:

Hi,

在 2023/11/02 7:02, Junxiao Bi 写道:

quoted

Looks like there is a race between md_write_start() and raid5d() which

Is this a real issue or just based on code review?

It's a real issue, we see this hung in a production system, it's with 
v5.4, but i didn't see these function has much difference.

crash> bt 2683
PID: 2683     TASK: ffff9d3b3e651f00  CPU: 65   COMMAND: "md0_raid5"
  #0 [ffffbd7a0252bd08] __schedule at ffffffffa8e68931
  #1 [ffffbd7a0252bd88] schedule at ffffffffa8e68c6f
  #2 [ffffbd7a0252bda8] raid5d at ffffffffc0b4df16 [raid456]
  #3 [ffffbd7a0252bea0] md_thread at ffffffffa8bc20b8
  #4 [ffffbd7a0252bf08] kthread at ffffffffa84dac05
  #5 [ffffbd7a0252bf50] ret_from_fork at ffffffffa9000364
crash> ps -m 2683
[ 0 00:11:08.244] [UN]  PID: 2683     TASK: ffff9d3b3e651f00  CPU: 65   
COMMAND: "md0_raid5"
crash>
crash> bt 96352
PID: 96352    TASK: ffff9cc587c95d00  CPU: 64   COMMAND: "kworker/64:0"
  #0 [ffffbd7a07533be0] __schedule at ffffffffa8e68931
  #1 [ffffbd7a07533c60] schedule at ffffffffa8e68c6f
  #2 [ffffbd7a07533c80] md_write_start at ffffffffa8bc47c5
  #3 [ffffbd7a07533ce0] raid5_make_request at ffffffffc0b4a4c9 [raid456]
  #4 [ffffbd7a07533dc8] md_handle_request at ffffffffa8bbfa54
  #5 [ffffbd7a07533e38] md_submit_flush_data at ffffffffa8bc04c1
  #6 [ffffbd7a07533e60] process_one_work at ffffffffa84d4289
  #7 [ffffbd7a07533ea8] worker_thread at ffffffffa84d50cf
  #8 [ffffbd7a07533f08] kthread at ffffffffa84dac05
  #9 [ffffbd7a07533f50] ret_from_fork at ffffffffa9000364
crash> ps -m 96352
[ 0 00:11:08.244] [UN]  PID: 96352    TASK: ffff9cc587c95d00  CPU: 64   
COMMAND: "kworker/64:0"
crash>
crash> bt 25542
PID: 25542    TASK: ffff9cb4cb528000  CPU: 32   COMMAND: "md0_resync"
  #0 [ffffbd7a09387c80] __schedule at ffffffffa8e68931
  #1 [ffffbd7a09387d00] schedule at ffffffffa8e68c6f
  #2 [ffffbd7a09387d20] md_do_sync at ffffffffa8bc613e
  #3 [ffffbd7a09387ea0] md_thread at ffffffffa8bc20b8
  #4 [ffffbd7a09387f08] kthread at ffffffffa84dac05
  #5 [ffffbd7a09387f50] ret_from_fork at ffffffffa9000364
crash>
crash> ps -m 25542
[ 0 00:11:18.370] [UN]  PID: 25542    TASK: ffff9cb4cb528000  CPU: 32   
COMMAND: "md0_resync"

quoted

can cause deadlock. Run into this issue while raid_check is running.

md_write_start: raid5d:
  if (mddev->safemode == 1)
      mddev->safemode = 0;
  /* sync_checkers is always 0 when writes_pending is in per-cpu mode */
  if (mddev->in_sync || mddev->sync_checkers) {
      spin_lock(&mddev->lock);
      if (mddev->in_sync) {
          mddev->in_sync = 0;
          set_bit(MD_SB_CHANGE_CLEAN, &mddev->sb_flags);
          set_bit(MD_SB_CHANGE_PENDING, &mddev->sb_flags);

quoted

quoted

quoted

running before md_write_start wake up it

if (mddev->sb_flags & ~(1 << MD_SB_CHANGE_PENDING)) {
spin_unlock_irq(&conf->device_lock);
md_check_recovery(mddev);
spin_lock_irq(&conf->device_lock);

/*
* Waiting on MD_SB_CHANGE_PENDING below may deadlock
* seeing md_check_recovery() is needed to clear
* the flag when using mdmon.
*/
continue;
}

wait_event_lock_irq(mddev->sb_wait, >>>>>>>>>>> hung
!test_bit(MD_SB_CHANGE_PENDING, &mddev->sb_flags),
conf->device_lock);
          md_wakeup_thread(mddev->thread);
          did_change = 1;
      }
      spin_unlock(&mddev->lock);
  }

  ...

  wait_event(mddev->sb_wait, >>>>>>>>>> hung
     !test_bit(MD_SB_CHANGE_PENDING, &mddev->sb_flags) ||
     mddev->suspended);

This is not correct, if daemon thread is running, md_wakeup_thread()
will make sure that daemon thread will run again, see details how
THREAD_WAKEUP worked in md_thread().

The daemon thread was waiting MD_SB_CHANGE_PENDING to be cleared, even 
wake up it, it will hung again as that flag is still not cleared?

Thanks,

Junxiao.

Thanks,
Kuai

quoted

commit 5e2cf333b7bd ("md/raid5: Wait for MD_SB_CHANGE_PENDING in 
raid5d")
introduced this issue, since it want to a reschedule for flushing 
blk_plug,
let do it explicitly to address that issue.

Fixes: 5e2cf333b7bd ("md/raid5: Wait for MD_SB_CHANGE_PENDING in 
raid5d")
Signed-off-by: Junxiao Bi <redacted>
---
  block/blk-core.c   | 1 +
  drivers/md/raid5.c | 9 +++++----
  2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 9d51e9894ece..bc8757a78477 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c

@@ -1149,6 +1149,7 @@ void __blk_flush_plug(struct blk_plug *plug, 

bool from_schedule)
      if (unlikely(!rq_list_empty(plug->cached_rq)))
          blk_mq_free_plug_rqs(plug);
  }
+EXPORT_SYMBOL(__blk_flush_plug);
    /**
   * blk_finish_plug - mark the end of a batch of submitted I/O

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 284cd71bcc68..25ec82f2813f 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c

@@ -6850,11 +6850,12 @@ static void raid5d(struct md_thread *thread)

               * the flag when using mdmon.
               */
              continue;
+        } else {
+            spin_unlock_irq(&conf->device_lock);
+            blk_flush_plug(current);
+            cond_resched();
+            spin_lock_irq(&conf->device_lock);
          }
-
-        wait_event_lock_irq(mddev->sb_wait,
-            !test_bit(MD_SB_CHANGE_PENDING, &mddev->sb_flags),
-            conf->device_lock);
      }
      pr_debug("%d stripes handled\n", handled);