From owner-svn-src-head@FreeBSD.ORG Tue Sep 2 17:27:48 2014 Return-Path: Delivered-To: svn-src-head@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 1BBE1E8B; Tue, 2 Sep 2014 17:27:48 +0000 (UTC) Received: from h2.funkthat.com (gate2.funkthat.com [208.87.223.18]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client CN "funkthat.com", Issuer "funkthat.com" (not verified)) by mx1.freebsd.org (Postfix) with ESMTPS id E397819DC; Tue, 2 Sep 2014 17:27:47 +0000 (UTC) Received: from h2.funkthat.com (localhost [127.0.0.1]) by h2.funkthat.com (8.14.3/8.14.3) with ESMTP id s82HRkVN057293 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Tue, 2 Sep 2014 10:27:47 -0700 (PDT) (envelope-from jmg@h2.funkthat.com) Received: (from jmg@localhost) by h2.funkthat.com (8.14.3/8.14.3/Submit) id s82HRkkk057292; Tue, 2 Sep 2014 10:27:46 -0700 (PDT) (envelope-from jmg) Date: Tue, 2 Sep 2014 10:27:46 -0700 From: John-Mark Gurney To: Roger Pau =?iso-8859-1?Q?Monn=E9?= Subject: Re: svn commit: r269814 - head/sys/dev/xen/blkfront Message-ID: <20140902172746.GY71691@funkthat.com> References: <53e8e31e.2179.30c1c657@svn.freebsd.org> <53FF7386.3050804@FreeBSD.org> <20140828184515.GV71691@funkthat.com> <53FF7BC4.6050801@FreeBSD.org> <5400BDC7.7020902@FreeBSD.org> <54058E1E.4050907@FreeBSD.org> <20140902171841.GX71691@funkthat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20140902171841.GX71691@funkthat.com> User-Agent: Mutt/1.4.2.3i X-Operating-System: FreeBSD 7.2-RELEASE i386 X-PGP-Fingerprint: 54BA 873B 6515 3F10 9E88 9322 9CB1 8F74 6D3F A396 X-Files: The truth is out there X-URL: http://resnet.uoregon.edu/~gurney_j/ X-Resume: http://resnet.uoregon.edu/~gurney_j/resume.html X-TipJar: bitcoin:13Qmb6AeTgQecazTWph4XasEsP7nGRbAPE X-to-the-FBI-CIA-and-NSA: HI! HOW YA DOIN? can i haz chizburger? X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.2.2 (h2.funkthat.com [127.0.0.1]); Tue, 02 Sep 2014 10:27:47 -0700 (PDT) Cc: src-committers@FreeBSD.org, Alexander Motin , scottl@FreeBSD.org, cperciva@FreeBSD.org, svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 02 Sep 2014 17:27:48 -0000 John-Mark Gurney wrote this message on Tue, Sep 02, 2014 at 10:18 -0700: > Roger Pau Monn wrote this message on Tue, Sep 02, 2014 at 11:30 +0200: > > El 29/08/14 a les 19.52, Roger Pau Monné ha escrit: > > > El 28/08/14 a les 20.58, Alexander Motin ha escrit: > > >> On 28.08.2014 21:45, John-Mark Gurney wrote: > > >>> Alexander Motin wrote this message on Thu, Aug 28, 2014 at 21:23 +0300: > > >>>> Hi, Roger. > > >>>> > > >>>> It looks to me like this commit does not work as it should. I got > > >>>> problem when I just tried `newfs /dev/ada0 ; mount /dev/ada0 /mnt`. > > >>>> Somehow newfs does not produce valid filesystem. Problem is reliably > > >>>> repeatable and reverting this commit fixes it. > > >>>> > > >>>> I found at least one possible cause there: If original data buffer is > > >>>> unmapped, misaligned and not physically contiguous, then present x86 > > >>>> bus_dmamap_load_bio() implementation will process each physically > > >>>> contiguous segment separately. Due to the misalignment first and last > > >>>> physical segments may have size not multiple to 512 bytes. Since each > > >>>> segment processed separately, they are not joined together, and > > >>>> xbd_queue_cb() is getting segments not multiple to 512 bytes. Attempt to > > >>>> convert them to exact number of sectors in the driver cause data corruption. > > >>> > > >>> Are you sure this isn't a problem w/ the tag not properly specifying > > >>> the correct alignement? > > >> > > >> I don't know how to specify it stronger then this: > > >> error = bus_dma_tag_create( > > >> bus_get_dma_tag(sc->xbd_dev), /* parent */ > > >> 512, PAGE_SIZE, /* algnmnt, boundary */ > > >> BUS_SPACE_MAXADDR, /* lowaddr */ > > >> BUS_SPACE_MAXADDR, /* highaddr */ > > >> NULL, NULL, /* filter, filterarg */ > > >> sc->xbd_max_request_size, > > >> sc->xbd_max_request_segments, > > >> PAGE_SIZE, /* maxsegsize */ > > >> BUS_DMA_ALLOCNOW, /* flags */ > > >> busdma_lock_mutex, /* lockfunc */ > > >> &sc->xbd_io_lock, /* lockarg */ > > >> &sc->xbd_io_dmat); > > >> > > >>> Also, I don't think there is a way for busdma > > >>> to say that you MUST have a segment be a multiple of 512, though you > > >>> could use a 512 boundary, but that would force all segments to only be > > >>> 512 bytes... > > >> > > >> As I understand, that is mandatory requirement for this "hardware". > > >> Alike 4K alignment requirement also exist at least for SDHCI, and IIRC > > >> UHCI/OHCI hardware. Even AHCI requires both segment addresses and > > >> lengths to be even. > > >> > > >> I may be wrong, but I think it is quite likely that hardware that > > >> requires segment address alignment quite likely will have the same > > >> requirements for segments length. > > > > Hello, > > > > I have the following fix, which makes sure the total length and the > > size of each segment is aligned. I'm not very knowledgeable of the > > busdma code, so someone has to review it. > > I feel that this alignment should only be enforced via a new option on > the tag... I don't see how alignment and segment size should be > conflated... I could totally see a device that requires an alignement > of 8 bytes, but has a segment size of 16, or vice versa, and requiring > them to be the same means we will bounce unnecesarily... > > cc'd scottl since he knows this code better than I... and cperciva as > he touched it for similar reasons.. > > Oh, I just found PR 152818, where cperciva did a similar fix to > bounce_bus_dmamap_load_buffer for the exact same reason... It was > committed in r216194... Also, if this change is made w/o adding a field to the tag, the busdma man page needs to be updated w/ the fact that alignment also forces segment sizes to be alignment sized... cperciva forgot this when he did his commit... -- John-Mark Gurney Voice: +1 415 225 5579 "All that I will do, has been done, All that I have, has not."