Date: Thu, 04 Sep 2014 16:59:23 +0200 From: =?windows-1252?Q?Roger_Pau_Monn=E9?= <royger@FreeBSD.org> To: Alexander Motin <mav@FreeBSD.org>, John-Mark Gurney <jmg@funkthat.com> Cc: src-committers@FreeBSD.org, svn-src-all@FreeBSD.org, scottl@FreeBSD.org, cperciva@FreeBSD.org, svn-src-head@FreeBSD.org, gibbs@freebsd.org Subject: Re: svn commit: r269814 - head/sys/dev/xen/blkfront Message-ID: <54087E4B.7070405@FreeBSD.org> In-Reply-To: <54073BC2.1000703@FreeBSD.org> 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>
next in thread | previous in thread | raw e-mail | index | archive | help
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.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?54087E4B.7070405>