Date: Fri, 13 Jul 2012 18:37:20 -0400 From: Mark Johnston <markjdb@gmail.com> To: Scott Long <scott4long@yahoo.com> Cc: freebsd-scsi@freebsd.org Subject: Re: [patch] MFI should set bio_resid on command failure Message-ID: <20120713223720.GA51989@oddish.sandvine.com> In-Reply-To: <1341950347.63294.YahooMailNeo@web45701.mail.sp1.yahoo.com> References: <909AAC62-7BB4-43E7-B04B-27466B038A07@averesystems.com> <1341340916.3370.6.camel@powernoodle.corp.yahoo.com> <2936EBFD-CA1E-4EC4-9790-80D1A5DC5567@averesystems.com> <1341938629.2573.7.camel@powernoodle.corp.yahoo.com> <1341950347.63294.YahooMailNeo@web45701.mail.sp1.yahoo.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, Jul 10, 2012 at 12:59:07PM -0700, Scott Long wrote:
>
>
>
>
> ----- Original Message -----
> > From: Sean Bruno <seanbru@yahoo-inc.com>
> > To: Andrew Boyer <aboyer@averesystems.com>
> > Cc: "freebsd-scsi@freebsd.org" <freebsd-scsi@freebsd.org>; scottl@freebsd.org; "ambrisko@FreeBSD.ORG" <ambrisko@freebsd.org>
> > Sent: Tuesday, July 10, 2012 10:43 AM
> > Subject: Re: [patch] MFI should set bio_resid on command failure
> >
> > On Tue, 2012-07-10 at 09:37 -0700, Andrew Boyer wrote:
> >> On Jul 3, 2012, at 2:41 PM, Sean Bruno wrote:
> >>
> >> > On Tue, 2012-07-03 at 06:47 -0700, Andrew Boyer wrote:
> >> >> When an MFI command fails, the driver needs to set
> > bio->bio_resid so that the upper levels notice. Otherwise we see commands
> > silently failing leading to data corruption. This mirrors dadone().
> >> >>
> >> >> -Andrew
> >> >>
> >> >> Index: sys/dev/mfi/mfi_disk.c
> >> >>
> > ===================================================================
> >> >> --- sys/dev/mfi/mfi_disk.c (revision 238071)
> >> >> +++ sys/dev/mfi/mfi_disk.c (working copy)
> >> >> @@ -298,6 +298,7 @@
> >> >> hdr = bio->bio_driver1;
> >> >>
> >> >> if (bio->bio_flags & BIO_ERROR) {
> >> >> + bio->bio_resid = bio->bio_bcount;
> >> >> if (bio->bio_error == 0)
> >> >> bio->bio_error = EIO;
> >> >> disk_err(bio, "hard error", -1, 1);
> >> >>
> >> >> --------------------------------------------------
> >> >> Andrew Boyer aboyer@averesystems.com
> >> >>
> >> >
> >> > This looks right to me. It mirrors the behavior in
> > mfi_disk_strategy()
> >> > as well.
> >> >
> >> > Sean
> >> >
> >>
> >> Anyone interested in committing? (And MFC'ing to stable/8…)
> >>
> >> -Andrew
> >>
> >> --------------------------------------------------
> >> Andrew Boyer aboyer@averesystems.com
> >>
> >>
> >>
> >
> > Let me hit dougA and scottl for validation. Their wisdom here can
> > validate my naivete.
> >
>
>
> Yeah, b_resid needs to be set, and a lot of drivers get this wrong.
>
> Scott
It seems that the analogous line is also missing from aac(4). Is the
patch below correct? Or am I missing something?
Thanks,
-Mark
diff --git a/sys/dev/aac/aac_disk.c b/sys/dev/aac/aac_disk.c
index e4042f7..f2f6636 100644
--- a/sys/dev/aac/aac_disk.c
+++ b/sys/dev/aac/aac_disk.c
@@ -334,8 +334,10 @@ aac_biodone(struct bio *bp)
{
fwprintf(NULL, HBA_FLAGS_DBG_FUNCTION_ENTRY_B, "");
- if (bp->bio_flags & BIO_ERROR)
+ if (bp->bio_flags & BIO_ERROR) {
+ bp->bio_resid = bp->bio_bcount;
disk_err(bp, "hard error", -1, 1);
+ }
biodone(bp);
}
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120713223720.GA51989>