Re: [PATCH v7 4/4] fs: unicode: Add utf8 module and a unicode layer

From: Eric Biggers <ebiggers@kernel.org>
To: Shreeya Patel <shreeya.patel@collabora.com>
Cc: tytso@mit.edu, adilger.kernel@dilger.ca, jaegeuk@kernel.org,
	chao@kernel.org, krisman@collabora.com, drosen@google.com,
	yuchao0@huawei.com, linux-ext4@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	linux-f2fs-devel@lists.sourceforge.net,
	linux-fsdevel@vger.kernel.org, kernel@collabora.com,
	andre.almeida@collabora.com
Subject: Re: [PATCH v7 4/4] fs: unicode: Add utf8 module and a unicode layer
Date: Wed, 7 Apr 2021 18:01:26 -0700	[thread overview]
Message-ID: <YG5V5l2pD3DCiyVA@gmail.com> (raw)
In-Reply-To: <20210407144845.53266-5-shreeya.patel@collabora.com>

On Wed, Apr 07, 2021 at 08:18:45PM +0530, Shreeya Patel wrote:
> diff --git a/fs/unicode/Kconfig b/fs/unicode/Kconfig
> index 2c27b9a5cd6c..0c69800a2a37 100644
> --- a/fs/unicode/Kconfig
> +++ b/fs/unicode/Kconfig
> @@ -2,13 +2,31 @@
>  #
>  # UTF-8 normalization
>  #
> +# CONFIG_UNICODE will be automatically enabled if CONFIG_UNICODE_UTF8
> +# is enabled. This config option adds the unicode subsystem layer which loads
> +# the UTF-8 module whenever any filesystem needs it.
>  config UNICODE
> -	bool "UTF-8 normalization and casefolding support"
> +	bool
> +
> +config UNICODE_UTF8
> +	tristate "UTF-8 module"
> +	select UNICODE
>  	help
> -	  Say Y here to enable UTF-8 NFD normalization and NFD+CF casefolding
> -	  support.
> +	  Say M here to enable UTF-8 NFD normalization and NFD+CF casefolding
> +	  support as a loadable module or say Y for building it into the kernel.
> +
> +	  utf8data.h_shipped has a large database table which is an
> +	  auto-generated decodification trie for the unicode normalization
> +	  functions and it is not necessary to carry this large table in the
> +	  kernel. Hence, enabling UNICODE_UTF8 as M will allow you to avoid
> +	  carrying this large table into the kernel and module will only be
> +	  loaded whenever required by any filesystem.
> +	  Please note, in this case utf8 module will only be available after
> +	  booting into the compiled kernel. If your filesystem requires it to
> +	  have utf8 during boot time then you should have it built into the
> +	  kernel by saying Y here to avoid boot failure.

This help text seems to contradict itself; it says "it is not necessary to carry
this large table in the kernel", and then later it says that in some cases it is
in fact necessary.

It would also be helpful for the help text to mention which filesystems actually
support this feature.

> diff --git a/fs/unicode/unicode-core.c b/fs/unicode/unicode-core.c
> index 730dbaedf593..d9e9e410893d 100644
> --- a/fs/unicode/unicode-core.c
> +++ b/fs/unicode/unicode-core.c
> @@ -1,228 +1,132 @@
>  /* SPDX-License-Identifier: GPL-2.0 */
>  #include <linux/module.h>
>  #include <linux/kernel.h>
> -#include <linux/string.h>
>  #include <linux/slab.h>
> -#include <linux/parser.h>
>  #include <linux/errno.h>
>  #include <linux/unicode.h>
> -#include <linux/stringhash.h>
> +#include <linux/spinlock.h>
>  
> -#include "utf8n.h"
> +DEFINE_SPINLOCK(utf8mod_lock);

This spinlock should be 'static'.

- Eric