From owner-freebsd-scsi@FreeBSD.ORG Fri Jul 13 22:35:42 2012 Return-Path: Delivered-To: freebsd-scsi@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id A8253106566C for ; Fri, 13 Jul 2012 22:35:42 +0000 (UTC) (envelope-from markjdb@gmail.com) Received: from mail-pb0-f54.google.com (mail-pb0-f54.google.com [209.85.160.54]) by mx1.freebsd.org (Postfix) with ESMTP id 73C558FC0A for ; Fri, 13 Jul 2012 22:35:42 +0000 (UTC) Received: by pbbro2 with SMTP id ro2so6757867pbb.13 for ; Fri, 13 Jul 2012 15:35:42 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:content-transfer-encoding :in-reply-to:user-agent; bh=xR4QllR6d9UyYEWNS5LT9Rg/Mdrtny+3kfRdU4ZtY7A=; b=HFrqejEGGwoTV3Dohf/Ec/eY/Mi/Z08RH/0jqKxN0UEClsfa44OCJVrv0L0xCKwMAR 0how/1QPolLFixsQyRl/dp1s+xqKVcy7yG/fJV9gvBW1VASSS2nUu2lqMpdt3wmC7Ck8 kPjr7pD9AnVGiuozJ9Uom+OHHzqag4gH3ne1NCTf/g+hI0LUhWuCP9QhVQeYj3HJWFs2 EkH7+7VkpEBFUk4ZY8Lz5vMW+BXbeRMkamb2cCMpmggH/NTR7WeXQzRVjlqCorHbPID1 EtYbxYj1bE5e+P/Tu1Dz+D7KWICWmAvzl9/vsLHH5x0nzDi2Xiq98rtq2wKJn1gtw3hi YGKA== Received: by 10.68.226.131 with SMTP id rs3mr6901817pbc.62.1342218942042; Fri, 13 Jul 2012 15:35:42 -0700 (PDT) Received: from oddish.sandvine.com ([64.7.137.182]) by mx.google.com with ESMTPS id oi5sm5493914pbb.39.2012.07.13.15.35.39 (version=TLSv1/SSLv3 cipher=OTHER); Fri, 13 Jul 2012 15:35:40 -0700 (PDT) Date: Fri, 13 Jul 2012 18:37:20 -0400 From: Mark Johnston To: Scott Long Message-ID: <20120713223720.GA51989@oddish.sandvine.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> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1341950347.63294.YahooMailNeo@web45701.mail.sp1.yahoo.com> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: freebsd-scsi@freebsd.org Subject: Re: [patch] MFI should set bio_resid on command failure X-BeenThere: freebsd-scsi@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: SCSI subsystem List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 13 Jul 2012 22:35:42 -0000 On Tue, Jul 10, 2012 at 12:59:07PM -0700, Scott Long wrote: > > > > > ----- Original Message ----- > > From: Sean Bruno > > To: Andrew Boyer > > Cc: "freebsd-scsi@freebsd.org" ; scottl@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); }