Date: Wed, 03 Sep 2014 19:03:14 +0300 From: Alexander Motin <mav@FreeBSD.org> To: =?windows-1252?Q?Roger_Pau_Monn=E9?= <royger@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: <54073BC2.1000703@FreeBSD.org> In-Reply-To: <5407385B.1000005@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>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?54073BC2.1000703>