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

[John Stoffel]

>>>>> "Mikulas" == Mikulas Patocka <mpatocka at redhat.com> writes:

Mikulas> On Wed, 19 Jul 2017, John Stoffel 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.
>> 
>> John

Mikulas> That BUG can only be triggered by a bug in the code, it can't be triggered 
Mikulas> by bad data on the disk, so why should we write code to recover from it?

Because it's damn obnoxious to kill a system because of bad coding,
without giving the user a chance to recover or even investigate
the root cause.   If it's a bug in the code, just warn the user and
then try to continue.  If you can't continue, then just make the block
device go read-only to protect the data.

Again, why do you think BUG_ON is appropriate thing to do?  What
advantage does it have for you?

John