| Message ID | 1476118086-2975-1-git-send-email-heinzm@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Mike Snitzer |
| Headers | show |
On Mon, Oct 10 2016 at 12:48pm -0400, Heinz Mauelshagen <heinzm@redhat.com> wrote: > In case legs of a "mirror" target fail, any read will cause a new, > operational default leg to be selected and the read be resubmitted > to it. If that new default leg fails the read too, no other still > accessible legs are used to resubmit the read again thus failing > the io. > > Fix by allowing the read to get resubmitted until there's no > operational legs any more. > > Resolves: rhbz1383444 > > Signed-off-by: Heinz Mauelshagen <heinzm@redhat.com> Nothing seems to be checking bio_record->details.bi_bdev anymore. (The one you've removed really seems like a complete hack to begin with) Shouldn't this patch go further by removing the other 2 places that set bio_record->details.bi_bdev = NULL; ? > --- > drivers/md/dm-raid1.c | 11 ----------- > 1 file changed, 11 deletions(-) > > diff --git a/drivers/md/dm-raid1.c b/drivers/md/dm-raid1.c > index 7a6254d..dd31019 100644 > --- a/drivers/md/dm-raid1.c > +++ b/drivers/md/dm-raid1.c > @@ -1266,16 +1266,6 @@ static int mirror_end_io(struct dm_target *ti, struct bio *bio, int error) > goto out; > > if (unlikely(error)) { > - if (!bio_record->details.bi_bdev) { > - /* > - * There wasn't enough memory to record necessary > - * information for a retry or there was no other > - * mirror in-sync. > - */ > - DMERR_LIMIT("Mirror read failed."); > - return -EIO; > - } > - > m = bio_record->m; > > DMERR("Mirror read failed from %s. Trying alternative device.", > @@ -1291,7 +1281,6 @@ static int mirror_end_io(struct dm_target *ti, struct bio *bio, int error) > bd = &bio_record->details; > > dm_bio_restore(bd, bio); > - bio_record->details.bi_bdev = NULL; > bio->bi_error = 0; > > queue_bio(ms, bio, rw); > -- > 2.7.4 > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On 10/10/2016 10:41 PM, Mike Snitzer wrote: > On Mon, Oct 10 2016 at 12:48pm -0400, > Heinz Mauelshagen <heinzm@redhat.com> wrote: > >> In case legs of a "mirror" target fail, any read will cause a new, >> operational default leg to be selected and the read be resubmitted >> to it. If that new default leg fails the read too, no other still >> accessible legs are used to resubmit the read again thus failing >> the io. >> >> Fix by allowing the read to get resubmitted until there's no >> operational legs any more. >> >> Resolves: rhbz1383444 >> >> Signed-off-by: Heinz Mauelshagen <heinzm@redhat.com> > Nothing seems to be checking bio_record->details.bi_bdev anymore. > (The one you've removed really seems like a complete hack to begin with) The very point of removing "if (!bio_record->details.bi_bdev) { ..." is to allow for resubmission of read bios to any other still operational leg after an initial resubmission failed. do_reads() will conditionally error the bio in case there are none or no in sync ones available. The "mirror" processing for read bios is: 1) bio gets remapped to the default mirror in mirror_map() if region is in sync our queued to the worker for submission 2) mirror_end_io() spots read error and requeues to the worker 3) worker - submits bio to new, selected default if respective region is in sync or - fails bio if it aims at a non synchronized region 4) if not already failed in do_reads(), mirror_end_io() processes like in step 1 _but_ errors the read because of the "if (!bio_record->details.bi_bdev) { ..." My patch changes step 4 by removing the conditional to resubmit it again in case any other operational leg is available. If none's available, mirror_end_io() errors the read bios or the do_reads error logic applies in case of a resubmission. Why do you think this to be a hack? > > Shouldn't this patch go further by removing the other 2 places that set > bio_record->details.bi_bdev = NULL; ? Yes, that's a good cleanup. Let's first settle any still open issues like other unspotted side effects and I'll do a patch v2 with it. > >> --- >> drivers/md/dm-raid1.c | 11 ----------- >> 1 file changed, 11 deletions(-) >> >> diff --git a/drivers/md/dm-raid1.c b/drivers/md/dm-raid1.c >> index 7a6254d..dd31019 100644 >> --- a/drivers/md/dm-raid1.c >> +++ b/drivers/md/dm-raid1.c >> @@ -1266,16 +1266,6 @@ static int mirror_end_io(struct dm_target *ti, struct bio *bio, int error) >> goto out; >> >> if (unlikely(error)) { >> - if (!bio_record->details.bi_bdev) { >> - /* >> - * There wasn't enough memory to record necessary >> - * information for a retry or there was no other >> - * mirror in-sync. >> - */ >> - DMERR_LIMIT("Mirror read failed."); >> - return -EIO; >> - } >> - >> m = bio_record->m; >> >> DMERR("Mirror read failed from %s. Trying alternative device.", >> @@ -1291,7 +1281,6 @@ static int mirror_end_io(struct dm_target *ti, struct bio *bio, int error) >> bd = &bio_record->details; >> >> dm_bio_restore(bd, bio); >> - bio_record->details.bi_bdev = NULL; >> bio->bi_error = 0; >> >> queue_bio(ms, bio, rw); >> -- >> 2.7.4 >> > -- > dm-devel mailing list > dm-devel@redhat.com > https://www.redhat.com/mailman/listinfo/dm-devel -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
diff --git a/drivers/md/dm-raid1.c b/drivers/md/dm-raid1.c index 7a6254d..dd31019 100644 --- a/drivers/md/dm-raid1.c +++ b/drivers/md/dm-raid1.c @@ -1266,16 +1266,6 @@ static int mirror_end_io(struct dm_target *ti, struct bio *bio, int error) goto out; if (unlikely(error)) { - if (!bio_record->details.bi_bdev) { - /* - * There wasn't enough memory to record necessary - * information for a retry or there was no other - * mirror in-sync. - */ - DMERR_LIMIT("Mirror read failed."); - return -EIO; - } - m = bio_record->m; DMERR("Mirror read failed from %s. Trying alternative device.", @@ -1291,7 +1281,6 @@ static int mirror_end_io(struct dm_target *ti, struct bio *bio, int error) bd = &bio_record->details; dm_bio_restore(bd, bio); - bio_record->details.bi_bdev = NULL; bio->bi_error = 0; queue_bio(ms, bio, rw);
In case legs of a "mirror" target fail, any read will cause a new, operational default leg to be selected and the read be resubmitted to it. If that new default leg fails the read too, no other still accessible legs are used to resubmit the read again thus failing the io. Fix by allowing the read to get resubmitted until there's no operational legs any more. Resolves: rhbz1383444 Signed-off-by: Heinz Mauelshagen <heinzm@redhat.com> --- drivers/md/dm-raid1.c | 11 ----------- 1 file changed, 11 deletions(-)