Re: softraid crypto: preallocate crypops and dma buffers.
"Todd T. Fries" writes: > Penned by roberth on 20110620 21:05.14, we have: > | On Mon, 20 Jun 2011 20:12:28 -0500 > | Marco Peereboom wrote: > | > | > I am liking this diff quite a bit but it needs more testers. So if > | > you are using softraid crypto please try this diff. > | > | Still working for me. > > And me. > > Volume Status Size Device > softraid0 0 Online 262939136 sd0 RAID1 > 0 Online 262939136 0:0.0 noencl > 1 Online 262939136 0:1.0 noencl > softraid0 1 Online99755925504 sd1 CRYPTO > 0 Online99755925504 1:0.0 noencl > softraid0 2 Online 9228489728 sd2 CRYPTO > 0 Online 9228489728 2:0.0 noencl And me for 2 days (on a simpler setup and already reported privately to Owain): Volume Status Size Device softraid0 0 Online53381796864 sd0 CRYPTO 0 Online53381796864 0:0.0 noencl -- Manuel Giraud
Re: softraid crypto: preallocate crypops and dma buffers.
Penned by roberth on 20110620 21:05.14, we have: | On Mon, 20 Jun 2011 20:12:28 -0500 | Marco Peereboom wrote: | | > I am liking this diff quite a bit but it needs more testers. So if | > you are using softraid crypto please try this diff. | | Still working for me. And me. Volume Status Size Device softraid0 0 Online 262939136 sd0 RAID1 0 Online 262939136 0:0.0 noencl 1 Online 262939136 0:1.0 noencl softraid0 1 Online99755925504 sd1 CRYPTO 0 Online99755925504 1:0.0 noencl softraid0 2 Online 9228489728 sd2 CRYPTO 0 Online 9228489728 2:0.0 noencl -- Todd Fries .. t...@fries.net _ | \ 1.636.410.0632 (voice) | Free Daemon Consulting, LLC \ 1.405.227.9094 (voice) | http://FreeDaemonConsulting.com \ 1.866.792.3418 (FAX) | 2525 NW Expy #525, Oklahoma City, OK 73112 \ sip:freedae...@ekiga.net | "..in support of free software solutions." \ sip:4052279...@ekiga.net \\ 37E7 D3EB 74D0 8D66 A68D B866 0326 204E 3F42 004A http://todd.fries.net/pgp.txt
Re: softraid crypto: preallocate crypops and dma buffers.
On Mon, 20 Jun 2011 20:12:28 -0500 Marco Peereboom wrote: > I am liking this diff quite a bit but it needs more testers. So if > you are using softraid crypto please try this diff. Still working for me.
Re: softraid crypto: preallocate crypops and dma buffers.
I am liking this diff quite a bit but it needs more testers. So if you are using softraid crypto please try this diff. On Fri, Jun 17, 2011 at 07:53:05PM +0100, Owain Ainsworth wrote: > So here's the problem: ENOMEM on io in a hba driver is bad juju. > > In more detail: > > When preparing for each io softraid crypto does a > sr_crypto_getcryptop(). This operation does a few things: > > - allocate a uio and iovec from a pool. > - if this is a read then allocate a dma buffer with dma_alloc (up to >MAXPHYS, i.e. 64k) > - allocate a cryptop to be used with the crypto(9) framework. > - initialise the above for use in the io. > > So if you are getting very low on memory, one of these operations > (probably the dma alloc) will fail. when this happens softraid returns > XS_DRIVER_STUFFUP to scsi land. This returns an EIO back to callers. > > Now it is fairly common to use softdep in these situations, synchronous > filesystems are slw. softdep has certain assumptions about how > things will work. if a queued dependancy fails that invalidates these > assumptions. softraid panics in this case. I used to hit this repeatedly > (several times a day) before I bought more memory. > > in general, most disk controllers prealloc everything they need (that > isn't passed down to them) so that this won't be a problem. > > This diff does the same for softraid crypto. It'll eat just over 1meg of > kva, but it is hardly the only driver to need to do that. > > The only slightly scary thing here is the cryptodesc list manipulations > (the comments explain why they are necesary). > > I'm typing from a machine that runs this. todd@ is also testing it and > marco kindly did some horrible io torture on a machine running this too > and it runs quite nicely. > > A side benefit is that due to missing out allocations in every disk io > this is actually slightly faster in io too. > > More testing would be appreciated. Cheers, > > -0- > > diff --git dev/softraid_crypto.c dev/softraid_crypto.c > index 3259121..a60824e 100644 > --- dev/softraid_crypto.c > +++ dev/softraid_crypto.c > @@ -56,9 +56,26 @@ > #include > #include > > -struct cryptop *sr_crypto_getcryptop(struct sr_workunit *, int); > +/* > + * the per-io data that we need to preallocate. We can't afford to allow io > + * to start failing when memory pressure kicks in. > + * We can store this in the WU because we assert that only one > + * ccb per WU will ever be active. > + */ > +struct sr_crypto_wu { > + TAILQ_ENTRY(sr_crypto_wu)cr_link; > + struct uio cr_uio; > + struct iovec cr_iov; > + struct cryptop *cr_crp; > + struct cryptodesc *cr_descs; > + struct sr_workunit *cr_wu; > + void*cr_dmabuf; > +}; > + > + > +struct sr_crypto_wu *sr_crypto_wu_get(struct sr_workunit *, int); > +void sr_crypto_wu_put(struct sr_crypto_wu *); > int sr_crypto_create_keys(struct sr_discipline *); > -void *sr_crypto_putcryptop(struct cryptop *); > int sr_crypto_get_kdf(struct bioc_createraid *, > struct sr_discipline *); > int sr_crypto_decrypt(u_char *, u_char *, u_char *, size_t, int); > @@ -78,7 +95,7 @@ int sr_crypto_meta_opt_load(struct sr_discipline *, > struct sr_meta_opt *); > int sr_crypto_write(struct cryptop *); > int sr_crypto_rw(struct sr_workunit *); > -int sr_crypto_rw2(struct sr_workunit *, struct cryptop *); > +int sr_crypto_rw2(struct sr_workunit *, struct sr_crypto_wu *); > void sr_crypto_intr(struct buf *); > int sr_crypto_read(struct cryptop *); > void sr_crypto_finish_io(struct sr_workunit *); > @@ -230,40 +247,35 @@ done: > return (rv); > } > > -struct cryptop * > -sr_crypto_getcryptop(struct sr_workunit *wu, int encrypt) > + > +struct sr_crypto_wu * > +sr_crypto_wu_get(struct sr_workunit *wu, int encrypt) > { > struct scsi_xfer*xs = wu->swu_xs; > struct sr_discipline*sd = wu->swu_dis; > - struct cryptop *crp = NULL; > + struct sr_crypto_wu *crwu; > struct cryptodesc *crd; > - struct uio *uio = NULL; > - int flags, i, n, s; > + int flags, i, n; > daddr64_t blk = 0; > u_int keyndx; > > - DNPRINTF(SR_D_DIS, "%s: sr_crypto_getcryptop wu: %p encrypt: %d\n", > + DNPRINTF(SR_D_DIS, "%s: sr_crypto_wu_get wu: %p encrypt: %d\n", > DEVNAME(sd->sd_sc), wu, encrypt); > > - s = splbio(); > - uio = pool_get(&sd->mds.mdd_crypto.sr_uiopl, PR_ZERO | PR_NOWAIT); > - if (uio == NULL) > - goto poolunwind; > - uio->uio_iov = pool_get(&sd->mds.mdd_crypto.sr_iovpl, > - PR_ZERO | PR_NOWAIT); > - if (uio->uio_iov == NULL) > -
Re: softraid crypto: preallocate crypops and dma buffers.
On Fri, 17 Jun 2011 19:53:05 +0100 Owain Ainsworth wrote: > More testing would be appreciated. Cheers, Works for me on amd64, with some io-pressure. Tested while compiling base, cp'n data off the crypted dev onto an usb-hd and filling a partition with data from the network. Stopped testing after ~80GB of traffic both ways, because nothing broke and the compile was finished... Cheers, gotta bounce, - Robert
softraid crypto: preallocate crypops and dma buffers.
So here's the problem: ENOMEM on io in a hba driver is bad juju. In more detail: When preparing for each io softraid crypto does a sr_crypto_getcryptop(). This operation does a few things: - allocate a uio and iovec from a pool. - if this is a read then allocate a dma buffer with dma_alloc (up to MAXPHYS, i.e. 64k) - allocate a cryptop to be used with the crypto(9) framework. - initialise the above for use in the io. So if you are getting very low on memory, one of these operations (probably the dma alloc) will fail. when this happens softraid returns XS_DRIVER_STUFFUP to scsi land. This returns an EIO back to callers. Now it is fairly common to use softdep in these situations, synchronous filesystems are slw. softdep has certain assumptions about how things will work. if a queued dependancy fails that invalidates these assumptions. softraid panics in this case. I used to hit this repeatedly (several times a day) before I bought more memory. in general, most disk controllers prealloc everything they need (that isn't passed down to them) so that this won't be a problem. This diff does the same for softraid crypto. It'll eat just over 1meg of kva, but it is hardly the only driver to need to do that. The only slightly scary thing here is the cryptodesc list manipulations (the comments explain why they are necesary). I'm typing from a machine that runs this. todd@ is also testing it and marco kindly did some horrible io torture on a machine running this too and it runs quite nicely. A side benefit is that due to missing out allocations in every disk io this is actually slightly faster in io too. More testing would be appreciated. Cheers, -0- diff --git dev/softraid_crypto.c dev/softraid_crypto.c index 3259121..a60824e 100644 --- dev/softraid_crypto.c +++ dev/softraid_crypto.c @@ -56,9 +56,26 @@ #include #include -struct cryptop *sr_crypto_getcryptop(struct sr_workunit *, int); +/* + * the per-io data that we need to preallocate. We can't afford to allow io + * to start failing when memory pressure kicks in. + * We can store this in the WU because we assert that only one + * ccb per WU will ever be active. + */ +struct sr_crypto_wu { + TAILQ_ENTRY(sr_crypto_wu)cr_link; + struct uio cr_uio; + struct iovec cr_iov; + struct cryptop *cr_crp; + struct cryptodesc *cr_descs; + struct sr_workunit *cr_wu; + void*cr_dmabuf; +}; + + +struct sr_crypto_wu *sr_crypto_wu_get(struct sr_workunit *, int); +void sr_crypto_wu_put(struct sr_crypto_wu *); intsr_crypto_create_keys(struct sr_discipline *); -void *sr_crypto_putcryptop(struct cryptop *); intsr_crypto_get_kdf(struct bioc_createraid *, struct sr_discipline *); intsr_crypto_decrypt(u_char *, u_char *, u_char *, size_t, int); @@ -78,7 +95,7 @@ int sr_crypto_meta_opt_load(struct sr_discipline *, struct sr_meta_opt *); intsr_crypto_write(struct cryptop *); intsr_crypto_rw(struct sr_workunit *); -intsr_crypto_rw2(struct sr_workunit *, struct cryptop *); +intsr_crypto_rw2(struct sr_workunit *, struct sr_crypto_wu *); void sr_crypto_intr(struct buf *); intsr_crypto_read(struct cryptop *); void sr_crypto_finish_io(struct sr_workunit *); @@ -230,40 +247,35 @@ done: return (rv); } -struct cryptop * -sr_crypto_getcryptop(struct sr_workunit *wu, int encrypt) + +struct sr_crypto_wu * +sr_crypto_wu_get(struct sr_workunit *wu, int encrypt) { struct scsi_xfer*xs = wu->swu_xs; struct sr_discipline*sd = wu->swu_dis; - struct cryptop *crp = NULL; + struct sr_crypto_wu *crwu; struct cryptodesc *crd; - struct uio *uio = NULL; - int flags, i, n, s; + int flags, i, n; daddr64_t blk = 0; u_int keyndx; - DNPRINTF(SR_D_DIS, "%s: sr_crypto_getcryptop wu: %p encrypt: %d\n", + DNPRINTF(SR_D_DIS, "%s: sr_crypto_wu_get wu: %p encrypt: %d\n", DEVNAME(sd->sd_sc), wu, encrypt); - s = splbio(); - uio = pool_get(&sd->mds.mdd_crypto.sr_uiopl, PR_ZERO | PR_NOWAIT); - if (uio == NULL) - goto poolunwind; - uio->uio_iov = pool_get(&sd->mds.mdd_crypto.sr_iovpl, - PR_ZERO | PR_NOWAIT); - if (uio->uio_iov == NULL) - goto poolunwind; - splx(s); + mtx_enter(&sd->mds.mdd_crypto.scr_mutex); + if ((crwu = TAILQ_FIRST(&sd->mds.mdd_crypto.scr_wus)) != NULL) + TAILQ_REMOVE(&sd->mds.mdd_crypto.scr_wus, crwu, cr_link); + mtx_leave(&sd->mds.mdd_crypto.scr_mutex); + if (crwu == NULL) + panic("sr_crypto_w