Re: dm-crypt: remove per-cpu structure

[Mike Snitzer <snitzer@redhat.com>]

On Sat, Feb 22 2014 at 12:34pm -0500,
Milan Broz <gmazyland@gmail.com> wrote:

> On 02/22/2014 03:25 PM, Mike Snitzer wrote:
> > On Sat, Feb 22 2014 at  5:28am -0500,
> > Milan Broz <gmazyland@gmail.com> wrote:
> > 
> >> On 02/21/2014 08:41 AM, Mikulas Patocka wrote:
> >>>>>> Hi Mikulas,
> >>>>>>
> >>>>>> Obviously avoiding crashes is more important than performance.
> >>
> >> Yes, but please keep these changes in linux-next for a while
> >> before submitting this for stable or you will fix one rare bug and
> >> slow down all existing users (with possible another fix to fix later)...
> >>
> >> I know that taking cpu offline becomes more used these days
> >> but still the performance is critical attribute for dmcrypt.
> >>
> >> (And IIRC this cpu work relocation problem is there for years.)
> > 
> > Unfortunately, we cannot leave the code in its current state.  Sure it
> > has been this way for years but taking cpus offline is _much_ more
> > common.
> > 
> > As I said here:
> > https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=for-next&id=fba2d94c5e0f3d848792981977b1f62ce96377ac
> > 
> > This change may undermine performance improvements intended by commit
> > c0297721 ("dm crypt: scale to multiple cpus").  But correctness is more
> > important than performance.  The use of per-cpu structures may be
> > revisited later (by using cpu hot-plug notifiers to make them safe).
> 
> Correctness? Yes. I think Tejun said how to fix it correctly.

OK, patches welcome.

> I have nothing against removal of percpu structure - but please assess
> what it breaks, how it will slow down dmcrypt and mention it in patch
> header if you want quick fix.
> 
> I couldn't resist but "may undermine" in patch header sounds like
> "we have no idea what it breaks but it no longer crashes".
> 
> Removal of headache by removing head is always 100% solution to
> the particular problem but it has usually also some side effects :-)

Show me that the percpu performance is worth the risk of crashing the
kernel.  I'd love to see you prove that.

If Mikulas' changes were embraced 2.5 years ago we'd be in a much better
place than we are now.  I'm making the decision to affect change.  You
may not agree with it but I'm confident dm-crypt will be better for it.
 
> > Even though you actively resisted it:
> > http://www.redhat.com/archives/dm-devel/2013-March/msg00107.html
> 
> I am repeating still repeating the same: Do we have performance
> numbers proving it is better in real world scenarios?
> Nobody will use ramdisk for dmcrypt.
> 
> These are the most used scenarios for dmcrypt I know :
> 
> - Notebook with single SSD / rotational disk and multicore (2 or 4) CPU
> with AES-NI acceleration, dmcrypt over plain partition or LVM
> (typical encrypted distro installation)
> 
> - dmcrypt over MD RAID1/5/6 with several multicore CPUs
> (think encrypted array for backups on server)
> [There is also variant with several dmcrypt devices with MD RAID
> over it - which is currently performance disaster - but I am
> not sure this is good idea. It was suggested as workaround
> before percpu changes were implemented.]
> 
> - several dmcrypt devices in system with many cores over LVM
> (encrypted storage for VMs. It would be nice to have
> fixed CPU limit per dmcrypt device - you do not want to one VM
> eat all processing power. I think we discussed with Mikulas
> that dmcrypt should have some tweaking parameters to limit used
> CPUs per device. But it was long time ago...)
> 
> It would be also interesting how it behaves when stacking dmcrypt
> devices on top of each other (this is used in TrueCrypt compatibility
> mode in crypsetup, but also in TrueCrypt itself).

Thanks for the configuration scenarios.  I think I've asked this of you
before but: Do you have any automated testing?  Something that we hand a
blockdevice to and it produces a result that you find meaningful to use
as a point of comparison.
 
> > Progress on improving dm-crypt is long overdue, I'd like to see Mikulas
> > rebase/repost/retest his dm-crypt parallelization patchset for 3.15 or
> > 3.16.
> 
> I agree and I would put accent on _improving_ in this sentence ;-)

Uh huh...