[dm-devel] dm-integrity: fix inefficient allocation of stack space

[John Stoffel]

>>>>> "Mike" == Mike Snitzer <snitzer at redhat.com> writes:

Mike> On Wed, Jul 19 2017 at  2:39pm -0400,
Mike> John Stoffel <john at stoffel.org> wrote:

>> >>>>> "Mikulas" == Mikulas Patocka <mpatocka at redhat.com> writes:
>> 
Mikulas> When using block size greater than 512 bytes, the dm-integrity target
Mikulas> allocates journal space inefficiently, it allocates one entry for each
Mikulas> 512-byte chunk of data, fills entries for each block of data and leaves
Mikulas> the remaining entries unused. This doesn't cause data corruption, but it
Mikulas> causes severe performance degradation.
>> 
Mikulas> This patch fixes the journal allocation. As a safety it also
Mikulas> adds BUG that fires if the variables representing journal
Mikulas> usage get out of sync (it's better to crash than continue and
Mikulas> corrupt data, so BUG is justified).
>> 
>> No, I don't agree.  You should lock down the block device(s) using the
>> dm-integrity target, but you should NOT take down the entire system
>> because of this.  Just return a failure up the stack that forces the
>> device into read only mode so that there's a chance to debug this
>> issue.
>> 
>> Using a BUG_ON, especially for code that isn't proven, is just wrong.
>> Do a WARN_ONCE() and then return an error instead.

Mike> I'll remove the BUG_ON from this stable@ fix.  Mikulas, please have a
Mike> look at handling the failure more gracefully (maybe like John suggests).
Mike> In general, there are too many BUG_ON()s in the dm-integrity code.  I
Mike> let them slide for inclusion but it would probably be a good idea to
Mike> look at eliminating them and putting the dm-integrity device into "fail
Mike> mode" in the face of critical errors -- much like dm-thinp and dm-cache
Mike> has.

I'd like to argue that you should never use BUG_ON at all, esp since
if you have integrity running on just one critical device, but have
other devices that work just fine, bringing down the entire system
because you don't think things are ok is terrible.

We should rip out ALL the BUG_ONs in the dm-integrity and just do
proper error handling instead.  For testing, sure, use them on your
code.  But please, not for general use!

John