On Wed, 3 Jul 2013 17:49:51 -0400 (EDT) Joe Lawrence
<joe.lawrence@xxxxxxxxxxx> 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:
signature.asc
Description: PGP signature
