Re: RAID1 repair GPF crash w/3.10-rc7

[NeilBrown <neilb@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 2718 bytes --]

On Wed, 3 Jul 2013 17:49:51 -0400 (EDT) Joe Lawrence
<joe.lawrence@stratus.com> wrote:

> On Mon, 1 Jul 2013, Joe Lawrence wrote:
> 
> > Hi Kent & Neil,
> > 
> > I've hit a crash in MD during RAID1 repair while running 3.10-rc7:
> >
> > [ ... snip ... ]
> 
> Hi Neil,
> 
> Looking through the MD source, I'm trying to understand part of the
> RAID1 repair path.  I came up with a few questions:
> 
> 1 - During user initiated RAID1 repair, is the loop at the bottom of
> sync_request(), under the bio_full label, responsible for submitting all
> of the initial read bios?

Yes.

> 
> 2 - Does process_checks() later find the first uptodate read bio and
> copy its data into the other r1_bio->bios[] for write repair to the
> other disks?

Yes.

> 
> If both are true, then perhaps the following applies to this crash...
> 
> Comments in commit f79ea416 "block: Refactor blk_update_request()" msg
> include:
> 
>     Note that req_bio_endio() now always calls bio_advance() - which
>     means it always loops over the biovec, not just on partial
>     completions.  Don't expect it to affect performance, but worth
>     noting.
> 
> Now that process_checks() has been further modified for immutable bio
> prep (commit d3b45c2 "raid1: use bio_copy_data()"), it calls
> bio_copy_data() to fill in the write repair bios... which starts
> indexing the bi_bio_vec[] from wherever bi_idx happens to be.
> 
> If this is indeed the case, I'm having trouble coming up with a good
> solution:
> 
>   - Immutable bios means drivers don't touch bi_idx.  So MD shouldn't
>     "re-wind" the source bi_idx before calling bio_copy_data().

But MD "owns" this bio.  It knows exactly how it was created, and can do
what ever it likes after it has been returned.

I would propose "bio_rewind" that exactly undoes any "bio_advance". 
Something like

void bio_rewind(struct bio *bio)
{
	int bytes = 0;

	if (bio->bi_idx < bio->bi_vcnt && bio_iovec(bio)->bv_offset > 0) {
		bytes = bio_iovec(bio)->bv_offset;
		bio_iovec(bio)->bv_offset -= bytes;
		bio_iovec(bio)->bv_len += bytes;
	}
	while (bio->bi_idx) {
		bio->bi_idx -= 1;
		bio_iovec(bio)->bv_len += bio_iovec(bio)->bv_offset;
		bio_iovec(bio)->bv_offset = 0;
		bytes += bio_iovec(bio)->bv_len;
	}
	bio->bi_size += bytes;
	bio->bi_bi_sector -= bytes >> 9;
}

Then call that on pbio at the same place we call bio_reset on sbio.

You could probably also call bio_rewind on sbio, and remove lots of that
code for setting the bio up again.
(or don't bother with bio_rewind, and use the same big lump of code on both
pbio and sbio).

Could you confirm that one of those works?

Thanks.

NeilBrown


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]