[dm-devel] [PATCH 08/10] dm-integrity: add a bitmap mode

[Mikulas Patocka]

On Wed, 8 May 2019, Mike Snitzer wrote:

> > +static struct bitmap_block_status *sector_to_bitmap_block(struct dm_integrity_c *ic, sector_t sector)
> > +{
> > +	unsigned bit = sector >> (ic->sb->log2_sectors_per_block + ic->log2_blocks_per_bitmap_bit);
> > +	unsigned bitmap_block = bit / (BITMAP_BLOCK_SIZE * 8);
> > +
> > +	BUG_ON(bitmap_block >= ic->n_bitmap_blocks);
> > +	return &ic->bbs[bitmap_block];
> > +}
> > +
> 
> Too many BUG()s and BUG_ON.  The BUG_ON could be left if you think it
> sufficiently unlikely.

The BUGs could only happen if there's a bug in the code. They can't happen 
on disk I/O errors or checksum errors.

> But in general I'd prefer factoring out an &error return but needing to
> check for such a return would slow things down.. so that's probably not
> an option.

Yes. I think it is not a good practice to write recovery code for 
situations that can't happen. Those spurious return codes will just make 
the code harder to understand.

> Maybe we just set a new 'invalid' flag and disallow all operations and
> fail IO?  Point is it is really bad to crash the system because
> dm-integrity lost its way.  Instead we should just mark the device
> invalid (like dm-snapshot does).
>
> Mike

It already has a flag "struct dm_integrity_c->failed" - but this is for 
unrecoverable I/O errors (such as journal errors), not for programming 
errors.

I suggest - if no one complains, you can let it be, if someone complains, 
you can just hide the BUGs behind "#ifdef CONFIG_BUG_ON_DATA_CORRUPTION".

Mikulas