On Thu, 31 Oct 2013 11:20:55 +0800 majianpeng [off-list ref] wrote:

Sorry for the delayed reply.

quoted

quoted

On Tue, 29 Oct 2013 09:30:14 +0800 majianpeng [off-list ref] wrote:


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

quoted

Signed-off-by: Jianpeng Ma <redacted>
---
 drivers/md/raid1.c | 122 +++++++++++++++++++++++++++++++++++++++++++++++------
 drivers/md/raid1.h |  14 ++++++
 2 files changed, 124 insertions(+), 12 deletions(-)

diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index b4a6dcd..5b311c0 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c

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

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.

Yes, I see that now, thanks.

quoted

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

quoted

 
 	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,
+			    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

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

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

quoted

quoted

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

Ok, apply this.

quoted

quoted

quoted

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

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?

Thanks!
Jianpeng Ma