Date: Fri, 29 Aug 2014 19:52:07 +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: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r269814 - head/sys/dev/xen/blkfront Message-ID: <5400BDC7.7020902@FreeBSD.org> In-Reply-To: <53FF7BC4.6050801@FreeBSD.org> References: <53e8e31e.2179.30c1c657@svn.freebsd.org> <53FF7386.3050804@FreeBSD.org> <20140828184515.GV71691@funkthat.com> <53FF7BC4.6050801@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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.
I've been able to trigger it quite easily by adding the following
KASSERTs and got a "panic: Invalid segment size 2080". I will try to
look into it tomorrow, or else revert the commit. Thanks for noticing.
Roger.
---
diff --git a/sys/dev/xen/blkfront/blkfront.c b/sys/dev/xen/blkfront/blkfront.c
index 7a1c974..6226534 100644
--- a/sys/dev/xen/blkfront/blkfront.c
+++ b/sys/dev/xen/blkfront/blkfront.c
@@ -199,6 +199,13 @@ xbd_queue_cb(void *arg, bus_dma_segment_t *segs, int nsegs, int error)
while (1) {
while (sg < last_block_sg) {
+
+ KASSERT((segs->ds_len % 512) == 0,
+ ("Invalid segment size %u", segs->ds_len));
+ KASSERT((segs->ds_addr % 512) == 0,
+ ("Invalid segment alignment ds_addr: %jx",
+ (uintmax_t) segs->ds_addr));
+
buffer_ma = segs->ds_addr;
fsect = (buffer_ma & PAGE_MASK) >> XBD_SECTOR_SHFT;
lsect = fsect + (segs->ds_len >> XBD_SECTOR_SHFT) - 1;
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?5400BDC7.7020902>