On Wed, 19 Jul 2017, John Stoffel wrote:
> >>>>> "Mikulas" == Mikulas Patocka <mpatocka@xxxxxxxxxx> 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@xxxxxxxxxx>
> Mikulas> Cc: stable@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxx
> Mikulas> https://www.redhat.com/mailman/listinfo/dm-devel
>
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
