Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210222125916.4168804-1-geert@linux-m68k.org>
In-Reply-To: <20210222125916.4168804-1-geert@linux-m68k.org>
From:   Masahiro Yamada <masahiroy@kernel.org>
Date:   Tue, 23 Feb 2021 15:30:54 +0900
Message-ID: <CAK7LNARxO6O7aRwzJ+i9hEGvWBTCukpwGBC6B79c7UdO=f0Ymw@mail.gmail.com>
Subject: Re: [PATCH] f2fs: compress: Allow modular (de)compression algorithms
To:     Geert Uytterhoeven <geert@linux-m68k.org>
Cc:     Jaegeuk Kim <jaegeuk@kernel.org>, Chao Yu <chao@kernel.org>,
        Randy Dunlap <rdunlap@infradead.org>,
        linux-f2fs-devel@lists.sourceforge.net,
        Linux Kbuild mailing list <linux-kbuild@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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