Skip to contentSkip to conversation

md: retry bailed bio

PATCH In review Shaohua Li 2 participants 5 messages Sep 21, 2017 – Sep 28, 2017

Subsystems (3)
Stable branch MD RAID Device mapper
lore
Share & tools
lore

Shaohua Li

author
#0 lore raw

cover md: retry bailed bio

From: Shaohua Li <[email protected]>

With cc27b0c78c79(md: fix deadlock between mddev_suspend() and md_write_start()), pers->make_request could bail out without handling the bio. If that happens, we should retry. There are two call sites we don't do the retry and cause problems. This set fixes the issues.

Thanks,
Shaohua

Shaohua Li (3):
md: separate request handling
md: fix a race condition for flush request handling
dm-raid: fix a race condition in request handling

 drivers/md/dm-raid.c |  2 +-
 drivers/md/md.c      | 72 +++++++++++++++++++++++++++++++---------------------
 drivers/md/md.h      |  1 +
 3 files changed, 45 insertions(+), 30 deletions(-)
Signature · 1 line
2.11.0
author
#1 lore raw

PATCH 1/3 md: separate request handling

From: Shaohua Li <[email protected]>

With commit cc27b0c78c79, pers->make_request could bail out without handling the bio. If that happens, we should retry. The commit fixes md_make_request but not other call sites. Separate the request handling part, so other call sites can use it.

Reported-by Nate Dailey <[email protected]>

Fix: cc27b0c78c79(md: fix deadlock between mddev_suspend() and md_write_start())

Cc NeilBrown <[email protected]>
Cc [email protected]
Signed-off-by Shaohua Li <[email protected]>
---
 drivers/md/md.c | 58 ++++++++++++++++++++++++++++++++-------------------------
 drivers/md/md.h |  1 +
 2 files changed, 34 insertions(+), 25 deletions(-)
drivers/md/md.c +33 -25
index 08fcaebc61bd..1db1a22ed835 100644
@@ -266,6 +266,37 @@ static DEFINE_SPINLOCK(all_mddevs_lock);
  * call has finished, the bio has been linked into some internal structure
  * and so is visible to ->quiesce(), so we don't need the refcount any more.
  */
+void md_handle_request(struct mddev *mddev, struct bio *bio)
+{
+check_suspended:
+	rcu_read_lock();
+	if (mddev->suspended) {
+		DEFINE_WAIT(__wait);
+		for (;;) {
+			prepare_to_wait(&mddev->sb_wait, &__wait,
+					TASK_UNINTERRUPTIBLE);
+			if (!mddev->suspended)
+				break;
+			rcu_read_unlock();
+			schedule();
+			rcu_read_lock();
+		}
+		finish_wait(&mddev->sb_wait, &__wait);
+	}
+	atomic_inc(&mddev->active_io);
+	rcu_read_unlock();
+
+	if (!mddev->pers->make_request(mddev, bio)) {
+		atomic_dec(&mddev->active_io);
+		wake_up(&mddev->sb_wait);
+		goto check_suspended;
+	}
+
+	if (atomic_dec_and_test(&mddev->active_io) && mddev->suspended)
+		wake_up(&mddev->sb_wait);
+}
+EXPORT_SYMBOL(md_handle_request);
+
 static blk_qc_t md_make_request(struct request_queue *q, struct bio *bio)
 {
 	const int rw = bio_data_dir(bio);

@@ -285,23 +316,6 @@ static blk_qc_t md_make_request(struct request_queue *q, struct bio *bio)
 		bio_endio(bio);
 		return BLK_QC_T_NONE;
 	}
-check_suspended:
-	rcu_read_lock();
-	if (mddev->suspended) {
-		DEFINE_WAIT(__wait);
-		for (;;) {
-			prepare_to_wait(&mddev->sb_wait, &__wait,
-					TASK_UNINTERRUPTIBLE);
-			if (!mddev->suspended)
-				break;
-			rcu_read_unlock();
-			schedule();
-			rcu_read_lock();
-		}
-		finish_wait(&mddev->sb_wait, &__wait);
-	}
-	atomic_inc(&mddev->active_io);
-	rcu_read_unlock();
 
 	/*
 	 * save the sectors now since our bio can

@@ -310,20 +324,14 @@ static blk_qc_t md_make_request(struct request_queue *q, struct bio *bio)
 	sectors = bio_sectors(bio);
 	/* bio could be mergeable after passing to underlayer */
 	bio->bi_opf &= ~REQ_NOMERGE;
-	if (!mddev->pers->make_request(mddev, bio)) {
-		atomic_dec(&mddev->active_io);
-		wake_up(&mddev->sb_wait);
-		goto check_suspended;
-	}
+
+	md_handle_request(mddev, bio);
 
 	cpu = part_stat_lock();
 	part_stat_inc(cpu, &mddev->gendisk->part0, ios[rw]);
 	part_stat_add(cpu, &mddev->gendisk->part0, sectors[rw], sectors);
 	part_stat_unlock();
 
-	if (atomic_dec_and_test(&mddev->active_io) && mddev->suspended)
-		wake_up(&mddev->sb_wait);
-
 	return BLK_QC_T_NONE;
 }
drivers/md/md.h +1 -0
index 561d22b9a9a8..d8287d3cd1bf 100644
@@ -692,6 +692,7 @@ extern void md_stop_writes(struct mddev *mddev);
 extern int md_rdev_init(struct md_rdev *rdev);
 extern void md_rdev_clear(struct md_rdev *rdev);
 
+extern void md_handle_request(struct mddev *mddev, struct bio *bio);
 extern void mddev_suspend(struct mddev *mddev);
 extern void mddev_resume(struct mddev *mddev);
 extern struct bio *bio_alloc_mddev(gfp_t gfp_mask, int nr_iovecs,
Signature · 1 line
2.11.0
author
#2 lore raw

PATCH 2/3 md: fix a race condition for flush request handling

In reply to Shaohua Li

From: Shaohua Li <[email protected]>

md_submit_flush_data calls pers->make_request, which missed the suspend check. Fix it with the new md_handle_request API.

Reported-by Nate Dailey <[email protected]>
Tested-by Nate Dailey <[email protected]>

Fix: cc27b0c78c79(md: fix deadlock between mddev_suspend() and md_write_start())

Cc NeilBrown <[email protected]>
Cc [email protected]
Signed-off-by Shaohua Li <[email protected]>
---
 drivers/md/md.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)
drivers/md/md.c +10 -4
index 1db1a22ed835..0ff1bbf6c90e 100644
@@ -447,16 +447,22 @@ static void md_submit_flush_data(struct work_struct *ws)
 	struct mddev *mddev = container_of(ws, struct mddev, flush_work);
 	struct bio *bio = mddev->flush_bio;
 
+	/*
+	 * must reset flush_bio before calling into md_handle_request to avoid a
+	 * deadlock, because other bios passed md_handle_request suspend check
+	 * could wait for this and below md_handle_request could wait for those
+	 * bios because of suspend check
+	 */
+	mddev->flush_bio = NULL;
+	wake_up(&mddev->sb_wait);
+
 	if (bio->bi_iter.bi_size == 0)
 		/* an empty barrier - all done */
 		bio_endio(bio);
 	else {
 		bio->bi_opf &= ~REQ_PREFLUSH;
-		mddev->pers->make_request(mddev, bio);
+		md_handle_request(mddev, bio);
 	}
-
-	mddev->flush_bio = NULL;
-	wake_up(&mddev->sb_wait);
 }
 
 void md_flush_request(struct mddev *mddev, struct bio *bio)
Signature · 1 line
2.11.0
author
#3 lore raw

PATCH 3/3 dm-raid: fix a race condition in request handling

In reply to Shaohua Li

From: Shaohua Li <[email protected]>

raid_map calls pers->make_request, which missed the suspend check. Fix it with the new md_handle_request API.

Fix: cc27b0c78c79(md: fix deadlock between mddev_suspend() and md_write_start())

Cc NeilBrown <[email protected]>
Cc Heinz Mauelshagen <[email protected]>
Cc Mike Snitzer <[email protected]>
Cc [email protected]
Signed-off-by Shaohua Li <[email protected]>
---
 drivers/md/dm-raid.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/md/dm-raid.c +1 -1
index 5bfe285ea9d1..1ac58c5651b7 100644
@@ -3238,7 +3238,7 @@ static int raid_map(struct dm_target *ti, struct bio *bio)
 	if (unlikely(bio_end_sector(bio) > mddev->array_sectors))
 		return DM_MAPIO_REQUEUE;
 
-	mddev->pers->make_request(mddev, bio);
+	md_handle_request(mddev, bio);
 
 	return DM_MAPIO_SUBMITTED;
 }
Signature · 1 line
2.11.0
Reviewed-by on the v1 series
#4 lore raw
In reply to Shaohua Li
Quoted 6 lines · Shaohua Li

From: Shaohua Li <[email protected]>

With cc27b0c78c79(md: fix deadlock between mddev_suspend() and md_write_start()), pers->make_request could bail out without handling the bio. If that happens, we should retry. There are two call sites we don't do the retry and cause problems. This set fixes the issues.

All look good to me, thanks.

Reviewed-by NeilBrown <[email protected]>

NeilBrown

Quoted 21 lines

Thanks,
Shaohua

Shaohua Li (3):
md: separate request handling
md: fix a race condition for flush request handling
dm-raid: fix a race condition in request handling

 drivers/md/dm-raid.c |  2 +-
 drivers/md/md.c      | 72 +++++++++++++++++++++++++++++++---------------------
 drivers/md/md.h      |  1 +
 3 files changed, 45 insertions(+), 30 deletions(-)
Signature · 6 lines
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html