From owner-svn-src-head@FreeBSD.ORG Thu Sep 4 14:59:36 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 4F127CA0; Thu, 4 Sep 2014 14:59:36 +0000 (UTC) Received: from mail-we0-x233.google.com (mail-we0-x233.google.com [IPv6:2a00:1450:400c:c03::233]) (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 05460137C; Thu, 4 Sep 2014 14:59:34 +0000 (UTC) Received: by mail-we0-f179.google.com with SMTP id t60so10273201wes.38 for ; Thu, 04 Sep 2014 07:59:33 -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=WlUf4QtExXxuzCgG4OgCOewyY32pi5j3OMOlziHnsA0=; b=UsE1G7dDxhxfo4BBfREuzRn5GEtJLcYqllMnhkxV+irsrEsq8zNpRmNj5bmLm5JGRx JBqqpc3cq97l3aZxVxaYno19J0HGiDEfDG+B0Cdcj0GZeINDgH5qUN61m7HjYamOrbC5 MYs+1soPLhBmgixCnA/pW7JuYW/NNQ162j4awSWCwMU0bWXIftoufo/o9L9+H86RnWPU Tt4FEyWS/0ZeE9/Uf4DR/BgYVs0TLgLCogQ44ZSvTmoMQw2A0AXHfWTr5iTWCxVqNMgy Weln9m8uPZKhXu0qWN3fVDensd6nRrdzu8Deqn2X7xk/3k0u1vOZP5xA6W6Q0V4cVs+r MAsw== X-Received: by 10.180.102.130 with SMTP id fo2mr6553087wib.29.1409842772586; Thu, 04 Sep 2014 07:59:32 -0700 (PDT) Received: from [172.16.1.30] (124.Red-83-33-238.dynamicIP.rima-tde.net. [83.33.238.124]) by mx.google.com with ESMTPSA id kw2sm19681177wjb.30.2014.09.04.07.59.30 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 04 Sep 2014 07:59:31 -0700 (PDT) Sender: =?UTF-8?Q?Roger_Pau_Monn=C3=A9?= Message-ID: <54087E4B.7070405@FreeBSD.org> Date: Thu, 04 Sep 2014 16:59:23 +0200 From: =?windows-1252?Q?Roger_Pau_Monn=E9?= User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:31.0) Gecko/20100101 Thunderbird/31.1.0 MIME-Version: 1.0 To: Alexander Motin , 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> <54073BC2.1000703@FreeBSD.org> In-Reply-To: <54073BC2.1000703@FreeBSD.org> 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: Thu, 04 Sep 2014 14:59:36 -0000 El 03/09/14 a les 18.03, Alexander Motin ha escrit: > 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. I've just reverted the commit, will look into fixing busdma when I'm back from vacations :). I really wanted to get this into 10.1 because it makes a noticeable speed improvement, but I think it's going to miss the release. Roger.