Re: Re: [PATCH 3/3] raid1: Rewrite the implementation of iobarrier.

[majianpeng <majianpeng@gmail.com>]

>On Mon, 19 Aug 2013 19:58:07 +0800 majianpeng <majianpeng@gmail.com> wrote:
>
[snip]
>> -
>> -	/* Now wait for all pending IO to complete */
>> +	/* For those conditions, we must wait
>> +	 * A:array is in freeze state.
>> +	 * B:When starting resync and there are normal IO
>> +	 * C:There is no nornal io after three times in resync window
>> +	 * D: total barrier are less than RESYNC_DEPTH*/
>>  	wait_event_lock_irq(conf->wait_barrier,
>> -			    !conf->freeze_array && 
>> -			    !conf->nr_pending && conf->barrier < RESYNC_DEPTH,
>> +			    !conf->freeze_array &&
>> +			    ((conf->next_resync < RESYNC_WINDOW_SECTORS
>> +				&& !conf->nr_pending)
>> +			    || (conf->next_resync + RESYNC_SECTORS
>> +				    <= conf->after_resync_sector))
>> +			    && conf->barrier < RESYNC_DEPTH,
>>  			    conf->resync_lock);
>
>For "C:There is no nornal io after three times in resync window" I expected
>to see
>          conf->after_resync_three_count == 0
>
>but I don't see that.  Why not?
>
I think there are two mistakes.
For those conditions, we must wait:
A:Array is in freeze state.
B:conf->nr_pending > 0 [For this, firtly, i think we should only consider the starting resync.If ->nr_pending > 0,we must wait
whether or not starting resync.]
C:conf->next_resync + RESYNC_SECTORS > conf->after_resync_sector.
D:Total barrier are more than RESYNC_DEPTH.

[snip]

>
>>  		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, old, bio->bi_sector);
>>  		md_wait_for_blocked_rdev(blocked_rdev, mddev);
>> -		wait_barrier(conf);
>> +		after_threetimes_resync_sector = wait_barrier(conf, bio);
>> +		/*
>> +		 * We must make sure the multi r1bios of bio have
>> +		 * the same value of after_threetimes_resync_sector
>> +		 */
>> +		if (bio->bi_phys_segments && old &&
>> +			old != after_threetimes_resync_sector)
>> +			/*wait the former r1bio(s) completed*/
>> +			wait_event(conf->wait_barrier,
>> +					bio->bi_phys_segments == 1);
>
>I think it is quite possible that bi_phys_segments==0 at this point.
>So you probably want "<= 1".
>
No, only there are multi r1bios of bio, it must wait previous r1bios completed.The left r1bio made'bi_phys_segments = 1'.
If there is one r1bio of bio, it can't wait.It only need allow_barrier and retry wait_barrier.



For other comments, i'll apply.
Thanks!
Jianpeng Ma