Date: Tue, 2 Sep 2014 10:27:46 -0700 From: John-Mark Gurney <jmg@funkthat.com> To: Roger Pau =?iso-8859-1?Q?Monn=E9?= <royger@FreeBSD.org> Cc: src-committers@FreeBSD.org, Alexander Motin <mav@FreeBSD.org>, scottl@FreeBSD.org, cperciva@FreeBSD.org, svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org Subject: Re: svn commit: r269814 - head/sys/dev/xen/blkfront Message-ID: <20140902172746.GY71691@funkthat.com> In-Reply-To: <20140902171841.GX71691@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>
next in thread | previous in thread | raw e-mail | index | archive | help
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."
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140902172746.GY71691>