Re: [PATCH 2/2] dm-integrity: introduce the Inline mode

Re: [PATCH 2/2] dm-integrity: introduce the Inline mode

[Geert Uytterhoeven]

 	Hi Mikulas,

On Tue, 25 Jun 2024, Mikulas Patocka wrote:
> This commit introduces a new 'I' mode for dm-integrity.
>
> The 'I' mode may be selected if the underlying device has non-power-of-2
> sector size. In this mode, dm-integrity will store integrity data
> directly in device's sectors and it will not use journal.
>
> This mode improves performance and reduces flash wear because there would
> be no journal writes.
>
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

Thanks for your patch, which is now commit fb0987682c629c1d
("dm-integrity: introduce the Inline mode") in v6.11-rc1.

> @@ -4433,9 +4737,12 @@ static int dm_integrity_ctr(struct dm_ta
> 		ti->error = "Block size doesn't match the information in superblock";
> 		goto bad;
> 	}
> -	if (!le32_to_cpu(ic->sb->journal_sections)) {
> +	if (!le32_to_cpu(ic->sb->journal_sections) != (ic->mode == 'I')) {

As reporting before in e.g. [1], this is causing build failures with
gcc-5 and -Werror:

     drivers/md/dm-integrity.c:4718:52: error: logical not is only applied to the left hand side of comparison [-Werror=logical-not-parentheses]
       if (!le32_to_cpu(ic->sb->journal_sections) != (ic->mode == 'I')) {
 							^

I could reproduce the issue with gcc-5(Ubuntu 5.5.0-12ubuntu1) on x86 .

The condition indeed looks hard to read. Perhaps it should be rewritten as
"le32_to_cpu(ic->sb->journal_sections) == (ic->mode == 'I')"?

> 		r = -EINVAL;
> -		ti->error = "Corrupted superblock, journal_sections is 0";
> +		if (ic->mode != 'I')
> +			ti->error = "Corrupted superblock, journal_sections is 0";
> +		else
> +			ti->error = "Corrupted superblock, journal_sections is not 0";
> 		goto bad;
> 	}
> 	/* make sure that ti->max_io_len doesn't overflow */

[1] https://lore.kernel.org/20240729092807.2235937-1-geert@linux-m68k.org

Gr{oetje,eeting}s,

 						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
 							    -- Linus Torvalds

Re: [PATCH 2/2] dm-integrity: introduce the Inline mode

[Mikulas Patocka]

On Mon, 26 Aug 2024, Geert Uytterhoeven wrote:

> 	Hi Mikulas,
> 
> On Tue, 25 Jun 2024, Mikulas Patocka wrote:
> > This commit introduces a new 'I' mode for dm-integrity.
> > 
> > The 'I' mode may be selected if the underlying device has non-power-of-2
> > sector size. In this mode, dm-integrity will store integrity data
> > directly in device's sectors and it will not use journal.
> > 
> > This mode improves performance and reduces flash wear because there would
> > be no journal writes.
> > 
> > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> 
> Thanks for your patch, which is now commit fb0987682c629c1d
> ("dm-integrity: introduce the Inline mode") in v6.11-rc1.
> 
> > @@ -4433,9 +4737,12 @@ static int dm_integrity_ctr(struct dm_ta
> > 		ti->error = "Block size doesn't match the information in
> > superblock";
> > 		goto bad;
> > 	}
> > -	if (!le32_to_cpu(ic->sb->journal_sections)) {
> > +	if (!le32_to_cpu(ic->sb->journal_sections) != (ic->mode == 'I')) {
> 
> As reporting before in e.g. [1], this is causing build failures with
> gcc-5 and -Werror:
> 
>     drivers/md/dm-integrity.c:4718:52: error: logical not is only applied to
> the left hand side of comparison [-Werror=logical-not-parentheses]
>       if (!le32_to_cpu(ic->sb->journal_sections) != (ic->mode == 'I')) {

Hi

I fixed it. The fix will be submitted upstream in the next merge window.

Mikulas