[PATCH 1/2] Revert "md/raid5: Wait for MD_SB_CHANGE_PENDING in raid5d"

[PATCH 1/2] Revert "md/raid5: Wait for MD_SB_CHANGE_PENDING in raid5d"

[Junxiao Bi]

This reverts commit 5e2cf333b7bd5d3e62595a44d598a254c697cd74.

That commit introduced the following race and can cause system hung.

 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);
                                                                                        >>> 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);

Next patch will fix the issue that reverted commit is fixing in a new way.

Fixes: 5e2cf333b7bd ("md/raid5: Wait for MD_SB_CHANGE_PENDING in raid5d")
Signed-off-by: Junxiao Bi <junxiao.bi@oracle.com>
---
 drivers/md/raid5.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index dc031d42f53b..fcc8a44dd4fd 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -36,7 +36,6 @@
  */
 
 #include <linux/blkdev.h>
-#include <linux/delay.h>
 #include <linux/kthread.h>
 #include <linux/raid/pq.h>
 #include <linux/async_tx.h>
@@ -6820,18 +6819,7 @@ static void raid5d(struct md_thread *thread)
 			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,
-			!test_bit(MD_SB_CHANGE_PENDING, &mddev->sb_flags),
-			conf->device_lock);
 	}
 	pr_debug("%d stripes handled\n", handled);
 
-- 
2.39.3 (Apple Git-145)

[PATCH 2/2] md: bypass block throttle for superblock update

[Junxiao Bi]

commit 5e2cf333b7bd ("md/raid5: Wait for MD_SB_CHANGE_PENDING in raid5d")
introduced a hung bug and got reverted in last patch, since the issue
that commit is fixing is due to md superblock write is throttled by wbt,
to fix it, we can have superblock write bypass block layer throttle.

Fixes: 5e2cf333b7bd ("md/raid5: Wait for MD_SB_CHANGE_PENDING in raid5d")
Suggested-by: Yu Kuai <yukuai1@huaweicloud.com>
Signed-off-by: Junxiao Bi <junxiao.bi@oracle.com>
---
 drivers/md/md.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 4ee4593c874a..7a5a22097365 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -1013,9 +1013,10 @@ void md_super_write(struct mddev *mddev, struct md_rdev *rdev,
 		return;
 
 	bio = bio_alloc_bioset(rdev->meta_bdev ? rdev->meta_bdev : rdev->bdev,
-			       1,
-			       REQ_OP_WRITE | REQ_SYNC | REQ_PREFLUSH | REQ_FUA,
-			       GFP_NOIO, &mddev->sync_set);
+			      1,
+			      REQ_OP_WRITE | REQ_SYNC | REQ_IDLE | REQ_META
+				  | REQ_PREFLUSH | REQ_FUA,
+			      GFP_NOIO, &mddev->sync_set);
 
 	atomic_inc(&rdev->nr_pending);
 
-- 
2.39.3 (Apple Git-145)

Re: [PATCH 2/2] md: bypass block throttle for superblock update

[Logan Gunthorpe]

On 2023-11-07 10:57, Junxiao Bi wrote:
> commit 5e2cf333b7bd ("md/raid5: Wait for MD_SB_CHANGE_PENDING in raid5d")
> introduced a hung bug and got reverted in last patch, since the issue
> that commit is fixing is due to md superblock write is throttled by wbt,
> to fix it, we can have superblock write bypass block layer throttle.
> 
> Fixes: 5e2cf333b7bd ("md/raid5: Wait for MD_SB_CHANGE_PENDING in raid5d")
> Suggested-by: Yu Kuai <yukuai1@huaweicloud.com>
> Signed-off-by: Junxiao Bi <junxiao.bi@oracle.com>

This makes sense to me. However, I haven't looked at that bug in a long
time though and I haven't tested the proposed solution to ensure the bug
is indeed fixed.

Would it not make sense to have the fixing commit first before the
revert? So there isn't a spot in the history with a known bug?

Besides that,

Reviewed-by: Logan Gunthorpe <logang@deltatee.com>

Thanks!

Logan

Re: [PATCH 2/2] md: bypass block throttle for superblock update

[junxiao.bi]

On 11/7/23 11:33 AM, Logan Gunthorpe wrote:

>
> On 2023-11-07 10:57, Junxiao Bi wrote:
>> commit 5e2cf333b7bd ("md/raid5: Wait for MD_SB_CHANGE_PENDING in raid5d")
>> introduced a hung bug and got reverted in last patch, since the issue
>> that commit is fixing is due to md superblock write is throttled by wbt,
>> to fix it, we can have superblock write bypass block layer throttle.
>>
>> Fixes: 5e2cf333b7bd ("md/raid5: Wait for MD_SB_CHANGE_PENDING in raid5d")
>> Suggested-by: Yu Kuai <yukuai1@huaweicloud.com>
>> Signed-off-by: Junxiao Bi <junxiao.bi@oracle.com>
> This makes sense to me. However, I haven't looked at that bug in a long
> time though and I haven't tested the proposed solution to ensure the bug
> is indeed fixed.
>
> Would it not make sense to have the fixing commit first before the
> revert? So there isn't a spot in the history with a known bug?

Yea, that makes sense.

Thanks for the review.

>
> Besides that,
>
> Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
>
> Thanks!
>
> Logan