raid1: Rewrite the implementation of iobarrier.

list_splice_init(NoIO2, NoIO3);
current_window_requests = next_window_requests;
next_window_requests = 0;

list_for_each_entry(NoIO3){
      clear_bit(NoIO2_FLAG),
      set_bit(NoIO3_FLAG,
}

There is a iobarrier in raid1 because the contend between normalIO and resyncIO.It suspend all normal IO when reysync.
But if normalIO is outrange the resync windwos,there is no contend. So I rewrite the iobarrier.

I patition the whole space into five parts.

|---------|-----------|------------|----------------|-------|
         Pa    next_resync   start_next_window      Pb

Firstly i introduce some concepts:
1:RESYNC_WINDOW: For resync, there are 32 sync requests at most.A sync request is RESYNC_BLOCK_SIZE(64*1024).So the RESYNC_WINDOW is 32 * RESYNC_BLOCK_SIZE, that is 2MB.
2:NEXT_NORMALIO_DISTANCE: it indicate the distance between next_resync and start_next_window.It also indicate the distance between start_next_window and Pb.Now it is 3 * RESYNC_WINDOW_SIZE.It can tune. 3:next_resync mean the next start sector which will being do sync.
4:Pa means a position which before RESYNC_WINDOW distance from next_resync.
5:start_next_window means a position which after NEXT_NORMALIO_DISTANCE distance from next_resync.For normalio after this position,it can't wait resyncio 6:Pb means a position which after 2 * NEXT_NORMALIO_DISTANCE distance from next_resync.
7:current_window_requests means the count of normalIO between Pb and start_next_window.
8:next_window_requests means the count of nonmalIO after Pb.

NormalIO will be partitioned into four types:
NoIO1: it means the end sector of bio is smaller or equal the Pa.
NoIO2: it means the start sector of bio larger or equal Pb
NoIO3: it means the start sector of bio larger or equal start_next_window.
NoIO4: it means the location between Pa and start_next_window

For NoIO1,it don't use iobarrier.
For NoIO4,it used the original iobarrier mechanism.The nornalIO and syncIO must be contend.
For NoIO2/3, i add two filed in struct r1conf "current_window_requests, next_window_requests".They indicate the count of two types. For those, they don't wait.They only add the relevant parameter.

For resync action, if there are NoIO4s, it must be wait.If not,it forward.But if current_window_requests > 0 and sync action reached to the start_next_window,it must wait until the current_window_requests became zero.If current_window_requests became zero,the start_next_window also forward. The current_window_requests will replaced by next_window_requests.

There is a problem which when and how to change from NoIO2 to NoIO3.Only so, the sync action can forward.

I add a filed in struct r1conf "start_next_window"..What condition will change?
A: if start_next_window == MaxSector, it means there are no NoIO2/3.
So start_next_window = next_resync + NEXT_NORMALIO_DISTANCE
B: if current_window_requests == 0 && next_window_requests != 0, it
means start_next_window move to Pb

There is anthor problem which how to differentiate when NoIO2 became NoIO3.
For example, there are many bios which are NoIO2 and a bio which is NoIO3. NoIO3 firstly completed,so the bios of NoIO2 became NoIO3. At generil,we should use flags and list_head. The codes should:

Quoted 7 lines

list_splice_init(NoIO2, NoIO3);
current_window_requests = next_window_requests;
next_window_requests = 0;

list_for_each_entry(NoIO3){
      clear_bit(NoIO2_FLAG),
      set_bit(NoIO3_FLAG,
}

If there are many NoIO2, it will take too long in list_for_each_entry. Avoid this, i add a filed in struct r1bio "start_next_window".
I used this to record the position conf->start_next_window when call wait_barrier in func make_request.
For NoIO1/4, the value is zero.
In func allow_barrier, it check the conf->start_next_window
If r1bio->stat_next_window == conf->start_next_window,it mean
there is no transition which NoIO2 to NoIO3.In this condtion
if bio->bi_sector > conf->start_next_window + NEXT_NORMALIO_DISTANCE

    it means the bio is NoIO2;

else
it means the bio si NoIO3.

If r1bio->start_next_window != conf->start_next_window,it mean there is a trnasiton which NoIO2 to NoIO3.Because there is at most one transtions.So it only means the bio is NoIO2.

For one bio,there are many r1bio.So we make sure the r1bio->start_next_window is the same value.
If we met blocked_dev, it must call allow_barrier and wait_barrier. So the first and the second value of conf->start_next_window will change.If there are many r1bio's with differnet start_next_window. For the relevant bio, it depent on the last value of r1bio.
It will cause error.To avoid this, we must wait previous r1bios completes.

Signed-off-by Jianpeng Ma <[email protected]>

---
 drivers/md/raid1.c | 120 +++++++++++++++++++++++++++++++++++++++++++++++------
 drivers/md/raid1.h |  14 +++++++
 2 files changed, 121 insertions(+), 13 deletions(-)

drivers/md/raid1.c +4 -1

index 08f076a..4499870 100644
@@ -66,7 +66,8 @@
  */
 static int max_queued_requests = 1024;
 
-static void allow_barrier(struct r1conf *conf);
+static void allow_barrier(struct r1conf *conf, sector_t start_next_window,
+				sector_t bi_sector);
 static void lower_barrier(struct r1conf *conf);
 
 static void * r1bio_pool_alloc(gfp_t gfp_flags, void *data)

@@ -227,6 +228,8 @@ static void call_bio_endio(struct r1bio *r1_bio)
 	struct bio *bio = r1_bio->master_bio;
 	int done;
 	struct r1conf *conf = r1_bio->mddev->private;
+	sector_t start_next_window = r1_bio->start_next_window;
+	sector_t bi_sector = bio->bi_sector;
 
 	if (bio->bi_phys_segments) {
 		unsigned long flags;

drivers/md/raid1.c +1 -1

 	} else
 		done = 1;
 

@@ -245,7 +253,7 @@ static void call_bio_endio(struct r1bio *r1_bio)
 		 * Wake up any possible resync thread that waits for the device
 		 * to go idle.
 		 */
-		allow_barrier(conf);
+		allow_barrier(conf, start_next_window, bi_sector);
 	}
 }
 

@@ -827,10 +835,19 @@ static void raise_barrier(struct r1conf *conf)
 	/* block any new IO from starting */
 	conf->barrier++;

drivers/md/raid1.c +19 -1

 			    conf->resync_lock);
 
 	spin_unlock_irq(&conf->resync_lock);

@@ -846,10 +863,43 @@ static void lower_barrier(struct r1conf *conf)
 	wake_up(&conf->wait_barrier);
 }
 
-static void wait_barrier(struct r1conf *conf)
+static bool need_to_wait_for_sync(struct r1conf *conf, struct bio *bio)
 {
+	bool wait = false;
+
+	if (conf->array_frozen || !bio)
+		wait = true;
+	else if (conf->barrier && bio_data_dir(bio) == WRITE) {
+		if (conf->next_resync < RESYNC_WINDOW_SECTORS)
+			wait = true;
+		else if (conf->next_resync - RESYNC_WINDOW_SECTORS >=
+				bio_end_sector(bio))
+			wait = false;
+		else if (conf->next_resync + NEXT_NORMALIO_DISTANCE
+				<= bio->bi_sector) {
+			if (conf->start_next_window == MaxSector)
+				conf->start_next_window =
+					conf->next_resync +
+						NEXT_NORMALIO_DISTANCE;
+
+			if ((conf->start_next_window + NEXT_NORMALIO_DISTANCE)

drivers/md/raid1.c +38 -4

+
+static sector_t wait_barrier(struct r1conf *conf, struct bio *bio)
+{
+	sector_t sector = 0;
+
 	spin_lock_irq(&conf->resync_lock);
-	if (conf->barrier) {
+	if (need_to_wait_for_sync(conf, bio)) {
 		conf->nr_waiting++;
 		/* Wait for the barrier to drop.
 		 * However if there are already pending

@@ -868,16 +918,42 @@ static void wait_barrier(struct r1conf *conf)
 				     !bio_list_empty(current->bio_list))),
 				    conf->resync_lock);
 		conf->nr_waiting--;
-	}
+	} else if (bio_data_dir(bio) == WRITE &&
+			bio->bi_sector >= conf->start_next_window)
+		sector = conf->start_next_window;
 	conf->nr_pending++;
 	spin_unlock_irq(&conf->resync_lock);
+	return sector;
 }
 
-static void allow_barrier(struct r1conf *conf)
+static void allow_barrier(struct r1conf *conf, sector_t start_next_window,
+				sector_t bi_sector)
 {
 	unsigned long flags;
+
 	spin_lock_irqsave(&conf->resync_lock, flags);
 	conf->nr_pending--;
+	if (start_next_window) {
+		if (start_next_window == conf->start_next_window) {
+			if (conf->start_next_window + NEXT_NORMALIO_DISTANCE
+				<= bi_sector)
+				conf->next_window_requests--;
+			else
+				conf->current_window_requests--;
+		} else
+			conf->current_window_requests--;
+
+		if (!conf->current_window_requests) {
+			if (conf->next_window_requests) {
+				conf->current_window_requests =
+					conf->next_window_requests;
+				conf->next_window_requests = 0;
+				conf->start_next_window +=
+					NEXT_NORMALIO_DISTANCE;
+			} else
+				conf->start_next_window = MaxSector;
+		}
+	}
 	spin_unlock_irqrestore(&conf->resync_lock, flags);
 	wake_up(&conf->wait_barrier);
 }

@@ -1012,6 +1088,7 @@ static void make_request(struct mddev *mddev, struct bio * bio)
 	int first_clone;
 	int sectors_handled;
 	int max_sectors;
+	sector_t start_next_window;
 
 	/*
 	 * Register the new request and wait if the reconstruction

@@ -1041,7 +1118,7 @@ static void make_request(struct mddev *mddev, struct bio * bio)
 		finish_wait(&conf->wait_barrier, &w);
 	}
 
-	wait_barrier(conf);
+	start_next_window = wait_barrier(conf, bio);
 
 	bitmap = mddev->bitmap;
 

@@ -1162,6 +1239,7 @@ read_again:
 
 	disks = conf->raid_disks * 2;
  retry_write:
+	r1_bio->start_next_window = start_next_window;
 	blocked_rdev = NULL;
 	rcu_read_lock();
 	max_sectors = r1_bio->sectors;

@@ -1230,14 +1308,24 @@ read_again:
 	if (unlikely(blocked_rdev)) {
 		/* Wait for this device to become unblocked */
 		int j;
+		sector_t old = start_next_window;
 
 		for (j = 0; j < i; j++)
 			if (r1_bio->bios[j])
 				rdev_dec_pending(conf->mirrors[j].rdev, mddev);

drivers/md/raid1.c +13 -2

+		if (bio->bi_phys_segments && old &&
+			old != start_next_window)
+			/*wait the former r1bio(s) completed*/
+			wait_event(conf->wait_barrier,
+					bio->bi_phys_segments == 1);
 		goto retry_write;
 	}
 

@@ -1437,11 +1525,14 @@ static void print_conf(struct r1conf *conf)
 
 static void close_sync(struct r1conf *conf)
 {
-	wait_barrier(conf);
-	allow_barrier(conf);
+	wait_barrier(conf, NULL);
+	allow_barrier(conf, 0, 0);
 
 	mempool_destroy(conf->r1buf_pool);
 	conf->r1buf_pool = NULL;
+
+	conf->next_resync = 0;
+	conf->start_next_window = MaxSector;
 }
 
 static int raid1_spare_active(struct mddev *mddev)

@@ -2712,6 +2803,9 @@ static struct r1conf *setup_conf(struct mddev *mddev)
 	conf->pending_count = 0;
 	conf->recovery_disabled = mddev->recovery_disabled - 1;
 
+	conf->start_next_window = MaxSector;
+	conf->current_window_requests = conf->next_window_requests = 0;
+
 	err = -EIO;
 	for (i = 0; i < conf->raid_disks * 2; i++) {
 

drivers/md/raid1.h +14 -0

index 331a98a..07425a1 100644
@@ -41,6 +41,19 @@ struct r1conf {
 	 */
 	sector_t		next_resync;
 
+	/*when raid1 start resync,we divide raid into four partitions
+	 * |---------|--------------|---------------------|-------------|
+	 *        next_resync   start_next_window        Pc
+	 * Now start_next_window = next_resync + NEXT_NORMALIO_DISTANCE
+	 * Pc = start_next_window + NEXT_NORMALIO_DISTANCE
+	 * current_window_requests means the count of normalIO between
+	 * start_next_window and Pc.
+	 * next_window_requests means the count of nornalIO after Pc.
+	 * */
+	sector_t		start_next_window;
+	int			current_window_requests;
+	int			next_window_requests;
+
 	spinlock_t		device_lock;
 
 	/* list of 'struct r1bio' that need to be processed by raid1d,

@@ -112,6 +125,7 @@ struct r1bio {
 						 * in this BehindIO request
 						 */
 	sector_t		sector;
+	sector_t		start_next_window;
 	int			sectors;
 	unsigned long		state;
 	struct mddev		*mddev;

Hi,
sorry for the excessive delay in reviewing these.

I've applied the first three, though I fixed some careless spelling errors in the patch descriptions and fixed up some of the grammar.

This one is fairly good - just a couple of fixes needed.
It is good that you have a nice details patch description at the top, but it is a bit hard to read. Fixing the spelling and improving the formatting would help a lot....

See below for comments in the code....

...

Review

Quoted 6 lines

(quoted diff) +3 -1

 	wait_event_lock_irq(conf->wait_barrier,
 			    !conf->array_frozen && 
-			    !conf->nr_pending && conf->barrier < RESYNC_DEPTH,
+			    !conf->nr_pending && conf->barrier < RESYNC_DEPTH &&
+			    (conf->next_resync + RESYNC_SECTORS
+				    <= conf->start_next_window),

I think conditions are much more readable if the thing that you expect to change comes first.
In this case we might need to wait until start_next_window increases, so
conf->start_next_window >= conf->next_resync + RESYNC_SECTORS

as it is, it looks like you are waiting for next_resync to decrease.

Quoted 28 lines

(quoted diff) +19 -1

 			    conf->resync_lock);
 
 	spin_unlock_irq(&conf->resync_lock);

@@ -846,10 +863,43 @@ static void lower_barrier(struct r1conf *conf)
 	wake_up(&conf->wait_barrier);
 }
 
-static void wait_barrier(struct r1conf *conf)
+static bool need_to_wait_for_sync(struct r1conf *conf, struct bio *bio)
 {
+	bool wait = false;
+
+	if (conf->array_frozen || !bio)
+		wait = true;
+	else if (conf->barrier && bio_data_dir(bio) == WRITE) {
+		if (conf->next_resync < RESYNC_WINDOW_SECTORS)
+			wait = true;
+		else if (conf->next_resync - RESYNC_WINDOW_SECTORS >=
+				bio_end_sector(bio))
+			wait = false;
+		else if (conf->next_resync + NEXT_NORMALIO_DISTANCE
+				<= bio->bi_sector) {
+			if (conf->start_next_window == MaxSector)
+				conf->start_next_window =
+					conf->next_resync +
+						NEXT_NORMALIO_DISTANCE;
+
+			if ((conf->start_next_window + NEXT_NORMALIO_DISTANCE)

Review static void lower_barrier(struct r1conf *conf) · line 895

Quoted 10 lines

(quoted diff) +10 -0

+				<= bio->bi_sector)
+				conf->next_window_requests++;
+			else
+				conf->current_window_requests++;
+		} else
+			wait = true;
+	}
+
+	return wait;
+}

I really don't like it that this function changes start_next_window and *_window_requests().
It looks like it is a simple function that tests a condition. But it actually has a side-effect as well. And it has the side-effect when it returns false which is extra confusing.

Also, you are incrementing *_window_requests for READ requests, which you don't need to do.

list_splice_init(NoIO2, NoIO3);
current_window_requests = next_window_requests;
next_window_requests = 0;

list_for_each_entry(NoIO3){
      clear_bit(NoIO2_FLAG),
      set_bit(NoIO3_FLAG,
}

Signed-off-by Jianpeng Ma <[email protected]>

---
 drivers/md/raid1.c | 122 +++++++++++++++++++++++++++++++++++++++++++++++------
 drivers/md/raid1.h |  14 ++++++
 2 files changed, 124 insertions(+), 12 deletions(-)

drivers/md/raid1.c +2 -1

index b4a6dcd..5b311c0 100644
@@ -66,7 +66,8 @@
  */
 static int max_queued_requests = 1024;
 
-static void allow_barrier(struct r1conf *conf);
+static void allow_barrier(struct r1conf *conf, sector_t start_next_window,
+				sector_t bi_sector);
 static void lower_barrier(struct r1conf *conf);
 
 static void * r1bio_pool_alloc(gfp_t gfp_flags, void *data)

drivers/md/raid1.c +10 -2

 	if (bio->bi_phys_segments) {
 		unsigned long flags;

@@ -234,6 +237,11 @@ static void call_bio_endio(struct r1bio *r1_bio)
 		bio->bi_phys_segments--;
 		done = (bio->bi_phys_segments == 0);
 		spin_unlock_irqrestore(&conf->device_lock, flags);
+		/*
+		 * make_request() might be waiting for
+		 * bi_phys_segments to decrease
+		 */
+		wake_up(&conf->wait_barrier);
 	} else
 		done = 1;
 

@@ -245,7 +253,7 @@ static void call_bio_endio(struct r1bio *r1_bio)
 		 * Wake up any possible resync thread that waits for the device
 		 * to go idle.
 		 */
-		allow_barrier(conf);
+		allow_barrier(conf, start_next_window, bi_sector);
 	}
 }
 

@@ -827,10 +835,18 @@ static void raise_barrier(struct r1conf *conf)
 	/* block any new IO from starting */
 	conf->barrier++;
 
-	/* Now wait for all pending IO to complete */
+	/* For those conditions we must wait:
+	 * A:while the array is in frozen state
+	 * B:while barrier >= RESYNC_DEPTH, meaning resync reach
+	 *   the max count which allowed.

drivers/md/raid1.c +31 -2

 	spin_unlock_irq(&conf->resync_lock);

@@ -846,10 +862,33 @@ static void lower_barrier(struct r1conf *conf)
 	wake_up(&conf->wait_barrier);
 }
 
-static void wait_barrier(struct r1conf *conf)
+static bool need_to_wait_for_sync(struct r1conf *conf, struct bio *bio)
+{
+	bool wait = false;
+
+	if (conf->array_frozen || !bio)
+		wait = true;
+	else if (conf->barrier && bio_data_dir(bio) == WRITE) {
+		if (conf->next_resync < RESYNC_WINDOW_SECTORS)
+			wait = true;
+		else if ((conf->next_resync - RESYNC_WINDOW_SECTORS
+				>= bio_end_sector(bio)) ||
+			 (conf->next_resync + NEXT_NORMALIO_DISTANCE
+				<= bio->bi_sector))
+			wait = false;
+		else
+			wait = true;
+	}
+
+	return wait;
+}
+
+static sector_t wait_barrier(struct r1conf *conf, struct bio *bio)
 {
+	sector_t sector = 0;
+
 	spin_lock_irq(&conf->resync_lock);
-	if (conf->barrier) {
+	if (need_to_wait_for_sync(conf, bio)) {
 		conf->nr_waiting++;
 		/* Wait for the barrier to drop.
 		 * However if there are already pending

@@ -868,16 +907,57 @@ static void wait_barrier(struct r1conf *conf)
 				     !bio_list_empty(current->bio_list))),
 				    conf->resync_lock);
 		conf->nr_waiting--;
+	} else if (bio_data_dir(bio) == WRITE) {
+		if (conf->next_resync + NEXT_NORMALIO_DISTANCE
+				<= bio->bi_sector) {
+			if (conf->start_next_window == MaxSector)
+				conf->start_next_window =
+					conf->next_resync +

drivers/md/raid1.c +29 -2

 	}
+
 	conf->nr_pending++;
 	spin_unlock_irq(&conf->resync_lock);
+	return sector;
 }
 
-static void allow_barrier(struct r1conf *conf)
+static void allow_barrier(struct r1conf *conf, sector_t start_next_window,
+				sector_t bi_sector)
 {
 	unsigned long flags;
+
 	spin_lock_irqsave(&conf->resync_lock, flags);
 	conf->nr_pending--;
+	if (start_next_window) {
+		if (start_next_window == conf->start_next_window) {
+			if (conf->start_next_window + NEXT_NORMALIO_DISTANCE
+				<= bi_sector)
+				conf->next_window_requests--;
+			else
+				conf->current_window_requests--;
+		} else
+			conf->current_window_requests--;
+
+		if (!conf->current_window_requests) {
+			if (conf->next_window_requests) {
+				conf->current_window_requests =
+					conf->next_window_requests;
+				conf->next_window_requests = 0;
+				conf->start_next_window +=
+					NEXT_NORMALIO_DISTANCE;
+			} else
+				conf->start_next_window = MaxSector;
+		}
+	}
 	spin_unlock_irqrestore(&conf->resync_lock, flags);
 	wake_up(&conf->wait_barrier);
 }

@@ -1012,6 +1092,7 @@ static void make_request(struct mddev *mddev, struct bio * bio)
 	int first_clone;
 	int sectors_handled;
 	int max_sectors;
+	sector_t start_next_window;
 
 	/*
 	 * Register the new request and wait if the reconstruction

@@ -1041,7 +1122,7 @@ static void make_request(struct mddev *mddev, struct bio * bio)
 		finish_wait(&conf->wait_barrier, &w);
 	}
 
-	wait_barrier(conf);
+	start_next_window = wait_barrier(conf, bio);
 
 	bitmap = mddev->bitmap;
 

@@ -1162,6 +1243,7 @@ read_again:
 
 	disks = conf->raid_disks * 2;
  retry_write:
+	r1_bio->start_next_window = start_next_window;
 	blocked_rdev = NULL;
 	rcu_read_lock();
 	max_sectors = r1_bio->sectors;

@@ -1230,14 +1312,24 @@ read_again:
 	if (unlikely(blocked_rdev)) {

drivers/md/raid1.c +18 -3

 		md_wait_for_blocked_rdev(blocked_rdev, mddev);
-		wait_barrier(conf);
+		start_next_window = wait_barrier(conf, bio);
+		/*
+		 * We must make sure the multi r1bios of bio have
+		 * the same value of bi_phys_segments
+		 */
+		if (bio->bi_phys_segments && old &&
+			old != start_next_window)
+			/*wait the former r1bio(s) completed*/
+			wait_event(conf->wait_barrier,
+					bio->bi_phys_segments == 1);
 		goto retry_write;
 	}
 

@@ -1437,11 +1529,14 @@ static void print_conf(struct r1conf *conf)
 
 static void close_sync(struct r1conf *conf)
 {
-	wait_barrier(conf);
-	allow_barrier(conf);
+	wait_barrier(conf, NULL);
+	allow_barrier(conf, 0, 0);
 
 	mempool_destroy(conf->r1buf_pool);
 	conf->r1buf_pool = NULL;
+
+	conf->next_resync = 0;
+	conf->start_next_window = MaxSector;
 }
 
 static int raid1_spare_active(struct mddev *mddev)

@@ -2713,6 +2808,9 @@ static struct r1conf *setup_conf(struct mddev *mddev)
 	conf->pending_count = 0;
 	conf->recovery_disabled = mddev->recovery_disabled - 1;
 
+	conf->start_next_window = MaxSector;
+	conf->current_window_requests = conf->next_window_requests = 0;
+
 	err = -EIO;
 	for (i = 0; i < conf->raid_disks * 2; i++) {
 

drivers/md/raid1.h +13 -0

index 331a98a..07425a1 100644
@@ -41,6 +41,19 @@ struct r1conf {
 	 */
 	sector_t		next_resync;
 
+	/*when raid1 start resync,we divide raid into four partitions
+	 * |---------|--------------|---------------------|-------------|
+	 *        next_resync   start_next_window        Pc
+	 * Now start_next_window = next_resync + NEXT_NORMALIO_DISTANCE
+	 * Pc = start_next_window + NEXT_NORMALIO_DISTANCE
+	 * current_window_requests means the count of normalIO between
+	 * start_next_window and Pc.
+	 * next_window_requests means the count of nornalIO after Pc.
+	 * */
+	sector_t		start_next_window;
+	int			current_window_requests;
+	int			next_window_requests;
+
 	spinlock_t		device_lock;
 
 	/* list of 'struct r1bio' that need to be processed by raid1d,

Nearly there!! Just a few more details. See below.

Quoted 19 lines

Signed-off-by Jianpeng Ma <[email protected]>

---
 drivers/md/raid1.c | 122 +++++++++++++++++++++++++++++++++++++++++++++++------
 drivers/md/raid1.h |  14 ++++++
 2 files changed, 124 insertions(+), 12 deletions(-)

drivers/md/raid1.c +2 -1

index b4a6dcd..5b311c0 100644
@@ -66,7 +66,8 @@
  */
 static int max_queued_requests = 1024;
 
-static void allow_barrier(struct r1conf *conf);
+static void allow_barrier(struct r1conf *conf, sector_t start_next_window,
+				sector_t bi_sector);
 static void lower_barrier(struct r1conf *conf);
 

drivers/md/raid1.c static void call_bio_endio(struct r1bio *r1_bio) · line 232

Quoted 7 lines

drivers/md/raid1.c +2 -0

 static void * r1bio_pool_alloc(gfp_t gfp_flags, void *data)

@@ -227,6 +228,8 @@ static void call_bio_endio(struct r1bio *r1_bio)
 	struct bio *bio = r1_bio->master_bio;
 	int done;
 	struct r1conf *conf = r1_bio->mddev->private;
+	sector_t start_next_window = r1_bio->start_next_window;
+	sector_t bi_sector = bio->bi_sector;

This should be r1_bio->sector, not bio->bi_sector.
They are often the same but if multiple r1_bios are needed for some reason (e.g. bad blocks) they may not be.

Quoted 39 lines

drivers/md/raid1.c +13 -3

 	if (bio->bi_phys_segments) {
 		unsigned long flags;

@@ -234,6 +237,11 @@ static void call_bio_endio(struct r1bio *r1_bio)
 		bio->bi_phys_segments--;
 		done = (bio->bi_phys_segments == 0);
 		spin_unlock_irqrestore(&conf->device_lock, flags);
+		/*
+		 * make_request() might be waiting for
+		 * bi_phys_segments to decrease
+		 */
+		wake_up(&conf->wait_barrier);
 	} else
 		done = 1;
 

@@ -245,7 +253,7 @@ static void call_bio_endio(struct r1bio *r1_bio)
 		 * Wake up any possible resync thread that waits for the device
 		 * to go idle.
 		 */
-		allow_barrier(conf);
+		allow_barrier(conf, start_next_window, bi_sector);
 	}
 }
 

@@ -827,10 +835,18 @@ static void raise_barrier(struct r1conf *conf)
 	/* block any new IO from starting */
 	conf->barrier++;
 
-	/* Now wait for all pending IO to complete */
+	/* For those conditions we must wait:
+	 * A:while the array is in frozen state
+	 * B:while barrier >= RESYNC_DEPTH, meaning resync reach
+	 *   the max count which allowed.
+	 * C:next_resync + RESYNC_SECTORS > start_next_window, meaning
+	 *   next resync will reach to window which normal bios are handling.
+	 */
 	wait_event_lock_irq(conf->wait_barrier,
 			    !conf->array_frozen &&
-			    !conf->nr_pending && conf->barrier < RESYNC_DEPTH,

drivers/md/raid1.c static void raise_barrier(struct r1conf *conf) · line 850

Quoted 4 lines

drivers/md/raid1.c +3 -0

+			    conf->barrier < RESYNC_DEPTH &&
+			    (conf->start_next_window >=
+			     conf->next_resync + RESYNC_SECTORS),
 			    conf->resync_lock);

You've removed the test on conf->nr_pending here, which I think is correct. It counts 'read' requests as well. Testing start_next_window serves the same purpose as it is increased whenever current_window_requests reaches zero.

However you having modified as similar test on nr_pending in wait_barrier(). That worries me a bit. Should that be changed to a test on start_next_window to match the above change?

Quoted 55 lines

drivers/md/raid1.c +36 -2

 	spin_unlock_irq(&conf->resync_lock);

@@ -846,10 +862,33 @@ static void lower_barrier(struct r1conf *conf)
 	wake_up(&conf->wait_barrier);
 }
 
-static void wait_barrier(struct r1conf *conf)
+static bool need_to_wait_for_sync(struct r1conf *conf, struct bio *bio)
+{
+	bool wait = false;
+
+	if (conf->array_frozen || !bio)
+		wait = true;
+	else if (conf->barrier && bio_data_dir(bio) == WRITE) {
+		if (conf->next_resync < RESYNC_WINDOW_SECTORS)
+			wait = true;
+		else if ((conf->next_resync - RESYNC_WINDOW_SECTORS
+				>= bio_end_sector(bio)) ||
+			 (conf->next_resync + NEXT_NORMALIO_DISTANCE
+				<= bio->bi_sector))
+			wait = false;
+		else
+			wait = true;
+	}
+
+	return wait;
+}
+
+static sector_t wait_barrier(struct r1conf *conf, struct bio *bio)
 {
+	sector_t sector = 0;
+
 	spin_lock_irq(&conf->resync_lock);
-	if (conf->barrier) {
+	if (need_to_wait_for_sync(conf, bio)) {
 		conf->nr_waiting++;
 		/* Wait for the barrier to drop.
 		 * However if there are already pending

@@ -868,16 +907,57 @@ static void wait_barrier(struct r1conf *conf)
 				     !bio_list_empty(current->bio_list))),
 				    conf->resync_lock);
 		conf->nr_waiting--;
+	} else if (bio_data_dir(bio) == WRITE) {
+		if (conf->next_resync + NEXT_NORMALIO_DISTANCE
+				<= bio->bi_sector) {
+			if (conf->start_next_window == MaxSector)
+				conf->start_next_window =
+					conf->next_resync +
+						NEXT_NORMALIO_DISTANCE;
+
+			if ((conf->start_next_window + NEXT_NORMALIO_DISTANCE)
+				<= bio->bi_sector)
+				conf->next_window_requests++;

drivers/md/raid1.c static void wait_barrier(struct r1conf *conf) · line 925

Quoted 5 lines

drivers/md/raid1.c +5 -0

+			else
+				conf->current_window_requests++;
+		}
+		if (bio->bi_sector >= conf->start_next_window)
+			sector = conf->start_next_window;

You aren't setting 'sector' if we needed to wait. I don't think that is correct, is it?

Quoted 71 lines

drivers/md/raid1.c +30 -2

 	}
+
 	conf->nr_pending++;
 	spin_unlock_irq(&conf->resync_lock);
+	return sector;
 }
 
-static void allow_barrier(struct r1conf *conf)
+static void allow_barrier(struct r1conf *conf, sector_t start_next_window,
+				sector_t bi_sector)
 {
 	unsigned long flags;
+
 	spin_lock_irqsave(&conf->resync_lock, flags);
 	conf->nr_pending--;
+	if (start_next_window) {
+		if (start_next_window == conf->start_next_window) {
+			if (conf->start_next_window + NEXT_NORMALIO_DISTANCE
+				<= bi_sector)
+				conf->next_window_requests--;
+			else
+				conf->current_window_requests--;
+		} else
+			conf->current_window_requests--;
+
+		if (!conf->current_window_requests) {
+			if (conf->next_window_requests) {
+				conf->current_window_requests =
+					conf->next_window_requests;
+				conf->next_window_requests = 0;
+				conf->start_next_window +=
+					NEXT_NORMALIO_DISTANCE;
+			} else
+				conf->start_next_window = MaxSector;
+		}
+	}
 	spin_unlock_irqrestore(&conf->resync_lock, flags);
 	wake_up(&conf->wait_barrier);
 }

@@ -1012,6 +1092,7 @@ static void make_request(struct mddev *mddev, struct bio * bio)
 	int first_clone;
 	int sectors_handled;
 	int max_sectors;
+	sector_t start_next_window;
 
 	/*
 	 * Register the new request and wait if the reconstruction

@@ -1041,7 +1122,7 @@ static void make_request(struct mddev *mddev, struct bio * bio)
 		finish_wait(&conf->wait_barrier, &w);
 	}
 
-	wait_barrier(conf);
+	start_next_window = wait_barrier(conf, bio);
 
 	bitmap = mddev->bitmap;
 

@@ -1162,6 +1243,7 @@ read_again:
 
 	disks = conf->raid_disks * 2;
  retry_write:
+	r1_bio->start_next_window = start_next_window;
 	blocked_rdev = NULL;
 	rcu_read_lock();
 	max_sectors = r1_bio->sectors;

@@ -1230,14 +1312,24 @@ read_again:
 	if (unlikely(blocked_rdev)) {
 		/* Wait for this device to become unblocked */
 		int j;
+		sector_t old = start_next_window;
 

drivers/md/raid1.c read_again: · line 1321

Quoted 6 lines

drivers/md/raid1.c +1 -1

 		for (j = 0; j < i; j++)
 			if (r1_bio->bios[j])
 				rdev_dec_pending(conf->mirrors[j].rdev, mddev);
 		r1_bio->state = 0;
-		allow_barrier(conf);
+		allow_barrier(conf, start_next_window, bio->bi_sector);

I think this should be r1_bio->sector, not bio->bi_sector, for the same reason as earlier.

Quoted 68 lines

drivers/md/raid1.c +18 -3

 		md_wait_for_blocked_rdev(blocked_rdev, mddev);
-		wait_barrier(conf);
+		start_next_window = wait_barrier(conf, bio);
+		/*
+		 * We must make sure the multi r1bios of bio have
+		 * the same value of bi_phys_segments
+		 */
+		if (bio->bi_phys_segments && old &&
+			old != start_next_window)
+			/*wait the former r1bio(s) completed*/
+			wait_event(conf->wait_barrier,
+					bio->bi_phys_segments == 1);
 		goto retry_write;
 	}
 

@@ -1437,11 +1529,14 @@ static void print_conf(struct r1conf *conf)
 
 static void close_sync(struct r1conf *conf)
 {
-	wait_barrier(conf);
-	allow_barrier(conf);
+	wait_barrier(conf, NULL);
+	allow_barrier(conf, 0, 0);
 
 	mempool_destroy(conf->r1buf_pool);
 	conf->r1buf_pool = NULL;
+
+	conf->next_resync = 0;
+	conf->start_next_window = MaxSector;
 }
 
 static int raid1_spare_active(struct mddev *mddev)

@@ -2713,6 +2808,9 @@ static struct r1conf *setup_conf(struct mddev *mddev)
 	conf->pending_count = 0;
 	conf->recovery_disabled = mddev->recovery_disabled - 1;
 
+	conf->start_next_window = MaxSector;
+	conf->current_window_requests = conf->next_window_requests = 0;
+
 	err = -EIO;
 	for (i = 0; i < conf->raid_disks * 2; i++) {
 

drivers/md/raid1.h +13 -0

index 331a98a..07425a1 100644
@@ -41,6 +41,19 @@ struct r1conf {
 	 */
 	sector_t		next_resync;
 
+	/*when raid1 start resync,we divide raid into four partitions
+	 * |---------|--------------|---------------------|-------------|
+	 *        next_resync   start_next_window        Pc
+	 * Now start_next_window = next_resync + NEXT_NORMALIO_DISTANCE
+	 * Pc = start_next_window + NEXT_NORMALIO_DISTANCE
+	 * current_window_requests means the count of normalIO between
+	 * start_next_window and Pc.
+	 * next_window_requests means the count of nornalIO after Pc.
+	 * */
+	sector_t		start_next_window;
+	int			current_window_requests;
+	int			next_window_requests;
+
 	spinlock_t		device_lock;
 
 	/* list of 'struct r1bio' that need to be processed by raid1d,

drivers/md/raid1.h struct r1bio { · line 131

Quoted 7 lines

drivers/md/raid1.h +1 -0

@@ -112,6 +125,7 @@ struct r1bio {
 						 * in this BehindIO request
 						 */
 	sector_t		sector;
+	sector_t		start_next_window;
 	int			sectors;
 	unsigned long		state;
 	struct mddev		*mddev;

Thanks,
NeilBrown

Quoted 25 lines · On Tue, 29 Oct 2013 09:30:14 +0800 majianpeng <[email protected]> wrote:

Nearly there!! Just a few more details. See below.

Quoted 19 lines

Signed-off-by Jianpeng Ma <[email protected]>

---
 drivers/md/raid1.c | 122 +++++++++++++++++++++++++++++++++++++++++++++++------
 drivers/md/raid1.h |  14 ++++++
 2 files changed, 124 insertions(+), 12 deletions(-)

drivers/md/raid1.c +2 -1

index b4a6dcd..5b311c0 100644
@@ -66,7 +66,8 @@
  */
 static int max_queued_requests = 1024;
 
-static void allow_barrier(struct r1conf *conf);
+static void allow_barrier(struct r1conf *conf, sector_t start_next_window,
+				sector_t bi_sector);
 static void lower_barrier(struct r1conf *conf);
 

Quoted 12 lines

drivers/md/raid1.c static void call_bio_endio(struct r1bio *r1_bio) · line 232

Quoted 7 lines

drivers/md/raid1.c +2 -0

 static void * r1bio_pool_alloc(gfp_t gfp_flags, void *data)

@@ -227,6 +228,8 @@ static void call_bio_endio(struct r1bio *r1_bio)
 	struct bio *bio = r1_bio->master_bio;
 	int done;
 	struct r1conf *conf = r1_bio->mddev->private;
+	sector_t start_next_window = r1_bio->start_next_window;
+	sector_t bi_sector = bio->bi_sector;

This should be r1_bio->sector, not bio->bi_sector.
They are often the same but if multiple r1_bios are needed for some reason (e.g. bad blocks) they may not be.

No, allow_barrier() only for bio not for r1bio.It only do when bio->bi_phys_segments == 0.

If this value is r1_bio->sector, the value may has different value as you said. So in allow_barrier():

       if (start_next_window) {
                if (start_next_window == conf->start_next_window) {
                        if (conf->start_next_window + NEXT_NORMALIO_DISTANCE
                                <= bi_sector)
                                conf->next_window_requests--;
                        else 
                                conf->current_window_requests--;
                } else 

It will has differnt result.

Quoted 39 lines

Quoted 39 lines

drivers/md/raid1.c +13 -3

 	if (bio->bi_phys_segments) {
 		unsigned long flags;

@@ -234,6 +237,11 @@ static void call_bio_endio(struct r1bio *r1_bio)
 		bio->bi_phys_segments--;
 		done = (bio->bi_phys_segments == 0);
 		spin_unlock_irqrestore(&conf->device_lock, flags);
+		/*
+		 * make_request() might be waiting for
+		 * bi_phys_segments to decrease
+		 */
+		wake_up(&conf->wait_barrier);
 	} else
 		done = 1;
 

@@ -245,7 +253,7 @@ static void call_bio_endio(struct r1bio *r1_bio)
 		 * Wake up any possible resync thread that waits for the device
 		 * to go idle.
 		 */
-		allow_barrier(conf);
+		allow_barrier(conf, start_next_window, bi_sector);
 	}
 }
 

@@ -827,10 +835,18 @@ static void raise_barrier(struct r1conf *conf)
 	/* block any new IO from starting */
 	conf->barrier++;
 
-	/* Now wait for all pending IO to complete */
+	/* For those conditions we must wait:
+	 * A:while the array is in frozen state
+	 * B:while barrier >= RESYNC_DEPTH, meaning resync reach
+	 *   the max count which allowed.
+	 * C:next_resync + RESYNC_SECTORS > start_next_window, meaning
+	 *   next resync will reach to window which normal bios are handling.
+	 */
 	wait_event_lock_irq(conf->wait_barrier,
 			    !conf->array_frozen &&
-			    !conf->nr_pending && conf->barrier < RESYNC_DEPTH,

Quoted 9 lines

drivers/md/raid1.c static void raise_barrier(struct r1conf *conf) · line 850

Quoted 4 lines

drivers/md/raid1.c +3 -0

+			    conf->barrier < RESYNC_DEPTH &&
+			    (conf->start_next_window >=
+			     conf->next_resync + RESYNC_SECTORS),
 			    conf->resync_lock);

You've removed the test on conf->nr_pending here, which I think is correct. It counts 'read' requests as well. Testing start_next_window serves the same purpose as it is increased whenever current_window_requests reaches zero.

In func wait_barrier():

        if (need_to_wait_for_sync(conf, bio)) {
              [snip]
        } else if (bio_data_dir(bio) == WRITE) {
                if (conf->next_resync + NEXT_NORMALIO_DISTANCE
                                <= bio->bi_sector) {
                        if (conf->start_next_window == MaxSector)
                                conf->start_next_window =
                                        conf->next_resync +
                                                NEXT_NORMALIO_DISTANCE;

                        if ((conf->start_next_window + NEXT_NORMALIO_DISTANCE)
                                <= bio->bi_sector)
                                conf->next_window_requests++;
                        else
                                conf->current_window_requests++;
                }
                if (bio->bi_sector >= conf->start_next_window)
                        sector = conf->start_next_window;
        }

start_next_window only for bio_data_dir(bio) == WRITE. So resync can't wait read io to complete.

Quoted 1 line

However you having modified as similar test on nr_pending in wait_barrier().

Quoted 2 lines

That worries me a bit. Should that be changed to a test on start_next_window to match the above change?

For this condition, i only copy it.For this condition, i'm not know clearly. Can you give me more details about this?

Quoted 55 lines

Quoted 55 lines

drivers/md/raid1.c +36 -2

 	spin_unlock_irq(&conf->resync_lock);

@@ -846,10 +862,33 @@ static void lower_barrier(struct r1conf *conf)
 	wake_up(&conf->wait_barrier);
 }
 
-static void wait_barrier(struct r1conf *conf)
+static bool need_to_wait_for_sync(struct r1conf *conf, struct bio *bio)
+{
+	bool wait = false;
+
+	if (conf->array_frozen || !bio)
+		wait = true;
+	else if (conf->barrier && bio_data_dir(bio) == WRITE) {
+		if (conf->next_resync < RESYNC_WINDOW_SECTORS)
+			wait = true;
+		else if ((conf->next_resync - RESYNC_WINDOW_SECTORS
+				>= bio_end_sector(bio)) ||
+			 (conf->next_resync + NEXT_NORMALIO_DISTANCE
+				<= bio->bi_sector))
+			wait = false;
+		else
+			wait = true;
+	}
+
+	return wait;
+}
+
+static sector_t wait_barrier(struct r1conf *conf, struct bio *bio)
 {
+	sector_t sector = 0;
+
 	spin_lock_irq(&conf->resync_lock);
-	if (conf->barrier) {
+	if (need_to_wait_for_sync(conf, bio)) {
 		conf->nr_waiting++;
 		/* Wait for the barrier to drop.
 		 * However if there are already pending

@@ -868,16 +907,57 @@ static void wait_barrier(struct r1conf *conf)
 				     !bio_list_empty(current->bio_list))),
 				    conf->resync_lock);
 		conf->nr_waiting--;
+	} else if (bio_data_dir(bio) == WRITE) {
+		if (conf->next_resync + NEXT_NORMALIO_DISTANCE
+				<= bio->bi_sector) {
+			if (conf->start_next_window == MaxSector)
+				conf->start_next_window =
+					conf->next_resync +
+						NEXT_NORMALIO_DISTANCE;
+
+			if ((conf->start_next_window + NEXT_NORMALIO_DISTANCE)
+				<= bio->bi_sector)
+				conf->next_window_requests++;

Quoted 9 lines

drivers/md/raid1.c static void wait_barrier(struct r1conf *conf) · line 925

Quoted 5 lines

drivers/md/raid1.c +5 -0

+			else
+				conf->current_window_requests++;
+		}
+		if (bio->bi_sector >= conf->start_next_window)
+			sector = conf->start_next_window;

You aren't setting 'sector' if we needed to wait. I don't think that is correct, is it?

Yes.How about those code:
spin_lock_irq(&conf->resync_lock);
retry_check:

        if (need_to_wait_for_sync(conf, bio)) {
                conf->nr_waiting++;
                /* Wait for the barrier to drop.
                 * However if there are already pending
                 * requests (preventing the barrier from
                 * rising completely), and the
                 * pre-process bio queue isn't empty,
                 * then don't wait, as we need to empty
                 * that queue to get the nr_pending
                 * count down.
                 */
                wait_event_lock_irq(conf->wait_barrier,

                                    !conf->array_frozen &&
                                    (!conf->barrier ||
                                    (conf->nr_pending &&
                                     current->bio_list &&
                                     !bio_list_empty(current->bio_list))),
                                    conf->resync_lock);
                conf->nr_waiting--;
				goto retry_check;

Quoted 1 line

Quoted 70 lines

Quoted 70 lines

drivers/md/raid1.c +30 -2

 	}
+
 	conf->nr_pending++;
 	spin_unlock_irq(&conf->resync_lock);
+	return sector;
 }
 
-static void allow_barrier(struct r1conf *conf)
+static void allow_barrier(struct r1conf *conf, sector_t start_next_window,
+				sector_t bi_sector)
 {
 	unsigned long flags;
+
 	spin_lock_irqsave(&conf->resync_lock, flags);
 	conf->nr_pending--;
+	if (start_next_window) {
+		if (start_next_window == conf->start_next_window) {
+			if (conf->start_next_window + NEXT_NORMALIO_DISTANCE
+				<= bi_sector)
+				conf->next_window_requests--;
+			else
+				conf->current_window_requests--;
+		} else
+			conf->current_window_requests--;
+
+		if (!conf->current_window_requests) {
+			if (conf->next_window_requests) {
+				conf->current_window_requests =
+					conf->next_window_requests;
+				conf->next_window_requests = 0;
+				conf->start_next_window +=
+					NEXT_NORMALIO_DISTANCE;
+			} else
+				conf->start_next_window = MaxSector;
+		}
+	}
 	spin_unlock_irqrestore(&conf->resync_lock, flags);
 	wake_up(&conf->wait_barrier);
 }

@@ -1012,6 +1092,7 @@ static void make_request(struct mddev *mddev, struct bio * bio)
 	int first_clone;
 	int sectors_handled;
 	int max_sectors;
+	sector_t start_next_window;
 
 	/*
 	 * Register the new request and wait if the reconstruction

@@ -1041,7 +1122,7 @@ static void make_request(struct mddev *mddev, struct bio * bio)
 		finish_wait(&conf->wait_barrier, &w);
 	}
 
-	wait_barrier(conf);
+	start_next_window = wait_barrier(conf, bio);
 
 	bitmap = mddev->bitmap;
 

@@ -1162,6 +1243,7 @@ read_again:
 
 	disks = conf->raid_disks * 2;
  retry_write:
+	r1_bio->start_next_window = start_next_window;
 	blocked_rdev = NULL;
 	rcu_read_lock();
 	max_sectors = r1_bio->sectors;

@@ -1230,14 +1312,24 @@ read_again:
 	if (unlikely(blocked_rdev)) {
 		/* Wait for this device to become unblocked */
 		int j;
+		sector_t old = start_next_window;
 

Quoted 10 lines

drivers/md/raid1.c read_again: · line 1321

Quoted 6 lines

drivers/md/raid1.c +1 -1

 		for (j = 0; j < i; j++)
 			if (r1_bio->bios[j])
 				rdev_dec_pending(conf->mirrors[j].rdev, mddev);
 		r1_bio->state = 0;
-		allow_barrier(conf);
+		allow_barrier(conf, start_next_window, bio->bi_sector);

I think this should be r1_bio->sector, not bio->bi_sector, for the same reason as earlier.

The exaplaination is the same at above.

Thanks!
Jianpeng Ma

Quoted 80 lines

Quoted 68 lines

drivers/md/raid1.c +18 -3

 		md_wait_for_blocked_rdev(blocked_rdev, mddev);
-		wait_barrier(conf);
+		start_next_window = wait_barrier(conf, bio);
+		/*
+		 * We must make sure the multi r1bios of bio have
+		 * the same value of bi_phys_segments
+		 */
+		if (bio->bi_phys_segments && old &&
+			old != start_next_window)
+			/*wait the former r1bio(s) completed*/
+			wait_event(conf->wait_barrier,
+					bio->bi_phys_segments == 1);
 		goto retry_write;
 	}
 

@@ -1437,11 +1529,14 @@ static void print_conf(struct r1conf *conf)
 
 static void close_sync(struct r1conf *conf)
 {
-	wait_barrier(conf);
-	allow_barrier(conf);
+	wait_barrier(conf, NULL);
+	allow_barrier(conf, 0, 0);
 
 	mempool_destroy(conf->r1buf_pool);
 	conf->r1buf_pool = NULL;
+
+	conf->next_resync = 0;
+	conf->start_next_window = MaxSector;
 }
 
 static int raid1_spare_active(struct mddev *mddev)

@@ -2713,6 +2808,9 @@ static struct r1conf *setup_conf(struct mddev *mddev)
 	conf->pending_count = 0;
 	conf->recovery_disabled = mddev->recovery_disabled - 1;
 
+	conf->start_next_window = MaxSector;
+	conf->current_window_requests = conf->next_window_requests = 0;
+
 	err = -EIO;
 	for (i = 0; i < conf->raid_disks * 2; i++) {
 

drivers/md/raid1.h +13 -0

index 331a98a..07425a1 100644
@@ -41,6 +41,19 @@ struct r1conf {
 	 */
 	sector_t		next_resync;
 
+	/*when raid1 start resync,we divide raid into four partitions
+	 * |---------|--------------|---------------------|-------------|
+	 *        next_resync   start_next_window        Pc
+	 * Now start_next_window = next_resync + NEXT_NORMALIO_DISTANCE
+	 * Pc = start_next_window + NEXT_NORMALIO_DISTANCE
+	 * current_window_requests means the count of normalIO between
+	 * start_next_window and Pc.
+	 * next_window_requests means the count of nornalIO after Pc.
+	 * */
+	sector_t		start_next_window;
+	int			current_window_requests;
+	int			next_window_requests;
+
 	spinlock_t		device_lock;
 
 	/* list of 'struct r1bio' that need to be processed by raid1d,

drivers/md/raid1.h struct r1bio { · line 131

Quoted 7 lines

drivers/md/raid1.h +1 -0

@@ -112,6 +125,7 @@ struct r1bio {
 						 * in this BehindIO request
 						 */
 	sector_t		sector;
+	sector_t		start_next_window;
 	int			sectors;
 	unsigned long		state;
 	struct mddev		*mddev;

Thanks,
NeilBrown

Sorry for the delayed reply.

Quoted 37 lines

Quoted 25 lines · On Tue, 29 Oct 2013 09:30:14 +0800 majianpeng <[email protected]> wrote:

Nearly there!! Just a few more details. See below.

Quoted 19 lines

Signed-off-by Jianpeng Ma <[email protected]>

---
 drivers/md/raid1.c | 122 +++++++++++++++++++++++++++++++++++++++++++++++------
 drivers/md/raid1.h |  14 ++++++
 2 files changed, 124 insertions(+), 12 deletions(-)

drivers/md/raid1.c +2 -1

index b4a6dcd..5b311c0 100644
@@ -66,7 +66,8 @@
  */
 static int max_queued_requests = 1024;
 
-static void allow_barrier(struct r1conf *conf);
+static void allow_barrier(struct r1conf *conf, sector_t start_next_window,
+				sector_t bi_sector);
 static void lower_barrier(struct r1conf *conf);
 

Quoted 12 lines

drivers/md/raid1.c static void call_bio_endio(struct r1bio *r1_bio) · line 232

Quoted 7 lines

drivers/md/raid1.c +2 -0

 static void * r1bio_pool_alloc(gfp_t gfp_flags, void *data)

@@ -227,6 +228,8 @@ static void call_bio_endio(struct r1bio *r1_bio)
 	struct bio *bio = r1_bio->master_bio;
 	int done;
 	struct r1conf *conf = r1_bio->mddev->private;
+	sector_t start_next_window = r1_bio->start_next_window;
+	sector_t bi_sector = bio->bi_sector;

This should be r1_bio->sector, not bio->bi_sector.
They are often the same but if multiple r1_bios are needed for some reason (e.g. bad blocks) they may not be.

Quoted 1 line

No, allow_barrier() only for bio not for r1bio.It only do when bio->bi_phys_segments == 0.

Yes, I see that now, thanks.

Quoted 80 lines

If this value is r1_bio->sector, the value may has different value as you said. So in allow_barrier():

       if (start_next_window) {
                if (start_next_window == conf->start_next_window) {
                        if (conf->start_next_window + NEXT_NORMALIO_DISTANCE
                                <= bi_sector)
                                conf->next_window_requests--;
                        else 
                                conf->current_window_requests--;
                } else 

It will has differnt result.

Quoted 39 lines

Quoted 39 lines

drivers/md/raid1.c +13 -3

 	if (bio->bi_phys_segments) {
 		unsigned long flags;

@@ -234,6 +237,11 @@ static void call_bio_endio(struct r1bio *r1_bio)
 		bio->bi_phys_segments--;
 		done = (bio->bi_phys_segments == 0);
 		spin_unlock_irqrestore(&conf->device_lock, flags);
+		/*
+		 * make_request() might be waiting for
+		 * bi_phys_segments to decrease
+		 */
+		wake_up(&conf->wait_barrier);
 	} else
 		done = 1;
 

@@ -245,7 +253,7 @@ static void call_bio_endio(struct r1bio *r1_bio)
 		 * Wake up any possible resync thread that waits for the device
 		 * to go idle.
 		 */
-		allow_barrier(conf);
+		allow_barrier(conf, start_next_window, bi_sector);
 	}
 }
 

@@ -827,10 +835,18 @@ static void raise_barrier(struct r1conf *conf)
 	/* block any new IO from starting */
 	conf->barrier++;
 
-	/* Now wait for all pending IO to complete */
+	/* For those conditions we must wait:
+	 * A:while the array is in frozen state
+	 * B:while barrier >= RESYNC_DEPTH, meaning resync reach
+	 *   the max count which allowed.
+	 * C:next_resync + RESYNC_SECTORS > start_next_window, meaning
+	 *   next resync will reach to window which normal bios are handling.
+	 */
 	wait_event_lock_irq(conf->wait_barrier,
 			    !conf->array_frozen &&
-			    !conf->nr_pending && conf->barrier < RESYNC_DEPTH,

Quoted 9 lines

drivers/md/raid1.c static void raise_barrier(struct r1conf *conf) · line 850

Quoted 4 lines

drivers/md/raid1.c +3 -0

+			    conf->barrier < RESYNC_DEPTH &&
+			    (conf->start_next_window >=
+			     conf->next_resync + RESYNC_SECTORS),
 			    conf->resync_lock);

You've removed the test on conf->nr_pending here, which I think is correct. It counts 'read' requests as well. Testing start_next_window serves the same purpose as it is increased whenever current_window_requests reaches zero.

In func wait_barrier():

        if (need_to_wait_for_sync(conf, bio)) {
              [snip]
        } else if (bio_data_dir(bio) == WRITE) {
                if (conf->next_resync + NEXT_NORMALIO_DISTANCE
                                <= bio->bi_sector) {
                        if (conf->start_next_window == MaxSector)
                                conf->start_next_window =
                                        conf->next_resync +
                                                NEXT_NORMALIO_DISTANCE;

                        if ((conf->start_next_window + NEXT_NORMALIO_DISTANCE)
                                <= bio->bi_sector)
                                conf->next_window_requests++;
                        else
                                conf->current_window_requests++;
                }
                if (bio->bi_sector >= conf->start_next_window)
                        sector = conf->start_next_window;
        }

Quoted 3 lines

start_next_window only for bio_data_dir(bio) == WRITE. So resync can't wait read io to complete.

Quoted 1 line

However you having modified as similar test on nr_pending in wait_barrier().

I meant: haven't modified a similar test ....

Quoted 4 lines

Quoted 2 lines

That worries me a bit. Should that be changed to a test on start_next_window to match the above change?

For this condition, i only copy it.For this condition, i'm not know clearly. Can you give me more details about this?

Before your patch we know if there are pending requests (submitted but not completed) which stop use from doing resync if conf->nr_pending > 0. After your patch we cannot use that that test as it counts READ requests, and requests that are outside the range being resynced.

The test is now a bit more complicated. I think it is:

		 conf->start_next_window <
                conf->next_resync + RESYNC_SECTORS

i.e. while start_next_window is less than next_resync+RESYNC_SECTORS there cold be outstanding writes that are blocking resync. As soon as those writes complete, start_next_window will increase and the resync can complete. So if start_next_window < next_resync+RESYNC_SECTORS and there are pending request in current->bio_list, then then there is no point waiting for resync (it cannot progress anyway) so we may as well submit another write.

So change:

		wait_event_lock_irq(conf->wait_barrier,
				    !conf->array_frozen &&
				    (!conf->barrier ||
				    (conf->nr_pending &&
				     current->bio_list &&
				     !bio_list_empty(current->bio_list))),
				    conf->resync_lock);

in wait barrier to

		wait_event_lock_irq(conf->wait_barrier,
				    !conf->array_frozen &&
				    (!conf->barrier ||
				    (conf->start_next_window < conf->next_resync + RESYNC_SECTORS &&
				     current->bio_list &&
				     !bio_list_empty(current->bio_list))),
				    conf->resync_lock);

Quoted 79 lines

Quoted 55 lines

Quoted 55 lines

drivers/md/raid1.c +36 -2

 	spin_unlock_irq(&conf->resync_lock);

@@ -846,10 +862,33 @@ static void lower_barrier(struct r1conf *conf)
 	wake_up(&conf->wait_barrier);
 }
 
-static void wait_barrier(struct r1conf *conf)
+static bool need_to_wait_for_sync(struct r1conf *conf, struct bio *bio)
+{
+	bool wait = false;
+
+	if (conf->array_frozen || !bio)
+		wait = true;
+	else if (conf->barrier && bio_data_dir(bio) == WRITE) {
+		if (conf->next_resync < RESYNC_WINDOW_SECTORS)
+			wait = true;
+		else if ((conf->next_resync - RESYNC_WINDOW_SECTORS
+				>= bio_end_sector(bio)) ||
+			 (conf->next_resync + NEXT_NORMALIO_DISTANCE
+				<= bio->bi_sector))
+			wait = false;
+		else
+			wait = true;
+	}
+
+	return wait;
+}
+
+static sector_t wait_barrier(struct r1conf *conf, struct bio *bio)
 {
+	sector_t sector = 0;
+
 	spin_lock_irq(&conf->resync_lock);
-	if (conf->barrier) {
+	if (need_to_wait_for_sync(conf, bio)) {
 		conf->nr_waiting++;
 		/* Wait for the barrier to drop.
 		 * However if there are already pending

@@ -868,16 +907,57 @@ static void wait_barrier(struct r1conf *conf)
 				     !bio_list_empty(current->bio_list))),
 				    conf->resync_lock);
 		conf->nr_waiting--;
+	} else if (bio_data_dir(bio) == WRITE) {
+		if (conf->next_resync + NEXT_NORMALIO_DISTANCE
+				<= bio->bi_sector) {
+			if (conf->start_next_window == MaxSector)
+				conf->start_next_window =
+					conf->next_resync +
+						NEXT_NORMALIO_DISTANCE;
+
+			if ((conf->start_next_window + NEXT_NORMALIO_DISTANCE)
+				<= bio->bi_sector)
+				conf->next_window_requests++;

Quoted 9 lines

drivers/md/raid1.c static void wait_barrier(struct r1conf *conf) · line 925

Quoted 5 lines

drivers/md/raid1.c +5 -0

+			else
+				conf->current_window_requests++;
+		}
+		if (bio->bi_sector >= conf->start_next_window)
+			sector = conf->start_next_window;

You aren't setting 'sector' if we needed to wait. I don't think that is correct, is it?

Yes.How about those code:
spin_lock_irq(&conf->resync_lock);
retry_check:

        if (need_to_wait_for_sync(conf, bio)) {
                conf->nr_waiting++;
                /* Wait for the barrier to drop.
                 * However if there are already pending
                 * requests (preventing the barrier from
                 * rising completely), and the
                 * pre-process bio queue isn't empty,
                 * then don't wait, as we need to empty
                 * that queue to get the nr_pending
                 * count down.
                 */
                wait_event_lock_irq(conf->wait_barrier,

Quoted 9 lines

                                    !conf->array_frozen &&
                                    (!conf->barrier ||
                                    (conf->nr_pending &&
                                     current->bio_list &&
                                     !bio_list_empty(current->bio_list))),
                                    conf->resync_lock);
                conf->nr_waiting--;
				goto retry_check;

Quoted 1 line

It would look a lot better if you made it a while loop:

while (need_to_wait....) {
    nr_waiting++
    wait_event.....
    nr_waiting--

if (bio_data_dir.....

Quoted 25 lines

Quoted 15 lines

Yes.How about those code:
spin_lock_irq(&conf->resync_lock);
retry_check:

        if (need_to_wait_for_sync(conf, bio)) {
                conf->nr_waiting++;
                /* Wait for the barrier to drop.
                 * However if there are already pending
                 * requests (preventing the barrier from
                 * rising completely), and the
                 * pre-process bio queue isn't empty,
                 * then don't wait, as we need to empty
                 * that queue to get the nr_pending
                 * count down.
                 */
                wait_event_lock_irq(conf->wait_barrier,

Quoted 9 lines

                                    !conf->array_frozen &&
                                    (!conf->barrier ||
                                    (conf->nr_pending &&
                                     current->bio_list &&
                                     !bio_list_empty(current->bio_list))),
                                    conf->resync_lock);
                conf->nr_waiting--;
				goto retry_check;

Quoted 1 line

Quoted 8 lines

It would look a lot better if you made it a while loop:

while (need_to_wait....) {
    nr_waiting++
    wait_event.....
    nr_waiting--

if (bio_data_dir.....

For this, i think it don;t need while() or recheck. The code should:

if (nedd_to_wait...) {
	nr_waiting++
	wait_event.....
	nr_waiting--
}
if (bio && bio_data_dir(bio) == WRITE) {
	 do;
}

I think again need_to_wait...(),it must return false. So we don't recheck this. Or am I missing something?

Quoted 46 lines

Quoted 26 lines

Quoted 16 lines

Yes.How about those code:
spin_lock_irq(&conf->resync_lock);
retry_check:

        if (need_to_wait_for_sync(conf, bio)) {
                conf->nr_waiting++;
                /* Wait for the barrier to drop.
                 * However if there are already pending
                 * requests (preventing the barrier from
                 * rising completely), and the
                 * pre-process bio queue isn't empty,
                 * then don't wait, as we need to empty
                 * that queue to get the nr_pending
                 * count down.
                 */
                wait_event_lock_irq(conf->wait_barrier,

Quoted 9 lines

                                    !conf->array_frozen &&
                                    (!conf->barrier ||
                                    (conf->nr_pending &&
                                     current->bio_list &&
                                     !bio_list_empty(current->bio_list))),
                                    conf->resync_lock);
                conf->nr_waiting--;
				goto retry_check;

Quoted 1 line

Quoted 8 lines

It would look a lot better if you made it a while loop:

while (need_to_wait....) {
    nr_waiting++
    wait_event.....
    nr_waiting--

if (bio_data_dir.....

For this, i think it don;t need while() or recheck. The code should:

if (nedd_to_wait...) {
	nr_waiting++
	wait_event.....
	nr_waiting--
}
if (bio && bio_data_dir(bio) == WRITE) {
	 do;
}

Quoted 3 lines

I think again need_to_wait...(),it must return false. So we don't recheck this. Or am I missing something?

An 'if' is fine. I just didn't like the goto.
If you can send me the updated patch in a day or 2 I should be able to get it into the current merge window.

thanks,
NeilBrown

list_splice_init(NoIO2, NoIO3);
current_window_requests = next_window_requests;
next_window_requests = 0;

list_for_each_entry(NoIO3){
      clear_bit(NoIO2_FLAG),
      set_bit(NoIO3_FLAG,
}

Quoted 6 lines

If you can send me the updated patch in a day or 2 I should be able to get it into the current merge window.

thanks,
NeilBrown

Thanks very much neil! Below the latest patch.

Subject:raid1: Rewrite the implementation of iobarrier.

Quoted 7 lines

Quoted 7 lines

If you can send me the updated patch in a day or 2 I should be able to get it into the current merge window.

thanks,
NeilBrown

Quoted 3 lines

Thanks very much neil! Below the latest patch.

Subject:raid1: Rewrite the implementation of iobarrier.

Thanks. I've applied that patch. I also cleaned up some of the formatting and clarified the patch description quite a bit.

NeilBrown