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

[Mikulas Patocka]

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

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

Mikulas

> Mikulas> Signed-off-by: Mikulas Patocka <mpatocka at redhat.com>
> Mikulas> Cc: stable at vger.kernel.org
> Mikulas> Fixes: 7eada909bfd7 ("dm: add integrity target")
> 
> Mikulas> ---
> Mikulas>  drivers/md/dm-integrity.c |    7 ++++---
> Mikulas>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> Mikulas> Index: linux-2.6/drivers/md/dm-integrity.c
> Mikulas> ===================================================================
> Mikulas> --- linux-2.6.orig/drivers/md/dm-integrity.c
> Mikulas> +++ linux-2.6/drivers/md/dm-integrity.c
> Mikulas> @@ -1589,14 +1589,14 @@ retry:
> Mikulas>  			unsigned next_entry, i, pos;
> Mikulas>  			unsigned ws, we;
>  
> Mikulas> -			dio->range.n_sectors = min(dio->range.n_sectors, ic->free_sectors);
> Mikulas> +			dio->range.n_sectors = min(dio->range.n_sectors, ic->free_sectors << ic->sb->log2_sectors_per_block);
> Mikulas>  			if (unlikely(!dio->range.n_sectors))
> Mikulas>  				goto sleep;
> Mikulas> -			ic->free_sectors -= dio->range.n_sectors;
> Mikulas> +			ic->free_sectors -= dio->range.n_sectors >> ic->sb->log2_sectors_per_block;
> Mikulas>  			journal_section = ic->free_section;
> Mikulas>  			journal_entry = ic->free_section_entry;
>  
> Mikulas> -			next_entry = ic->free_section_entry + dio->range.n_sectors;
> Mikulas> +			next_entry = ic->free_section_entry + (dio->range.n_sectors >> ic->sb->log2_sectors_per_block);
> ic-> free_section_entry = next_entry % ic->journal_section_entries;
> ic-> free_section += next_entry / ic->journal_section_entries;
> ic-> n_uncommitted_sections += next_entry / ic->journal_section_entries;
> Mikulas> @@ -1727,6 +1727,7 @@ static void pad_uncommitted(struct dm_in
> Mikulas>  		wraparound_section(ic, &ic->free_section);
> ic-> n_uncommitted_sections++;
> Mikulas>  	}
> Mikulas> +	BUG_ON((ic->n_uncommitted_sections + ic->n_committed_sections) * ic->journal_section_entries + ic->free_sectors != ic->journal_sections * ic->journal_section_entries);
> Mikulas>  }
>  
> Mikulas>  static void integrity_commit(struct work_struct *w)
> 
> Mikulas> --
> Mikulas> dm-devel mailing list
> Mikulas> dm-devel at redhat.com
> Mikulas> https://www.redhat.com/mailman/listinfo/dm-devel
>