DragonFlyBSD

IIUC, in old-ATA, ad_strategy took care of transfer sizes larger than
the maximum I/O size of the device, but not in NATA (I guess GEOM layer
is handling it, right?). Adjusting si_iosize_max (initially faked to
be 131072 when attached) in ccdinit() seems to work around this problem.

Cheers.

Index: ccd.c ===================================================================
RCS file: /home/source/dragonfly/cvs/src/sys/dev/disk/ccd/ccd.c,v
retrieving revision 1.48
diff u -r1.48 ccd.c
-- ccd.c 19 Jun 2007 19:09:46 0000 1.48
++ ccd.c 9 Jul 2007 11:29:55 -0000
@ -381,6 +381,7 @
u_int64_t size;
u_int64_t minsize;
int maxsecsize;
int maxiosize;
struct partinfo dpart;
struct ccdgeom *ccg = &cs>sc_geom;
char tmppath[MAXPATHLEN];
@ -405,6 +406,7 @
*/
maxsecsize = 0;
minsize = 0;
+ maxiosize = cs->sc_dev->si_iosize_max;
for (ix = 0; ix < cs->sc_nccdisks; ix++) {
vp = ccd->ccd_vpp[ix];
ci = &cs->sc_cinfo[ix];
@ -427,6 +429,8 @
bcopy(tmppath, ci->ci_path, ci->ci_pathlen);

ci->ci_dev = vn_todev(vp);
+        if (maxiosize > ci->ci_dev->si_iosize_max)
+            maxiosize = ci->ci_dev->si_iosize_max;

/*
 * Get partition information for the component.
@ -574,6 +578,11 @
     cs->sc_flags |= CCDF_INITED;
     cs->sc_cflags = ccd->ccd_flags;    /* So we can find out later... */
     cs->sc_unit = ccd->ccd_unit;
+    if (bootverbose) {
+        kprintf("%s: ccd%d: si_iosize_max %d > %d\n", func,
+            ccd>ccd_unit, cs->sc_dev->si_iosize_max, maxiosize);
+    }
+    cs->sc_dev->si_iosize_max = maxiosize;
     return (0);
 fail:
     while (ci > cs->sc_cinfo) {

:IIUC, in old-ATA, ad_strategy took care of transfer sizes larger than
:the maximum I/O size of the device, but not in NATA (I guess GEOM layer
:is handling it, right?). Adjusting si_iosize_max (initially faked to
:be 131072 when attached) in ccdinit() seems to work around this problem.
:
:Cheers.

There's even an XXX comment next to that assignment.  At the time
    that assignment is made the CCD hasn't loaded its components yet
    so we do not know what the actual limitations are.

CCD has an iterator in ccdstart() to deal with components.  I think
    what we need to do is to add some logic to ccdbuffer() to incorporate
    the limits of the lower level device.

So, please try this patch.  I've included a kprintf() so we can get
    positive confirmation that it actually does what it is supposed to.

The advantage of this patch is that it does not artificially limit
    the max iosize for the CCD request itself.  e.g. if a person happens
    to configure an interleave of 64K across two disks CCD which are
    each limited to 64K I/O's, CCD will still process a 128K I/O in
    parallel.

Please note this patch is almost completely untested.

-Matt

Index: sys/ccdvar.h ===================================================================
RCS file: /cvs/src/sys/sys/ccdvar.h,v
retrieving revision 1.6
diff u -p -r1.6 ccdvar.h
-- sys/ccdvar.h 17 May 2007 03:20:11 -0000 1.6
++ sys/ccdvar.h 9 Jul 2007 17:29:55 -0000
@ -206,6 +206,7 @ int sc_flags; /* flags /
int sc_cflags; / configuration flags /
u_int64_t sc_size; / size of ccd in sectors /
int sc_ileave; / interleave /
int sc_maxiosize; / maximum I/O size /
u_int sc_nccdisks; / number of components /
#define CCD_MAXNDISKS 65536
struct ccdcinfo *sc_cinfo; / component info */
Index: dev/disk/ccd/ccd.c ===================================================================
RCS file: /cvs/src/sys/dev/disk/ccd/ccd.c,v
retrieving revision 1.48
diff u -p -r1.48 ccd.c
-- dev/disk/ccd/ccd.c 19 Jun 2007 19:09:46 0000 1.48
++ dev/disk/ccd/ccd.c 9 Jul 2007 17:42:52 -0000
@ -398,6 +398,7 @
/* Allocate space for the component info. */
cs>sc_cinfo = kmalloc(cs->sc_nccdisks * sizeof(struct ccdcinfo),
M_DEVBUF, M_WAITOK);
cs->sc_maxiosize = MAXPHYS;

/*
 * Verify that each component piece exists and record
@ -427,6 +428,10 @         ci->ci_path = kmalloc(ci->ci_pathlen, 
         bcopy(tmppath, ci->ci_path, ci->ci_pathlen);

ci->ci_dev = vn_todev(vp);
+        if (ci->ci_dev->si_iosize_max &&
+            cs->sc_maxiosize > ci->ci_dev->si_iosize_max) {
+            cs->sc_maxiosize = ci->ci_dev->si_iosize_max;
+        }

/*
 * Get partition information for the component.
@ -489,6 +494,8 @         ci->ci_skip = skip;
         ci->ci_size = size;
         cs->sc_size = size;
     }
    kprintf("ccd%d: max component iosize is %d\n",
+        cs->sc_unit, cs->sc_maxiosize);

/*
 * Don't allow the interleave to be smaller than
@ -1048,9 +1055,11 @     cbp->cb_buf.b_flags |= bio->bio_buf->b_
     cbp->cb_buf.b_data = addr;
     cbp->cb_vp = ci->ci_vp;
     if (cs->sc_ileave == 0)
-              cbc = dbtob((off_t)(ci->ci_size - cbn));
+        cbc = dbtob((off_t)(ci->ci_size - cbn));
     else
-              cbc = dbtob((off_t)(cs->sc_ileave - cboff));
+        cbc = dbtob((off_t)(cs->sc_ileave - cboff));
+    if (cbc > cs->sc_maxiosize)
+        cbc = cs->sc_maxiosize;
     cbp->cb_buf.b_bcount = (cbc < bcount) ? cbc : bcount;
      cbp->cb_buf.b_bufsize = cbp->cb_buf.b_bcount;

@ -1090,6 +1099,8 @ if (cs->sc_ileave == 0)
cbc = dbtob((off_t)(ci->ci_size - cbn));
else
cbc = dbtob((off_t)(cs->sc_ileave - cboff));
+ if (cbc > cs->sc_maxiosize)
+ cbc = cs->sc_maxiosize;
cbp->cb_buf.b_bcount = (cbc < bcount) ? cbc : bcount;
cbp->cb_buf.b_bufsize = cbp->cb_buf.b_bcount;

It worked fine as I tested lightly(-j2 buildworld). I'll try to test later
with different max iosize of component devices.

Thanks.

Hi, I tried to reduce the default max_iosize in ata_dmainit()
to 64 * DEV_BSIZE(=32768), expecting that it reduces si_iosize_max
as it's assigned ch->dma->max_iosize in ata_attach(), but now NATA
fails setting up DMA. Is there any other way to achive the reduced
iosize?

Cheers.

:On Tue, Jul 10, 2007 at 12:58:16PM +0900, YONETANI Tomokazu wrote:
:> It worked fine as I tested lightly(j2 buildworld). I'll try to test later
:> with different max iosize of component devices.
:
:Hi, I tried to reduce the default max_iosize in ata_dmainit()
:to 64 * DEV_BSIZE(=32768), expecting that it reduces si_iosize_max
:as it's assigned ch>dma->max_iosize in ata_attach(), but now NATA
:fails setting up DMA. Is there any other way to achive the reduced
:iosize?
:
:Cheers.

Are you running the patch for CCD?  Which I did commit.

-Matt
                    Matthew Dillon 
                    <dillon@backplane.com>

Yes, I saw that it was committed. I have the latest source,
and in this case CCD is irrelavant (because it happens on non-ccd
system, too). The system using CCD is working just fine as my
build machine.

Cheers.

:On Tue, Jul 17, 2007 at 10:52:05PM 0700, Matthew Dillon wrote:
:>
:> :On Tue, Jul 10, 2007 at 12:58:16PM +0900, YONETANI Tomokazu wrote:
:> :> It worked fine as I tested lightly(-j2 buildworld). I'll try to test later
:> :> with different max iosize of component devices.
:> :
:> :Hi, I tried to reduce the default max_iosize in ata_dmainit()
:> :to 64 * DEV_BSIZE(=32768), expecting that it reduces si_iosize_max
:> :as it's assigned ch>dma->max_iosize in ata_attach(), but now NATA
:> :fails setting up DMA. Is there any other way to achive the reduced
:> :iosize?
:> :
:> :Cheers.
:>
:> Are you running the patch for CCD? Which I did commit.
:
:Yes, I saw that it was committed. I have the latest source,
:and in this case CCD is irrelavant (because it happens on non-ccd
:system, too). The system using CCD is working just fine as my
:build machine.
:
:Cheers.

Can you get a ddb backtrace?

-Matt
                    Matthew Dillon 
                    <dillon@backplane.com>

Sure, are function names enough?

ata0: FAILURE - oversized DMA transfer attempt 37888 > 32768
Trace beginning at frame 0xc86155a4
ata_dmaload
ata_dmaload
ata_begin_transaction
ata_start
ata_queue_request
ad_strategy
dev_dstrategy
diskstrategy
dev_dstrategy_chain
spec_strategy
vop_strategy
vn_strategy
spec_getpages
vop_getpages
ffs_getpages
vop_getpages
vnode_pager_getpages
vm_fault_object
vm_fault_object_page
elf_load_section
exec_elf_imgact
kern_execve
sys_execve
start_init
fork_trampoline

:Sure, are function names enough?
:
:ata0: FAILURE - oversized DMA transfer attempt 37888 > 32768
:Trace beginning at frame 0xc86155a4
:ata_dmaload
:ata_dmaload
:ata_begin_transaction
:ata_start
:ata_queue_request
:ad_strategy
:dev_dstrategy
:diskstrategy
:dev_dstrategy_chain
:spec_strategy
:vop_strategy
:vn_strategy
:spec_getpages
:vop_getpages
:ffs_getpages
:vop_getpages
:vnode_pager_getpages
:vm_fault_object
:vm_fault_object_page
:elf_load_section
:exec_elf_imgact
:kern_execve
:sys_execve
:start_init
:fork_trampoline

Yah.  I see the problem.  spec_getpages() is not limiting the 
    transfer size.  And there's a secondary problem with spec_read()
    and spec_write() also not doing the right thing.  I'll work on it
    this afternoon.

-Matt

This is turning out to be a small headache, but it has to be fixed
before we can release so I'm going to slog throug it.

It turns out that the vnode pager getpages code, the swap pager getpages
    and putpages code, and the pageout code can all make calls to 
    vn_strategy() which ignore the device I/O size limit.  It is also
    possible for filesystems to attempt to do I/O to devices in larger
    chunks then IDE can handle though UFS filesystems are not normally
    created with a 64K block size so it doesn't actually happen at the
    moment.

The only proper place to fix this is in spec_strategy().  That is, to
    make specfs break up requests which are too large into smaller pieces.
    I am looking into how best to do this right now.

-Matt
                    Matthew Dillon 
                    <dillon@backplane.com>

Please test this patch. It survived a couple of a buildworld runs
on my test box with the chunk size forced to 4K but it does mess
with low level disk I/O so make sure anything important is backed up
first.

-Matt

Index: kern/subr_diskgpt.c ===================================================================
RCS file: /cvs/src/sys/kern/subr_diskgpt.c,v
retrieving revision 1.3
diff u -p -r1.3 subr_diskgpt.c
-- kern/subr_diskgpt.c 19 Jun 2007 06:07:57 -0000 1.3
+++ kern/subr_diskgpt.c 19 Jul 2007 02:42:50 -0000
@ -136,6 +136,9 @ error = EINVAL;
goto done;
}

+ /*
+ * XXX subject to device dma size limitations
+ */
bp2 = geteblk((int)(table_blocks * info->d_media_blksize));
bp2->b_bio1.bio_offset = (off_t)table_lba * info->d_media_blksize;
bp2->b_bcount = table_blocks * info->d_media_blksize;
Index: kern/subr_disklabel64.c ===================================================================
RCS file: /cvs/src/sys/kern/subr_disklabel64.c,v
retrieving revision 1.4
diff u -p -r1.4 subr_disklabel64.c
-- kern/subr_disklabel64.c 19 Jun 2007 06:39:06 -0000 1.4
+++ kern/subr_disklabel64.c 19 Jul 2007 02:43:56 -0000
@ -118,6 +118,9 @ size_t dlpcrcsize;
size_t bpsize;
int secsize;

+ /*
+ * XXX I/O size is subject to device DMA limitations
+ */
secsize = info->d_media_blksize;
bpsize = (sizeof(*dlp) + secsize - 1) & ~(secsize - 1);

@ -289,6 +292,9 @ int secsize;

lp = lpx.lab64;

+ /*
+ * XXX I/O size is subject to device DMA limitations
+ */
secsize = ssp->dss_secsize;
bpsize = (sizeof(*lp) + secsize - 1) & ~(secsize - 1);

Index: kern/vfs_aio.c ===================================================================
RCS file: /cvs/src/sys/kern/vfs_aio.c,v
retrieving revision 1.41
diff u -p -r1.41 vfs_aio.c
-- kern/vfs_aio.c 29 Jun 2007 21:54:08 0000 1.41
+++ kern/vfs_aio.c 19 Jul 2007 02:49:35 -0000
@ -944,8 +944,12 @ bp>b_error = 0;

crit_exit();

- /* Perform transfer. /
- dev_dstrategy(vp->v_rdev, &bp->b_bio1);
+ /
+ * Perform the transfer. vn_strategy must be used even though we
+ * know we have a device in order to deal with requests which exceed
+ * device DMA limitations.
+ */
+ vn_strategy(vp, &bp->b_bio1); ===================================================================
RCS file: /cvs/src/sys/vfs/specfs/spec_vnops.c,v
retrieving revision 1.51
diff u -p -r1.51 spec_vnops.c
-- vfs/specfs/spec_vnops.c 9 May 2007 00:53:36 -0000 1.51
+++ vfs/specfs/spec_vnops.c 19 Jul 2007 02:39:25 -0000
@ -60,6 +60,15 @ #include <sys/buf2.h>

#include <sys/thread2.h>

/*
* Specfs chained debugging (bitmask)
+ *
+ * 0 - disable debugging
+ * 1 - report chained I/Os
+ * 2 - force 4K chained I/Os
+ */
#define SPEC_CHAIN_DEBUG 0

static int spec_advlock (struct vop_advlock_args *);
static int spec_bmap (struct vop_bmap_args *);
static int spec_close (struct vop_close_args *);
@ -75,6 +84,7 @
static int spec_read (struct vop_read_args *);
static int spec_strategy (struct vop_strategy_args *);
static int spec_write (struct vop_write_args *);
+static void spec_strategy_done(struct bio *nbio); ===================================================================
RCS file: /cvs/src/sys/vm/vm_swap.c,v
retrieving revision 1.35
diff u -p -r1.35 vm_swap.c
-- vm/vm_swap.c 15 May 2007 22:44:21 -0000 1.35
+++ vm/vm_swap.c 19 Jul 2007 02:47:43 -0000
@ -142,8 +142,9 @ * Issue a strategy call on the appropr * bp->b_vp is not modified. Strategy code is always supposed to * use the passed vp. *
- * XXX do a dev_dstrategy() call on sp->sw_device instead of on
- * sp->sw_vp ?
+ * We have to use vn_strategy() here even if we know we have a
+ * device in order to properly break up requests which exceed the
+ * device's DMA limits.
*/
vn_strategy(sp->sw_vp, nbio);
return 0;

struct vop_ops spec_vnode_vops = {
     .vop_default =        vop_defaultop,
@ -438,7 +448,9 @     return (0);
 }

/*
- * Just call the device strategy routine
+ * Convert a vnode strategy call into a device strategy call.  Vnode strategy
+ * calls are not limited to device DMA limits so we have to deal with the
+ * case.
 *
 * spec_strategy(struct vnode *a_vp, struct bio *a_bio)
  */
@ -447,8 +459,11 @  */
{
     struct bio *bio = ap->a_bio;
     struct buf *bp = bio->bio_buf;
+    struct buf *nbp;
     struct vnode *vp;
     struct mount *mp;
+    int chunksize;
+    int maxiosize;

if (bp->b_cmd != BUF_CMD_READ &&
         (LIST_FIRST(&bp->b_dep)) != NULL && bioops.io_start) {
@ -474,11 +489,151 @             else
                 mp->mnt_stat.f_syncwrites++;
         }
     }
-    dev_dstrategy_chain(vp->v_rdev, bio);

        /*
+         * Device iosize limitations only apply to read and write.  Shortcut
+         * the I/O if it fits.
+         /
+    maxiosize = vp->v_rdev->si_iosize_max;
#if SPEC_CHAIN_DEBUG & 2
    maxiosize = 4096;
#endif
        if (bp->b_bcount <= maxiosize ||
+            (bp->b_cmd != BUF_CMD_READ && bp->b_cmd != BUF_CMD_WRITE)) {
+                dev_dstrategy_chain(vp->v_rdev, bio);
+                return (0);
+        }

    /
+     * Clone the buffer and set up an I/O chain to chunk up the I/O.
+     /
+    nbp = kmalloc(sizeof(*bp), M_DEVBUF, M_INTWAIT|M_ZERO);
+    initbufbio(nbp);
+    LIST_INIT(&nbp->b_dep);
+    BUF_LOCKINIT(nbp);
+    BUF_LOCK(nbp, LK_EXCLUSIVE);
+    BUF_KERNPROC(nbp);
+    nbp->b_vp = vp;
+    nbp->b_flags = B_PAGING | (bp->b_flags & B_BNOCLIP);
+    nbp->b_data = bp->b_data;
+    nbp->b_bio1.bio_done = spec_strategy_done;
+    nbp->b_bio1.bio_offset = bio->bio_offset;
+    nbp->b_bio1.bio_caller_info1.ptr = bio;

    /
+     * Start the first transfer
+     /
+    if (vn_isdisk(vp, NULL))
+        chunksize = vp->v_rdev->si_bsize_phys;
+    else
+        chunksize = DEV_BSIZE;
+    chunksize = maxiosize / chunksize * chunksize;
#if SPEC_CHAIN_DEBUG & 1
    kprintf("spec_strategy chained I/O chunksize=%d\n", chunksize);
#endif
    nbp->b_cmd = bp->b_cmd;
+    nbp->b_bcount = chunksize;
+    nbp->b_bufsize = chunksize;    / used to detect a short I/O */
+    nbp->b_bio1.bio_caller_info2.index = chunksize;

#if SPEC_CHAIN_DEBUG & 1
+    kprintf("spec_strategy: chain %p offset %d/%d bcount %d\n",
+        bp, 0, bp->b_bcount, nbp->b_bcount);
#endif

+    dev_dstrategy(vp->v_rdev, &nbp->b_bio1);
     return (0);
 }

/*
+ * Chunked up transfer completion routine - chain transfers until done
+ /
static
+void
+spec_strategy_done(struct bio *nbio)
{
+    struct buf *nbp = nbio->bio_buf;
+    struct bio *bio = nbio->bio_caller_info1.ptr;    / original bio /
+    struct buf *bp = bio->bio_buf;            / original bp /
+    int chunksize = nbio->bio_caller_info2.index;    / chunking /
+    int boffset = nbp->b_data - bp->b_data;

    if (nbp->b_flags & B_ERROR) {
+        /
+         * An error terminates the chain, propogate the error back
+         * to the original bp
+         /
+        bp->b_flags |= B_ERROR;
+        bp->b_error = nbp->b_error;
+        bp->b_resid = bp->b_bcount - boffset 
                  (nbp->b_bcount - nbp->b_resid);
#if SPEC_CHAIN_DEBUG & 1
        kprintf("spec_strategy: chain p error %d bcount %d/%d\n",
+            bp, bp->b_error, bp->b_bcount,
+            bp->b_bcount - bp->b_resid);
#endif
        kfree(nbp, M_DEVBUF);
+        biodone(bio);
+    } else if (nbp->b_resid) {
+        /
+         * A short read or write terminates the chain
+         /
+        bp->b_error = nbp->b_error;
+        bp->b_resid = bp->b_bcount - boffset 
                  (nbp->b_bcount - nbp->b_resid);
#if SPEC_CHAIN_DEBUG x%x 1
        kprintf("spec_strategy: chain p short read(1) bcount %d/%d\n",
+            bp, bp->b_bcount - bp->b_resid, bp->b_bcount);
#endif
        kfree(nbp, M_DEVBUF);
+        biodone(bio);
+    } else if (nbp->b_bcount != nbp->b_bufsize) {
+        /
+         * A short read or write can also occur by truncating b_bcount
+         /
#if SPEC_CHAIN_DEBUG x%x 1
        kprintf("spec_strategy: chain p short read(2) bcount %d/%d\n",
+            bp, nbp->b_bcount + boffset, bp->b_bcount);
#endif
        bp->b_error = 0;
+        bp->b_bcount = nbp->b_bcount + boffset; 
+        bp->b_resid = nbp->b_resid;
+        kfree(nbp, M_DEVBUF);
+        biodone(bio);
+    } else if (nbp->b_bcount + boffset == bp->b_bcount) {
+        /
+         * No more data terminates the chain
+         /
#if SPEC_CHAIN_DEBUG x%x 1
        kprintf("spec_strategy: chain p finished bcount %d\n",
+            bp, bp->b_bcount);
#endif
        bp->b_error = 0;
+        bp->b_resid = 0;
+        kfree(nbp, M_DEVBUF);
+        biodone(bio);
+    } else {
+        /
+         * Continue the chain
+         /
+        boffset = nbp->b_bcount;
        nbp->b_data = bp->b_data + boffset;
+        nbp->b_bcount = bp->b_bcount - boffset;
+        if (nbp->b_bcount > chunksize)
+            nbp->b_bcount = chunksize;
+        nbp->b_bio1.bio_done = spec_strategy_done;
+        nbp->b_bio1.bio_offset = bio->bio_offset + boffset;

#if SPEC_CHAIN_DEBUG x%x 1
+        kprintf("spec_strategy: chain %p offset %d/%d bcount %d\n",
+            bp, boffset, bp->b_bcount, nbp->b_bcount);
#endif

+        dev_dstrategy(nbp->b_vp->v_rdev, &nbp->b_bio1);
+    }
}

+/
 * spec_freeblks(struct vnode *a_vp, daddr_t a_addr, daddr_t a_length)
  */
 static int
Index: vm/vm_swap.c

Ok, I tried the patched kernel, with a modification to ata-dma.c to
reduce max DMA size of the second channel to 16k. ccd devices span
on these two channels, and one of them is used as /usr/obj and another
for /home. I observed no errors during `make -sj3 buildworld'. I also
tried to enlarge the stripe size from 1152 to 64k, but newfs barfed
on it (Invalid argument), so I could only use 65535.

Cheers.

:Ok, I tried the patched kernel, with a modification to ata-dma.c to
:reduce max DMA size of the second channel to 16k. ccd devices span
:on these two channels, and one of them is used as /usr/obj and another
:for /home. I observed no errors during `make -sj3 buildworld'. I also
: tried to enlarge the stripe size from 1152 to 64k, but newfs barfed
:on it (Invalid argument), so I could only use 65535.
:
:Cheers.

Excellent.

64k?  Those numbers are in sectors!  1152 = 576KB. 64K = 32MB stripe
    size.   I'll check over the CCD code to see why the larger stripe
    size didn't work, it should be something simple stupid.  Newfs should
    have had no visibility into the stripe size.

Could you explain the modification you made to ata-dma.c to
    reduce the max DMA size of the second channel?  Does your second
    channel not work without that?  I'd like to see the patch too.

I will commit the strategy fix now.

-Matt

This happens with unpatched kernel, too, and it's not limited to newfs. # hd /dev/ccd0s0a | less
displays the same error message, too.

I made the modification only to see if ccd works with components
with different DMA I/O sizes. It works with or without the modification.

Thanks!

:This happens with unpatched kernel, too, and it's not limited to newfs.
: # hd /dev/ccd0s0a | less
:displays the same error message, too.
:..
:
:I made the modification only to see if ccd works with components
:with different DMA I/O sizes. It works with or without the modification.
:
:..
:Thanks!

I haven't found the CCD interleave bug yet but I don't think its a
    show-stopper for the release since normal interleaves work.

We'll call the NATA issue fixed.

Core and I almost have hdac dealt with for the release.

Joerg has to make a fix to pkg_install before we can release.

-Matt

This can be closed.