[PATCH] f2fs: compress: Allow modular (de)compression algorithms

[PATCH] f2fs: compress: Allow modular (de)compression algorithms

[Geert Uytterhoeven]

If F2FS_FS is modular, enabling the compressions options
F2FS_FS_{LZ4,LZ4HZ,LZO,LZORLE,ZSTD} will make the (de)compression
algorithms {LZ4,LZ4HC,LZO,ZSTD}_{,DE}COMPRESS builtin instead of
modular, as the former depend on an intermediate boolean
F2FS_FS_COMPRESSION, which in-turn depends on tristate F2FS_FS.

Indeed, if a boolean symbol A depends directly on a tristate symbol B
and selects another tristate symbol C:

    tristate B

    tristate C

    bool A
      depends on B
      select C

and B is modular, then C will also be modular.

However, if there is an intermediate boolean D in the dependency chain
between A and B:

    tristate B

    tristate C

    bool D
      depends on B

    bool A
      depends on D
      select C

then the modular state won't propagate from B to C, and C will be
builtin instead of modular.

Fix this by making the various compression options depend directly on
F2FS_FS using a big if/endif block.  Drop the now superfluous
dependencies on F2FS_FS from individual symbols.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
Perhaps the propagation logic in Kconfig should be fixed instead?
Else people may reintroduce this issue when removing seemingly-unneeded
dependencies.
---
 fs/f2fs/Kconfig | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/fs/f2fs/Kconfig b/fs/f2fs/Kconfig
index 62e638a49bbf089a..20a82ecb72b42f84 100644
--- a/fs/f2fs/Kconfig
+++ b/fs/f2fs/Kconfig
@@ -20,9 +20,10 @@ config F2FS_FS
 
 	  If unsure, say N.
 
+if F2FS_FS
+
 config F2FS_STAT_FS
 	bool "F2FS Status Information"
-	depends on F2FS_FS
 	default y
 	help
 	  /sys/kernel/debug/f2fs/ contains information about all the partitions
@@ -35,7 +36,6 @@ config F2FS_STAT_FS
 
 config F2FS_FS_XATTR
 	bool "F2FS extended attributes"
-	depends on F2FS_FS
 	default y
 	help
 	  Extended attributes are name:value pairs associated with inodes by
@@ -70,7 +70,6 @@ config F2FS_FS_SECURITY
 
 config F2FS_CHECK_FS
 	bool "F2FS consistency checking feature"
-	depends on F2FS_FS
 	help
 	  Enables BUG_ONs which check the filesystem consistency in runtime.
 
@@ -78,7 +77,6 @@ config F2FS_CHECK_FS
 
 config F2FS_FAULT_INJECTION
 	bool "F2FS fault injection facility"
-	depends on F2FS_FS
 	help
 	  Test F2FS to inject faults such as ENOMEM, ENOSPC, and so on.
 
@@ -86,7 +84,6 @@ config F2FS_FAULT_INJECTION
 
 config F2FS_FS_COMPRESSION
 	bool "F2FS compression feature"
-	depends on F2FS_FS
 	help
 	  Enable filesystem-level compression on f2fs regular files,
 	  multiple back-end compression algorithms are supported.
@@ -137,3 +134,5 @@ config F2FS_FS_LZORLE
 	default y
 	help
 	  Support LZO-RLE compress algorithm, if unsure, say Y.
+
+endif
-- 
2.25.1

Re: [PATCH] f2fs: compress: Allow modular (de)compression algorithms

[Masahiro Yamada]

On Mon, Feb 22, 2021 at 9:59 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> If F2FS_FS is modular, enabling the compressions options
> F2FS_FS_{LZ4,LZ4HZ,LZO,LZORLE,ZSTD} will make the (de)compression
> algorithms {LZ4,LZ4HC,LZO,ZSTD}_{,DE}COMPRESS builtin instead of
> modular, as the former depend on an intermediate boolean
> F2FS_FS_COMPRESSION, which in-turn depends on tristate F2FS_FS.
>
> Indeed, if a boolean symbol A depends directly on a tristate symbol B
> and selects another tristate symbol C:
>
>     tristate B
>
>     tristate C
>
>     bool A
>       depends on B
>       select C
>
> and B is modular, then C will also be modular.
>
> However, if there is an intermediate boolean D in the dependency chain
> between A and B:
>
>     tristate B
>
>     tristate C
>
>     bool D
>       depends on B
>
>     bool A
>       depends on D
>       select C
>
> then the modular state won't propagate from B to C, and C will be
> builtin instead of modular.
>
> Fix this by making the various compression options depend directly on
> F2FS_FS using a big if/endif block.  Drop the now superfluous
> dependencies on F2FS_FS from individual symbols.
>
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
> Perhaps the propagation logic in Kconfig should be fixed instead?
> Else people may reintroduce this issue when removing seemingly-unneeded
> dependencies.

I checked the code in menu_finalize(), and this seems to work like this.

I discussed the oddity of the select behavior before
(https://lore.kernel.org/linux-kbuild/e1a6228d-1341-6264-d97a-e2bd52a65c82@infradead.org/),
but I was not confident about what the right direction was.


Anyway, the behavior is obscure from the current code.

If you want to make this more robust,
you can write as follows:

config F2FS_FS
        tristate "F2FS filesystem support"
        depends on BLOCK
        select NLS
        select CRYPTO
        select CRYPTO_CRC32
        select F2FS_FS_XATTR if FS_ENCRYPTION
        select FS_ENCRYPTION_ALGS if FS_ENCRYPTION
        select LZO_COMPRESS if F2FS_FS_LZO
        select LZO_DECOMPRESS if F2FS_FS_LZO
        select LZ4_COMPRESS if F2FS_FS_LZ4
        select LZ4_DECOMPRESS if F2FS_FS_LZ4
        select LZ4HC_COMPRESS if F2FS_FS_LZ4HC
        select ZSTD_COMPRESS if F2FS_FS_ZSTD
        select ZSTD_DECOMPRESS if F2FS_FS_ZSTD

The code is a bit clumsy, but it is clear
that the module (F2FS_FS) is selecting the
compress/decompress libraries.






> ---
>  fs/f2fs/Kconfig | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/fs/f2fs/Kconfig b/fs/f2fs/Kconfig
> index 62e638a49bbf089a..20a82ecb72b42f84 100644
> --- a/fs/f2fs/Kconfig
> +++ b/fs/f2fs/Kconfig
> @@ -20,9 +20,10 @@ config F2FS_FS
>
>           If unsure, say N.
>
> +if F2FS_FS
> +
>  config F2FS_STAT_FS
>         bool "F2FS Status Information"
> -       depends on F2FS_FS
>         default y
>         help
>           /sys/kernel/debug/f2fs/ contains information about all the partitions
> @@ -35,7 +36,6 @@ config F2FS_STAT_FS
>
>  config F2FS_FS_XATTR
>         bool "F2FS extended attributes"
> -       depends on F2FS_FS
>         default y
>         help
>           Extended attributes are name:value pairs associated with inodes by
> @@ -70,7 +70,6 @@ config F2FS_FS_SECURITY
>
>  config F2FS_CHECK_FS
>         bool "F2FS consistency checking feature"
> -       depends on F2FS_FS
>         help
>           Enables BUG_ONs which check the filesystem consistency in runtime.
>
> @@ -78,7 +77,6 @@ config F2FS_CHECK_FS
>
>  config F2FS_FAULT_INJECTION
>         bool "F2FS fault injection facility"
> -       depends on F2FS_FS
>         help
>           Test F2FS to inject faults such as ENOMEM, ENOSPC, and so on.
>
> @@ -86,7 +84,6 @@ config F2FS_FAULT_INJECTION
>
>  config F2FS_FS_COMPRESSION
>         bool "F2FS compression feature"
> -       depends on F2FS_FS
>         help
>           Enable filesystem-level compression on f2fs regular files,
>           multiple back-end compression algorithms are supported.
> @@ -137,3 +134,5 @@ config F2FS_FS_LZORLE
>         default y
>         help
>           Support LZO-RLE compress algorithm, if unsure, say Y.
> +
> +endif
> --
> 2.25.1
>


--
Best Regards
Masahiro Yamada

Re: [PATCH] f2fs: compress: Allow modular (de)compression algorithms

[Geert Uytterhoeven]

Hi Yamada-san,

On Tue, Feb 23, 2021 at 7:31 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
> On Mon, Feb 22, 2021 at 9:59 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > If F2FS_FS is modular, enabling the compressions options
> > F2FS_FS_{LZ4,LZ4HZ,LZO,LZORLE,ZSTD} will make the (de)compression
> > algorithms {LZ4,LZ4HC,LZO,ZSTD}_{,DE}COMPRESS builtin instead of
> > modular, as the former depend on an intermediate boolean
> > F2FS_FS_COMPRESSION, which in-turn depends on tristate F2FS_FS.
> >
> > Indeed, if a boolean symbol A depends directly on a tristate symbol B
> > and selects another tristate symbol C:
> >
> >     tristate B
> >
> >     tristate C
> >
> >     bool A
> >       depends on B
> >       select C
> >
> > and B is modular, then C will also be modular.
> >
> > However, if there is an intermediate boolean D in the dependency chain
> > between A and B:
> >
> >     tristate B
> >
> >     tristate C
> >
> >     bool D
> >       depends on B
> >
> >     bool A
> >       depends on D
> >       select C
> >
> > then the modular state won't propagate from B to C, and C will be
> > builtin instead of modular.
> >
> > Fix this by making the various compression options depend directly on
> > F2FS_FS using a big if/endif block.  Drop the now superfluous
> > dependencies on F2FS_FS from individual symbols.
> >
> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > ---
> > Perhaps the propagation logic in Kconfig should be fixed instead?
> > Else people may reintroduce this issue when removing seemingly-unneeded
> > dependencies.
>
> I checked the code in menu_finalize(), and this seems to work like this.
>
> I discussed the oddity of the select behavior before
> (https://lore.kernel.org/linux-kbuild/e1a6228d-1341-6264-d97a-e2bd52a65c82@infradead.org/),
> but I was not confident about what the right direction was.
>
>
> Anyway, the behavior is obscure from the current code.
>
> If you want to make this more robust,
> you can write as follows:
>
> config F2FS_FS
>         tristate "F2FS filesystem support"
>         depends on BLOCK
>         select NLS
>         select CRYPTO
>         select CRYPTO_CRC32
>         select F2FS_FS_XATTR if FS_ENCRYPTION
>         select FS_ENCRYPTION_ALGS if FS_ENCRYPTION
>         select LZO_COMPRESS if F2FS_FS_LZO
>         select LZO_DECOMPRESS if F2FS_FS_LZO
>         select LZ4_COMPRESS if F2FS_FS_LZ4
>         select LZ4_DECOMPRESS if F2FS_FS_LZ4
>         select LZ4HC_COMPRESS if F2FS_FS_LZ4HC
>         select ZSTD_COMPRESS if F2FS_FS_ZSTD
>         select ZSTD_DECOMPRESS if F2FS_FS_ZSTD
>
> The code is a bit clumsy, but it is clear
> that the module (F2FS_FS) is selecting the
> compress/decompress libraries.

Actually the above is what I tried first ;-)  Works fine.

Then I started to look for similar cases in other file systems (e.g.
EROFS_FS_ZIP), and discovered the issue doesn't happen there, which
sparked my investigation.  So I settled on the direct dependency,
because it keeps all compression-related logic together.

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: [f2fs-dev] [PATCH] f2fs: compress: Allow modular (de)compression algorithms

[Chao Yu]

Hi Geert,

On 2021/2/23 15:42, Geert Uytterhoeven wrote:
>> I checked the code in menu_finalize(), and this seems to work like this.
>>
>> I discussed the oddity of the select behavior before
>> (https://lore.kernel.org/linux-kbuild/e1a6228d-1341-6264-d97a-e2bd52a65c82@infradead.org/),
>> but I was not confident about what the right direction was.
>>
>>
>> Anyway, the behavior is obscure from the current code.
>>
>> If you want to make this more robust,
>> you can write as follows:
>>
>> config F2FS_FS
>>          tristate "F2FS filesystem support"
>>          depends on BLOCK
>>          select NLS
>>          select CRYPTO
>>          select CRYPTO_CRC32
>>          select F2FS_FS_XATTR if FS_ENCRYPTION
>>          select FS_ENCRYPTION_ALGS if FS_ENCRYPTION
>>          select LZO_COMPRESS if F2FS_FS_LZO
>>          select LZO_DECOMPRESS if F2FS_FS_LZO
>>          select LZ4_COMPRESS if F2FS_FS_LZ4
>>          select LZ4_DECOMPRESS if F2FS_FS_LZ4
>>          select LZ4HC_COMPRESS if F2FS_FS_LZ4HC
>>          select ZSTD_COMPRESS if F2FS_FS_ZSTD
>>          select ZSTD_DECOMPRESS if F2FS_FS_ZSTD
>>
>> The code is a bit clumsy, but it is clear
>> that the module (F2FS_FS) is selecting the
>> compress/decompress libraries.
> Actually the above is what I tried first ;-)  Works fine.
> 
> Then I started to look for similar cases in other file systems (e.g.
> EROFS_FS_ZIP), and discovered the issue doesn't happen there, which
> sparked my investigation.  So I settled on the direct dependency,
> because it keeps all compression-related logic together.

It looks above way is more explicit, how about using your previous implementation?

Thank,

> 
> Gr{oetje,eeting}s,