Re: [md PATCH 10/15] md/raid1: stop using bi_phys_segment

Shaohua Li
2017-03-16T00:13:55Z

On Wed, Mar 15, 2017 at 02:05:14PM +1100, Neil Brown wrote:
> Change to use bio->__bi_remaining to count number of r1bio attached
> to a bio.
> See precious raid10 patch for more details.
>
> Like the raid10.c patch, this fixes a bug as nr_queued and nr_pending
> used to measure different things, but were being compared.
>
> This patch fixes another bug in that nr_pending previously did not
> could write-behind requests, so behind writes could continue while
> resync was happening. How that nr_pending counts all r1_bio,
> the resync cannot commence until the behind writes have completed.
>
> Signed-off-by: NeilBrown <neilb@suse.com>
> ---
> drivers/md/raid1.c | 87 +++++++++++++---------------------------------------
> 1 file changed, 22 insertions(+), 65 deletions(-)
>
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 7e509a894f15..e566407b196f 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -246,35 +246,18 @@ static void reschedule_retry(struct r1bio *r1_bio)
> 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 bi_sector = bio->bi_iter.bi_sector;
>
> - if (bio->bi_phys_segments) {
> - unsigned long flags;
> - spin_lock_irqsave(&conf->device_lock, flags);
> - 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;
> -
> if (!test_bit(R1BIO_Uptodate, &r1_bio->state))
> bio->bi_error = -EIO;
>
> - if (done) {
> - bio_endio(bio);
> - /*
> - * Wake up any possible resync thread that waits for the device
> - * to go idle.
> - */
> - allow_barrier(conf, bi_sector);
> - }
> + bio_endio(bio);
> + /*
> + * Wake up any possible resync thread that waits for the device
> + * to go idle.
> + */
> + allow_barrier(conf, bi_sector);

I think this one should be r1_bio->sector instead of master_bio->sector,
because multiple r1_bio could be attached to a master_bio. Maybe not change
anything, because both sector should be in the same barrier unit, but we'd
better to be consistent.

Thanks,
Shaohua