From owner-svn-src-head@FreeBSD.ORG Wed Sep 3 16:03:21 2014 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id BC5C2640; Wed, 3 Sep 2014 16:03:21 +0000 (UTC) Received: from mail-la0-x22a.google.com (mail-la0-x22a.google.com [IPv6:2a00:1450:4010:c03::22a]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 1AA2E1E71; Wed, 3 Sep 2014 16:03:19 +0000 (UTC) Received: by mail-la0-f42.google.com with SMTP id mc6so10143727lab.1 for ; Wed, 03 Sep 2014 09:03:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=sender:message-id:date:from:user-agent:mime-version:to:cc:subject :references:in-reply-to:content-type:content-transfer-encoding; bh=/4P1Fuosth6xxyAGnExTlaVK46ATL8oTYWyyhzOfwwA=; b=JZg0WMQOTRjSFksX6nXkJM713ztT/brfF+MSX5wPxdSUgJ8hH7Ft6HBsbTPWK1QvTI yfMAI7PLffCIgcNH83Cw0Hx2/FWm1BT2ACvS1XaaYTBNzD8WTh0ueykwaOhR6UhCZKEB xmgqHtwV8WV7HOZV0bkvzR6CDC/ZjG9yKqyK85Fq0SaIbxjBCL0/z0IjGxtQ2uEX745a oEIGVL5V04jne+rOfl20HAIWn6Wl+zBBjL6yKUsxD0NP4W+7uLKQKl6JcWwXjEEci6/E DsDS1x0uAZfFC2Y0UzXjsOjUIfgZTf6Ra621oScb1+nAHfHmRX92al2HwsQtznfopyWt Xeqw== X-Received: by 10.152.4.9 with SMTP id g9mr43019213lag.14.1409760198036; Wed, 03 Sep 2014 09:03:18 -0700 (PDT) Received: from mavbook.mavhome.dp.ua ([134.249.139.101]) by mx.google.com with ESMTPSA id sr4sm1300574lac.39.2014.09.03.09.03.16 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Wed, 03 Sep 2014 09:03:17 -0700 (PDT) Sender: Alexander Motin Message-ID: <54073BC2.1000703@FreeBSD.org> Date: Wed, 03 Sep 2014 19:03:14 +0300 From: Alexander Motin User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:24.0) Gecko/20100101 Thunderbird/24.6.0 MIME-Version: 1.0 To: =?windows-1252?Q?Roger_Pau_Monn=E9?= , John-Mark Gurney Subject: Re: svn commit: r269814 - head/sys/dev/xen/blkfront 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> <5407385B.1000005@FreeBSD.org> In-Reply-To: <5407385B.1000005@FreeBSD.org> X-Enigmail-Version: 1.6 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Cc: src-committers@FreeBSD.org, svn-src-all@FreeBSD.org, scottl@FreeBSD.org, cperciva@FreeBSD.org, svn-src-head@FreeBSD.org, gibbs@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: Wed, 03 Sep 2014 16:03:21 -0000 On 03.09.2014 18:48, Roger Pau Monné wrote: > El 02/09/14 a les 19.18, John-Mark Gurney ha escrit: >> 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... > > Since Xen blkfront seems to be the only driver to have such segment > size requirements, No, it is not. I've already posted other examples I can recall: SDHCI, UHCI/OHCI and AHCI. Their limitations are different and less strict, but still may need handling. For SDHCI, since it is quite slow and has many other bugs, I practically implemented custom buffer bouncing. AHCI I suppose works only because limitation is only for even addresses, and odd ones happen extremely rarely (does not happen). For USB I am not sure, but at least umass driver does not support unmapped I/O. > it might be best to just fix blkfront to always > roundup segment size to 512, like the following: I think some coffee is needed here. ;) Rounding addresses won't make data properly aligned. Some copy is unavoidable in such cases. It would be good if it was done properly by default buffer bouncer. > diff --git a/sys/dev/xen/blkfront/blkfront.c b/sys/dev/xen/blkfront/blkfront.c > index 26b8f09..2d284d9 100644 > --- a/sys/dev/xen/blkfront/blkfront.c > +++ b/sys/dev/xen/blkfront/blkfront.c > @@ -209,7 +209,8 @@ xbd_queue_cb(void *arg, bus_dma_segment_t *segs, int nsegs, int error) > > buffer_ma = segs->ds_addr; > fsect = (buffer_ma & PAGE_MASK) >> XBD_SECTOR_SHFT; > - lsect = fsect + (segs->ds_len >> XBD_SECTOR_SHFT) - 1; > + lsect = fsect + (roundup2(segs->ds_len, 512) > + >> XBD_SECTOR_SHFT) - 1; > > KASSERT(lsect <= 7, ("XEN disk driver data cannot " > "cross a page boundary")); -- Alexander Motin