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

[Mikulas Patocka]

On Thu, 20 Jul 2017, John Stoffel wrote:

> >>>>> "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?

When writing the code, I occasionally make mistakes, such as passing a 
wrong variable to the function. This BUG_ON catches the problem 
immediatelly, I can instantly see what problem occured.

If that BUG_ON is omitted, I get a crash that is harder to debug (it is 
needed to disassemble the function and find out which registers are used 
for which variables) or I get a data corruption, which is even harder to 
debug than a crash.

If I pass a wrong variable to a function, the correct fix is to pass the 
correct variable to the function, not to write additional code that tries 
to recover from the wrong variable. If you disagree, just show us some 
program that uses your own techniques for recovering from your bugs and we 
can see.

Mikulas

> John